On Wed, Feb 5, 2025 at 6:26 AM Theodore Ts'o <tytso@xxxxxxx> wrote: > > On Tue, Feb 04, 2025 at 10:25:29PM +0100, Mateusz Guzik wrote: > > > > > > My question is if that's legitimate, I'm guessing not. If not, then > > > ext4 should complain about it. > > > > > > On stock kernel this happens to work because strlen finds the "right" size. > > > > > > > So it occurred to me to check what fsck thinks about it. > > > > I ran it twice in a row, it *removed* the problematic symlink. > > Can you show me what's in the problematic symlink? And does the > syzbot reproducer trigger a problem before adding your symlink > caching? > > What would be really great if you couldcreate small focused test case > that shows what's going on --- ideally something like a 100k file > system, ala the file systems in the tests directory of the e2fsprogs > sources.... > Everything is in the first e-mail I sent you, albeit a little spread out. Corrupted image: https://storage.googleapis.com/syzbot-assets/7c2919610764/mount_0.gz The bogus link is under file0/file1 and readlinks to /tmp/syz-imagegen43743633/file0/file0. ext4 sets i_size to 131109, while strlen on the thing is 37 The problem happens to not reproduce with this testcase because of the nul terminator in the corrupted symlink. Because of it the kernel prior to my change only attempts to copy the 37 bytes. Suppose the corrupted image got massaged to *NOT* have a nul terminator in that symlink. Then the kernel-side ext4 code without my change would still only nul terminate So this: nd_terminate_link(ei->i_data, inode->i_size, sizeof(ei->i_data) - 1); Clamps it to whichever is lower -- inode->i_size or sizeof(ei->i_data) - 1. The call added by my patch uses inode->i_size unconditionally and trips over, so one could argue this is a bug on my end: inode_set_cached_link(inode, (char *)ei->i_data, inode->i_size); It definitely fixes itself if one strlen()s and that would respect the termination, I'm going to send a patch to that extent later. However, that aside, there is definitely something going wrong with the symlink to begin with (size vs actual size disparity) and the fs should most likely reject it in the first place. So for this particular case I argue my bug only manifested itself because of the prior bug of ext4 accepting this link. -- Mateusz Guzik <mjguzik gmail.com>
