| 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: <xc5jgiqsyaqxxsqgjsspmmxji5hzuhu24a57dunkwtjzyqj2ld@l4wyerk6gbdj>
Date: Mon, 7 Apr 2025 15:50:47 +0200
From: Jan Kara <jack@...e.cz>
To: Christian Brauner <brauner@...nel.org>
Cc: Christoph Hellwig <hch@...radead.org>,
Mateusz Guzik <mjguzik@...il.com>, Penglei Jiang <superman.xpt@...il.com>, viro@...iv.linux.org.uk,
jack@...e.cz, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
syzbot+5d8e79d323a13aa0b248@...kaller.appspotmail.com
Subject: Re: [PATCH] anon_inode: use a proper mode internally
On Fri 04-04-25 12:39:14, Christian Brauner wrote:
> This allows the VFS to not trip over anonymous inodes and we can add
> asserts based on the mode into the vfs. When we report it to userspace
> we can simply hide the mode to avoid regressions. I've audited all
> direct callers of alloc_anon_inode() and only secretmen overrides i_mode
> and i_op inode operations but it already uses a regular file.
>
> Fixes: af153bb63a336 ("vfs: catch invalid modes in may_open()")
> Reported-by: syzbot+5d8e79d323a13aa0b248@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/all/67ed3fb3.050a0220.14623d.0009.GAE@google.com"
> Signed-off-by: Christian Brauner <brauner@...nel.org>
It seems better to have anon inodes consistent with the rest of the kernel
so feel free to add:
Reviewed-by: Jan Kara <jack@...e.cz>
Honza
> ---
> fs/anon_inodes.c | 36 ++++++++++++++++++++++++++++++++++++
> fs/internal.h | 3 +++
> fs/libfs.c | 2 +-
> fs/pidfs.c | 24 +-----------------------
> 4 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 583ac81669c2..42e4b9c34f89 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -24,9 +24,43 @@
>
> #include <linux/uaccess.h>
>
> +#include "internal.h"
> +
> static struct vfsmount *anon_inode_mnt __ro_after_init;
> static struct inode *anon_inode_inode __ro_after_init;
>
> +/*
> + * User space expects anonymous inodes to have no file type in st_mode.
> + *
> + * In particular, 'lsof' has this legacy logic:
> + *
> + * type = s->st_mode & S_IFMT;
> + * switch (type) {
> + * ...
> + * case 0:
> + * if (!strcmp(p, "anon_inode"))
> + * Lf->ntype = Ntype = N_ANON_INODE;
> + *
> + * to detect our old anon_inode logic.
> + *
> + * Rather than mess with our internal sane inode data, just fix it
> + * up here in getattr() by masking off the format bits.
> + */
> +int anon_inode_getattr(struct mnt_idmap *idmap, const struct path *path,
> + struct kstat *stat, u32 request_mask,
> + unsigned int query_flags)
> +{
> + struct inode *inode = d_inode(path->dentry);
> +
> + generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> + stat->mode &= ~S_IFMT;
> + return 0;
> +}
> +
> +static const struct inode_operations anon_inode_operations = {
> + .getattr = anon_inode_getattr,
> +};
> +
> /*
> * anon_inodefs_dname() is called from d_path().
> */
> @@ -66,6 +100,7 @@ static struct inode *anon_inode_make_secure_inode(
> if (IS_ERR(inode))
> return inode;
> inode->i_flags &= ~S_PRIVATE;
> + inode->i_op = &anon_inode_operations;
> error = security_inode_init_security_anon(inode, &QSTR(name),
> context_inode);
> if (error) {
> @@ -313,6 +348,7 @@ static int __init anon_inode_init(void)
> anon_inode_inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> if (IS_ERR(anon_inode_inode))
> panic("anon_inode_init() inode allocation failed (%ld)\n", PTR_ERR(anon_inode_inode));
> + anon_inode_inode->i_op = &anon_inode_operations;
>
> return 0;
> }
> diff --git a/fs/internal.h b/fs/internal.h
> index b9b3e29a73fd..717dc9eb6185 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -343,3 +343,6 @@ static inline bool path_mounted(const struct path *path)
> void file_f_owner_release(struct file *file);
> bool file_seek_cur_needs_f_lock(struct file *file);
> int statmount_mnt_idmap(struct mnt_idmap *idmap, struct seq_file *seq, bool uid_map);
> +int anon_inode_getattr(struct mnt_idmap *idmap, const struct path *path,
> + struct kstat *stat, u32 request_mask,
> + unsigned int query_flags);
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 6393d7c49ee6..0ad3336f5b49 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1647,7 +1647,7 @@ struct inode *alloc_anon_inode(struct super_block *s)
> * that it already _is_ on the dirty list.
> */
> inode->i_state = I_DIRTY;
> - inode->i_mode = S_IRUSR | S_IWUSR;
> + inode->i_mode = S_IFREG | S_IRUSR | S_IWUSR;
> inode->i_uid = current_fsuid();
> inode->i_gid = current_fsgid();
> inode->i_flags |= S_PRIVATE;
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index d64a4cbeb0da..809c3393b6a3 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -572,33 +572,11 @@ static int pidfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> return -EOPNOTSUPP;
> }
>
> -
> -/*
> - * User space expects pidfs inodes to have no file type in st_mode.
> - *
> - * In particular, 'lsof' has this legacy logic:
> - *
> - * type = s->st_mode & S_IFMT;
> - * switch (type) {
> - * ...
> - * case 0:
> - * if (!strcmp(p, "anon_inode"))
> - * Lf->ntype = Ntype = N_ANON_INODE;
> - *
> - * to detect our old anon_inode logic.
> - *
> - * Rather than mess with our internal sane inode data, just fix it
> - * up here in getattr() by masking off the format bits.
> - */
> static int pidfs_getattr(struct mnt_idmap *idmap, const struct path *path,
> struct kstat *stat, u32 request_mask,
> unsigned int query_flags)
> {
> - struct inode *inode = d_inode(path->dentry);
> -
> - generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> - stat->mode &= ~S_IFMT;
> - return 0;
> + return anon_inode_getattr(idmap, path, stat, request_mask, query_flags);
> }
>
> static const struct inode_operations pidfs_inode_operations = {
> --
> 2.47.2
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists