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

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

[syzbot]

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()

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@syzkaller.appspotmail.com
Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")

EXT4-fs (loop0): mounted filesystem 00000000-0000-0000-0000-000000000000 r/w without journal. Quota mode: writeback.
usercopy: Kernel memory exposure attempt detected from SLUB object 'ext4_inode_cache' (offset 0, size 176)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
CPU: 1 UID: 0 PID: 5826 Comm: syz-executor349 Not tainted 6.13.0-syzkaller-09793-g69b8923f5003 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 12/27/2024
RIP: 0010:usercopy_abort+0x84/0x90 mm/usercopy.c:102
Code: 49 89 ce 48 c7 c3 80 5f 18 8c 48 0f 44 de 48 c7 c7 20 5e 18 8c 4c 89 de 48 89 c1 41 52 41 56 53 e8 d1 3a f5 fe 48 83 c4 18 90 <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
RSP: 0018:ffffc90003fa7c90 EFLAGS: 00010296
RAX: 000000000000006b RBX: ffffffff8c185f80 RCX: ba71ae4bd9cf0200
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffff819f1fbc R09: 1ffff920007f4f2c
R10: dffffc0000000000 R11: fffff520007f4f2d R12: ffffea0001e70200
R13: 00000000000000b0 R14: 0000000000000000 R15: 00000000000000b0
FS:  00005555602a7380(0000) GS:ffff8880b8700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000066c7e0 CR3: 0000000078aac000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __check_heap_object+0xb1/0x100 mm/slub.c:5818
 check_heap_object mm/usercopy.c:196 [inline]
 __check_object_size+0x1da/0x730 mm/usercopy.c:251
 check_object_size include/linux/thread_info.h:228 [inline]
 check_copy_size include/linux/thread_info.h:264 [inline]
 copy_to_user include/linux/uaccess.h:219 [inline]
 readlink_copy fs/namei.c:5284 [inline]
 vfs_readlink+0x1cf/0x550 fs/namei.c:5307
 do_readlinkat+0x249/0x3a0 fs/stat.c:576
 __do_sys_readlinkat fs/stat.c:593 [inline]
 __se_sys_readlinkat fs/stat.c:590 [inline]
 __x64_sys_readlinkat+0x9a/0xb0 fs/stat.c:590
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fdaffabc639
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffeb88c4948 EFLAGS: 00000246 ORIG_RAX: 000000000000010b
RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007fdaffabc639
RDX: 00000000200002c0 RSI: 0000000020000240 RDI: 00000000ffffff9c
RBP: 00007fdaffb30610 R08: 0000000000000000 R09: 0000000000000000
R10: 00000000000000b0 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffeb88c4b18 R14: 0000000000000001 R15: 0000000000000001
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:usercopy_abort+0x84/0x90 mm/usercopy.c:102
Code: 49 89 ce 48 c7 c3 80 5f 18 8c 48 0f 44 de 48 c7 c7 20 5e 18 8c 4c 89 de 48 89 c1 41 52 41 56 53 e8 d1 3a f5 fe 48 83 c4 18 90 <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
RSP: 0018:ffffc90003fa7c90 EFLAGS: 00010296
RAX: 000000000000006b RBX: ffffffff8c185f80 RCX: ba71ae4bd9cf0200
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffff819f1fbc R09: 1ffff920007f4f2c
R10: dffffc0000000000 R11: fffff520007f4f2d R12: ffffea0001e70200
R13: 00000000000000b0 R14: 0000000000000000 R15: 00000000000000b0
FS:  00005555602a7380(0000) GS:ffff8880b8700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055dd00353bf8 CR3: 0000000078aac000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

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

[Mateusz Guzik]

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
to get exposed with:

  5012                         inode_set_cached_link(inode, (char *)ei->i_data,
  5013                                               inode->i_size);


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.

