[syzbot] [ext4?] WARNING in __fortify_report

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 0450d2083be6 Merge tag '6.10-rc-smb-fix' of git://git.samb..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=152aa7d0980000
kernel config: https://syzkaller.appspot.com/x/.config?x=769d2e801ee872cc
dashboard link: https://syzkaller.appspot.com/bug?extid=50835f73143cc2905b9e
compiler: arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=103cb2a4980000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16502844980000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/8ead8862021c/non_bootable_disk-0450d208.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/11a51fed42ca/vmlinux-0450d208.xz
kernel image: https://storage.googleapis.com/syzbot-assets/1df7cb920c72/zImage-0450d208.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+50835f...@syzkaller.appspotmail.com

------------[ cut here ]------------
WARNING: CPU: 0 PID: 3004 at lib/string_helpers.c:1029 __fortify_report+0x6c/0x74 lib/string_helpers.c:1029
strnlen: detected buffer overflow: 17 byte read of buffer size 16
Modules linked in:
Kernel panic - not syncing: kernel: panic_on_warn set ...
CPU: 0 PID: 3004 Comm: syz-executor296 Not tainted 6.9.0-syzkaller #0
Hardware name: ARM-Versatile Express
Call trace:
[<818df678>] (dump_backtrace) from [<818df774>] (show_stack+0x18/0x1c arch/arm/kernel/traps.c:256)
r7:00000000 r6:82622e04 r5:00000000 r4:81fe2534
[<818df75c>] (show_stack) from [<818fcdd8>] (__dump_stack lib/dump_stack.c:88 [inline])
[<818df75c>] (show_stack) from [<818fcdd8>] (dump_stack_lvl+0x54/0x7c lib/dump_stack.c:114)
[<818fcd84>] (dump_stack_lvl) from [<818fce18>] (dump_stack+0x18/0x1c lib/dump_stack.c:123)
r5:00000000 r4:82860d18
[<818fce00>] (dump_stack) from [<818e021c>] (panic+0x120/0x358 kernel/panic.c:347)
[<818e00fc>] (panic) from [<80243d54>] (check_panic_on_warn kernel/panic.c:240 [inline])
[<818e00fc>] (panic) from [<80243d54>] (print_tainted+0x0/0xa0 kernel/panic.c:235)
r3:8260c5c4 r2:00000001 r1:81fcb130 r0:81fd2d44
r7:8080fe7c
[<80243ce0>] (check_panic_on_warn) from [<80243f48>] (__warn+0x7c/0x180 kernel/panic.c:693)
[<80243ecc>] (__warn) from [<80244234>] (warn_slowpath_fmt+0x1e8/0x1f4 kernel/panic.c:726)
r8:00000009 r7:8202fe0c r6:df969db4 r5:836e6c00 r4:00000000
[<80244050>] (warn_slowpath_fmt) from [<8080fe7c>] (__fortify_report+0x6c/0x74 lib/string_helpers.c:1029)
r10:8271c1c8 r9:00000005 r8:df969ec3 r7:8372e000 r6:00000000 r5:836be478
r4:82e27000
[<8080fe10>] (__fortify_report) from [<818e9a40>] (__fortify_panic+0x10/0x14 lib/string_helpers.c:1036)
[<818e9a30>] (__fortify_panic) from [<8062a3b0>] (strnlen include/linux/fortify-string.h:221 [inline])
[<818e9a30>] (__fortify_panic) from [<8062a3b0>] (sized_strscpy include/linux/fortify-string.h:295 [inline])
[<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_ioctl_getlabel fs/ext4/ioctl.c:1154 [inline])
[<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_fileattr_get+0x0/0x78 fs/ext4/ioctl.c:1609)
[<8062829c>] (__ext4_ioctl) from [<8062aaac>] (ext4_ioctl+0x10/0x14 fs/ext4/ioctl.c:1626)
r10:836e6c00 r9:00000005 r8:845e7900 r7:00000000 r6:845e7900 r5:00000000
r4:81009431
[<8062aa9c>] (ext4_ioctl) from [<80518930>] (vfs_ioctl fs/ioctl.c:51 [inline])
[<8062aa9c>] (ext4_ioctl) from [<80518930>] (do_vfs_ioctl fs/ioctl.c:861 [inline])
[<8062aa9c>] (ext4_ioctl) from [<80518930>] (__do_sys_ioctl fs/ioctl.c:905 [inline])
[<8062aa9c>] (ext4_ioctl) from [<80518930>] (sys_ioctl+0x134/0xda4 fs/ioctl.c:893)
[<805187fc>] (sys_ioctl) from [<80200060>] (ret_fast_syscall+0x0/0x1c arch/arm/mm/proc-v7.S:67)
Exception stack(0xdf969fa8 to 0xdf969ff0)
9fa0: 00000000 00000000 00000005 81009431 00000000 00000000
9fc0: 00000000 00000000 0008e060 00000036 000f4240 00000000 00000001 00003a97
9fe0: 7e98ac70 7e98ac60 00010764 0002e8c0
r10:00000036 r9:836e6c00 r8:8020029c r7:00000036 r6:0008e060 r5:00000000
r4:00000000
Rebooting in 86400 seconds..


---
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 syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

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

