| Message ID | 20240930151056.10158-1-davthompson@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,RESEND] EDAC/bluefield - fix potential integer overflow | expand |
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.
> -----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
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.
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;
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(-)