Re: [PATCH 1/2] jffs2: initialize filesystem-private inode info in ->alloc_inode callback

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 2024/11/18 2:44, Fedor Pchelkin 写道:
> 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@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>   fs/jffs2/fs.c    | 2 --
>   fs/jffs2/super.c | 3 ++-
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> 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);
>   }
>   
>