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 <[email protected]>
---
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
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 <[email protected]>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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;