linux-kernel - Re: [PATCH v2] 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: <4shdmryv5gq7lwc6zp5riiiaoatgfv7fji4isuhnbwvkoliiqb@uwwlt5n2juuq>
Date: Sun, 24 Nov 2024 08:45:12 +0000
From: Karan Sanghavi <karansanghvi98@...il.com>
To: Yazen Ghannam <yazen.ghannam@....com>
Cc: Tony Luck <tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>, 
	linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from
 coverity

On Tue, Nov 12, 2024 at 10:04:19AM -0500, Yazen Ghannam wrote:
> On Fri, Nov 08, 2024 at 04:40:41PM +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.
> > 
> > Following the code styleof the file, assigning the u16
> 
> styleof -> style of
> 
> > value to u32 variable and using it for the bit wise
> > operation, thus ensuring no unintentional sign
> > extension occurs.
> >
> 
> Please make sure you use an imperative voice here. For example, "assign
> the value...and use it...". This should read like you are giving
> commands.
> 
> > Signed-off-by: Karan Sanghavi <karansanghvi98@...il.com>
> 
> Overall, looks good to me.
> 
> Reviewed-by: Yazen Ghannam <yazen.ghannam@....com>
> 
> Thanks,
> Yazen
> 
> > ---
> > Coverity  Link: 
> > https://scan7.scan.coverity.com/#/project-view/51975/11354?selectedIssue=1593397
> > ---
> > Changes in v2:
> > - Assigning pc value to temp variable before left shifting as mentioned
> >   in feedback rather then typecasting pc to u32. 
> > - Link to v1: https://lore.kernel.org/r/20241104-coverity1593397signextension-v1-1-4cfae6532140@gmail.com
> > ---
> >  drivers/ras/amd/atl/umc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
> > index dc8aa12f63c8..3f4b1f31e14f 100644
> > --- a/drivers/ras/amd/atl/umc.c
> > +++ b/drivers/ras/amd/atl/umc.c
> > @@ -293,7 +293,8 @@ static unsigned long convert_dram_to_norm_addr_mi300(unsigned long addr)
> >  	}
> >  
> >  	/* PC bit */
> > -	addr |= pc << bit_shifts.pc;
> > +	temp = pc;
> > +	addr |= temp << bit_shifts.pc;
> >  
> >  	/* SID bits */
> >  	for (i = 0; i < NUM_SID_BITS; i++) {
> > 
> > ---
> > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
> > change-id: 20241104-coverity1593397signextension-78c9b2c21d51
> > 
> > Best regards,
> > -- 
> > Karan Sanghavi <karansanghvi98@...il.com>
> >

Dear Yazen,

I hope this email finds you well. 
I'm following up on the patch I recently submitted linked below.
https://lore.kernel.org/all/20241108-coverity1593397signextension-v2-1-4acdf3968d2d@gmail.com/

I noticed it hasn't been applied yet, and I wanted to see if there was 
anything else needed from my end. 
Please let me know if any further information or modifications are required.
I appreciate your time and feedback.  

Thank you! 

Sincerely,
Karan.