linux-kernel - Re: [PATCH] PCI: take the rescan lock when adding devices during host probe

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: <CAMRc=MeFww5wyj8XmUMT0zkD-D_EUS+4+7xNQYwgzsMaZ4zXBQ@mail.gmail.com>
Date: Wed, 2 Oct 2024 10:36:38 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH] PCI: take the rescan lock when adding devices during host probe

On Tue, Oct 1, 2024 at 11:11 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Thu, Sep 26, 2024 at 03:09:23PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > Since adding the PCI power control code, we may end up with a race
> > between the pwrctl platform device rescanning the bus and the host
> > controller probe function. The latter needs to take the rescan lock when
> > adding devices or may crash.
> >
> > Reported-by: Konrad Dybcio <konradybcio@...nel.org>
> > Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > ---
> >  drivers/pci/probe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 4f68414c3086..f1615805f5b0 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -3105,7 +3105,9 @@ int pci_host_probe(struct pci_host_bridge *bridge)
> >       list_for_each_entry(child, &bus->children, node)
> >               pcie_bus_configure_settings(child);
> >
> > +     pci_lock_rescan_remove();
> >       pci_bus_add_devices(bus);
> > +     pci_unlock_rescan_remove();
>
> Seems like we do need locking here, but don't we need a more
> comprehensive change?  There are many other callers of
> pci_bus_add_devices(), and most of them look similarly unprotected.
>

>From a quick glance it looks like the majority of users are specific
drivers (controller, hotplug, etc.). The calls inside pci_rescan_bus()
and pci_rescan_bus_bridge_resize() are already protected from what I
can tell. I'm not saying that the driver calls shouldn't be fixed but
there's no immediate danger. This however fixes an issue we hit with
PCI core so I'd send it upstream now and then we can think about the
other use-cases.

Bart