On Tue 02-07-24 21:23:48, libaokun@xxxxxxxxxxxxxxx wrote: > From: Baokun Li <libaokun1@xxxxxxxxxx> > > Syzbot reports a issue as follows: > ============================================ > BUG: unable to handle page fault for address: ffffed11022e24fe > PGD 23ffee067 P4D 23ffee067 PUD 0 > Oops: Oops: 0000 [#1] PREEMPT SMP KASAN PTI > CPU: 0 PID: 5079 Comm: syz-executor306 Not tainted 6.10.0-rc5-g55027e689933 #0 > Call Trace: > <TASK> > make_indexed_dir+0xdaf/0x13c0 fs/ext4/namei.c:2341 > ext4_add_entry+0x222a/0x25d0 fs/ext4/namei.c:2451 > ext4_rename fs/ext4/namei.c:3936 [inline] > ext4_rename2+0x26e5/0x4370 fs/ext4/namei.c:4214 > [...] > ============================================ > > The immediate cause of this problem is that there is only one valid dentry > for the block to be split during do_split, so split==0 results in out of > bounds accesses to the map triggering the issue. > > do_split > unsigned split > dx_make_map > count = 1 > split = count/2 = 0; > continued = hash2 == map[split - 1].hash; > ---> map[4294967295] > > The maximum length of a filename is 255 and the minimum block size is 1024, > so it is always guaranteed that the number of entries is greater than or > equal to 2 when do_split() is called. > > But syzbot's crafted image has no dot and dotdot in dir, and the dentry > distribution in dirblock is as follows: > > bus dentry1 hole dentry2 free > |xx--|xx-------------|...............|xx-------------|...............| > 0 12 (8+248)=256 268 256 524 (8+256)=264 788 236 1024 > > So when renaming dentry1 increases its name_len length by 1, neither hole > nor free is sufficient to hold the new dentry, and make_indexed_dir() is > called. > > In make_indexed_dir() it is assumed that the first two entries of the > dirblock must be dot and dotdot, so bus and dentry1 are left in dx_root > because they are treated as dot and dotdot, and only dentry2 is moved > to the new leaf block. That's why count is equal to 1. > > Therefore add the ext4_check_dx_root() helper function to add more sanity > checks to dot and dotdot before starting the conversion to avoid the above > issue. > > Reported-by: syzbot+ae688d469e36fb5138d0@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0 > Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3") > Cc: stable@xxxxxxxxxx > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx> Thanks! The patch looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> Honza > --- > fs/ext4/namei.c | 56 ++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 51 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index e6769b97a970..35881e3dd880 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2172,6 +2172,52 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname, > return err ? err : err2; > } > > +static bool ext4_check_dx_root(struct inode *dir, struct dx_root *root) > +{ > + struct fake_dirent *fde; > + const char *error_msg; > + unsigned int rlen; > + unsigned int blocksize = dir->i_sb->s_blocksize; > + char *blockend = (char *)root + dir->i_sb->s_blocksize; > + > + fde = &root->dot; > + if (unlikely(fde->name_len != 1)) { > + error_msg = "invalid name_len for '.'"; > + goto corrupted; > + } > + if (unlikely(strncmp(root->dot_name, ".", fde->name_len))) { > + error_msg = "invalid name for '.'"; > + goto corrupted; > + } > + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize); > + if (unlikely((char *)fde + rlen >= blockend)) { > + error_msg = "invalid rec_len for '.'"; > + goto corrupted; > + } > + > + fde = &root->dotdot; > + if (unlikely(fde->name_len != 2)) { > + error_msg = "invalid name_len for '..'"; > + goto corrupted; > + } > + if (unlikely(strncmp(root->dotdot_name, "..", fde->name_len))) { > + error_msg = "invalid name for '..'"; > + goto corrupted; > + } > + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize); > + if (unlikely((char *)fde + rlen >= blockend)) { > + error_msg = "invalid rec_len for '..'"; > + goto corrupted; > + } > + > + return true; > + > +corrupted: > + EXT4_ERROR_INODE(dir, "Corrupt dir, %s, running e2fsck is recommended", > + error_msg); > + return false; > +} > + > /* > * This converts a one block unindexed directory to a 3 block indexed > * directory, and adds the dentry to the indexed directory. > @@ -2206,17 +2252,17 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname, > brelse(bh); > return retval; > } > + > root = (struct dx_root *) bh->b_data; > + if (!ext4_check_dx_root(dir, root)) { > + brelse(bh); > + return -EFSCORRUPTED; > + } > > /* The 0th block becomes the root, move the dirents out */ > fde = &root->dotdot; > de = (struct ext4_dir_entry_2 *)((char *)fde + > ext4_rec_len_from_disk(fde->rec_len, blocksize)); > - if ((char *) de >= (((char *) root) + blocksize)) { > - EXT4_ERROR_INODE(dir, "invalid rec_len for '..'"); > - brelse(bh); > - return -EFSCORRUPTED; > - } > len = ((char *) root) + (blocksize - csum_size) - (char *) de; > > /* Allocate new block for the 0th block's dirents */ > -- > 2.39.2 > -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
