Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode
@ 2020-01-06  8:04 zhangyi (F)
  2020-01-20 11:44 ` zhangyi (F)
  2020-07-23  9:58 ` Hou Tao
  0 siblings, 2 replies; 4+ messages in thread
From: zhangyi (F) @ 2020-01-06  8:04 UTC (permalink / raw)
  To: viro
  Cc: zhongguohua1, daniel, yi.zhang, yihuaijie, ast, linux-kernel,
	linux-mtd, linux-fsdevel, chenjie6

After commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
traversal"), it expose a freeing uninitialized memory problem due to
this commit move the operaion of freeing f->target to
jffs2_i_callback(), which may not be initialized in some error path of
allocating jffs2 inode (eg: jffs2_iget()->iget_locked()->
destroy_inode()->..->jffs2_i_callback()->kfree(f->target)).

Fix this by initialize the jffs2_inode_info just after allocating it.

Reported-by: Guohua Zhong <zhongguohua1@huawei.com>
Reported-by: Huaijie Yi <yihuaijie@huawei.com>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Cc: stable@vger.kernel.org
---
 fs/jffs2/fs.c    | 2 --
 fs/jffs2/super.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index ab8cdd9e9325..50a9df7d43a5 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -270,7 +270,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);
@@ -438,7 +437,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 0e6406c4f362..90373898587f 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 = kmem_cache_alloc(jffs2_inode_cachep, GFP_KERNEL);
 	if (!f)
 		return NULL;
+
+	jffs2_init_inode_info(f);
 	return &f->vfs_inode;
 }
 
