[PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow

[PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow

[David Thompson]

The 64-bit argument for the "get DIMM info" SMC call consists of
"mem_ctrl_idx" left-shifted 16 bits and OR-ed with DIMM index.
With "mem_ctrl_idx" defined as 32-bits wide the left-shift operation
truncates the upper 16 bits of information during the calculation
of the SMC argument. The "mem_ctrl_idx" stack variable must be
defined as 64-bits wide to prevent any potential integer overflow,
i.e. loss of data from upper 16 bits.

Fixes: 82413e562ea6 ("EDAC, mellanox: Add ECC support for BlueField DDR4")
Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
Signed-off-by: David Thompson <davthompson@nvidia.com>
---
 drivers/edac/bluefield_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
index 5b3164560648..0e539c107351 100644
--- a/drivers/edac/bluefield_edac.c
+++ b/drivers/edac/bluefield_edac.c
@@ -180,7 +180,7 @@ static void bluefield_edac_check(struct mem_ctl_info *mci)
 static void bluefield_edac_init_dimms(struct mem_ctl_info *mci)
 {
 	struct bluefield_edac_priv *priv = mci->pvt_info;
-	int mem_ctrl_idx = mci->mc_idx;
+	u64 mem_ctrl_idx = mci->mc_idx;
 	struct dimm_info *dimm;
 	u64 smc_info, smc_arg;
 	int is_empty = 1, i;
-- 
2.30.1

Re: [PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow

[Borislav Petkov]

On Mon, Sep 30, 2024 at 11:10:56AM -0400, David Thompson wrote:
> The 64-bit argument for the "get DIMM info" SMC call consists of
> "mem_ctrl_idx" left-shifted 16 bits and OR-ed with DIMM index.
> With "mem_ctrl_idx" defined as 32-bits wide the left-shift operation
> truncates the upper 16 bits of information during the calculation
> of the SMC argument. The "mem_ctrl_idx" stack variable must be
> defined as 64-bits wide to prevent any potential integer overflow,
> i.e. loss of data from upper 16 bits.
> 
> Fixes: 82413e562ea6 ("EDAC, mellanox: Add ECC support for BlueField DDR4")
> Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> Signed-off-by: David Thompson <davthompson@nvidia.com>
> ---
>  drivers/edac/bluefield_edac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
> index 5b3164560648..0e539c107351 100644
> --- a/drivers/edac/bluefield_edac.c
> +++ b/drivers/edac/bluefield_edac.c
> @@ -180,7 +180,7 @@ static void bluefield_edac_check(struct mem_ctl_info *mci)
>  static void bluefield_edac_init_dimms(struct mem_ctl_info *mci)
>  {
>  	struct bluefield_edac_priv *priv = mci->pvt_info;
> -	int mem_ctrl_idx = mci->mc_idx;
> +	u64 mem_ctrl_idx = mci->mc_idx;
>  	struct dimm_info *dimm;
>  	u64 smc_info, smc_arg;
>  	int is_empty = 1, i;
> -- 

Is this something you're hitting in real workloads so that it needs to go to
stable or is it rather something caught through code review or so?

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow

[David Thompson]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, October 16, 2024 12:50 PM
> To: David Thompson <davthompson@nvidia.com>
> Cc: tony.luck@intel.com; james.morse@arm.com; mchehab@kernel.org;
> rric@kernel.org; linux-edac@vger.kernel.org; Shravan Ramani
> <shravankr@nvidia.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow
> 
> On Mon, Sep 30, 2024 at 11:10:56AM -0400, David Thompson wrote:
> > The 64-bit argument for the "get DIMM info" SMC call consists of
> > "mem_ctrl_idx" left-shifted 16 bits and OR-ed with DIMM index.
> > With "mem_ctrl_idx" defined as 32-bits wide the left-shift operation
> > truncates the upper 16 bits of information during the calculation of
> > the SMC argument. The "mem_ctrl_idx" stack variable must be defined as
> > 64-bits wide to prevent any potential integer overflow, i.e. loss of
> > data from upper 16 bits.
> >
> > Fixes: 82413e562ea6 ("EDAC, mellanox: Add ECC support for BlueField
> > DDR4")
> > Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> > Signed-off-by: David Thompson <davthompson@nvidia.com>
> > ---
> >  drivers/edac/bluefield_edac.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/edac/bluefield_edac.c
> > b/drivers/edac/bluefield_edac.c index 5b3164560648..0e539c107351
> > 100644
> > --- a/drivers/edac/bluefield_edac.c
> > +++ b/drivers/edac/bluefield_edac.c
> > @@ -180,7 +180,7 @@ static void bluefield_edac_check(struct
> > mem_ctl_info *mci)  static void bluefield_edac_init_dimms(struct
> > mem_ctl_info *mci)  {
> >  	struct bluefield_edac_priv *priv = mci->pvt_info;
> > -	int mem_ctrl_idx = mci->mc_idx;
> > +	u64 mem_ctrl_idx = mci->mc_idx;
> >  	struct dimm_info *dimm;
> >  	u64 smc_info, smc_arg;
> >  	int is_empty = 1, i;
> > --
> 
> Is this something you're hitting in real workloads so that it needs to go to stable or
> is it rather something caught through code review or so?
> 

Hi Boris,

This patch addresses an issue that was raised during a scan by Coverity (static code analyzer).
I don't believe we have evidence that it will be hit with real workloads.

Regards, Dave

> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v1 RESEND] EDAC/bluefield - fix potential integer overflow

[Borislav Petkov]

On Wed, Oct 16, 2024 at 05:30:49PM +0000, David Thompson wrote:
> This patch addresses an issue that was raised during a scan by Coverity
> (static code analyzer).  I don't believe we have evidence that it will be
> hit with real workloads.

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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