netdev - Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string in nsim_nexthop_bucket_activity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241024152243.GY1202098@kernel.org>
Date: Thu, 24 Oct 2024 16:22:43 +0100
From: Simon Horman <horms@...nel.org>
To: Petr Machata <petrm@...dia.com>
Cc: Gax-c <zichenxie0106@...il.com>, kuba@...nel.org, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	idosch@...dia.com, netdev@...r.kernel.org, zzjas98@...il.com,
	chenyuan0y@...il.com
Subject: Re: [PATCH v2] netdevsim: Add trailing zero to terminate the string
 in nsim_nexthop_bucket_activity_write()

On Thu, Oct 24, 2024 at 02:15:54PM +0200, Petr Machata wrote:
> 
> Simon Horman <horms@...nel.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.