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

[Simon Horman <horms@kernel.org>]

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.