[PATCH] EDAC, sb_edac: Fix out of bound in PCI multi segment env

[PATCH] EDAC, sb_edac: Fix out of bound in PCI multi segment env

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

KASAN reported the following slab-out-of-bounds when sb_edac
module was loaded on Broadwell machine which has two PCI segments.

==================================================================
BUG: KASAN: slab-out-of-bounds in sbridge_get_all_devices.constprop.14+0x75f/0x96a [sb_edac]
Read of size 8 at addr ffff8c0d44dfe850 by task modprobe/4221

CPU: 19 PID: 4221 Comm: modprobe Not tainted 4.18.0-rc5 #2
Call Trace:
 dump_stack+0xc2/0x16b
 ? show_regs_print_info+0x5/0x5
 ? kmsg_dump_rewind_nolock+0xd9/0xd9
 ? pci_get_dev_by_id+0x57/0x70
 ? pci_get_device+0x155/0x210
 print_address_description+0x6a/0x270
 kasan_report+0x258/0x380
 ? sbridge_get_all_devices.constprop.14+0x75f/0x96a [sb_edac]
 sbridge_get_all_devices.constprop.14+0x75f/0x96a [sb_edac]
...
Allocated by task 4221:
 kasan_kmalloc+0xa0/0xd0
 __kmalloc+0x112/0x230
 sbridge_get_all_devices.constprop.14+0x555/0x96a [sb_edac]
 sbridge_init+0x1ba/0x4000 [sb_edac]
 do_one_initcall+0xaa/0x401
 do_init_module+0x20b/0x676
 load_module+0x443f/0x6dc0
 __do_sys_finit_module+0x25f/0x2c0
 do_syscall_64+0x14e/0x4b0
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
==================================================================