> 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@syzkaller.appspotmail.com
> Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
>
> EXT4-fs (loop0): mounted filesystem 00000000-0000-0000-0000-000000000000 r/w without journal. Quota mode: writeback.
> usercopy: Kernel memory exposure attempt detected from SLUB object 'ext4_inode_cache' (offset 0, size 176)!
> ------------[ cut here ]------------
> kernel BUG at mm/usercopy.c:102!
> Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 1 UID: 0 PID: 5826 Comm: syz-executor349 Not tainted 6.13.0-syzkaller-09793-g69b8923f5003 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 12/27/2024
> RIP: 0010:usercopy_abort+0x84/0x90 mm/usercopy.c:102
> Code: 49 89 ce 48 c7 c3 80 5f 18 8c 48 0f 44 de 48 c7 c7 20 5e 18 8c 4c 89 de 48 89 c1 41 52 41 56 53 e8 d1 3a f5 fe 48 83 c4 18 90 <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
> RSP: 0018:ffffc90003fa7c90 EFLAGS: 00010296
> RAX: 000000000000006b RBX: ffffffff8c185f80 RCX: ba71ae4bd9cf0200
> RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
> RBP: 0000000000000000 R08: ffffffff819f1fbc R09: 1ffff920007f4f2c
> R10: dffffc0000000000 R11: fffff520007f4f2d R12: ffffea0001e70200
> R13: 00000000000000b0 R14: 0000000000000000 R15: 00000000000000b0
> FS:  00005555602a7380(0000) GS:ffff8880b8700000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000000066c7e0 CR3: 0000000078aac000 CR4: 00000000003526f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  __check_heap_object+0xb1/0x100 mm/slub.c:5818
>  check_heap_object mm/usercopy.c:196 [inline]
>  __check_object_size+0x1da/0x730 mm/usercopy.c:251
>  check_object_size include/linux/thread_info.h:228 [inline]
>  check_copy_size include/linux/thread_info.h:264 [inline]
>  copy_to_user include/linux/uaccess.h:219 [inline]
>  readlink_copy fs/namei.c:5284 [inline]
>  vfs_readlink+0x1cf/0x550 fs/namei.c:5307
>  do_readlinkat+0x249/0x3a0 fs/stat.c:576
>  __do_sys_readlinkat fs/stat.c:593 [inline]
>  __se_sys_readlinkat fs/stat.c:590 [inline]
>  __x64_sys_readlinkat+0x9a/0xb0 fs/stat.c:590
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7fdaffabc639
> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007ffeb88c4948 EFLAGS: 00000246 ORIG_RAX: 000000000000010b
> RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007fdaffabc639
> RDX: 00000000200002c0 RSI: 0000000020000240 RDI: 00000000ffffff9c
> RBP: 00007fdaffb30610 R08: 0000000000000000 R09: 0000000000000000
> R10: 00000000000000b0 R11: 0000000000000246 R12: 0000000000000001
> R13: 00007ffeb88c4b18 R14: 0000000000000001 R15: 0000000000000001
>  </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:usercopy_abort+0x84/0x90 mm/usercopy.c:102
> Code: 49 89 ce 48 c7 c3 80 5f 18 8c 48 0f 44 de 48 c7 c7 20 5e 18 8c 4c 89 de 48 89 c1 41 52 41 56 53 e8 d1 3a f5 fe 48 83 c4 18 90 <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
> RSP: 0018:ffffc90003fa7c90 EFLAGS: 00010296
> RAX: 000000000000006b RBX: ffffffff8c185f80 RCX: ba71ae4bd9cf0200
> RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
> RBP: 0000000000000000 R08: ffffffff819f1fbc R09: 1ffff920007f4f2c
> R10: dffffc0000000000 R11: fffff520007f4f2d R12: ffffea0001e70200
> R13: 00000000000000b0 R14: 0000000000000000 R15: 00000000000000b0
> FS:  00005555602a7380(0000) GS:ffff8880b8700000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055dd00353bf8 CR3: 0000000078aac000 CR4: 00000000003526f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
>
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
>
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
>
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
>
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
>
> If you want to undo deduplication, reply with:
> #syz undup



