linux-kernel - Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

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: <Zqp8vbbIC8E/XrQY@embed-PC.myguest.virtualbox.org>
Date: Wed, 31 Jul 2024 23:34:45 +0530
From: Abhishek Tamboli <abhishektamboli9@...il.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: gregkh@...uxfoundation.org, oneukum@...e.com,
	usb-storage@...ts.one-eyed-alien.net, linux-usb@...r.kernel.org,
	skhan@...uxfoundation.org, dan.carpenter@...aro.org,
	rbmarliere@...il.com,
	linux-kernel-mentees@...ts.linuxfoundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings

On Wed, Jul 31, 2024 at 10:04:33AM -0400, Alan Stern wrote:
> On Wed, Jul 31, 2024 at 11:15:28AM +0200, 'Oliver Neukum' via USB Mass Storage on Linux wrote:
> > Hi,
> > 
> > On 30.07.24 19:56, Abhishek Tamboli wrote:
> > > On Tue, Jul 30, 2024 at 09:09:05AM +0200, Oliver Neukum wrote:
> > 
> > > > 1. use a constant, where a constant is used
> > > I think you are suggesting that I should replace hard-coded values like the
> > > buffer size with named constants. For example:
> > > 
> > > #define BUF_SIZE 8
> > > unsigned char buf[BUF_SIZE];
> > 
> > Yes, but the constant we need to look at here is bl_len.
> > This is a variable needlessly.
> 
> The code in ms_scsi_read_capacity() is written that way to be consistent 
> with the sd_scsi_read_capacity() routine.  Or maybe it was just 
> copied-and-pasted, and then the variable's type was changed for no good 
> reason.
> 
> Replacing the variable with a constant won't make much difference.  The 
> compiler will realize that bl_len has a constant value and will generate 
> appropriate code anyway.  I think just changing the type is a fine fix.
> 
> > > > 2. use the macros for converting endianness
> > > Can I use macros like cpu_to_le32 for converting the bl_num and bl_len values.
> > > Should I replace all instances of manual bitwise shifts with these macros?
> > 
> > Yes.
> > 
> > > For example:
> > > 
> > >      u32 bl_len = 0x200;
> > >      buf[0] = cpu_to_le32(bl_num) >> 24;
> > >      buf[4] = cpu_to_le32(bl_len) >> 24;
> > > 
> > > Is using cpu_to_le32 appropriate for the data format required by this
> > > device?
> > 
> > Well, the format is big endian. So, cpu_to_be32() will be required.
> 
> Better yet, use put_unaligned_be32().
Would you recommend submitting a follow-up patch to incorporate this change, or should I leave it as is for now.
>However, there's nothing really 
>wrong with the code as it stands. It doesn't need to be changed now.
As you mentioned there's no need to change the code, So my initial patch is okay as is?

Thanks & Regards,
Abhishek Tamboli