On Thu, May 14, 2020 at 12:39 AM Rob Herring <robh@xxxxxxxxxx> 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>
Thanks for working your way through that bit of code
and fixing my mistakes!
Your description makes a lot of sense and the code looks
reasonable, but I don't understand my own work enough any
more to be sure I didn't miss anything.
Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
