[syzbot] [bpf?] KMSAN: uninit-value in strnchr

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 04b8076df253 Merge tag 'firewire-fixes-6.8-rc7' of git://g..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=10bb9306180000
kernel config: https://syzkaller.appspot.com/x/.config?x=80c7a82a572c0de3
dashboard link: https://syzkaller.appspot.com/bug?extid=9b8be5e35747291236c8
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=11093316180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15a53082180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/a4610b1ff2a7/disk-04b8076d.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/991e9d902d39/vmlinux-04b8076d.xz
kernel image: https://storage.googleapis.com/syzbot-assets/a5b8e8e98121/bzImage-04b8076d.xz

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

=====================================================
BUG: KMSAN: uninit-value in strnchr+0x90/0xd0 lib/string.c:388
strnchr+0x90/0xd0 lib/string.c:388
bpf_bprintf_prepare+0x1c2/0x23b0 kernel/bpf/helpers.c:829
____bpf_trace_printk kernel/trace/bpf_trace.c:385 [inline]
bpf_trace_printk+0xec/0x3e0 kernel/trace/bpf_trace.c:375
___bpf_prog_run+0x2180/0xdb80 kernel/bpf/core.c:1986
__bpf_prog_run32+0xb2/0xe0 kernel/bpf/core.c:2225
bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
__bpf_prog_run include/linux/filter.h:651 [inline]
bpf_prog_run include/linux/filter.h:658 [inline]
bpf_test_run+0x482/0xaf0 net/bpf/test_run.c:423
bpf_prog_test_run_skb+0x14e5/0x1f20 net/bpf/test_run.c:1056
bpf_prog_test_run+0x6af/0xac0 kernel/bpf/syscall.c:4107
__sys_bpf+0x649/0xd60 kernel/bpf/syscall.c:5475
__do_sys_bpf kernel/bpf/syscall.c:5561 [inline]
__se_sys_bpf kernel/bpf/syscall.c:5559 [inline]
__x64_sys_bpf+0xa0/0xe0 kernel/bpf/syscall.c:5559
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcf/0x1e0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b

Local variable stack created at:
__bpf_prog_run32+0x43/0xe0 kernel/bpf/core.c:2225
bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
__bpf_prog_run include/linux/filter.h:651 [inline]
bpf_prog_run include/linux/filter.h:658 [inline]
bpf_test_run+0x482/0xaf0 net/bpf/test_run.c:423

CPU: 0 PID: 5019 Comm: syz-executor938 Not tainted 6.8.0-rc6-syzkaller-00250-g04b8076df253 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
=====================================================


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

[Martin KaFai Lau]

This is the same as the dev_map_lookup_elem report when running in interpreter
mode. It loads a different program to call bpf_trace_printk instead.

0: (6a) *(u16 *)(r10 -8) = 628106613
1: (bf) r1 = r10
2: (07) r1 += -8
3: (b7) r2 = 5
4: (bf) r3 = r1
5: (85) call bpf_trace_printk#-315920
6: (b7) r0 = 0
7: (95) exit


#syz dup: [syzbot] [bpf?] [net?] KMSAN: uninit-value in dev_map_lookup_elem

[Edward Adam Davis]

please test uini in strnchr

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 04b8076df253

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 449b9a5d3fe3..07490eba24fe 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -826,7 +826,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
u64 cur_arg;
char fmt_ptype, cur_ip[16], ip_spec[] = "%pXX";

- fmt_end = strnchr(fmt, fmt_size, 0);
+ fmt_end = strnchrnul(fmt, fmt_size, 0);
if (!fmt_end)
return -EINVAL;
fmt_size = fmt_end - fmt;

[Edward Adam Davis]

According to the context in bpf_bprintf_prepare(), this is checking if fmt ends
with a NUL word. Therefore, strnchrnul() should be used for validation instead
of strnchr().

Reported-by: syzbot+9b8be5...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
kernel/bpf/helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 449b9a5d3fe3..07490eba24fe 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -826,7 +826,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
u64 cur_arg;
char fmt_ptype, cur_ip[16], ip_spec[] = "%pXX";

- fmt_end = strnchr(fmt, fmt_size, 0);
+ fmt_end = strnchrnul(fmt, fmt_size, 0);
if (!fmt_end)
return -EINVAL;
fmt_size = fmt_end - fmt;

--
2.43.0

[Edward Adam Davis]

The patch title is incorrect. It is used to fix errors using strnchr.

[Edward Adam Davis]

please test uini in strnchr

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 04b8076df253

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 449b9a5d3fe3..54abc67c48c7 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -826,7 +826,8 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,

u64 cur_arg;
char fmt_ptype, cur_ip[16], ip_spec[] = "%pXX";

- fmt_end = strnchr(fmt, fmt_size, 0);

+ kmsan_unpoison_memory(fmt, fmt_size);

[Martin KaFai Lau]

On 4/9/24 4:37 AM, Edward Adam Davis wrote:
> According to the context in bpf_bprintf_prepare(), this is checking if fmt ends
> with a NUL word. Therefore, strnchrnul() should be used for validation instead
> of strnchr().

As your another email, this is not fixing the uninit KMSAN report.

If there was a separate bug, please post a separate patch instead of replying to
an unrelated thread and confuse syzbot.

>
> Reported-by: syzbot+9b8be5...@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> kernel/bpf/helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 449b9a5d3fe3..07490eba24fe 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -826,7 +826,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> u64 cur_arg;
> char fmt_ptype, cur_ip[16], ip_spec[] = "%pXX";
>
> - fmt_end = strnchr(fmt, fmt_size, 0);
> + fmt_end = strnchrnul(fmt, fmt_size, 0);

