Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in vfs_readlink

[Kees Cook <kees@kernel.org>]

On Tue, Feb 04, 2025 at 12:38:30PM +0100, Mateusz Guzik wrote:
> On Tue, Feb 4, 2025 at 10:46 AM syzbot
> <syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com> 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@gmail.com>
> > Date:   Wed Nov 20 11:20:35 2024 +0000
> >
> >     ext4: use inode_set_cached_link()
> >
> 
> This looks like a case of a deliberately corrupted image.
> 
> I added back a debug printk I used when writing the patch before. For
> correct cases it reports:
> 
> [   19.574861] __ext4_iget: inode ff18d9bec05977c8 [/etc/environment]
> i_size 16 strlen 16
> [   19.585987] __ext4_iget: inode ff18d9bed6f25c68
> [/etc/alternatives/awk] i_size 21 strlen 21
> [   19.590419] __ext4_iget: inode ff18d9bed6f24108 [/usr/bin/gawk]
> i_size 13 strlen 13
> [   19.592199] __ext4_iget: inode ff18d9bed6abeea8
> [libassuan.so.0.8.5] i_size 18 strlen 18
> [   19.593804] __ext4_iget: inode ff18d9bed6f29368
> [libsigsegv.so.2.0.7] i_size 19 strlen 19
> 
> etc.
> 
> However, the bogus case is:
> [   52.161476] __ext4_iget: inode ff18d9bed1cc2a38
> [/tmp/syz-imagegen43743633/file0/file0] i_size 131109
> strlen 37
> 
> That is i_size is out of sync with strlen.
> 
> Prior to my patch this happened to work because the copying was in
> fact issuing strlen to find the size every time.
> 
> Given this code in fs/ext4/inode.c:
>   5008                 } else if (ext4_inode_is_fast_symlink(inode)) {
>   5009                         inode->i_op = &ext4_fast_symlink_inode_operations;
>   5010                         nd_terminate_link(ei->i_data, inode->i_size,
>   5011                                 sizeof(ei->i_data) - 1);
> 
> To me this looks like a pre-existing bug in ext4 which just happened

This gets clamped to i_data size, though, so I don't see a bug. Is there
still a code path where i_size is getting used? It looks like the buffer
overflow was introduced with bae80473f7b0 ("ext4: use inode_set_cached_link()"),
more details below...

> to get exposed with:
> 
>   5012                         inode_set_cached_link(inode, (char *)ei->i_data,
>   5013                                               inode->i_size);

The sanity checker said:
> usercopy: Kernel memory exposure attempt detected from SLUB object 'ext4_inode_cache' (offset 0, size 176)!

The cache was created to make only the i_data field visible:

        ext4_inode_cachep = kmem_cache_create_usercopy("ext4_inode_cache",
                                sizeof(struct ext4_inode_info), 0,
                                SLAB_RECLAIM_ACCOUNT | SLAB_ACCOUNT,
                                offsetof(struct ext4_inode_info, i_data),
                                sizeof_field(struct ext4_inode_info, i_data),
                                init_once);

which is 15 bytes, at offset 0:

struct ext4_inode_info {
        __le32  i_data[15];     /* unconverted */
        __u32   i_dtime;

So, yes, this seems like a buffer overflow caught by usercopy, where the
bug was as described above. I don't think there was an existing flaw in
ext4, though?

> However, if the strlen thing is indeed the source of truth, the
> inode_set_cached_link call can be trivially patched to use it instead
> of i_size.

Agreed. Please CC me and I can review it. :)

-Kees

-- 
Kees Cook