Received-SPF: pass (google.com: domain of linux-kernel+bounces-136258-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-379-gabd37849b7-fm-20240408.001-gabd37849
Precedence: bulk
MIME-Version: 1.0
Message-Id: <b5740321-455e-4939-8a94-72c01e4c09f3@app.fastmail.com>
In-Reply-To: 
 <wb5h2syqmxpoxmajwiiyqvlkyojjsjql5xpfmris6djegtqzya@6xfio4dmqjdp>
References: <20240408194821.3183462-1-arnd@kernel.org>
 <wb5h2syqmxpoxmajwiiyqvlkyojjsjql5xpfmris6djegtqzya@6xfio4dmqjdp>
Date: Tue, 09 Apr 2024 07:34:41 +0200
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Justin Stitt" <justinstitt@google.com>, "Arnd Bergmann" <arnd@kernel.org>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
 "Dan Carpenter" <dan.carpenter@linaro.org>, linux-staging@lists.linux.dev,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] [v2] staging: rts5208: replace weird strncpy() with memcpy()
Content-Type: text/plain

On Mon, Apr 8, 2024, at 22:28, Justin Stitt wrote:
> On Mon, Apr 08, 2024 at 09:48:09PM +0200, Arnd Bergmann wrote:

>> @@ -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 must say I am not the biggest fan of manual string management with raw
> pointer offsets. I wonder if scnprintf() could achieve your goal here of
> combining inquiry_buf with inquiry_string into buf (perhaps utilizing
> %.*s format specifier).

scnprintf() would be wrong here, since it's not actually a string
but a SCSI HW data structure with both binary and ASCII members
and no NUL termination. It's supposed to be padded with spaces.

> With that being said, I am just a casual reader of this code and I
> really don't know much about the expected behavior of `buf`
> (NUL-terminated, NUL-padded, etc) or even what the next line buf[4]=0x33
> does.

I believe this sets the length of the buffer to 51 bytes when
pro_formatter_flag is set, overriding the 0x1f (31 bytes) for the
normal length.

The root of the problem here is that the driver emulates a SCSI
command set on a card reader for both SD cards and memory sticks
instead of using the existing abstractions from
drivers/{mmc,memstick}.

Apparently drivers/misc/cardreader/rtsx_pcr.c does this the
right way for all later devices (rts5209 and up) but is
not compatible with this variant of the hardware.

    Arnd