| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <99294acf-7275-8f4d-a129-d5df208b7b2a@linux.ibm.com>
Date: Fri, 29 Sep 2023 08:39:48 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Christian Brauner <brauner@...nel.org>,
Mimi Zohar <zohar@...ux.ibm.com>,
Jeff Layton <jlayton@...nel.org>,
Casey Schaufler <casey@...aufler-ca.com>,
syzbot <syzbot+a67fc5321ffb4b311c98@...kaller.appspotmail.com>,
linux-fsdevel@...r.kernel.org, linux-integrity@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-unionfs@...r.kernel.org,
miklos@...redi.hu, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [integrity] [overlayfs] general protection fault in
d_path
On 9/29/23 00:25, Amir Goldstein wrote:
> On Fri, Sep 29, 2023 at 3:02 AM Stefan Berger <stefanb@...ux.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
*/
Powered by blists - more mailing lists