linux-kernel - Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

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: <20241014181647.GQZw1gDwIhBdnFnleH@fat_crate.local>
Date: Mon, 14 Oct 2024 20:16:47 +0200
From: Borislav Petkov <bp@...en8.de>
To: Frank Li <Frank.Li@....com>
Cc: York Sun <york.sun@....com>, Tony Luck <tony.luck@...el.com>,
	James Morse <james.morse@....com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Robert Richter <rric@...nel.org>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	Priyanka Singh <priyanka.singh@....com>,
	Sherry Sun <sherry.sun@....com>, Li Yang <leoyang.li@....com>
Subject: Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

On Fri, Oct 11, 2024 at 11:31:31AM -0400, Frank Li wrote:
> From: Priyanka Singh <priyanka.singh@....com>
> 
> Fix undefined behavior caused by left-shifting a negative value in the
> expression:
> 
>     cap_high ^ (1 << (bad_data_bit - 32))
> 
> The variable `bad_data_bit` ranges from 0 to 63. When `bad_data_bit` is
> less than 32, `bad_data_bit - 32` becomes negative, and left-shifting by a
> negative value in C is undefined behavior.
> 
> Fix this by checking the range of `bad_data_bit` before performing the
> shift.
> 
> Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")

Is this an urgent fix which needs to go to stable or someone just caught it
from code review?

Does it trigger in real life, IOW?

> Signed-off-by: Priyanka Singh <priyanka.singh@....com>
> Reviewed-by: Sherry Sun <sherry.sun@....com>
> Signed-off-by: Li Yang <leoyang.li@....com>
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
>  drivers/edac/fsl_ddr_edac.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 7a9fb1202f1a0..ccc13c2adfd6f 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  			fsl_mc_printk(mci, KERN_ERR,
>  				"Faulty ECC bit: %d\n", bad_ecc_bit);
>  
> -		fsl_mc_printk(mci, KERN_ERR,
> -			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> -			cap_high ^ (1 << (bad_data_bit - 32)),
> -			cap_low ^ (1 << bad_data_bit),
> -			syndrome ^ (1 << bad_ecc_bit));
> +		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
> +			fsl_mc_printk(mci, KERN_ERR,
> +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> +				      cap_high, cap_low ^ (1 << bad_data_bit),
> +				      syndrome ^ (1 << bad_ecc_bit));
> +		}
> +		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
> +			fsl_mc_printk(mci, KERN_ERR,
> +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> +				      cap_high ^ (1 << (bad_data_bit - 32)),
> +				      cap_low, syndrome ^ (1 << bad_ecc_bit));
> +		}

This is getting unnecessarily clumsy than it should be. Please do the
following:

	if (bad_data_bit != 1 && bad_ecc_bit != -1) {

		// prep the values you need to print

		// do an exactly one fsl_mc_printk() with the prepared values.

	}

Not have 4 fsl_mc_printks with a bunch of silly if-checks in front.

Thx.

-- 
Regards/Gruss,
    Boris.

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