linux-kernel - Re: [PATCH] staging: rtl8192e: Replace strcpy with strscpy in rtl819x_translate

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] [day] [month] [year] [list]

Message-ID: <aa6c0c3e-43fc-4a50-a1ee-7afc20388ac6@stanley.mountain>
Date: Sat, 24 Aug 2024 14:18:22 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Abhishek Tamboli <abhishektamboli9@...il.com>,
	gregkh@...uxfoundation.org, tdavies@...kphysics.net,
	philipp.g.hortmann@...il.com, garyrookard@...tmail.org,
	linux-staging@...ts.linux.dev, skhan@...uxfoundation.org,
	rbmarliere@...il.com,
	linux-kernel-mentees@...ts.linuxfoundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8192e: Replace strcpy with strscpy in
 rtl819x_translate_scan

On Sat, Aug 24, 2024 at 08:35:03AM +0200, Christophe JAILLET wrote:
> Le 21/08/2024 à 20:23, Dan Carpenter a écrit :
> > On Tue, Aug 20, 2024 at 09:38:22PM +0200, Christophe JAILLET wrote:
> > >     - if a "mode" matches, do we need to iterate the whole rtllib_modes
> > > array? (have a look at wireless_mode)
> > > 
> > 
> > Can only one mode be set at a time?
> > 
> > regards,
> > dan carpenter
> > 
> > 
> > 
> 
> Hmm, apparently several can be set (see [1])
> 
> Base on a few lines below, it looks that WIRELESS_MODE_N_24G is exclusive
> from the other ones.
> 
> So the 6 char array seems to be sized either for "N-24G", either for a
> concatenation of a few other modes that won't exceed the size of the buffer.
> 

Yeah.  I started to review this patch and found the same thing but I never hit
send on my review.  6 chars is enough.  If you look at the commit which changed
the buffer size to 6, anything larger would cause a GCC checker warning about
truncating strings.

Still, it's kind of ugly that we need to do this much research to verify that
the code isn't a memory corruption bug.  It doesn't feel future proof either.
It would be nicer if this were obviously safe from just reviewing the function.

regards,
dan carpenter