Re: [PATCH] RAS/AMD/ATL: Fix bit overflow in denorm_addr_df4_np2()

[Borislav Petkov]

On Thu, Feb 22, 2024 at 10:54:49AM -0600, Yazen Ghannam wrote:
> The hash_pa8 and hashed_bit values in denorm_addr_df4_np2() are
> currently defined as u8 types. These variables represent single bits.
> 
> 'hash_pa8' is set based on logical AND operations using masks with more
> than 8 bits. So the calculated value will not fit in this variable. It
> will always be '0'. The 'hash_pa8' check later in the function will fail
> which produces incorrect results for some cases.
> 
> Change these variables to bool type. This clarifies that they are
> single bit values. Also, this allows the compiler to ensure they hold
> the proper results. Remove an unnecessary shift operation.
> 
> Fixes: 3f3174996be6 ("RAS: Introduce AMD Address Translation Library")
> Signed-off-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> ---
>  drivers/ras/amd/atl/denormalize.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/ras/amd/atl/denormalize.c b/drivers/ras/amd/atl/denormalize.c
> index 49a900e066f1..f46bce119255 100644
> --- a/drivers/ras/amd/atl/denormalize.c
> +++ b/drivers/ras/amd/atl/denormalize.c
> @@ -545,7 +545,7 @@ static int denorm_addr_df4_np2(struct addr_ctx *ctx)
>  	unsigned int mod_value, shift_value;
>  	u16 mask = df_cfg.component_id_mask;
>  	u64 temp_addr_a, temp_addr_b;
> -	u8 hash_pa8, hashed_bit;
> +	bool hash_pa8, hashed_bit;
>  
>  	switch (ctx->map.intlv_mode) {
>  	case DF4_NPS4_3CHAN_HASH:
> @@ -578,7 +578,6 @@ static int denorm_addr_df4_np2(struct addr_ctx *ctx)
>  		temp_addr_a	= remove_bits(shift_value, shift_value, ctx->ret_addr);
>  	} else {
>  		hash_pa8	= (ctx->coh_st_fabric_id & df_cfg.socket_id_mask);
> -		hash_pa8	>>= df_cfg.socket_id_shift;
>  		temp_addr_a	= ctx->ret_addr;
>  	}
>  
> -- 

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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