All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare
@ 2022-04-14  2:52 Ye Bin
  2022-04-14  9:22 ` Jan Kara
  2022-05-13 21:13 ` Theodore Ts'o
  0 siblings, 2 replies; 4+ messages in thread
From: Ye Bin @ 2022-04-14  2:52 UTC (permalink / raw)
  To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, lczerner, Ye Bin

We got issue as follows:
EXT4-fs (loop0): mounted filesystem without journal. Opts: ,errors=continue
ext4_get_first_dir_block: bh->b_data=0xffff88810bee6000 len=34478
ext4_get_first_dir_block: *parent_de=0xffff88810beee6ae bh->b_data=0xffff88810bee6000
ext4_rename_dir_prepare: [1] parent_de=0xffff88810beee6ae
==================================================================
BUG: KASAN: use-after-free in ext4_rename_dir_prepare+0x152/0x220
Read of size 4 at addr ffff88810beee6ae by task rep/1895

CPU: 13 PID: 1895 Comm: rep Not tainted 5.10.0+ #241
Call Trace:
 dump_stack+0xbe/0xf9
 print_address_description.constprop.0+0x1e/0x220
 kasan_report.cold+0x37/0x7f
 ext4_rename_dir_prepare+0x152/0x220
 ext4_rename+0xf44/0x1ad0
 ext4_rename2+0x11c/0x170
 vfs_rename+0xa84/0x1440
 do_renameat2+0x683/0x8f0
 __x64_sys_renameat+0x53/0x60
 do_syscall_64+0x33/0x40
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f45a6fc41c9
RSP: 002b:00007ffc5a470218 EFLAGS: 00000246 ORIG_RAX: 0000000000000108
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f45a6fc41c9
RDX: 0000000000000005 RSI: 0000000020000180 RDI: 0000000000000005
RBP: 00007ffc5a470240 R08: 00007ffc5a470160 R09: 0000000020000080
R10: 00000000200001c0 R11: 0000000000000246 R12: 0000000000400bb0
R13: 00007ffc5a470320 R14: 0000000000000000 R15: 0000000000000000

The buggy address belongs to the page:
page:00000000440015ce refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x10beee
flags: 0x200000000000000()
raw: 0200000000000000 ffffea00043ff4c8 ffffea0004325608 0000000000000000
raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88810beee580: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ffff88810beee600: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>ffff88810beee680: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
                                  ^
 ffff88810beee700: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ffff88810beee780: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================
Disabling lock debugging due to kernel taint
ext4_rename_dir_prepare: [2] parent_de->inode=3537895424
ext4_rename_dir_prepare: [3] dir=0xffff888124170140
ext4_rename_dir_prepare: [4] ino=2
ext4_rename_dir_prepare: ent->dir->i_ino=2 parent=-757071872

Reason is first directory entry which 'rec_len' is 34478, then will get illegal
parent entry. Now, we do not check directory entry after read directory block
in 'ext4_get_first_dir_block'.
To solve this issue, check directory entry in 'ext4_get_first_dir_block'.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/namei.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e37da8d5cd0c..2f78544b1d47 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3455,6 +3455,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 	struct buffer_head *bh;
 
 	if (!ext4_has_inline_data(inode)) {
+		struct ext4_dir_entry_2 *de;
+		unsigned int offset;
+
 		/* The first directory block must not be a hole, so
 		 * treat it as DIRENT_HTREE
 		 */
@@ -3463,9 +3466,28 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 			*retval = PTR_ERR(bh);
 			return NULL;
 		}
