Received-SPF: pass (google.com: domain of linux-kernel+bounces-137433-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <3cbc70e6-04e9-4523-9d4d-84d0794cfc74@linux.dev>
Date: Tue, 9 Apr 2024 10:59:17 -0700
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH] bpf: fix uninit-value in strnchr
To: Edward Adam Davis <eadavis@qq.com>
Cc: syzbot+9b8be5e35747291236c8@syzkaller.appspotmail.com, andrii@kernel.org,
 ast@kernel.org, bpf@vger.kernel.org, daniel@iogearbox.net,
 haoluo@google.com, john.fastabend@gmail.com, jolsa@kernel.org,
 kpsingh@kernel.org, linux-kernel@vger.kernel.org, sdf@google.com,
 song@kernel.org, syzkaller-bugs@googlegroups.com, yonghong.song@linux.dev
References: <0000000000009e2ff406130de279@google.com>
 <tencent_AABA5D95191FCFD28DB325F58D8212525D07@qq.com>
Content-Language: en-US
From: Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <tencent_AABA5D95191FCFD28DB325F58D8212525D07@qq.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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;