Messages in this thread Patch in this message | | | Date | Fri, 29 Sep 2023 08:39:48 -0400 | Subject | Re: [syzbot] [integrity] [overlayfs] general protection fault in d_path | From | Stefan Berger <> |
| |
On 9/29/23 00:25, Amir Goldstein wrote: > On Fri, Sep 29, 2023 at 3:02 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> On 9/21/23 07:48, Christian Brauner wrote: >>> Imho, this is all very wild but I'm not judging. >>> >>> Two solutions imho: >>> (1) teach stacking filesystems like overlayfs and ecryptfs to use >>> vfs_getattr_nosec() in their ->getattr() implementation when they >>> are themselves called via vfs_getattr_nosec(). This will fix this by >>> not triggering another LSM hook. >> >> You can avoid all this churn. >> Just use the existing query_flags arg. >> Nothing outside the AT_STATX_SYNC_TYPE query_flags is >> passed into filesystems from userspace. >> >> Mast out AT_STATX_SYNC_TYPE in vfs_getattr() >> And allow kernel internal request_flags in vfs_getattr_nosec() Hm, I thought that vfs_getattr_nosec needs to pass AT_GETATTR_NOSEC into ->getattr(). >> >> The AT_ flag namespace is already a challenge, but mixing user >> flags and kernel-only flags in vfs interfaces has been done before. >> >> ...
That's what I wanted to avoid since now all filesystems' getattr() may have the AT_GETATTR_NOSEC mixed into the query_flags.
Anyway, here's what I currently have:
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 992d9c7e64ae..f7b5b1843dcc 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -998,16 +998,28 @@ static int ecryptfs_getattr_link(struct mnt_idmap *idmap, return rc; }
+static int ecryptfs_do_getattr(bool nosec, const struct path *path, + struct kstat *stat, u32 request_mask, + unsigned int flags) +{ + if (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) { struct dentry *dentry = path->dentry; struct kstat lower_stat; + bool nosec = flags & AT_GETATTR_NOSEC; int rc;
- rc = vfs_getattr(ecryptfs_dentry_to_lower_path(dentry), &lower_stat, - request_mask, flags); + flags &= ~AT_INTERNAL_MASK; + + rc = ecryptfs_do_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 83ef66644c21..ec4ceb5b4ebf 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -166,12 +166,15 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path, int fsid = 0; int err; bool metacopy_blocks = false; + bool nosec = flags & AT_GETATTR_NOSEC; + + flags &= ~AT_INTERNAL_MASK;
metacopy_blocks = ovl_is_metacopy_dentry(dentry);
type = ovl_path_real(dentry, &realpath); old_cred = ovl_override_creds(dentry->d_sb); - err = vfs_getattr(&realpath, stat, request_mask, flags); + err = ovl_do_getattr(nosec, &realpath, stat, request_mask, flags); if (err) goto out;
@@ -196,8 +199,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path, (!is_dir ? STATX_NLINK : 0);
ovl_path_lower(dentry, &realpath); - err = vfs_getattr(&realpath, &lowerstat, - lowermask, flags); + err = ovl_do_getattr(nosec, &realpath, &lowerstat, + lowermask, flags); if (err) goto out;
@@ -249,8 +252,9 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
ovl_path_lowerdata(dentry, &realpath); if (realpath.dentry) { - err = vfs_getattr(&realpath, &lowerdatastat, - lowermask, flags); + err = ovl_do_getattr(nosec, &realpath, + &lowerdatastat, lowermask, + flags); if (err) goto out; } else { diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 9817b2dcb132..cbee3ff3bab7 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -397,6 +397,15 @@ 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(bool nosec, const struct path *path, + struct kstat *stat, u32 request_mask, + unsigned int flags) +{ + if (nosec) + return vfs_getattr_nosec(path, stat, request_mask, flags); + return vfs_getattr(path, stat, request_mask, flags); +} + /* util.c */ int ovl_want_write(struct dentry *dentry); void ovl_drop_write(struct dentry *dentry); diff --git a/fs/stat.c b/fs/stat.c index d43a5cc1bfa4..3250e427e1aa 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -133,7 +133,8 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat, idmap = mnt_idmap(path->mnt); if (inode->i_op->getattr) return inode->i_op->getattr(idmap, path, stat, - request_mask, query_flags); + request_mask, + query_flags | AT_GETATTR_NOSEC);
generic_fillattr(idmap, request_mask, inode, stat); return 0; diff --git a/include/linux/fs.h b/include/linux/fs.h index b528f063e8ff..9069d6a301f0 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2027,6 +2027,12 @@ struct super_operations { void (*shutdown)(struct super_block *sb); };
+/* + * Internal query flags. See fcntl.h AT_xxx flags for the rest. + */ +#define AT_GETATTR_NOSEC 0x80000000 +#define AT_INTERNAL_MASK 0x80000000 + /* * Inode flags - they have no relation to superblock flags now */
| |