On Fri, 1 Nov 2024 at 20:37, Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> wrote:
>
> From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>
> Commit 8a924db2d7b5 ("fs: Pass AT_GETATTR_NOSEC flag to getattr interface
> function")' introduced the AT_GETATTR_NOSEC flag to ensure that the
> call paths only call vfs_getattr_nosec if it is set instead of vfs_getattr.
> Now, simplify the getattr interface functions of filesystems where the flag
> AT_GETATTR_NOSEC is checked.
>
> There is only a single caller of inode_operations getattr function and it
> is located in fs/stat.c in vfs_getattr_nosec. The caller there is the only
> one from which the AT_GETATTR_NOSEC flag is passed from.
>
> Two filesystems are checking this flag in .getattr and the flag is always
> passed to them unconditionally from only vfs_getattr_nosec:
>
> - ecryptfs: Simplify by always calling vfs_getattr_nosec in
> ecryptfs_getattr. From there the flag is passed to no other
> function and this function is not called otherwise.
>
> - overlayfs: Simplify by always calling vfs_getattr_nosec in
> ovl_getattr. From there the flag is passed to no other
> function and this function is not called otherwise.
>
> The query_flags in vfs_getattr_nosec will mask-out AT_GETATTR_NOSEC from
> any caller using AT_STATX_SYNC_TYPE as mask so that the flag is not
> important inside this function. Also, since no filesystem is checking the
> flag anymore, remove the flag entirely now, including the BUG_ON check that
> never triggered.
>
> The net change of the changes here combined with the originan commit is
> that ecryptfs and overlayfs do not call vfs_getattr but only
> vfs_getattr_nosec.
[Adding LSM list.]
The original intention was I think that security_inode_getattr()
should be called on the backing inode IFF it was called on the backed
(overlay/ecryptfs) inode.
The implementation was broken, but the question remains whether this
is needed or not.
Thanks,
Miklos
>
> Fixes: 8a924db2d7b5 ("fs: Pass AT_GETATTR_NOSEC flag to getattr interface function")
> Reported-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-fsdevel/20241101011724.GN1350452@ZenIV/T/#u
> Cc: Tyler Hicks <code@xxxxxxxxxxx>
> Cc: ecryptfs@xxxxxxxxxxxxxxx
> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> Cc: linux-unionfs@xxxxxxxxxxxxxxx
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> ---
> fs/ecryptfs/inode.c | 12 ++----------
> fs/overlayfs/inode.c | 10 +++++-----
> fs/overlayfs/overlayfs.h | 8 --------
> fs/stat.c | 5 +----
> include/uapi/linux/fcntl.h | 4 ----
> 5 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index cbdf82f0183f..a9819ddb1ab8 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1008,14 +1008,6 @@ static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
> return rc;
> }
>
> -static int ecryptfs_do_getattr(const struct path *path, struct kstat *stat,
> - u32 request_mask, unsigned int flags)
> -{
> - if (flags & AT_GETATTR_NOSEC)
> - return vfs_getattr_nosec(path, stat, request_mask, flags);
> - return vfs_getattr(path, stat, request_mask, flags);
> -}
> -
> static int ecryptfs_getattr(struct mnt_idmap *idmap,
> const struct path *path, struct kstat *stat,
> u32 request_mask, unsigned int flags)
> @@ -1024,8 +1016,8 @@ static int ecryptfs_getattr(struct mnt_idmap *idmap,
> struct kstat lower_stat;
> int rc;
>
> - rc = ecryptfs_do_getattr(ecryptfs_dentry_to_lower_path(dentry),
> - &lower_stat, request_mask, flags);
> + rc = vfs_getattr_nosec(ecryptfs_dentry_to_lower_path(dentry),
> + &lower_stat, request_mask, flags);
> if (!rc) {
> fsstack_copy_attr_all(d_inode(dentry),
> ecryptfs_inode_to_lower(d_inode(dentry)));
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 35fd3e3e1778..8b31f44c12cd 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -170,7 +170,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>
> type = ovl_path_real(dentry, &realpath);
> old_cred = ovl_override_creds(dentry->d_sb);
> - err = ovl_do_getattr(&realpath, stat, request_mask, flags);
> + err = vfs_getattr_nosec(&realpath, stat, request_mask, flags);
> if (err)
> goto out;
>
> @@ -195,8 +195,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
> (!is_dir ? STATX_NLINK : 0);
>
> ovl_path_lower(dentry, &realpath);
> - err = ovl_do_getattr(&realpath, &lowerstat, lowermask,
> - flags);
> + err = vfs_getattr_nosec(&realpath, &lowerstat, lowermask,
> + flags);
> if (err)
> goto out;
>
> @@ -248,8 +248,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>
> ovl_path_lowerdata(dentry, &realpath);
> if (realpath.dentry) {
> - err = ovl_do_getattr(&realpath, &lowerdatastat,
> - lowermask, flags);
> + err = vfs_getattr_nosec(&realpath, &lowerdatastat,
> + lowermask, flags);
> if (err)
> goto out;
> } else {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 0bfe35da4b7b..910dbbb2bb7b 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -412,14 +412,6 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
> return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
> }
>
> -static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
> - u32 request_mask, unsigned int flags)
> -{
> - if (flags & AT_GETATTR_NOSEC)
> - return vfs_getattr_nosec(path, stat, request_mask, flags);
> - return vfs_getattr(path, stat, request_mask, flags);
> -}
> -
> /* util.c */
> int ovl_get_write_access(struct dentry *dentry);
> void ovl_put_write_access(struct dentry *dentry);
> diff --git a/fs/stat.c b/fs/stat.c
> index 41e598376d7e..cbc0fcd4fba3 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -165,7 +165,7 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> if (inode->i_op->getattr)
> return inode->i_op->getattr(idmap, path, stat,
> request_mask,
> - query_flags | AT_GETATTR_NOSEC);
> + query_flags);
>
> generic_fillattr(idmap, request_mask, inode, stat);
> return 0;
> @@ -198,9 +198,6 @@ int vfs_getattr(const struct path *path, struct kstat *stat,
> {
> int retval;
>
> - if (WARN_ON_ONCE(query_flags & AT_GETATTR_NOSEC))
> - return -EPERM;
> -
> retval = security_inode_getattr(path);
> if (retval)
> return retval;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 87e2dec79fea..a40833bf2855 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -154,8 +154,4 @@
> usable with open_by_handle_at(2). */
> #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
>
> -#if defined(__KERNEL__)
> -#define AT_GETATTR_NOSEC 0x80000000
> -#endif
> -
> #endif /* _UAPI_LINUX_FCNTL_H */
> --
> 2.47.0
>
