linux-kernel - Re: [PATCH v3] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

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: <20241216154358.GA460991@yaz-khff2.amd.com>
Date: Mon, 16 Dec 2024 10:43:58 -0500
From: Yazen Ghannam <yazen.ghannam@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: Karan Sanghavi <karansanghvi98@...il.com>,
	Tony Luck <tony.luck@...el.com>, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v3] RAS/AMD/ATL: Fix unintended sign extension issue from
 coverity

On Sun, Dec 15, 2024 at 07:15:57PM +0100, Borislav Petkov wrote:
> On Wed, Dec 11, 2024 at 11:01:13AM -0500, Yazen Ghannam wrote:
> > On Wed, Nov 27, 2024 at 06:23:48PM +0000, Karan Sanghavi wrote:
> > > This error is reported by coverity scan stating as
> > > 
> > > CID 1593397: (#1 of 1): Unintended sign extension (SIGN_EXTENSION)
> > > sign_extension: Suspicious implicit sign extension: pc
> > > with type u16 (16 bits, unsigned) is promoted in
> > > pc << bit_shifts.pc to type int (32 bits, signed),
> > > then sign-extended to type unsigned long (64 bits, unsigned).
> > > If pc << bit_shifts.pc is greater than 0x7FFFFFFF,
> > > the upper bits of the result will all be 1.
> > > 
> > > Use u32 for bitwise operations to prevent unintentional
> > > sign extension by assigning the u16 value to a u32
> > > variable before performing the bitwise operation to
> > > avoid unintended sign extension and maintain
> > > consistency with the existing code style.
> > > 
> > > Reviewed-by: Yazen Ghannam <yazen.ghannam@....com>
> > > Signed-off-by: Karan Sanghavi <karansanghvi98@...il.com>
> 
> > Boris, can you please take this patch if no objections?
> 
> Lemme see:
> 
> bit_shifts.pc     = 5 + FIELD_GET(ADDR_SEL_2_CHAN, temp);
> 
> #define ADDR_SEL_2_CHAN         GENMASK(15, 12)
> 
> that register field is 4 bits, so 0xf is the highest value it can contain.
> 
> Thus, bit_shifts.pc can have 20 as its max value.
> 
> So all that coverity OMG OMG sign-extension overflow above cannot actually
> really happen, can it?
> 
> Because pc is promoted to an int, as the text rightfully points out.
> 
> Or am I way off here?
> 

Right, the warning is highlighting the implicit sign-extension, and it
doesn't seem to be a functional bug.

The 'temp' variable in this function is there to avoid these types of
warnings. But the 'pc' case was missed.

What do you recommend? Should we adjust the code to be more explicit and
avoid the warning? Or just ignore it?

Thanks,
Yazen