On Wed 05-02-25 10:39:37, Kees Cook wrote: > On Wed, Feb 05, 2025 at 07:21:04PM +0100, Jan Kara wrote: > > On Tue 04-02-25 01:46:28, syzbot wrote: > > > Hello, > > > > > > syzbot found the following issue on: > > > > > > HEAD commit: 69b8923f5003 Merge tag 'for-linus-6.14-ofs4' of git://git... > > > git tree: upstream > > > console+strace: https://syzkaller.appspot.com/x/log.txt?x=1258aeb0580000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=57ab43c279fa614d > > > dashboard link: https://syzkaller.appspot.com/bug?extid=48a99e426f29859818c0 > > > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15825724580000 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1658aeb0580000 > > > > > > Downloadable assets: > > > disk image: https://storage.googleapis.com/syzbot-assets/ea84ac864e92/disk-69b8923f.raw.xz > > > vmlinux: https://storage.googleapis.com/syzbot-assets/6a465997b4e0/vmlinux-69b8923f.xz > > > kernel image: https://storage.googleapis.com/syzbot-assets/d72b67b2bd15/bzImage-69b8923f.xz > > > mounted in repro: https://storage.googleapis.com/syzbot-assets/7c2919610764/mount_0.gz > > > > > > The issue was bisected to: > > > > > > commit bae80473f7b0b25772619e7692019b1549d4a82c > > > Author: Mateusz Guzik <mjguzik@xxxxxxxxx> > > > Date: Wed Nov 20 11:20:35 2024 +0000 > > > > > > ext4: use inode_set_cached_link() > > > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1248c3df980000 > > > final oops: https://syzkaller.appspot.com/x/report.txt?x=1148c3df980000 > > > console output: https://syzkaller.appspot.com/x/log.txt?x=1648c3df980000 > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > > Reported-by: syzbot+48a99e426f29859818c0@xxxxxxxxxxxxxxxxxxxxxxxxx > > > Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()") > > > > Please check attached patch: > > > > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > Honza > > -- > > Jan Kara <jack@xxxxxxxx> > > SUSE Labs, CR > > > From df00b84402fb67d94a9eb6b86633092983cb388c Mon Sep 17 00:00:00 2001 > > From: Jan Kara <jack@xxxxxxx> > > Date: Wed, 5 Feb 2025 19:02:35 +0100 > > Subject: [PATCH] ext4: Verify fast symlink length > > > > Verify fast symlink length stored in inode->i_size matches the string > > stored in the inode to avoid surprises from corrupted filesystems. > > > > Reported-by: syzbot+48a99e426f29859818c0@xxxxxxxxxxxxxxxxxxxxxxxxx > > Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()") > > Suggested-by: "Darrick J. Wong" <djwong@xxxxxxxxxx> > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > --- > > fs/ext4/inode.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 7c54ae5fcbd4..fbda5a67f7f9 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, > > inode->i_op = &ext4_encrypted_symlink_inode_operations; > > } else if (ext4_inode_is_fast_symlink(inode)) { > > inode->i_op = &ext4_fast_symlink_inode_operations; > > - nd_terminate_link(ei->i_data, inode->i_size, > > - sizeof(ei->i_data) - 1); > > + if (inode->i_size == 0 || > > + inode->i_size >= EXT4_N_BLOCKS * 4 || > > This took me a while to understand. ei->i_data is u32[15]. It looks like > EXT4_N_BLOCKS is also 15. I feel like it would be much more readable to > have the above check be: > > inode->i_size >= sizeof(ei->i_data) || > > instead of using a literal "4" for sizeof(u32) as "4", and having a > EXT4_N_BLOCKS standing in for the literal "15" in i_data. > sizeof(ei->i_data) is precisely what is being checked, so why not use > it? Yeah, I've used the tradidional way ext4 uses for this (e.g. in ext4_inode_is_fast_symlink()) but I agree it is rather cryptic for those not intimately familiar with ext4 on-disk format. Switched to the sizeof() as you suggested. Thanks for review! Honza > > And while at it, the definition of i_data could be adjusted to to use > EXT4_N_BLOCKS, e.g.: > > struct ext4_inode_info { > __le32 i_data[EXT4_N_BLOCKS]; /* unconverted */ > > ? > > > + strnlen((char *)ei->i_data, inode->i_size + 1) != > > + inode->i_size) { > > + ext4_error_inode(inode, function, line, 0, > > + "invalid fast symlink length %llu", > > + (unsigned long long)inode->i_size); > > + ret = -EFSCORRUPTED; > > + goto bad_inode; > > + } > > But regardless, yes, the math checks out, and looks correct to me. > > > inode_set_cached_link(inode, (char *)ei->i_data, > > inode->i_size); > > } else { > > -- > > 2.43.0 > > > > > -Kees > > -- > Kees Cook -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
