linux-kernel - Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

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: <948ab982-cb4d-46d4-a3c4-cc1c6369eb92@moroto.mountain>
Date: Mon, 8 Apr 2024 18:59:47 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: Arnd Bergmann <arnd@...nel.org>, linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kees Cook <keescook@...omium.org>,
	Daniel Micay <danielmicay@...il.com>, linux-staging@...ts.linux.dev
Subject: Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

On Mon, Apr 08, 2024 at 04:45:55PM +0200, Arnd Bergmann wrote:
> On Thu, Mar 28, 2024, at 17:35, Dan Carpenter wrote:
> > On Thu, Mar 28, 2024 at 03:04:47PM +0100, Arnd Bergmann wrote:
> >> This partially reverts an earlier bugfix that replaced the original
> >> incorrect memcpy() with a less bad strncpy(), but it now also avoids
> >> the original overflow.
> >> 
> >> Fixes: 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
> >
> > I don't see a problem with this commit.  The "sendbytes - 8" prevents
> > a write overflow to buf, and the strncpy() prevents read overflow from
> > inquiry_string.
> 
> I agree the commit did not introduce a runtime bug, but it did
> introduce a warning about the string being truncated.
> 
> >> diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
> >> index 08bd768ad34d..a73b0959f5a9 100644
> >> --- a/drivers/staging/rts5208/rtsx_scsi.c
> >> +++ b/drivers/staging/rts5208/rtsx_scsi.c
> >> @@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
> >>  
> >>  	if (sendbytes > 8) {
> >>  		memcpy(buf, inquiry_buf, 8);
> >> -		strncpy(buf + 8, inquiry_string, sendbytes - 8);
> >> +		memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
> >
> > I think your math is off.  The string is 29 characters + NUL.  So it
> > should be "min(sendbytes, 38) - 8".  You're chopping off the space and
> > the NUL terminator.
> >
> > This only affects pro_formatter_flag code...
> 
> The extra two bytes were clearly a mistake in the original version
> at the time it got added to drivers/staging. Note how the code
> immediately below it would overwrite the space and NUL byte again:
> 
>         if (pro_formatter_flag) {
>                 if (sendbytes > 36)
>                         memcpy(buf + 36, formatter_inquiry_str, sendbytes - 36);
>         }
> 

Ah.  Okay.  Fair enough.

I do think this code is really suspect...  What is the point of allowing
sendbytes < 36?  But that's not related to your patch.

regards,
dan carpenter