[Theodore Ts'o]

On Wed, May 22, 2024 at 11:29:25PM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>

> dashboard link: https://syzkaller.appspot.com/bug?extid=50835f73143cc2905b9e

> ...

> strnlen: detected buffer overflow: 17 byte read of buffer size 16

> [<8080fe10>] (__fortify_report) from [<818e9a40>] (__fortify_panic+0x10/0x14 lib/string_helpers.c:1036)
> [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (strnlen include/linux/fortify-string.h:221 [inline])
> [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (sized_strscpy include/linux/fortify-string.h:295 [inline])
> [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_ioctl_getlabel fs/ext4/ioctl.c:1154 [inline])

> [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_fileattr_get+0x0/0x78 fs/ext4/ioctl.c:1609)
> [<8062829c>] (__ext4_ioctl) from [<8062aaac>] (ext4_ioctl+0x10/0x14 fs/ext4/ioctl.c:1626)
> r10:836e6c00 r9:00000005 r8:845e7900 r7:00000000 r6:845e7900 r5:00000000

This is caused by commit 744a56389f73 ("ext4: replace deprecated
strncpy with alternatives") and it's unclear whether this is being
caused by a buggy implementation of strscpy_pad(), or a buggy fortify,
but a simple way to fix is to go back to the good-old strncpy(), which
is perfectly safe, and perfectly secure.

(And this is a great example of "security initiatives" being an
exercise in pain alocation tradeoffs between overworked maintainers
and security teams... regardless of whether the bug is in fortify,
syzkaller, or an effort to completely convert away from strncpy()
because it makes security analysis easier.)

- Ted

[Kees Cook]

It looks like this is another case of a non-terminated string being made
terminated by strncpy into a string with 1 extra byte at the end:

char label[EXT4_LABEL_MAX + 1];
...
- memset(label, 0, sizeof(label));
lock_buffer(sbi->s_sbh);
- strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
+ strscpy_pad(label, sbi->s_es->s_volume_name);
unlock_buffer(sbi->s_sbh);

This should be using memtostr_pad() as:

memtostr_pad(label, sbi->s_es->s_volume_name);

I'll send a patch. It looks like __nonstring markings from commit
072ebb3bffe6 ("ext4: add nonstring annotations to ext4.h") were
incomplete.

-Kees

--
Kees Cook

[Theodore Ts'o]

On Thu, May 23, 2024 at 03:48:01PM -0700, Kees Cook wrote:
> It looks like this is another case of a non-terminated string being made
> terminated by strncpy into a string with 1 extra byte at the end:
>
> char label[EXT4_LABEL_MAX + 1];
> ...
> - memset(label, 0, sizeof(label));
> lock_buffer(sbi->s_sbh);
> - strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
> + strscpy_pad(label, sbi->s_es->s_volume_name);
> unlock_buffer(sbi->s_sbh);
>
> This should be using memtostr_pad() as:
>
> memtostr_pad(label, sbi->s_es->s_volume_name);
>

Ah... I see what is going on. The two argument variants of
memtostr_pad() and strscpy_pad() are confusing and dangerous. These
don't exist in the original OpenBSD strscpy() function, because the
size in the third argument is explicit, while with strscpy_pad(), the
automagic size is intuited from the first argument (the destination),
while with memtostr_pad(), the size is automagically intuited from the
second argument (the source).

This confused me, and I couldn't figure out the bug even when I was
given the stack trace from syzkaller. So it's an accident waiting to
happen, I clearly I was not smart enough not to fall into the trap,

So perhaps it might be nice if the descriptions of strscpy() is moved
out of process/deprecated.rst (and BTW, memstrtopad isn't mentioned at
all), and moved inta separate doumentation which safe string handling
in C --- so instead of talking about what functions *shouldn't* used,
such as strncpy(), it talks about how the various functions *should*
be used instead.

I'll also note that figuring out what was going on from looking at
include/linu/string.h was confusing, because there is so much #define
magic to provide the magic multiple argument handling. Personally, I
was never a fan of C++'s function overloading where different function
signatures could result in different implementations being called, and
doing with C preprocessor magic makes it even worse. To be fair,
there is the kernel-doc inline documentation, but my eyes were drawn
to the C++ implementation, and the kernel-doc documentation is more of
a reference and not a tutorial style "this is how you should do
things".

Anyway, thanks for sending the patch. I spent a good 30 minutes
trying to figure out the bug, and was half-tempted to just revert the
patch and go back to strncpy(), which at least I could obviously see
was correct, unlike the strscpy_pad() transmogrification.

> It looks like __nonstring markings from commit 072ebb3bffe6 ("ext4:
> add nonstring annotations to ext4.h") were incomplete.

Yes, I'll patch ext4.h to add a __nonstring annotation to
s_volume_name. As I recall, the reason why we had added the
__nonstring markings was to shut up gcc's -Wstringpop-truncation
warnings, and apparently it was needed for s_volume_name, which is why
it was never annotated.

Out of curiosity, though, would this have caused some analysis tool to
trigger a warning when the strscpy_pad() commit was added? I've
tried, and it doesn't seem to have triggered any kind of warning with
either gcc, clang, or sparse.

Anyway, since I'm an old fart, it was pretty obvious to *me* that the
how strcnpy() was used was obviouly correct, whereas I actually have
to do more careful thinking and analysis with strscpy() and
memtostr(). So it would be nice if there were some automated tools
that warn if those new functions aren't used correctly, because this
bug shows that these functions are definitely not fool proof --- both
by the original patch submitter, and the fool who reviewed the patch
and missed the problem. :-)

- Ted