-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Kees Cook]

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

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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 4:30 PM Kees Cook <kees@kernel.org> 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@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?
>

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.

-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 4:58 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Tue, Feb 4, 2025 at 4:30 PM Kees Cook <kees@kernel.org> 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@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?
> >
>
> 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>

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

[Theodore Ts'o]

On Tue, Feb 04, 2025 at 05:49:48PM +0100, Mateusz Guzik wrote:
> 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.

There are two kinds of symlinks in ext4.  "Fast symlinks", where the
symlink target can fit in i_block[].  And normal, sometimes called
"slow" symlinks, where i_block[] contains a pointer to data block
(either i_blocks[0] for a traditional inode, or the root of an extent
tree that will fit in i_block[] if EXTENTS_FL is set).  In the case
of a fast symlink, the maximum size of the file system is
sizeof(i_block[])-1.  For a normal/slow symlink, the maximum size is
super->s_blocksize.

We determine whether a symlink is "fast" or not by checking whether
i_size is less than sizeof(i_blocks[]).  In other words. if i_size
less than 60 (and it's not an encrypted inode), it's a fast symlink
and we set i_link to point to the i_block array.  From __ext4_iget()
in fs/ext4/inode.c:

		if (IS_ENCRYPTED(inode)) {
			inode->i_op = &ext4_encrypted_symlink_inode_operations;
		} else if (ext4_inode_is_fast_symlink(inode)) {
			inode->i_link = (char *)ei->i_data;
			inode->i_op = &ext4_fast_symlink_inode_operations;
			nd_terminate_link(ei->i_data, inode->i_size,
				sizeof(ei->i_data) - 1);
		} else {
			inode->i_op = &ext4_symlink_inode_operations;
		}


The call to nd_terminate_link() guarantees that inode->i_link is
NUL-terminated, although the on-disk formatshould have already been
NUL-terminated when the symlink is set.

Also note that there really is no point to caching inodes where
inode->i_link is not a NUL pointer, since in those cases we never call
file system's get_link() function; the VFS will just fetch it straight
from i_link.  And in those cases, it's because it's a "fast symlink"
(many file systems have this concept; not just ext[234]) and the
symlink target was read in as part of the on-disk inode, and so there
is no separate disk block read read the symlink.  And so if you are
attempting to cache symlinks where inode->i_link is non-NULL, you're
just wasting a small amount of memory, and wasting the CPU time to do
the strcpy.

It's also true that the vast majority of symlink targets are less than
60 bytes, which is why I was surprised that your symlink caching was
atually making a difference or many systems.  I guess if we have lots
of unicode file names with characters like ❤ and ❤️ and symlinks to
these files, you might have a bunch of slow symlinks.  But in the case
where you have symlinks like:

0 lrwxrwxrwx 1 root root 7 Dec 19  2020 /bin -> usr/bin/

Symlink caching won't do any good.

Cheers,

						- Ted

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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 9:31 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Tue, Feb 04, 2025 at 05:49:48PM +0100, Mateusz Guzik wrote:
> > 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.
>
> There are two kinds of symlinks in ext4.  "Fast symlinks", where the
> symlink target can fit in i_block[].  And normal, sometimes called
> "slow" symlinks, where i_block[] contains a pointer to data block
> (either i_blocks[0] for a traditional inode, or the root of an extent
> tree that will fit in i_block[] if EXTENTS_FL is set).  In the case
> of a fast symlink, the maximum size of the file system is
> sizeof(i_block[])-1.  For a normal/slow symlink, the maximum size is
> super->s_blocksize.
>
> We determine whether a symlink is "fast" or not by checking whether
> i_size is less than sizeof(i_blocks[]).  In other words. if i_size
> less than 60 (and it's not an encrypted inode), it's a fast symlink
> and we set i_link to point to the i_block array.  From __ext4_iget()
> in fs/ext4/inode.c:
>
>                 if (IS_ENCRYPTED(inode)) {
>                         inode->i_op = &ext4_encrypted_symlink_inode_operations;
>                 } else if (ext4_inode_is_fast_symlink(inode)) {
>                         inode->i_link = (char *)ei->i_data;
>                         inode->i_op = &ext4_fast_symlink_inode_operations;
>                         nd_terminate_link(ei->i_data, inode->i_size,
>                                 sizeof(ei->i_data) - 1);
>                 } else {
>                         inode->i_op = &ext4_symlink_inode_operations;
>                 }
>
>
> The call to nd_terminate_link() guarantees that inode->i_link is
> NUL-terminated, although the on-disk formatshould have already been
> NUL-terminated when the symlink is set.
>

It is nul-terminated, but inode->i_size does not line up with it -- as
in the inode size pulled from the disk image is different than what
strlen would suggest.

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.

> Also note that there really is no point to caching inodes where
> inode->i_link is not a NUL pointer, since in those cases we never call
> file system's get_link() function; the VFS will just fetch it straight
> from i_link.  And in those cases, it's because it's a "fast symlink"
> (many file systems have this concept; not just ext[234]) and the
> symlink target was read in as part of the on-disk inode, and so there
> is no separate disk block read read the symlink.  And so if you are
> attempting to cache symlinks where inode->i_link is non-NULL, you're
> just wasting a small amount of memory, and wasting the CPU time to do
> the strcpy.
>

My code does not allocate any extra memory or do any extra copies.

The code prior to my change would grab i_link, strlen on it and pass
that to copy_to_user every single time readlink is issued.

My code stores the size in the inode (unioned with a field not used
for symlinks, so no again no increase in memory usage) so that the
strlen call can be avoided. Past that it's the same thing.

-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 9:48 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Tue, Feb 4, 2025 at 9:31 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > On Tue, Feb 04, 2025 at 05:49:48PM +0100, Mateusz Guzik wrote:
> > > 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.
> >
> > There are two kinds of symlinks in ext4.  "Fast symlinks", where the
> > symlink target can fit in i_block[].  And normal, sometimes called
> > "slow" symlinks, where i_block[] contains a pointer to data block
> > (either i_blocks[0] for a traditional inode, or the root of an extent
> > tree that will fit in i_block[] if EXTENTS_FL is set).  In the case
> > of a fast symlink, the maximum size of the file system is
> > sizeof(i_block[])-1.  For a normal/slow symlink, the maximum size is
> > super->s_blocksize.
> >
> > We determine whether a symlink is "fast" or not by checking whether
> > i_size is less than sizeof(i_blocks[]).  In other words. if i_size
> > less than 60 (and it's not an encrypted inode), it's a fast symlink
> > and we set i_link to point to the i_block array.  From __ext4_iget()
> > in fs/ext4/inode.c:
> >
> >                 if (IS_ENCRYPTED(inode)) {
> >                         inode->i_op = &ext4_encrypted_symlink_inode_operations;
> >                 } else if (ext4_inode_is_fast_symlink(inode)) {
> >                         inode->i_link = (char *)ei->i_data;
> >                         inode->i_op = &ext4_fast_symlink_inode_operations;
> >                         nd_terminate_link(ei->i_data, inode->i_size,
> >                                 sizeof(ei->i_data) - 1);
> >                 } else {
> >                         inode->i_op = &ext4_symlink_inode_operations;
> >                 }
> >
> >
> > The call to nd_terminate_link() guarantees that inode->i_link is
> > NUL-terminated, although the on-disk formatshould have already been
> > NUL-terminated when the symlink is set.
> >
>
> It is nul-terminated, but inode->i_size does not line up with it -- as
> in the inode size pulled from the disk image is different than what
> strlen would suggest.
>
> 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.
e2fsck 1.47.0 (5-Feb-2023)
One or more block group descriptor checksums are invalid.  Fix<y>? yes
Group descriptor 0 checksum is 0xd7c5, should be 0xa2fe.  FIXED.
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Symlink /file0/file1 (inode #14) is invalid.
Clear<y>? yes
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
[ERROR] ../../../../lib/support/quotaio_tree.c:546:check_reference:
Illegal reference (4294967295 >= 6) in user quota file
Update quota info for quota type 0<y>? yes
[QUOTA WARNING] Usage inconsistent for ID 4294967295:actual (0, 0) !=
expected (458752, 8)
[QUOTA WARNING] Usage inconsistent for ID 0:actual (458752, 8) !=
expected (0, 0)
[ERROR] ../../../../lib/support/quotaio_tree.c:546:check_reference:
Illegal reference (256 >= 6) in group quota file
Update quota info for quota type 1<y>? yes

syzkaller: ***** FILE SYSTEM WAS MODIFIED *****
syzkaller: 16/32 files (0.0% non-contiguous), 160/512 blocks

So I stand by my assessment that the symlink fetching code should
check for the problem of size vs strlen disparity.

> > Also note that there really is no point to caching inodes where
> > inode->i_link is not a NUL pointer, since in those cases we never call
> > file system's get_link() function; the VFS will just fetch it straight
> > from i_link.  And in those cases, it's because it's a "fast symlink"
> > (many file systems have this concept; not just ext[234]) and the
> > symlink target was read in as part of the on-disk inode, and so there
> > is no separate disk block read read the symlink.  And so if you are
> > attempting to cache symlinks where inode->i_link is non-NULL, you're
> > just wasting a small amount of memory, and wasting the CPU time to do
> > the strcpy.
> >
>
> My code does not allocate any extra memory or do any extra copies.
>
> The code prior to my change would grab i_link, strlen on it and pass
> that to copy_to_user every single time readlink is issued.
>
> My code stores the size in the inode (unioned with a field not used
> for symlinks, so no again no increase in memory usage) so that the
> strlen call can be avoided. Past that it's the same thing.
>
> --
> Mateusz Guzik <mjguzik gmail.com>



-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Theodore Ts'o]

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....

						- Ted

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

[Mateusz Guzik]

On Wed, Feb 5, 2025 at 6:26 AM Theodore Ts'o <tytso@mit.edu> 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>

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

[Mateusz Guzik]

On Tue, Feb 04, 2025 at 01:46:28AM -0800, 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
> 

#syz test  upstream  master

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7c54ae5fcbd4..30cff983e601 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5010,7 +5010,7 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			nd_terminate_link(ei->i_data, inode->i_size,
 				sizeof(ei->i_data) - 1);
 			inode_set_cached_link(inode, (char *)ei->i_data,
-					      inode->i_size);
+					      strlen((char *)ei->i_data));
 		} else {
 			inode->i_op = &ext4_symlink_inode_operations;
 		}

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com
Tested-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com

Tested on:

commit:         5c8c2292 Merge tag 'kthreads-fixes-2025-02-04' of git:..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=108993df980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=207eb10d2de1b054
dashboard link: https://syzkaller.appspot.com/bug?extid=48a99e426f29859818c0
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=112ecdf8580000

Note: testing is done by a robot and is best-effort only.

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

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

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@gmail.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@syzkaller.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@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-ext4-Verify-fast-symlink-length.patch --]
[-- Type: text/x-patch, Size: 1552 bytes --]

From df00b84402fb67d94a9eb6b86633092983cb388c Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.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@syzkaller.appspotmail.com
Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Jan Kara <jack@suse.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 ||
+			    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;
+			}
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);
 		} else {
-- 
2.43.0

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

[Kees Cook]

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@gmail.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@syzkaller.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@suse.com>
> SUSE Labs, CR

> From df00b84402fb67d94a9eb6b86633092983cb388c Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.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@syzkaller.appspotmail.com
> Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
> Signed-off-by: Jan Kara <jack@suse.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?

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

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

[Jan Kara]

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

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com
Tested-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com

Tested on:

commit:         92514ef2 Merge tag 'for-6.14-rc1-tag' of git://git.ker..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1770beb0580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=207eb10d2de1b054
dashboard link: https://syzkaller.appspot.com/bug?extid=48a99e426f29859818c0
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=16dde318580000

Note: testing is done by a robot and is best-effort only.