-- 
2.17.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode
  2020-01-06  8:04 [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode zhangyi (F)
@ 2020-01-20 11:44 ` zhangyi (F)
  2020-07-23  3:17   ` Jubin Zhong
  2020-07-23  9:58 ` Hou Tao
  1 sibling, 1 reply; 4+ messages in thread
From: zhangyi (F) @ 2020-01-20 11:44 UTC (permalink / raw)
  To: viro
  Cc: zhongguohua1, daniel, yihuaijie, ast, linux-kernel, linux-mtd,
	linux-fsdevel, chenjie6

ping.

On 2020/1/6 16:04, zhangyi (F) wrote:
> After commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
> traversal"), it expose a freeing uninitialized memory problem due to
> this commit move the operaion of freeing f->target to
> jffs2_i_callback(), which may not be initialized in some error path of
> allocating jffs2 inode (eg: jffs2_iget()->iget_locked()->
> destroy_inode()->..->jffs2_i_callback()->kfree(f->target)).
> 
> Fix this by initialize the jffs2_inode_info just after allocating it.
> 
> Reported-by: Guohua Zhong <zhongguohua1@huawei.com>
> Reported-by: Huaijie Yi <yihuaijie@huawei.com>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/jffs2/fs.c    | 2 --
>  fs/jffs2/super.c | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index ab8cdd9e9325..50a9df7d43a5 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -270,7 +270,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);
> @@ -438,7 +437,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 0e6406c4f362..90373898587f 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 = kmem_cache_alloc(jffs2_inode_cachep, GFP_KERNEL);
>  	if (!f)
>  		return NULL;
> +
> +	jffs2_init_inode_info(f);
>  	return &f->vfs_inode;
>  }
>  
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode
  2020-01-20 11:44 ` zhangyi (F)
@ 2020-07-23  3:17   ` Jubin Zhong
  0 siblings, 0 replies; 4+ messages in thread
From: Jubin Zhong @ 2020-07-23  3:17 UTC (permalink / raw)
  To: yi.zhang
  Cc: zhongguohua1, daniel, linux-kernel, ast, linux-mtd, viro,
	wangfangpeng1, linux-fsdevel, chenjie6

On 2020/1/6 16:04, zhangyi (F) wrote:
> After commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
> traversal"), it expose a freeing uninitialized memory problem due to
> this commit move the operaion of freeing f->target to
> jffs2_i_callback(), which may not be initialized in some error path of
> allocating jffs2 inode (eg: jffs2_iget()->iget_locked()->
> destroy_inode()->..->jffs2_i_callback()->kfree(f->target)).
> 
> Fix this by initialize the jffs2_inode_info just after allocating it.

We are having the same problem. After commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
> traversal"), f->target is freed before it is initialized in the iget_locked() path. This is dangerous and may trigger slub BUG_ON:

kernel BUG at mm/slub.c:3824!
Internal error: Oops - BUG: 0 [#1] SMP ARM
CPU: 2 PID: 9 Comm: rcuos/0 Tainted: P           O    4.4.185 #1
task: cf4a3f68 task.stack: cf4ca000
PC is at kfree+0xfc/0x264
LR is at jffs2_i_callback+0x10/0x28 [jffs2]
pc : [<c032a4f0>]    lr : [<bf0ab188>]    psr: 400e0213
sp : cf4cbec8  ip : 00000000  fp : c0273df8
r10: ceb12848  r9 : 0000000c  r8 : cdd52000
r7 : bf0ab188  r6 : 0000000c  r5 : e7fddef0  r4 : c1121ba0
r3 : 00000100  r2 : c0ac4010  r1 : 00000002  r0 : e7fddef0
Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 32c5387d  Table: 0e315940  DAC: 55555555
Process rcuos/0 (pid: 9, stack limit = 0xcf4ca190)
Stack: (0xcf4cbec8 to 0xcf4cc000)
bec0:                   c086efa8 c032a3a8 00000001 c0273e9c c0a29214 c3931db8
bee0: 00000000 0000000c ffffe000 cdd52000 0000000c ceb12848 c0273df8 bf0ab188
bf00: c0adf980 c0273e9c c0adf980 00000001 00000000 ffffff7c 00000000 cf4a3f68
bf20: c025bc18 cf4cbf24 cf4cbf24 c0a22448 c0adf980 cf4ca000 cf485ac0 00000000
bf40: c0adf980 c02739b0 00000000 00000000 00000000 c02380bc 00000000 c0adf380
bf60: c0adf980 00000000 00000000 00000000 00008001 cf4cbf74 cf4cbf74 00000000
bf80: 00000000 00000000 00008001 cf4cbf8c cf4cbf8c c0a22448 cf485ac0 c0237fb8
bfa0: 00000000 00000000 00000000 c0202db4 00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c032a4f0>] (kfree) from [<bf0ab188>] (jffs2_i_callback+0x10/0x28 [jffs2])
[<bf0ab188>] (jffs2_i_callback [jffs2]) from [<c0273e9c>] (rcu_nocb_kthread+0x4ec/0x504)
[<c0273e9c>] (rcu_nocb_kthread) from [<c02380bc>] (kthread+0x104/0x118)
[<c02380bc>] (kthread) from [<c0202db4>] (ret_from_fork+0x14/0x20)
Code: 0300001a 143094e5 010013e3 0000001a (f201f0e7)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode
  2020-01-06  8:04 [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode zhangyi (F)
  2020-01-20 11:44 ` zhangyi (F)
@ 2020-07-23  9:58 ` Hou Tao
  1 sibling, 0 replies; 4+ messages in thread
From: Hou Tao @ 2020-07-23  9:58 UTC (permalink / raw)
  To: zhangyi (F), viro, linux-mtd, David Woodhouse, Richard Weinberger
  Cc: zhongguohua1, daniel, yihuaijie, ast, linux-kernel,
	linux-fsdevel, chenjie6

Hi,

Cc +Richard +David

On 2020/1/6 16:04, zhangyi (F) wrote:
> After commit 4fdcfab5b553 ("jffs2: fix use-after-free on symlink
> traversal"), it expose a freeing uninitialized memory problem due to
> this commit move the operaion of freeing f->target to
> jffs2_i_callback(), which may not be initialized in some error path of
> allocating jffs2 inode (eg: jffs2_iget()->iget_locked()->
> destroy_inode()->..->jffs2_i_callback()->kfree(f->target)).
> 
Could you please elaborate the scenario in which the use of a uninitialized
f->target is possible ? IMO one case is that there are concurrent
jffs2_lookup() and jffs2 GC on an evicted inode, and two new inodes
are created, and then one needless inode is destroyed.

> Fix this by initialize the jffs2_inode_info just after allocating it.
> 
> Reported-by: Guohua Zhong <zhongguohua1@huawei.com>
> Reported-by: Huaijie Yi <yihuaijie@huawei.com>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Cc: stable@vger.kernel.org
> ---
A Fixes tag is also needed here.

>  fs/jffs2/fs.c    | 2 --
>  fs/jffs2/super.c | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index ab8cdd9e9325..50a9df7d43a5 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -270,7 +270,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);
> @@ -438,7 +437,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 0e6406c4f362..90373898587f 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 = kmem_cache_alloc(jffs2_inode_cachep, GFP_KERNEL);
>  	if (!f)
>  		return NULL;
> +
> +	jffs2_init_inode_info(f);
>  	return &f->vfs_inode;
>  }
>  
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-06  8:04 [PATCH] jffs2: move jffs2_init_inode_info() just after allocating inode zhangyi (F)
2020-01-20 11:44 ` zhangyi (F)
2020-07-23  3:17   ` Jubin Zhong
2020-07-23  9:58 ` Hou Tao

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git