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: <8734klit9w.fsf@nvidia.com>
Date: Thu, 24 Oct 2024 14:15:54 +0200
From: Petr Machata <petrm@...dia.com>
To: Simon Horman <horms@...nel.org>
CC: Gax-c <zichenxie0106@...il.com>, <kuba@...nel.org>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <petrm@...dia.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()


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?