linux-kernel - Re: [PATCH 4/8] PCI: make pci_host

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAErSpo7XZG3OqpBQMH46ptN9ZnAu9vF0wuNuhKCfQ_xZsNjaMg@mail.gmail.com>
Date:	Mon, 27 Feb 2012 10:44:48 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Jesse Barnes <jbarnes@...tuousgeek.org>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] PCI: make pci_host_bridge more robust

On Sun, Feb 26, 2012 at 4:33 PM, Yinghai Lu <yinghai@...nel.org> wrote:
> 1. change pci_host_bridge to find_pci_root_bridge.
> 2. separate find_pci_root_bus().
> 3. on any possible path return NULL.
>
> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
>
> ---
>  drivers/pci/host-bridge.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> Index: linux-2.6/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/host-bridge.c
> +++ linux-2.6/drivers/pci/host-bridge.c
> @@ -16,19 +16,31 @@ void add_to_pci_host_bridges(struct pci_
>        list_add_tail(&bridge->list, &pci_host_bridges);
>  }
>
> -static struct pci_host_bridge *pci_host_bridge(struct pci_dev *dev)
> +static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
>  {
>        struct pci_bus *bus;
> -       struct pci_host_bridge *bridge;
>
>        bus = dev->bus;
>        while (bus->parent)
>                bus = bus->parent;
>
> -       list_for_each_entry(bridge, &pci_host_bridges, list) {
> +       if (!pci_is_root_bus(bus))
> +               return NULL;

pci_is_root_bus() returns "!(pbus->parent)", so the effect of this patch is:

    while (bus->parent)
        bus = bus->parent;

    if (bus->parent)
        return NULL;

The only reason we exited the "while" loop is because "bus->parent ==
NULL", so what's the point of adding the "if" check afterwards?

> +       return bus;
> +}
> +
> +static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
> +{
> +       struct pci_bus *bus = find_pci_root_bus(dev);
> +       struct pci_host_bridge *bridge;
> +
> +       if (!bus)
> +               return NULL;

This should never happen.  If there's a way we can create a pci_dev
that's not under a pci_host_bridge, I think that is a bug, and we
should fix that rather than papering over it here.

I don't think we should apply this patch.

Bjorn

> +       list_for_each_entry(bridge, &pci_host_bridges, list)
>                if (bridge->bus == bus)
>                        return bridge;
> -       }
>
>        return NULL;
>  }
> @@ -42,10 +54,13 @@ void __weak pcibios_resource_to_bus(stru
>                                    struct pci_bus_region *region,
>                                    struct resource *res)
>  {
> -       struct pci_host_bridge *bridge = pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;
>        resource_size_t offset = 0;
>
> +       if (!bridge)
> +               goto no_bridge;
> +
>        list_for_each_entry(window, &bridge->windows, list) {
>                if (resource_type(res) != resource_type(window->res))
>                        continue;
> @@ -56,6 +71,7 @@ void __weak pcibios_resource_to_bus(stru
>                }
>        }
>
> +no_bridge:
>        region->start = res->start - offset;
>        region->end = res->end - offset;
>  }
> @@ -70,12 +86,16 @@ static bool region_contains(struct pci_b
>  void __weak pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>                                    struct pci_bus_region *region)
>  {
> -       struct pci_host_bridge *bridge = pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>        struct pci_host_bridge_window *window;
> -       struct pci_bus_region bus_region;
>        resource_size_t offset = 0;
>
> +       if (!bridge)
> +               goto no_bridge;
> +
>        list_for_each_entry(window, &bridge->windows, list) {
> +               struct pci_bus_region bus_region;
> +
>                if (resource_type(res) != resource_type(window->res))
>                        continue;
>
> @@ -88,6 +108,7 @@ void __weak pcibios_bus_to_resource(stru
>                }
>        }
>
> +no_bridge:
>        res->start = region->start + offset;
>        res->end = region->end + offset;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/