On Tue, Feb 4, 2025 at 4:58 PM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote: > > On Tue, Feb 4, 2025 at 4:30 PM Kees Cook <kees@xxxxxxxxxx> wrote: > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx> 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() > > > > > > > > > > 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? > > > > I don't follow. Are you claiming the cached symlinks can only be up to > 15 sizeof(i_data) in size? > > In the long above you can see lengths bigger than that. > > I had the vm still running, dmesg shows some more printks with lengths > above that: > [ 695.648078] __ext4_iget: inode ff18d9bed33c3c78 > [../Pacific/Pago_Pago] i_size 20 strlen 20 > [ 695.650647] __ext4_iget: inode ff18d9bed33c60f8 > [../America/Los_Angeles] i_size 22 strlen 22 > [ 695.653160] __ext4_iget: inode ff18d9bec8be8128 [../America/Denver] > i_size 17 strlen 17 > [ 695.656716] __ext4_iget: inode ff18d9bed325dc68 > [../America/Detroit] i_size 18 strlen 18 > [ 695.660450] __ext4_iget: inode ff18d9bed325ca28 > [../America/Indiana/Knox] i_size 23 strlen 23 > [ 695.664270] __ext4_iget: inode ff18d9bed325c108 > [../Pacific/Honolulu] i_size 19 strlen 19 > [ 695.666569] __ext4_iget: inode ff18d9bed325aec8 > [../America/Indiana/Indianapolis] i_size 31 strlen 31 > > Note with and without the patch there is copy_to_user from that area > taking place. > > Regardless, as you can see all the symlinks so far have i_size lining > up with strlen. > > The only case which does not is the (presumably intentionally > corrupted) fs image from syzbot. Sounds like the link should be > rejected by ext4 instead? > > Again I can do that strlen() call, but it seems like papering over the > problem for me. > I'm going to restate: the original behavior can be restored by replacing i_size usage with a strlen call. However, as is I have no basis to think that the disparity between the two is legitimate. If an ext4 person (Ted cc'ed) tells me the disparity can legally happen and is the expected way, I'm going to patch it up no problem. -- Mateusz Guzik <mjguzik gmail.com>
