linux-kernel - Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk

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: <19E72C72-AFD3-4D13-8F14-9A1F96B971D3@linux.dev>
Date: Tue, 12 Aug 2025 23:28:56 +0200
From: Thorsten Blum <thorsten.blum@...ux.dev>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-usb@...r.kernel.org,
 usb-storage@...ts.one-eyed-alien.net,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: storage: realtek_cr: Simplify
 rts51x_bulk_transport()

Hi Alan,

On 12. Aug 2025, at 22:06, Alan Stern wrote:
> On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
>> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
>> only update the local variable 'residue' if needed.
>> 
>> Update the rts51x_read_status() function signature accordingly.
> 
> That last part isn't really necessary, is it?  It doesn't make the code 
> any clearer, less buggy, or quicker to execute.

It's mostly for consistency because the parameter 'len' is used to call
rts51x_bulk_transport() which now expects an unsigned integer. I'd still
argue that it makes the code and the function signature a bit clearer
because now the type communicates that 'len' cannot be negative.

>> 	- if (residue)
>> 		- residue = residue < buf_len ? residue : buf_len;
>> 	+ if (residue > buf_len)
>> 		+ residue = buf_len;
> 
> This really has nothing at all to do with whether buf_len is a signed 
> quantity -- it should never be negative.  (And I have no idea why the 
> original code includes that test for residue being nonzero.)

I agree with "it should never be negative" and ideally the type should
reflect this if possible.

It's also easier to reason about the code when comparing two unsigned
integers than having to think about implicit type conversion.

> Much more serious is something you didn't change: Just above these lines 
> it says:
> 
> 	residue = bcs->Residue;
> 
> It should say:
> 
> 	residue = le32_to_cpu(bcs->Residue);

That should probably be another patch, no?

Thanks,
Thorsten