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: <d8ee72ec-c16f-443a-b9c1-555dcbc69182@app.fastmail.com>
Date: Mon, 08 Apr 2024 16:45:55 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Dan Carpenter" <dan.carpenter@...aro.org>,
 "Arnd Bergmann" <arnd@...nel.org>
Cc: 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 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);
        }

> This code is such a mess.  I'm not sure your fix is the complete fix.
> When I see code that's clearly buggy like this and it's not sure the fix
> is complete then I generally prefer to leave the static checker warning
> as is so that we are reminded of the bug occasionally.

I still think my patch is correct here, but I'll remove the confusing
spaces at the end of the strings and try to improve the commit
text.

A more readable version of the code might construct the entire
56 byte buffer on the stack and then do a single memcpy, but I
think the simpler change is sufficient here.

> How close are
> you to removing all these -Wstringop-truncation warnings?  Maybe we just
> add a comment or a TODO item in the drivers/staging/rts5208/TODO file.

I'm down to eight warnings for clang now (on x86, arm and arm64 randconfigs
as well as allmodconfig and defconfig elsewhere), and hope to have it
enabled by default in either 6.10 or 6.11 after those are all
addressed.

I think gcc shows more warnings because it analyses buffer sizes
across inlining, while clang only does it within a given function.

      Arnd