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

[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+9b8be5e35747291236c8@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 syzkaller@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

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

[Martin KaFai Lau]

On 3/7/24 12:30 AM, syzbot wrote:
> 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+9b8be5e35747291236c8@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

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

[PATCH] bpf: fix uninit-value in strnchr

[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+9b8be5e35747291236c8@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@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

Re: [PATCH] bpf: fix uninit-value in strnchr

[Edward Adam Davis]

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

Re: [PATCH] bpf: fix uninit-value in strnchr

[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+9b8be5e35747291236c8@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@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)


>   		return -EINVAL;
>   	fmt_size = fmt_end - fmt;

Re: [PATCH] bpf: fix uninit-value in strnchr

[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 }
> 
> 
> >   		return -EINVAL;
> >   	fmt_size = fmt_end - fmt;

Re: [PATCH] bpf: fix uninit-value in strnchr

[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.
> >
> >
> > >   		return -EINVAL;
> > >   	fmt_size = fmt_end - fmt;