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

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

[Karan Sanghavi]

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@amd.com>
Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
---
Coverity  Link: 
https://scan7.scan.coverity.com/#/project-view/51975/11354?selectedIssue=1593397
---
Changes in v3:
- Updated commit summary log
- Link to v2: https://lore.kernel.org/r/20241108-coverity1593397signextension-v2-1-4acdf3968d2d@gmail.com

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@gmail.com>

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

[Yazen Ghannam]

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@amd.com>
> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>

Thanks Karan.

One minor nit: the Reviewed-by tag should go after the Signed-off-by.
This is noted in the "tip tree handbook":
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags

The EDAC and RAS subsystems generally follow the "TIP" group guidelines,
in my understanding.

I see you're using "b4", so you may want to adjust the "trailer-order"
config option. I've been trying out b4 myself, so this is fresh in my
mind.
https://b4.docs.kernel.org/en/latest/config.html

Boris, can you please take this patch if no objections?

Thanks,
Yazen

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

[Konstantin Ryabitsev]

On Wed, Dec 11, 2024 at 11:01:13AM -0500, Yazen Ghannam wrote:
> One minor nit: the Reviewed-by tag should go after the Signed-off-by.
> This is noted in the "tip tree handbook":
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags

It depends on who is applying the trailer, because b4 uses the
"chain-of-custody" approach to trailer ordering:

https://lore.kernel.org/tools/20221031165842.vxr4kp6h7qnkc53l@meerkat.local/

The gist of that approach is that the "Signed-off-by" trailer indicates the
custody boundary. Anything above that line is the responsibility of the person
mentioned in the S-o-B. Anything below that line is the responsibility of the
person who is the next S-o-B.

> 
> The EDAC and RAS subsystems generally follow the "TIP" group guidelines,
> in my understanding.
> 
> I see you're using "b4", so you may want to adjust the "trailer-order"
> config option. I've been trying out b4 myself, so this is fresh in my
> mind.
> https://b4.docs.kernel.org/en/latest/config.html

The trailer-order config option will only apply within the custody boundary.

Best wishes,
-K

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

[Borislav Petkov]

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@amd.com>
> > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Yazen Ghannam]

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@amd.com>
> > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.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

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

[Borislav Petkov]

On Mon, Dec 16, 2024 at 10:43:58AM -0500, Yazen Ghannam wrote:
> 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?

If there's no way for this to happen in the current code, then I'd suggest
to ignore it. Tools are not always right.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette