[PATCH v2 0/2] EDAC/i5000: Fix the slot loop and re-enable the driver

[PATCH v2 0/2] EDAC/i5000: Fix the slot loop and re-enable the driver

[Andrew Luebke]

In 2022 i5000_edac was marked BROKEN instead of taking Aristeu's fix for
an out-of-bounds read in i5000_init_csrows(). Nobody had the hardware to
check that the driver handles both single- and dual-ranked DIMMs [1].

I tested on an HP xw8400 (5000X, four channels) with v7.4-rc2, KASAN,
UBSAN bounds and EDAC debug. Without patch 1, UBSAN reports at driver
load:

  UBSAN: array-index-out-of-bounds in drivers/edac/i5000_edac.c:959:20
  index 4 is out of range for type 'u16 [4]'

and the same at line 961. With it there are no reports, and all eight
slots (4 x 2 GB 2Rx4 in slot 0, 4 x 512 MB 1Rx8 in slot 1, both branches)
show up at the right branch/channel/slot with the right size and rank,
10240 MB in total. On 7.2.14 the same holds, also with 4 x 1 GB 2Rx8 in
slot 1 (12288 MB). A two-pass memtester run with the driver polling (two
DIMMs) saw no errors.

Not tested: two-channel boards (5000V/5000Z) and error reporting, as no
memory errors occurred.

Patch 1 is Aristeu's patch as posted, plus my tags. Patch 2 reverts the
BROKEN marking and adds me as a reviewer in MAINTAINERS.

6.1, 5.15 and 5.10 do not have the BROKEN marking, so the out-of-bounds
read is live there. Patch 1 applies to them as is.

Analysis and test kernels were prepared with LLM help; the tests ran on my
hardware.

Changes in v2:
- Patch 2: add myself as a reviewer in MAINTAINERS (Boris)
- Patch 1: no change

v1: https://lore.kernel.org/r/20261111232529.407154-1-andrew.luebke@gmail.com

[1] https://lore.kernel.org/all/YyxpbRkuLPIcW1Om@zn.tnic/

Andrew Luebke (1):
  Revert "EDAC/i5000: Mark as BROKEN"

Aristeu Rozanski (1):
  i5000_edac: fix slot number passed to determine_mtr()

 MAINTAINERS               | 1 +
 drivers/edac/Kconfig      | 1 -
 drivers/edac/i5000_edac.c | 4 +---
 3 files changed, 2 insertions(+), 4 deletions(-)


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.47.3

[PATCH v2 1/2] i5000_edac: fix slot number passed to determine_mtr()

[Andrew Luebke]

From: Aristeu Rozanski <aris@redhat.com>

When the logic mapping branch/slot/channel was reworked back in
64e1fdaf55d6 ("i5000_edac: Fix the logic that retrieves memory information")
i5000_init_csrows() was not updated and kept passing twice the number
of slots to determine_mtr(), which leads to acessing past the end of
i5000_pvt.b1_mtr[]. This was found by KASAN.

Fixes: 64e1fdaf55d6 ("i5000_edac: Fix the logic that retrieves memory information")
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>
Link: https://lore.kernel.org/all/20220921181009.oxytvicy6sry6it7@redhat.com/
Cc: stable@vger.kernel.org
Tested-by: Andrew Luebke <andrew.luebke@gmail.com>
Signed-off-by: Andrew Luebke <andrew.luebke@gmail.com>
---
v2: no change

 drivers/edac/i5000_edac.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index c0faf55f7812..342b69909600 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -1253,14 +1253,12 @@ static int i5000_init_csrows(struct mem_ctl_info *mci)
 	struct i5000_pvt *pvt;
 	struct dimm_info *dimm;
 	int empty;
-	int max_csrows;
 	int mtr;
 	int csrow_megs;
 	int channel;
 	int slot;
 
 	pvt = mci->pvt_info;
-	max_csrows = pvt->maxdimmperch * 2;
 
 	empty = 1;		/* Assume NO memory */
 
@@ -1271,7 +1269,7 @@ static int i5000_init_csrows(struct mem_ctl_info *mci)
 	 * to map the dimms. A good cleanup would be to remove this array,
 	 * and do a loop here with branch, channel, slot
 	 */
-	for (slot = 0; slot < max_csrows; slot++) {
+	for (slot = 0; slot < pvt->maxdimmperch; slot++) {
 		for (channel = 0; channel < pvt->maxch; channel++) {
 
 			mtr = determine_mtr(pvt, slot, channel);
-- 
2.47.3

[PATCH v2 2/2] Revert "EDAC/i5000: Mark as BROKEN"

[Andrew Luebke]

This reverts commit 7556419180a3 ("EDAC/i5000: Mark as BROKEN").

The driver was marked BROKEN, instead of taking the slot loop fix,
because nobody had the hardware to check it with single- and dual-ranked
DIMMs [1]. With the previous patch it loads on an HP xw8400 (5000X) with
all eight slots populated, reports single- and dual-ranked FB-DIMMs with
the right size at the right branch, channel and slot, and stays clean
under KASAN and UBSAN bounds checking.

Two-channel boards (5000V/5000Z) and error reporting are not tested.

Add myself as a reviewer for the driver, as I have the hardware to test
future patches.

[1] https://lore.kernel.org/all/YyxpbRkuLPIcW1Om@zn.tnic/

Assisted-by: LLM
Signed-off-by: Andrew Luebke <andrew.luebke@gmail.com>
---
v2: add myself as a reviewer in MAINTAINERS (Boris)

 MAINTAINERS          | 1 +
 drivers/edac/Kconfig | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 222b7fc2c3aa..e1aa80d5bda3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9702,6 +9702,7 @@ S:	Orphan
 F:	drivers/edac/i3200_edac.c
 
 EDAC-I5000
+R:	Andrew Luebke <andrew.luebke@gmail.com>
 L:	linux-edac@vger.kernel.org
 S:	Maintained
 F:	drivers/edac/i5000_edac.c
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 4c70729b0d3a..63f263c4263d 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -218,7 +218,6 @@ config EDAC_I82860
 config EDAC_I5000
 	tristate "Intel Greencreek/Blackford chipset"
 	depends on X86 && PCI
-	depends on BROKEN
 	help
 	  Support for error detection and correction the Intel
 	  Greekcreek/Blackford chipsets.
-- 
2.47.3