2010-02-25 15:46:56

by Thomas Gleixner

Subject: [patch] x86: pci: Prevent mmconfig memory corruption

commit ff097ddd4 (x86/PCI: MMCONFIG: manage pci_mmcfg_region as a
list, not a table) introduced a nasty memory corruption when
pci_mmcfg_list is empty.

pci_mmcfg_check_end_bus_number() dereferences pci_mmcfg_list.prev even
when the list is empty. The following write hits some variable near to
pci_mmcfg_list.

Further down a similar problem exists, where cfg->list.next is
dereferenced unconditionally and a comparison with some variable near
to pci_mmcfg_list happens.

Add a check for the last element into the for_each_entry() loop and
remove all the other crappy logic which is just a leftover of the old
array based code which was replaced by the list conversion.

Reported-by: Ingo Molnar <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: Jesse Barnes <[email protected]>
Cc: [email protected]
---
arch/x86/pci/mmconfig-shared.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -303,22 +303,17 @@ static void __init pci_mmcfg_check_end_b
{
struct pci_mmcfg_region *cfg, *cfgx;

- /* last one*/
- cfg = list_entry(pci_mmcfg_list.prev, typeof(*cfg), list);
- if (cfg)
- if (cfg->end_bus < cfg->start_bus)
- cfg->end_bus = 255;
-
- if (list_is_singular(&pci_mmcfg_list))
- return;
-
- /* don't overlap please */
+ /* Fixup overlaps */
list_for_each_entry(cfg, &pci_mmcfg_list, list) {
if (cfg->end_bus < cfg->start_bus)
cfg->end_bus = 255;

+ /* Don't access the list head ! */
+ if (cfg->list.next == &pci_mmcfg_list)
+ break;
+
cfgx = list_entry(cfg->list.next, typeof(*cfg), list);
- if (cfg != cfgx && cfg->end_bus >= cfgx->start_bus)
+ if (cfg->end_bus >= cfgx->start_bus)
cfg->end_bus = cfgx->start_bus - 1;
}
}

2010-02-25 15:48:49

by Bjorn Helgaas

Subject: Re: [patch] x86: pci: Prevent mmconfig memory corruption

On Thursday 25 February 2010 08:42:11 am Thomas Gleixner wrote:
> commit ff097ddd4 (x86/PCI: MMCONFIG: manage pci_mmcfg_region as a
> list, not a table) introduced a nasty memory corruption when
> pci_mmcfg_list is empty.

Ouch, I'm really sorry about that. Thanks for fixing it.

Bjorn

2010-02-25 16:31:15

by Jesse Barnes

Subject: Re: [patch] x86: pci: Prevent mmconfig memory corruption

On Thu, 25 Feb 2010 16:42:11 +0100 (CET)
Thomas Gleixner <[email protected]> wrote:

> commit ff097ddd4 (x86/PCI: MMCONFIG: manage pci_mmcfg_region as a
> list, not a table) introduced a nasty memory corruption when
> pci_mmcfg_list is empty.
>
> pci_mmcfg_check_end_bus_number() dereferences pci_mmcfg_list.prev even
> when the list is empty. The following write hits some variable near to
> pci_mmcfg_list.
>
> Further down a similar problem exists, where cfg->list.next is
> dereferenced unconditionally and a comparison with some variable near
> to pci_mmcfg_list happens.
>
> Add a check for the last element into the for_each_entry() loop and
> remove all the other crappy logic which is just a leftover of the old
> array based code which was replaced by the list conversion.
>
> Reported-by: Ingo Molnar <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: Jesse Barnes <[email protected]>
> Cc: [email protected]
> ---

Applied to my linux-next branch, thanks. I'll be part of my pull
request to Linus tomorrow.

--
Jesse Barnes, Intel Open Source Technology Center