Re: [syzbot] [integrity] [overlayfs] general protection fault in d_path

From: Stefan Berger
Date: Fri Sep 29 2023 - 08:40:07 EST



On 9/29/23 00:25, Amir Goldstein wrote:
On Fri, Sep 29, 2023 at 3:02 AM Stefan Berger <stefanb@xxxxxxxxxxxxx> 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
  */