-		*parent_de = ext4_next_entry(
-					(struct ext4_dir_entry_2 *)bh->b_data,
-					inode->i_sb->s_blocksize);
+
+		de = (struct ext4_dir_entry_2 *) bh->b_data;
+		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
+					 bh->b_size, 0) ||
+		    le32_to_cpu(de->inode) != inode->i_ino ||
+		    strcmp(".", de->name)) {
+			ext4_warning_inode(inode, "directory missing '.'");
+			brelse(bh);
+			return NULL;
+		}
+		offset = ext4_rec_len_from_disk(de->rec_len,
+						inode->i_sb->s_blocksize);
+		de = ext4_next_entry(de, inode->i_sb->s_blocksize);
+		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
+					 bh->b_size, offset) ||
+		    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
+			ext4_warning_inode(inode, "directory missing '..'");
+			brelse(bh);
+			return NULL;
+		}
+		*parent_de = de;
+
 		return bh;
 	}
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare
  2022-04-14  2:52 [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare Ye Bin
@ 2022-04-14  9:22 ` Jan Kara
  2022-05-13 21:13 ` Theodore Ts'o
  1 sibling, 0 replies; 4+ messages in thread
From: Jan Kara @ 2022-04-14  9:22 UTC (permalink / raw)
  To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack, lczerner

On Thu 14-04-22 10:52:23, Ye Bin wrote:
> We got issue as follows:
> EXT4-fs (loop0): mounted filesystem without journal. Opts: ,errors=continue
> ext4_get_first_dir_block: bh->b_data=0xffff88810bee6000 len=34478
> ext4_get_first_dir_block: *parent_de=0xffff88810beee6ae bh->b_data=0xffff88810bee6000
> ext4_rename_dir_prepare: [1] parent_de=0xffff88810beee6ae
> ==================================================================
> BUG: KASAN: use-after-free in ext4_rename_dir_prepare+0x152/0x220
> Read of size 4 at addr ffff88810beee6ae by task rep/1895
> 
> CPU: 13 PID: 1895 Comm: rep Not tainted 5.10.0+ #241
> Call Trace:
>  dump_stack+0xbe/0xf9
>  print_address_description.constprop.0+0x1e/0x220
>  kasan_report.cold+0x37/0x7f
>  ext4_rename_dir_prepare+0x152/0x220
>  ext4_rename+0xf44/0x1ad0
>  ext4_rename2+0x11c/0x170
>  vfs_rename+0xa84/0x1440
>  do_renameat2+0x683/0x8f0
>  __x64_sys_renameat+0x53/0x60
>  do_syscall_64+0x33/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f45a6fc41c9
> RSP: 002b:00007ffc5a470218 EFLAGS: 00000246 ORIG_RAX: 0000000000000108
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f45a6fc41c9
> RDX: 0000000000000005 RSI: 0000000020000180 RDI: 0000000000000005
> RBP: 00007ffc5a470240 R08: 00007ffc5a470160 R09: 0000000020000080
> R10: 00000000200001c0 R11: 0000000000000246 R12: 0000000000400bb0
> R13: 00007ffc5a470320 R14: 0000000000000000 R15: 0000000000000000
> 
> The buggy address belongs to the page:
> page:00000000440015ce refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x10beee
> flags: 0x200000000000000()
> raw: 0200000000000000 ffffea00043ff4c8 ffffea0004325608 0000000000000000
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  ffff88810beee580: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>  ffff88810beee600: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> >ffff88810beee680: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>                                   ^
>  ffff88810beee700: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>  ffff88810beee780: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ==================================================================
> Disabling lock debugging due to kernel taint
> ext4_rename_dir_prepare: [2] parent_de->inode=3537895424
> ext4_rename_dir_prepare: [3] dir=0xffff888124170140
> ext4_rename_dir_prepare: [4] ino=2
> ext4_rename_dir_prepare: ent->dir->i_ino=2 parent=-757071872
> 
> Reason is first directory entry which 'rec_len' is 34478, then will get illegal
> parent entry. Now, we do not check directory entry after read directory block
> in 'ext4_get_first_dir_block'.
> To solve this issue, check directory entry in 'ext4_get_first_dir_block'.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e37da8d5cd0c..2f78544b1d47 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3455,6 +3455,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  	struct buffer_head *bh;
>  
>  	if (!ext4_has_inline_data(inode)) {
> +		struct ext4_dir_entry_2 *de;
> +		unsigned int offset;
> +
>  		/* The first directory block must not be a hole, so
>  		 * treat it as DIRENT_HTREE
>  		 */
> @@ -3463,9 +3466,28 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  			*retval = PTR_ERR(bh);
>  			return NULL;
>  		}
> -		*parent_de = ext4_next_entry(
> -					(struct ext4_dir_entry_2 *)bh->b_data,
> -					inode->i_sb->s_blocksize);
> +
> +		de = (struct ext4_dir_entry_2 *) bh->b_data;
> +		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
> +					 bh->b_size, 0) ||
> +		    le32_to_cpu(de->inode) != inode->i_ino ||
> +		    strcmp(".", de->name)) {
> +			ext4_warning_inode(inode, "directory missing '.'");
> +			brelse(bh);
> +			return NULL;
> +		}
> +		offset = ext4_rec_len_from_disk(de->rec_len,
> +						inode->i_sb->s_blocksize);
> +		de = ext4_next_entry(de, inode->i_sb->s_blocksize);
> +		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
> +					 bh->b_size, offset) ||
> +		    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
> +			ext4_warning_inode(inode, "directory missing '..'");
> +			brelse(bh);
> +			return NULL;
> +		}
> +		*parent_de = de;
> +
>  		return bh;
>  	}
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare
  2022-04-14  2:52 [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare Ye Bin
  2022-04-14  9:22 ` Jan Kara
@ 2022-05-13 21:13 ` Theodore Ts'o
  2022-05-13 21:53   ` Theodore Ts'o
  1 sibling, 1 reply; 4+ messages in thread
From: Theodore Ts'o @ 2022-05-13 21:13 UTC (permalink / raw)
  To: Ye Bin; +Cc: adilger.kernel, linux-ext4, linux-kernel, jack, lczerner

On Thu, Apr 14, 2022 at 10:52:23AM +0800, Ye Bin wrote:
> We got issue as follows:
> EXT4-fs (loop0): mounted filesystem without journal. Opts: ,errors=continue
> ext4_get_first_dir_block: bh->b_data=0xffff88810bee6000 len=34478
> ext4_get_first_dir_block: *parent_de=0xffff88810beee6ae bh->b_data=0xffff88810bee6000
> ext4_rename_dir_prepare: [1] parent_de=0xffff88810beee6ae
> ==================================================================
> BUG: KASAN: use-after-free in ext4_rename_dir_prepare+0x152/0x220
> Read of size 4 at addr ffff88810beee6ae by task rep/1895
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e37da8d5cd0c..2f78544b1d47 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3463,9 +3466,28 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  			*retval = PTR_ERR(bh);
>  			return NULL;
>  		}
> -		*parent_de = ext4_next_entry(
> -					(struct ext4_dir_entry_2 *)bh->b_data,
> -					inode->i_sb->s_blocksize);
> +
> +		de = (struct ext4_dir_entry_2 *) bh->b_data;
> +		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
> +					 bh->b_size, 0) ||
> +		    le32_to_cpu(de->inode) != inode->i_ino ||
> +		    strcmp(".", de->name)) {
> +			ext4_warning_inode(inode, "directory missing '.'");

I think we should be calling ext4_error_inode()?  If the directory is
missing '.' or '..' below, the file system is corrupt, so we probably
should mark the file system as inconsistent, so that e2fsck can fix
the file system.

					- Ted

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare
  2022-05-13 21:13 ` Theodore Ts'o
@ 2022-05-13 21:53   ` Theodore Ts'o
  0 siblings, 0 replies; 4+ messages in thread
From: Theodore Ts'o @ 2022-05-13 21:53 UTC (permalink / raw)
  To: Ye Bin; +Cc: adilger.kernel, linux-ext4, linux-kernel, jack, lczerner

On Fri, May 13, 2022 at 05:13:47PM -0400, Theodore Ts'o wrote:
> 
> I think we should be calling ext4_error_inode()?  If the directory is
> missing '.' or '..' below, the file system is corrupt, so we probably
> should mark the file system as inconsistent, so that e2fsck can fix
> the file system.

I noticed one other problem; which is that by returning NULL and not
setting retval, ext4_rename_dir_prepare() will end up returning
uninitialized stack garbage as the "error code".

So I'm going to be applying this commit with the following change:

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 50be41dc5831..b202626391ff 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3471,8 +3471,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 					 bh->b_size, 0) ||
 		    le32_to_cpu(de->inode) != inode->i_ino ||
 		    strcmp(".", de->name)) {
-			ext4_warning_inode(inode, "directory missing '.'");
+			EXT4_ERROR_INODE(inode, "directory missing '.'");
 			brelse(bh);
+			*retval = -EFSCORRUPTED;
 			return NULL;
 		}
 		offset = ext4_rec_len_from_disk(de->rec_len,
@@ -3481,8 +3482,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
 					 bh->b_size, offset) ||
 		    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
-			ext4_warning_inode(inode, "directory missing '..'");
+			EXT4_ERROR_INODE(inode, "directory missing '..'");
 			brelse(bh);
+			*retval = -EFSCORRUPTED;
 			return NULL;
 		}
 		*parent_de = de;

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-13 21:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14  2:52 [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare Ye Bin
2022-04-14  9:22 ` Jan Kara
2022-05-13 21:13 ` Theodore Ts'o
2022-05-13 21:53   ` Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.