diff mbox series

[v3,3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Message ID 20241016-imx95_edac-v3-3-86ae6fc2756a@nxp.com (mailing list archive)
State New
Headers show
Series EDAC/fsl-ddr: Add imx9 support | expand

Commit Message

Frank Li Oct. 16, 2024, 8:31 p.m. UTC 
From: Priyanka Singh <priyanka.singh@nxp.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 combining 'cap_high' and 'cap_low' into a 64-bit variable.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Li Yang <leoyang.li@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Borislav Petkov Oct. 22, 2024, 9:44 a.m. UTC | #1 
On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> From: Priyanka Singh <priyanka.singh@nxp.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 combining 'cap_high' and 'cap_low' into a 64-bit variable.
> 
> Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> Reviewed-by: Sherry Sun <sherry.sun@nxp.com>

You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst

> Signed-off-by: Li Yang <leoyang.li@nxp.com>

What does that SOB tag mean?

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 7a9fb1202f1a0..846a4ba25342a 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u32 s = syndrome;
> +
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>  
> @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  			fsl_mc_printk(mci, KERN_ERR,
>  				"Faulty ECC bit: %d\n", bad_ecc_bit);
>  
> +		if (bad_data_bit >= 0)

>= 0 implies != -1, right?

IOW?

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 846a4ba25342..fe822cb9b562 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 	 * TODO: Add support for 32-bit wide buses
 	 */
 	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
-		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
+		u64 cap = (u64)cap_high << 32 | cap_low;
 		u32 s = syndrome;
 
 		sbe_ecc_decode(cap_high, cap_low, syndrome,
 				&bad_data_bit, &bad_ecc_bit);
 
-		if (bad_data_bit != -1)
-			fsl_mc_printk(mci, KERN_ERR,
-				"Faulty Data bit: %d\n", bad_data_bit);
-		if (bad_ecc_bit != -1)
-			fsl_mc_printk(mci, KERN_ERR,
-				"Faulty ECC bit: %d\n", bad_ecc_bit);
-
-		if (bad_data_bit >= 0)
+		if (bad_data_bit >= 0) {
+			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
 			cap ^= 1ULL << bad_data_bit;
+		}
 
-		if (bad_ecc_bit >= 0)
+		if (bad_ecc_bit >= 0) {
+			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
 			s ^= 1 << bad_ecc_bit;
+		}
 
 		fsl_mc_printk(mci, KERN_ERR,
 			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
Frank Li Oct. 22, 2024, 3:29 p.m. UTC | #2 
On Tue, Oct 22, 2024 at 11:44:29AM +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> > From: Priyanka Singh <priyanka.singh@nxp.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 combining 'cap_high' and 'cap_low' into a 64-bit variable.
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> > Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> > Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
>
> You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst

Sorry, I omitted it since it is nxp internal reviewer. Do I need repost
it?

>
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
>
> What does that SOB tag mean?

It is original nxp layerscape platform maintainer. He leave NXP recently
and some item in MAINTANERS already been removed. It intents to fix
layerscape platform problem at beginning. And orginal patch have his SOB.
So I kept as original one.

>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 7a9fb1202f1a0..846a4ba25342a 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u32 s = syndrome;
> > +
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  			fsl_mc_printk(mci, KERN_ERR,
> >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > +		if (bad_data_bit >= 0)
>
> >= 0 implies != -1, right?
>
> IOW?
>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 846a4ba25342..fe822cb9b562 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u64 cap = (u64)cap_high << 32 | cap_low;
>  		u32 s = syndrome;
>
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>
> -		if (bad_data_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty Data bit: %d\n", bad_data_bit);
> -		if (bad_ecc_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> -
> -		if (bad_data_bit >= 0)
> +		if (bad_data_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
>  			cap ^= 1ULL << bad_data_bit;
> +		}
>
> -		if (bad_ecc_bit >= 0)
> +		if (bad_ecc_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
>  			s ^= 1 << bad_ecc_bit;
> +		}
>
>  		fsl_mc_printk(mci, KERN_ERR,
>  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov Oct. 22, 2024, 4:16 p.m. UTC | #3 
On Tue, Oct 22, 2024 at 11:29:03AM -0400, Frank Li wrote:
> > You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst
> 
> Sorry, I omitted it since it is nxp internal reviewer. Do I need repost
> it?

No, I can zap it.

> > > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> >
> > What does that SOB tag mean?
> 
> It is original nxp layerscape platform maintainer. He leave NXP recently
> and some item in MAINTANERS already been removed. It intents to fix
> layerscape platform problem at beginning. And orginal patch have his SOB.
> So I kept as original one.

Ok.

You forgot to comment on this part:

> > >= 0 implies != -1, right?
> >
> > IOW?
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 846a4ba25342..fe822cb9b562 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u64 cap = (u64)cap_high << 32 | cap_low;
> >  		u32 s = syndrome;
> >
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > -		if (bad_data_bit != -1)
> > -			fsl_mc_printk(mci, KERN_ERR,
> > -				"Faulty Data bit: %d\n", bad_data_bit);
> > -		if (bad_ecc_bit != -1)
> > -			fsl_mc_printk(mci, KERN_ERR,
> > -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> > -
> > -		if (bad_data_bit >= 0)
> > +		if (bad_data_bit >= 0) {
> > +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
> >  			cap ^= 1ULL << bad_data_bit;
> > +		}
> >
> > -		if (bad_ecc_bit >= 0)
> > +		if (bad_ecc_bit >= 0) {
> > +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
> >  			s ^= 1 << bad_ecc_bit;
> > +		}
> >
> >  		fsl_mc_printk(mci, KERN_ERR,
> >  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> >
Frank Li Oct. 22, 2024, 7:52 p.m. UTC | #4 
On Tue, Oct 22, 2024 at 11:44:29AM +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> > From: Priyanka Singh <priyanka.singh@nxp.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 combining 'cap_high' and 'cap_low' into a 64-bit variable.
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> > Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> > Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
>
> You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst
>
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
>
> What does that SOB tag mean?
>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 7a9fb1202f1a0..846a4ba25342a 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u32 s = syndrome;
> > +
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  			fsl_mc_printk(mci, KERN_ERR,
> >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > +		if (bad_data_bit >= 0)
>
> >= 0 implies != -1, right?
>
> IOW?

Yes, Your code is better.

>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 846a4ba25342..fe822cb9b562 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u64 cap = (u64)cap_high << 32 | cap_low;
>  		u32 s = syndrome;
>
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>
> -		if (bad_data_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty Data bit: %d\n", bad_data_bit);
> -		if (bad_ecc_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> -
> -		if (bad_data_bit >= 0)
> +		if (bad_data_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
>  			cap ^= 1ULL << bad_data_bit;
> +		}
>
> -		if (bad_ecc_bit >= 0)
> +		if (bad_ecc_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
>  			s ^= 1 << bad_ecc_bit;
> +		}
>
>  		fsl_mc_printk(mci, KERN_ERR,
>  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
diff mbox series

Patch

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 7a9fb1202f1a0..846a4ba25342a 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -328,6 +328,9 @@  static void fsl_mc_check(struct mem_ctl_info *mci)
 	 * TODO: Add support for 32-bit wide buses
 	 */
 	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
+		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
+		u32 s = syndrome;
+
 		sbe_ecc_decode(cap_high, cap_low, syndrome,
 				&bad_data_bit, &bad_ecc_bit);
 
@@ -338,11 +341,15 @@  static void fsl_mc_check(struct mem_ctl_info *mci)
 			fsl_mc_printk(mci, KERN_ERR,
 				"Faulty ECC bit: %d\n", bad_ecc_bit);
 
+		if (bad_data_bit >= 0)
+			cap ^= 1ULL << bad_data_bit;
+
+		if (bad_ecc_bit >= 0)
+			s ^= 1 << 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));
+			upper_32_bits(cap), lower_32_bits(cap), s);
 	}
 
 	fsl_mc_printk(mci, KERN_ERR,