Re: [PATCH 2/2] PCI: Fix pci_host_bridge struct device release/free handling

[Rob Herring <robh@kernel.org>]

On Thu, May 14, 2020 at 5:30 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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@linaro.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  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