bpf: fix uninit-value in strnchr

Commit Message

Edward Adam Davis April 9, 2024, 11:37 a.m. UTC

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: [e-mail]
Signed-off-by: Edward Adam Davis <[e-mail]>
---
 kernel/bpf/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Edward Adam Davis April 9, 2024, 11:52 a.m. UTC | #1

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

Martin KaFai Lau April 9, 2024, 5:59 p.m. UTC | #2

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: [e-mail]
> Signed-off-by: Edward Adam Davis <[e-mail]>
> ---
>   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;

Edward Adam Davis April 10, 2024, 12:28 a.m. UTC | #3

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;

Edward Adam Davis April 11, 2024, 12:13 p.m. UTC | #4

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;

Patch

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;