On Thu, May 14, 2020 at 5:30 AM Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
>
> On Wed, May 13, 2020 at 05:38:59PM -0500, Rob Herring wrote:
> > The PCI code has several paths where the struct pci_host_bridge is freed
> > directly. This is wrong because it contains a struct device which is
> > refcounted and should be freed using put_device(). This can result in
> > use-after-free errors. I think this problem has existed since 2012 with
> > commit 7b5436635800 ("PCI: add generic device into pci_host_bridge
> > struct"). It generally hasn't mattered as most host bridge drivers are
> > still built-in and can't unbind.
> >
> > The problem is a struct device should never be freed directly once
> > device_initialize() is called and a ref is held, but that doesn't happen
> > until pci_register_host_bridge(). There's then a window between
> > allocating the host bridge and pci_register_host_bridge() where kfree
> > should be used. This is fragile and requires callers to do the right
> > thing. To fix this, we need to split device_register() into
> > device_initialize() and device_add() calls, so that the host bridge
> > struct is always freed by using a put_device().
> >
> > devm_pci_alloc_host_bridge() is using devm_kzalloc() to allocate struct
> > pci_host_bridge which will be freed directly. Instead, we can use a
> > custom devres action to call put_device().
> >
> > Reported-by: Anders Roxell <anders.roxell@xxxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> > drivers/pci/probe.c | 36 +++++++++++++++++++-----------------
> > drivers/pci/remove.c | 2 +-
> > 2 files changed, 20 insertions(+), 18 deletions(-)
[...]
> > @@ -908,7 +910,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > if (err)
> > goto free;
> >
> > - err = device_register(&bridge->dev);
> > + err = device_add(&bridge->dev);
> > if (err) {
> > put_device(&bridge->dev);
> > goto free;
> > @@ -978,7 +980,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> >
> > unregister:
> > put_device(&bridge->dev);
> > - device_unregister(&bridge->dev);
> > + device_del(&bridge->dev);
>
> I think we need to execute device_del() first, then put_device().
No, because after the device_add, there's a get_device() adding the
bridge ptr to the bus. So the put_device here is for that. The final
put_device() is in the bridge free or devm action.
Rob
