linux-kernel - Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in vfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <gksdmj6duotvgrqrdf5omv5mlbvolw7jofsxqbnqvd2a5t7wpu@ukqah3y2dzqg>
Date: Thu, 6 Feb 2025 10:43:23 +0100
From: Jan Kara <jack@...e.cz>
To: Kees Cook <kees@...nel.org>
Cc: Jan Kara <jack@...e.cz>, 
	syzbot <syzbot+48a99e426f29859818c0@...kaller.appspotmail.com>, akpm@...ux-foundation.org, brauner@...nel.org, 
	gustavoars@...nel.org, linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, mjguzik@...il.com, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in vfs_readlink

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@...il.com>
> > > 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@...kaller.appspotmail.com
> > > 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@...e.com>
> > SUSE Labs, CR
> 
> > From df00b84402fb67d94a9eb6b86633092983cb388c Mon Sep 17 00:00:00 2001
> > From: Jan Kara <jack@...e.cz>
> > 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@...kaller.appspotmail.com
> > Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> > Suggested-by: "Darrick J. Wong" <djwong@...nel.org>
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> >  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@...e.com>
SUSE Labs, CR