On Sun, 2024-11-17 at 21:44 +0300, Fedor Pchelkin wrote:
> The symlink body (->target) should be freed at the same time as the inode
> itself per commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
> traversal"). It is a filesystem-specific field but there exist several
> error paths during generic inode allocation when ->free_inode(), namely
> jffs2_free_inode(), is called with still uninitialized private info.
>
> The calltrace looks like:
> alloc_inode
> inode_init_always // fails
> i_callback
> free_inode
> jffs2_free_inode // touches uninit ->target field
>
> Commit af9a8730ddb6 ("jffs2: Fix potential illegal address access in
> jffs2_free_inode") approached the observed problem but fixed it only
> partially. Our local Syzkaller instance is still hitting these kinds of
> failures.
>
> The thing is that jffs2_i_init_once(), where the initialization of
> f->target has been moved, is called once per slab allocation so it won't
> be called for the object structure possibly retrieved later from the slab
> cache for reuse.
>
> The practice followed by many other filesystems is to initialize
> filesystem-private inode contents in the corresponding ->alloc_inode()
> callbacks. This also allows to drop initialization from jffs2_iget() and
> jffs2_new_inode() as ->alloc_inode() is called in those places.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 4fdcfab5b553 ("jffs2: fix use-after-free on symlink traversal")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> ---
> fs/jffs2/fs.c | 2 --
> fs/jffs2/super.c | 3 ++-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index d175cccb7c55..85c4b273918f 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -271,7 +271,6 @@ struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
> f = JFFS2_INODE_INFO(inode);
> c = JFFS2_SB_INFO(inode->i_sb);
>
> - jffs2_init_inode_info(f);
> mutex_lock(&f->sem);
>
> ret = jffs2_do_read_inode(c, f, inode->i_ino, &latest_node);
> @@ -439,7 +438,6 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
> return ERR_PTR(-ENOMEM);
>
> f = JFFS2_INODE_INFO(inode);
> - jffs2_init_inode_info(f);
> mutex_lock(&f->sem);
>
> memset(ri, 0, sizeof(*ri));
> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
> index 4545f885c41e..b56ff63357f3 100644
> --- a/fs/jffs2/super.c
> +++ b/fs/jffs2/super.c
> @@ -42,6 +42,8 @@ static struct inode *jffs2_alloc_inode(struct super_block *sb)
> f = alloc_inode_sb(sb, jffs2_inode_cachep, GFP_KERNEL);
> if (!f)
> return NULL;
> +
> + jffs2_init_inode_info(f);
> return &f->vfs_inode;
> }
>
> @@ -58,7 +60,6 @@ static void jffs2_i_init_once(void *foo)
> struct jffs2_inode_info *f = foo;
>
> mutex_init(&f->sem);
> - f->target = NULL;
> inode_init_once(&f->vfs_inode);
> }
>
Has this one been lost?
