[PATCH 0/2] jffs2: fix a couple of uninit value errors

[PATCH 0/2] jffs2: fix a couple of uninit value errors

[Fedor Pchelkin]

There is a couple of places where uninit value is touched on error
handling paths in jffs2 code. Fix them.

Fedor Pchelkin (2):
  jffs2: initialize filesystem-private inode info in ->alloc_inode
    callback
  jffs2: initialize inocache earlier

 fs/jffs2/fs.c       | 2 --
 fs/jffs2/os-linux.h | 1 +
 fs/jffs2/super.c    | 3 ++-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.39.5

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

[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(-)

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);
 }
 
-- 
2.39.5

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

[Zhihao Cheng]

在 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);
>   }
>   
> 

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

[Joakim Tjernlund]

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@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(-)
> 
> 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?

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

[Joakim Tjernlund]

Ping MTD maintainers?

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@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(-)
> 
> 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);
>  }
>  

[PATCH 2/2] jffs2: initialize inocache earlier

[Fedor Pchelkin]

Inside jffs2_new_inode() there is a small gap when jffs2_init_acl_pre() or
jffs2_do_new_inode() may fail e.g. due to a memory allocation error while
uninit inocache field is touched upon subsequent inode eviction.

general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
CPU: 0 PID: 10592 Comm: syz-executor.1 Not tainted 5.10.209-syzkaller #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
RIP: 0010:jffs2_xattr_delete_inode+0x35/0x130 fs/jffs2/xattr.c:602
Call Trace:
 jffs2_do_clear_inode+0x4c/0x570 fs/jffs2/readinode.c:1418
 evict+0x281/0x6b0 fs/inode.c:577
 iput_final fs/inode.c:1697 [inline]
 iput.part.0+0x4df/0x6d0 fs/inode.c:1723
 iput+0x58/0x80 fs/inode.c:1713
 jffs2_new_inode+0xb12/0xdb0 fs/jffs2/fs.c:469
 jffs2_create+0x90/0x400 fs/jffs2/dir.c:177
 lookup_open.isra.0+0xead/0x1260 fs/namei.c:3169
 open_last_lookups fs/namei.c:3239 [inline]
 path_openat+0x96c/0x2670 fs/namei.c:3428
 do_filp_open+0x1a4/0x3f0 fs/namei.c:3458
 do_sys_openat2+0x171/0x420 fs/open.c:1186
 do_sys_open fs/open.c:1202 [inline]
 __do_sys_openat fs/open.c:1218 [inline]
 __se_sys_openat fs/open.c:1213 [inline]
 __x64_sys_openat+0x13c/0x1f0 fs/open.c:1213
 do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46

Initialize the inocache pointer to a NULL value while preparing an inode
in jffs2_init_inode_info(). jffs2_xattr_delete_inode() will handle it
later just fine.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 fs/jffs2/os-linux.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 86ab014a349c..39b6565f10c9 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -55,6 +55,7 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
 	f->metadata = NULL;
 	f->dents = NULL;
 	f->target = NULL;
+	f->inocache = NULL;
 	f->flags = 0;
 	f->usercompr = 0;
 }
-- 
2.39.5

Re: [PATCH 2/2] jffs2: initialize inocache earlier

[Zhihao Cheng]

在 2024/11/18 2:44, Fedor Pchelkin 写道:
> Inside jffs2_new_inode() there is a small gap when jffs2_init_acl_pre() or
> jffs2_do_new_inode() may fail e.g. due to a memory allocation error while
> uninit inocache field is touched upon subsequent inode eviction.
> 
> general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN NOPTI
> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
> CPU: 0 PID: 10592 Comm: syz-executor.1 Not tainted 5.10.209-syzkaller #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> RIP: 0010:jffs2_xattr_delete_inode+0x35/0x130 fs/jffs2/xattr.c:602
> Call Trace:
>   jffs2_do_clear_inode+0x4c/0x570 fs/jffs2/readinode.c:1418
>   evict+0x281/0x6b0 fs/inode.c:577
>   iput_final fs/inode.c:1697 [inline]
>   iput.part.0+0x4df/0x6d0 fs/inode.c:1723
>   iput+0x58/0x80 fs/inode.c:1713
>   jffs2_new_inode+0xb12/0xdb0 fs/jffs2/fs.c:469
>   jffs2_create+0x90/0x400 fs/jffs2/dir.c:177
>   lookup_open.isra.0+0xead/0x1260 fs/namei.c:3169
>   open_last_lookups fs/namei.c:3239 [inline]
>   path_openat+0x96c/0x2670 fs/namei.c:3428
>   do_filp_open+0x1a4/0x3f0 fs/namei.c:3458
>   do_sys_openat2+0x171/0x420 fs/open.c:1186
>   do_sys_open fs/open.c:1202 [inline]
>   __do_sys_openat fs/open.c:1218 [inline]
>   __se_sys_openat fs/open.c:1213 [inline]
>   __x64_sys_openat+0x13c/0x1f0 fs/open.c:1213
>   do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> 
> Initialize the inocache pointer to a NULL value while preparing an inode
> in jffs2_init_inode_info(). jffs2_xattr_delete_inode() will handle it
> later just fine.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>   fs/jffs2/os-linux.h | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 86ab014a349c..39b6565f10c9 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -55,6 +55,7 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
>   	f->metadata = NULL;
>   	f->dents = NULL;
>   	f->target = NULL;
> +	f->inocache = NULL;
>   	f->flags = 0;
>   	f->usercompr = 0;
>   }
>