| Message ID | 20261002004127.1332310-1-adrianox@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: wipe moved dirents with their real length | expand |
> 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. > > Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd > Tested-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > Signed-off-by: Adriano Cordova <adrianox@gmail.com> Sashiko has reviewed this patch and found no issues. It looks great!
On Thu 01-10-26 21:41:27, Adriano Cordova wrote: > 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. > > Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd > Tested-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > Signed-off-by: Adriano Cordova <adrianox@gmail.com> 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 > --- > fs/ext4/namei.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index a6386c1d237f..71e9e7c9a7fd 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -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; > -- > 2.51.0 >
El vie, 2 oct 2026 a las 9:45, Jan Kara (<jack@suse.cz>) escribió: > > On Thu 01-10-26 21:41:27, Adriano Cordova wrote: > > 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. > > > > Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd > > Tested-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com > > Signed-off-by: Adriano Cordova <adrianox@gmail.com> > > 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 > > > --- > > fs/ext4/namei.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > index a6386c1d237f..71e9e7c9a7fd 100644 > > --- a/fs/ext4/namei.c > > +++ b/fs/ext4/namei.c > > @@ -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; > > -- > > 2.51.0 > > > -- > Jan Kara <jack@suse.com> > 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
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index a6386c1d237f..71e9e7c9a7fd 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -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;