diff mbox series

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

Message ID 20241022171907.8606-1-zichenxie0106@gmail.com (mailing list archive)
State Accepted
Commit 4ce1f56a1eaced2523329bef800d004e30f2f76c
Delegated to: Netdev Maintainers
Headers show
Series [v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity_write() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 13 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-10-23--12-00 (tests: 777)

Commit Message

Gax-c Oct. 22, 2024, 5:19 p.m. UTC 
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(-)

Comments

Petr Machata Oct. 23, 2024, 10:29 a.m. UTC | #1 
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>
Petr Machata Oct. 23, 2024, 10:32 a.m. UTC | #2 
The subject is a bit long though. Maybe make it just this?

netdevsim: nsim_nexthop_bucket_activity_write(): Add a terminating \0
Ido Schimmel Oct. 23, 2024, 1:38 p.m. UTC | #3 
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
Simon Horman Oct. 24, 2024, 10:55 a.m. UTC | #4 
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
> 
>
Petr Machata Oct. 24, 2024, 12:15 p.m. UTC | #5 
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?
Simon Horman Oct. 24, 2024, 3:22 p.m. UTC | #6 
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.
patchwork-bot+netdevbpf@kernel.org Oct. 29, 2024, 6:50 p.m. UTC | #7 
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!
diff mbox series

Patch

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;