The out bounds happened because the index of sbridge_dev->pdev[] is over
than the allocated size.
---
static int sbridge_get_onedevice(struct pci_dev **prev,
...
        if (sbridge_dev->pdev[sbridge_dev->i_devs]) { <= !! out bounds HERE !!
                sbridge_printk(KERN_ERR,
                        "Duplicated device for %04x:%04x\n",
                        PCI_VENDOR_ID_INTEL, dev_descr->dev_id);
---

If a sbridge_dev which has the same bus as the new device is already exists,
the sbridge_dev is assigned to the new one.

---
static struct sbridge_dev *get_sbridge_dev(u8 bus, enum domain dom, int multi_bus,
                                           struct sbridge_dev *prev)
...
        list_for_each_entry_from(sbridge_dev, &sbridge_edac_list, list) {
                if (sbridge_dev->bus == bus && (dom == SOCK || dom == sbridge_dev->dom))
                        return sbridge_dev;
        }
---

If the system has multi PCI segment and the each bus number is same,
the same sbridge_dev is assigned. So, the index of sbridge_dev->pdev[]
is incremented by each segments.
As the result, the index is over than the allocated size, then the
out-of-bounds happens.

This patch introduces a segment member to sbridge_dev to separate
it each PCI segments.

Fixes: e2f747b1f42a ("EDAC, sb_edac: Assign EDAC memory controller per h/w controller")

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 drivers/edac/sb_edac.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 4a89c80..07726fb 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -352,6 +352,7 @@ struct pci_id_table {
 
 struct sbridge_dev {
 	struct list_head	list;
+	int			seg;
 	u8			bus, mc;
 	u8			node_id, source_id;
 	struct pci_dev		**pdev;
@@ -729,7 +730,8 @@ static inline int numcol(u32 mtr)
 	return 1 << cols;
 }
 
-static struct sbridge_dev *get_sbridge_dev(u8 bus, enum domain dom, int multi_bus,
+static struct sbridge_dev *get_sbridge_dev(int seg, u8 bus, enum domain dom,
+					   int multi_bus,
 					   struct sbridge_dev *prev)
 {
 	struct sbridge_dev *sbridge_dev;
@@ -747,14 +749,15 @@ static struct sbridge_dev *get_sbridge_dev(u8 bus, enum domain dom, int multi_bu
 				      : sbridge_edac_list.next, struct sbridge_dev, list);
 
 	list_for_each_entry_from(sbridge_dev, &sbridge_edac_list, list) {
-		if (sbridge_dev->bus == bus && (dom == SOCK || dom == sbridge_dev->dom))
+		if ((sbridge_dev->seg == seg) && (sbridge_dev->bus == bus) &&
+				(dom == SOCK || dom == sbridge_dev->dom))
 			return sbridge_dev;
 	}
 
 	return NULL;
 }
 
-static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum domain dom,
+static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain dom,
 					     const struct pci_id_table *table)
 {
 	struct sbridge_dev *sbridge_dev;
@@ -771,6 +774,7 @@ static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum domain dom,
 		return NULL;
 	}
 
+	sbridge_dev->seg = seg;
 	sbridge_dev->bus = bus;
 	sbridge_dev->dom = dom;
 	sbridge_dev->n_devs = table->n_devs_per_imc;
@@ -2246,6 +2250,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
 	struct sbridge_dev *sbridge_dev = NULL;
 	const struct pci_id_descr *dev_descr = &table->descr[devno];
 	struct pci_dev *pdev = NULL;
+	int seg = 0;
 	u8 bus = 0;
 	int i = 0;
 
@@ -2276,10 +2281,12 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
 		/* End of list, leave */
 		return -ENODEV;
 	}
+	seg = pci_domain_nr(pdev->bus);
 	bus = pdev->bus->number;
 
 next_imc:
-	sbridge_dev = get_sbridge_dev(bus, dev_descr->dom, multi_bus, sbridge_dev);
+	sbridge_dev = get_sbridge_dev(seg, bus, dev_descr->dom,
+				      multi_bus, sbridge_dev);
 	if (!sbridge_dev) {
 		/* If the HA1 wasn't found, don't create EDAC second memory controller */
 		if (dev_descr->dom == IMC1 && devno != 1) {
@@ -2292,7 +2299,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
 		if (dev_descr->dom == SOCK)
 			goto out_imc;
 
-		sbridge_dev = alloc_sbridge_dev(bus, dev_descr->dom, table);
+		sbridge_dev = alloc_sbridge_dev(seg, bus, dev_descr->dom, table);
 		if (!sbridge_dev) {
 			pci_dev_put(pdev);
 			return -ENOMEM;
-- 
2.18.0

Re: [PATCH] EDAC, sb_edac: Fix out of bound in PCI multi segment env

[Tony Luck]

On Thu, Jul 19, 2018 at 7:07 AM, Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>
> KASAN reported the following slab-out-of-bounds when sb_edac
> module was loaded on Broadwell machine which has two PCI segments.

Although you found this with KASAN as an out of bounds array reference,
the real problem is that the sb_edac.c driver didn't know about systems
with segmented PCI busses.

So the Subject: for the e-mail (and thus the commit message) should
be:

[PATCH] EDAC, sb_edac: Add support for systems with segmented PCI busses

Otherwise this looks fine.

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony

Re: [PATCH] EDAC, sb_edac: Fix out of bound in PCI multi segment env

[Masayoshi Mizuma]

Hi Tony,

Thank you for your review!

On 07/24/2018 01:01 PM, Tony Luck wrote:
> On Thu, Jul 19, 2018 at 7:07 AM, Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>>
>> KASAN reported the following slab-out-of-bounds when sb_edac
>> module was loaded on Broadwell machine which has two PCI segments.
> 
> Although you found this with KASAN as an out of bounds array reference,
> the real problem is that the sb_edac.c driver didn't know about systems
> with segmented PCI busses.
> 
> So the Subject: for the e-mail (and thus the commit message) should
> be:
> 
> [PATCH] EDAC, sb_edac: Add support for systems with segmented PCI busses

You are right. I will modify the subject and commit message, and resend it.

> 
> Otherwise this looks fine.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>

Thanks!
Masa

> 
> -Tony
>