ext4: wipe moved dirents with their real length
PATCH 1/1 ext4: wipe moved dirents with their real length
After copying a dirent to the new block, dx_move_dirents() wipes the source entry using the on-disk rec_len. Use the length already computed for the copy instead because the on-disk rec_len is untrusted and a corrupt value makes the memset() run out of bounds.
--- fs/ext4/namei.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
fs/ext4/namei.c +2 -4
@@ -1860,10 +1860,8 @@ dx_move_dirents(struct inode *dir, char *from, char *to,/* wipe dir_entry excluding the rec_len field */ de->inode =0 ; -memset (&de->name_len,0 ,ext4_rec_len_from_disk (de->rec_len, - blocksize) - - offsetof(struct ext4_dir_entry_2 , - name_len)); +memset (&de->name_len,0 , rec_len - + offsetof(struct ext4_dir_entry_2 , name_len)); map++; to += rec_len;
Signature · 1 line
2.51.0
[email protected]
AI reviewSashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
After copying a dirent to the new block,
dx_move_dirents()wipes the source entry using the on-diskrec_len. Use the length already computed for the copy instead because the on-diskrec_lenis untrusted and a corrupt value makes thememset()run out of bounds.Reported-by [email protected]Tested-by [email protected]Signed-off-by Adriano Cordova <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Jan Kara
reviewerQuoted 9 lines · Adriano Cordova
After copying a dirent to the new block,
dx_move_dirents()wipes the source entry using the on-diskrec_len. Use the length already computed for the copy instead because the on-diskrec_lenis untrusted and a corrupt value makes thememset()run out of bounds.Reported-by [email protected]Tested-by [email protected]Signed-off-by Adriano Cordova <[email protected]>
Not that this would be wrong per se but the explanation doesn't really satisfy me :) If the rec_len on disk is wrong, we have a larger problem with corrupted source directory and we shouldn't have reached dx_move_dirents() in the first place. So how come directory entry validation in dx_make_map() didn't catch the corruption? Did syzkaller somehow managed to corrupt the directory entry between dx_make_map() and dx_move_dirents()? If yes, that is the bug that needs fixing, not trying to suck up corrupted directory entries in dx_move_dirents()...
Honza
Quoted 24 lines
--- fs/ext4/namei.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)fs/ext4/namei.c +2
-4@@ -1860,10 +1860,8 @@ dx_move_dirents(struct inode *dir, char *from, char *to,/* wipe dir_entry excluding the rec_len field */ de->inode =0 ; -memset (&de->name_len,0 ,ext4_rec_len_from_disk (de->rec_len, - blocksize) - - offsetof(struct ext4_dir_entry_2 , - name_len)); +memset (&de->name_len,0 , rec_len - + offsetof(struct ext4_dir_entry_2 , name_len)); map++; to += rec_len;Signature · 1 line
2.51.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 42 lines · Jan Kara
Quoted 9 lines · Adriano Cordova
After copying a dirent to the new block,
dx_move_dirents()wipes the source entry using the on-diskrec_len. Use the length already computed for the copy instead because the on-diskrec_lenis untrusted and a corrupt value makes thememset()run out of bounds.Reported-by [email protected]Tested-by [email protected]Signed-off-by Adriano Cordova <[email protected]>Not that this would be wrong per se but the explanation doesn't really satisfy me :) If the
rec_lenon disk is wrong, we have a larger problem with corrupted source directory and we shouldn't have reacheddx_move_dirents()in the first place. So how come directory entry validation indx_make_map()didn't catch the corruption? Did syzkaller somehow managed to corrupt the directory entry betweendx_make_map()anddx_move_dirents()? If yes, that is the bug that needs fixing, not trying to suck up corrupted directory entries indx_move_dirents()...Honza
Quoted 19 lines
--- fs/ext4/namei.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)fs/ext4/namei.c +2
-4@@ -1860,10 +1860,8 @@ dx_move_dirents(struct inode *dir, char *from, char *to,/* wipe dir_entry excluding the rec_len field */ de->inode =0 ; -memset (&de->name_len,0 ,ext4_rec_len_from_disk (de->rec_len, - blocksize) - - offsetof(struct ext4_dir_entry_2 , - name_len)); +memset (&de->name_len,0 , rec_len - + offsetof(struct ext4_dir_entry_2 , name_len));
Quoted 8 lines
Quoted 5 lines
fs/ext4/namei.c +0
-1map++; to += rec_len; --2.51.0
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hi Jan,
Right. The syzbot C repro works by corrupting the block bitmap of the in-disk filesystem, so it
has nothing to do with dirents. I will send a v2 with a proposed sanity check.
Adriano