[PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Gax-c]

From: Zichen Xie <zichenxie0106@gmail.com>

This was found by a static analyzer.
We should not forget the trailing zero after copy_from_user()
if we will further do some string operations, sscanf() in this
case. Adding a trailing zero will ensure that the function
performs properly.

Fixes: c6385c0b67c5 ("netdevsim: Allow reporting activity on nexthop buckets")
Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
---
v2: adjust code format.
---
 drivers/net/netdevsim/fib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index 41e80f78b316..16c382c42227 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -1377,10 +1377,12 @@ static ssize_t nsim_nexthop_bucket_activity_write(struct file *file,
 
 	if (pos != 0)
 		return -EINVAL;
-	if (size > sizeof(buf))
+	if (size > sizeof(buf) - 1)
 		return -EINVAL;
 	if (copy_from_user(buf, user_buf, size))
 		return -EFAULT;
+	buf[size] = 0;
+
 	if (sscanf(buf, "%u %hu", &nhid, &bucket_index) != 2)
 		return -EINVAL;
 
-- 
2.34.1

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Petr Machata]

Gax-c <zichenxie0106@gmail.com> writes:

> From: Zichen Xie <zichenxie0106@gmail.com>
>
> This was found by a static analyzer.
> We should not forget the trailing zero after copy_from_user()
> if we will further do some string operations, sscanf() in this
> case. Adding a trailing zero will ensure that the function
> performs properly.
>
> Fixes: c6385c0b67c5 ("netdevsim: Allow reporting activity on nexthop buckets")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Petr Machata]

The subject is a bit long though. Maybe make it just this?

netdevsim: nsim_nexthop_bucket_activity_write(): Add a terminating \0

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Ido Schimmel]

On Tue, Oct 22, 2024 at 12:19:08PM -0500, Gax-c wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
> 
> This was found by a static analyzer.
> We should not forget the trailing zero after copy_from_user()
> if we will further do some string operations, sscanf() in this
> case. Adding a trailing zero will ensure that the function
> performs properly.
> 
> Fixes: c6385c0b67c5 ("netdevsim: Allow reporting activity on nexthop buckets")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

> ---
> v2: adjust code format.
> ---

For next time:

1. Subject prefix should be "[PATCH net v2]"
2. Wait 24h before reposting

See:
https://docs.kernel.org/process/maintainer-netdev.html

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Simon Horman]

On Tue, Oct 22, 2024 at 12:19:08PM -0500, Gax-c wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
> 
> This was found by a static analyzer.
> We should not forget the trailing zero after copy_from_user()
> if we will further do some string operations, sscanf() in this
> case. Adding a trailing zero will ensure that the function
> performs properly.
> 
> Fixes: c6385c0b67c5 ("netdevsim: Allow reporting activity on nexthop buckets")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> ---
> v2: adjust code format.
> ---
>  drivers/net/netdevsim/fib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
> index 41e80f78b316..16c382c42227 100644
> --- a/drivers/net/netdevsim/fib.c
> +++ b/drivers/net/netdevsim/fib.c
> @@ -1377,10 +1377,12 @@ static ssize_t nsim_nexthop_bucket_activity_write(struct file *file,
>  
>  	if (pos != 0)
>  		return -EINVAL;
> -	if (size > sizeof(buf))
> +	if (size > sizeof(buf) - 1)

I don't think this change for the best.
If the input data is well formatted it will end with a '\0'.
Which may be copied into the last byte of buf.

With this change the maximum size of the input data is
unnecessarily reduced by one.

>  		return -EINVAL;
>  	if (copy_from_user(buf, user_buf, size))
>  		return -EFAULT;
> +	buf[size] = 0;
> +
>  	if (sscanf(buf, "%u %hu", &nhid, &bucket_index) != 2)
>  		return -EINVAL;
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Petr Machata]

Simon Horman <horms@kernel.org> writes:

> On Tue, Oct 22, 2024 at 12:19:08PM -0500, Gax-c wrote:
>
>> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>> index 41e80f78b316..16c382c42227 100644
>> --- a/drivers/net/netdevsim/fib.c
>> +++ b/drivers/net/netdevsim/fib.c
>> @@ -1377,10 +1377,12 @@ static ssize_t nsim_nexthop_bucket_activity_write(struct file *file,
>>  
>>  	if (pos != 0)
>>  		return -EINVAL;
>> -	if (size > sizeof(buf))
>> +	if (size > sizeof(buf) - 1)
>
> I don't think this change for the best.
> If the input data is well formatted it will end with a '\0'.
> Which may be copied into the last byte of buf.
>
> With this change the maximum size of the input data is
> unnecessarily reduced by one.

The buffer is 128 bytes, and sscanf'd into it is a u32 and u16, say 18
bytes or so total. Arguably the buffer is unnecessarily large. I think
the -1 above doesn't hurt.

Though if (user_buf[size - 1]) return -EINVAL; would work, too?

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[Simon Horman]

On Thu, Oct 24, 2024 at 02:15:54PM +0200, Petr Machata wrote:
> 
> Simon Horman <horms@kernel.org> writes:
> 
> > On Tue, Oct 22, 2024 at 12:19:08PM -0500, Gax-c wrote:
> >
> >> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
> >> index 41e80f78b316..16c382c42227 100644
> >> --- a/drivers/net/netdevsim/fib.c
> >> +++ b/drivers/net/netdevsim/fib.c
> >> @@ -1377,10 +1377,12 @@ static ssize_t nsim_nexthop_bucket_activity_write(struct file *file,
> >>  
> >>  	if (pos != 0)
> >>  		return -EINVAL;
> >> -	if (size > sizeof(buf))
> >> +	if (size > sizeof(buf) - 1)
> >
> > I don't think this change for the best.
> > If the input data is well formatted it will end with a '\0'.
> > Which may be copied into the last byte of buf.
> >
> > With this change the maximum size of the input data is
> > unnecessarily reduced by one.
> 
> The buffer is 128 bytes, and sscanf'd into it is a u32 and u16, say 18
> bytes or so total. Arguably the buffer is unnecessarily large. I think
> the -1 above doesn't hurt.
> 
> Though if (user_buf[size - 1]) return -EINVAL; would work, too?

It might be fun if size is 0.

I realised after sending that 128 is really just an arbitrary value,
and losing 1 byte is unlikely to hurt. So I withdraw my previous comment:
I think this patch is fine module the review already given by yourself
and Ido.

Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 22 Oct 2024 12:19:08 -0500 you wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
> 
> This was found by a static analyzer.
> We should not forget the trailing zero after copy_from_user()
> if we will further do some string operations, sscanf() in this
> case. Adding a trailing zero will ensure that the function
> performs properly.
> 
> [...]

Here is the summary with links:
  - [v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write()
    https://git.kernel.org/netdev/net/c/4ce1f56a1eac

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html