I don't think it is correct either.

> if (!fmt_end)

e.g. what will strnchrnul return if fmt is not NULL terminated?

The current code is correct as is. Comment snippet from strnchr:

/*
* ...
*
* Note that the %NUL-terminator is considered part of the string, and can
* be searched for.
*/
char *strnchr(const char *s, size_t count, int c)

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KMSAN: uninit-value in strnchrnul

=====================================================
BUG: KMSAN: uninit-value in strnchrnul+0xb5/0xf0 lib/string.c:352
strnchrnul+0xb5/0xf0 lib/string.c:352

bpf_bprintf_prepare+0x1c2/0x23b0 kernel/bpf/helpers.c:829
____bpf_trace_printk kernel/trace/bpf_trace.c:385 [inline]
bpf_trace_printk+0xec/0x3e0 kernel/trace/bpf_trace.c:375
___bpf_prog_run+0x2180/0xdb80 kernel/bpf/core.c:1986
__bpf_prog_run32+0xb2/0xe0 kernel/bpf/core.c:2225

bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
__bpf_prog_run include/linux/filter.h:651 [inline]
bpf_prog_run include/linux/filter.h:658 [inline]
bpf_test_run+0x482/0xaf0 net/bpf/test_run.c:423
bpf_prog_test_run_skb+0x14e5/0x1f20 net/bpf/test_run.c:1056
bpf_prog_test_run+0x6af/0xac0 kernel/bpf/syscall.c:4107
__sys_bpf+0x649/0xd60 kernel/bpf/syscall.c:5475
__do_sys_bpf kernel/bpf/syscall.c:5561 [inline]
__se_sys_bpf kernel/bpf/syscall.c:5559 [inline]
__x64_sys_bpf+0xa0/0xe0 kernel/bpf/syscall.c:5559
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcf/0x1e0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b

Local variable stack created at:
__bpf_prog_run32+0x43/0xe0 kernel/bpf/core.c:2225
bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
__bpf_prog_run include/linux/filter.h:651 [inline]
bpf_prog_run include/linux/filter.h:658 [inline]
bpf_test_run+0x482/0xaf0 net/bpf/test_run.c:423

CPU: 0 PID: 5507 Comm: syz-executor.0 Not tainted 6.8.0-rc6-syzkaller-00250-g04b8076df253-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
=====================================================


Tested on:

commit: 04b8076d Merge tag 'firewire-fixes-6.8-rc7' of git://g..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1470b105180000

kernel config: https://syzkaller.appspot.com/x/.config?x=80c7a82a572c0de3
dashboard link: https://syzkaller.appspot.com/bug?extid=9b8be5e35747291236c8
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=112caf9d180000

[Edward Adam Davis]

On Tue, 9 Apr 2024 10:59:17 -0700, Martin KaFai Lau wrote:
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 449b9a5d3fe3..07490eba24fe 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -826,7 +826,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> > u64 cur_arg;
> > char fmt_ptype, cur_ip[16], ip_spec[] = "%pXX";
> >
> > - fmt_end = strnchr(fmt, fmt_size, 0);
> > + fmt_end = strnchrnul(fmt, fmt_size, 0);
>
> I don't think it is correct either.
>
> > if (!fmt_end)
>
> e.g. what will strnchrnul return if fmt is not NULL terminated?
>
> The current code is correct as is. Comment snippet from strnchr:
>
> /*
> * ...
> *
> * Note that the %NUL-terminator is considered part of the string, and can
> * be searched for.
> */
> char *strnchr(const char *s, size_t count, int c)

lib/string.c
9 /**
8 * strnchr - Find a character in a length limited string
7 * @s: The string to be searched
6 * @count: The number of characters to be searched
5 * @c: The character to search for
4 *
3 * Note that the %NUL-terminator is considered part of the string, and can
2 * be searched for.
1 */
384 char *strnchr(const char *s, size_t count, int c)
1 {
2 while (count--) {
3 if (*s == (char)c) // Only when the length of s is 1, can NUL char be obtained
4 return (char *)s;
5 if (*s++ == '\0') // When the length of s is greater than 1, the loop will terminate and return NULL, without obtaining a pointer to a NUL char
6 break;
7 }
8 return NULL;
9 }

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+9b8be5...@syzkaller.appspotmail.com

Tested on:

commit: 04b8076d Merge tag 'firewire-fixes-6.8-rc7' of git://g..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=1499545d180000

kernel config: https://syzkaller.appspot.com/x/.config?x=80c7a82a572c0de3
dashboard link: https://syzkaller.appspot.com/bug?extid=9b8be5e35747291236c8
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=13b200cb180000

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

[Edward Adam Davis]

on Wed, 10 Apr 2024 08:28:01 +0800, Edward Adam Davis

> > * Note that the %NUL-terminator is considered part of the string, and can
> > * be searched for.
> > */
> > char *strnchr(const char *s, size_t count, int c)
> lib/string.c
> 9 /**
> 8 * strnchr - Find a character in a length limited string
> 7 * @s: The string to be searched
> 6 * @count: The number of characters to be searched
> 5 * @c: The character to search for
> 4 *
> 3 * Note that the %NUL-terminator is considered part of the string, and can
> 2 * be searched for.
> 1 */
> 384 char *strnchr(const char *s, size_t count, int c)
> 1 {
> 2 while (count--) {
> 3 if (*s == (char)c) // Only when the length of s is 1, can NUL char be obtained
> 4 return (char *)s;
> 5 if (*s++ == '\0') // When the length of s is greater than 1, the loop will terminate and return NULL, without obtaining a pointer to a NUL char
> 6 break;
> 7 }
> 8 return NULL;
> 9 }

My comments is wrong, strnchr() work well.