| Message ID | 20241121094020.3679787-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI/pwrctl: Do not assume device node presence | expand |
On Thu, Nov 21, 2024 at 10:40 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > A PCI device normally does not have a device node, since the bus is > fully enumerable. Assuming that a device node is presence is likely s/presence/present/ > bad. > > The newly added pwrctl code assumes such and crashes with a NULL > pointer dereference. Besides that, of_find_device_by_node(NULL) > is likely going to return some random device. > > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/ > Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers") > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: Krzysztof Wilczyński <kwilczynski@kernel.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Cc: stable+noautosel@kernel.org # Depends on power supply check > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/pci/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 98910bc0fcc4..eca72e0c3b6c 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev) > * before PCI client drivers. > */ > pdev = of_find_device_by_node(dn); > - if (pdev && of_pci_supply_present(dn)) { > + if (dn && pdev && of_pci_supply_present(dn)) { > if (!device_link_add(&dev->dev, &pdev->dev, > DL_FLAG_AUTOREMOVE_CONSUMER)) > pci_err(dev, "failed to add device link to power control device %s\n", > -- > 2.47.0.338.g60cca15819-goog > > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
On 2024-11-21 10:40, Chen-Yu Tsai wrote: > A PCI device normally does not have a device node, since the bus is > fully enumerable. Assuming that a device node is presence is likely > bad. > > The newly added pwrctl code assumes such and crashes with a NULL > pointer dereference. Besides that, of_find_device_by_node(NULL) > is likely going to return some random device. > > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/ > Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers") > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: Krzysztof Wilczyński <kwilczynski@kernel.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Cc: stable+noautosel@kernel.org # Depends on power supply check > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/pci/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 98910bc0fcc4..eca72e0c3b6c 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev) > * before PCI client drivers. > */ > pdev = of_find_device_by_node(dn); > - if (pdev && of_pci_supply_present(dn)) { > + if (dn && pdev && of_pci_supply_present(dn)) { > if (!device_link_add(&dev->dev, &pdev->dev, > DL_FLAG_AUTOREMOVE_CONSUMER)) > pci_err(dev, "failed to add device link to power control device %s\n", Thanks for the fix, Tested-by: Klara Modin <klarasmodin@gmail.com>
On Thu, Nov 21, 2024 at 05:40:19PM +0800, Chen-Yu Tsai wrote: > A PCI device normally does not have a device node, since the bus is > fully enumerable. Assuming that a device node is presence is likely > bad. > I missed the fact that NULL ptr check is removed from of_pci_supply_present(). > The newly added pwrctl code assumes such and crashes with a NULL > pointer dereference. Besides that, of_find_device_by_node(NULL) > is likely going to return some random device. > Yeah, good catch. > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/ > Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers") > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: Krzysztof Wilczyński <kwilczynski@kernel.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Cc: stable+noautosel@kernel.org # Depends on power supply check > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Thanks for the fix! Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/pci/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 98910bc0fcc4..eca72e0c3b6c 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev) > * before PCI client drivers. > */ > pdev = of_find_device_by_node(dn); > - if (pdev && of_pci_supply_present(dn)) { > + if (dn && pdev && of_pci_supply_present(dn)) { > if (!device_link_add(&dev->dev, &pdev->dev, > DL_FLAG_AUTOREMOVE_CONSUMER)) > pci_err(dev, "failed to add device link to power control device %s\n", > -- > 2.47.0.338.g60cca15819-goog >
[+cc OF folks] On Thu, Nov 21, 2024 at 05:40:19PM +0800, Chen-Yu Tsai wrote: > A PCI device normally does not have a device node, since the bus is > fully enumerable. Assuming that a device node is presence is likely > bad. > The newly added pwrctl code assumes such and crashes with a NULL > pointer dereference. > Besides that, of_find_device_by_node(NULL) > is likely going to return some random device. I thought this sounded implausible, but after looking at the code, I think you're right, because bus_find_device() will use device_match_of_node(), which decides the device matches if "dev->of_node == np" (where "np" is NULL in this case). I'm sure many devices will have "dev->of_node == NULL", so it does seem like of_find_device_by_node(NULL) will return the first one it finds. This seems ... pretty janky and unexpected to me, but it's been this way for years, so maybe it's safe? Cc'ing the OF folks just in case. > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/ > Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers") > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: Krzysztof Wilczyński <kwilczynski@kernel.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Cc: stable+noautosel@kernel.org # Depends on power supply check > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/pci/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 98910bc0fcc4..eca72e0c3b6c 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev) > * before PCI client drivers. > */ > pdev = of_find_device_by_node(dn); > - if (pdev && of_pci_supply_present(dn)) { > + if (dn && pdev && of_pci_supply_present(dn)) { Thanks for this fix. Krzysztof restored the NULL pointer check in of_pci_supply_present(), so of_pci_supply_present(NULL) will return false, which should also solve this problem. If of_find_device_by_node(NULL) returned NULL, we wouldn't need this, but for now I guess we do. > if (!device_link_add(&dev->dev, &pdev->dev, > DL_FLAG_AUTOREMOVE_CONSUMER)) > pci_err(dev, "failed to add device link to power control device %s\n", > -- > 2.47.0.338.g60cca15819-goog >
On Thu, Nov 21, 2024 at 10:46 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc OF folks] > > On Thu, Nov 21, 2024 at 05:40:19PM +0800, Chen-Yu Tsai wrote: > > A PCI device normally does not have a device node, since the bus is > > fully enumerable. Assuming that a device node is presence is likely > > bad. > > > The newly added pwrctl code assumes such and crashes with a NULL > > pointer dereference. > > > Besides that, of_find_device_by_node(NULL) > > is likely going to return some random device. > > I thought this sounded implausible, but after looking at the code, I > think you're right, because bus_find_device() will use > device_match_of_node(), which decides the device matches if > "dev->of_node == np" (where "np" is NULL in this case). > > I'm sure many devices will have "dev->of_node == NULL", so it does > seem like of_find_device_by_node(NULL) will return the first one it > finds. > > This seems ... pretty janky and unexpected to me, but it's been this > way for years, so maybe it's safe? Cc'ing the OF folks just in case. This is a surprise to me, too. I think ACPI matching is broken in this way too. I'm sending out a fix. Rob
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 98910bc0fcc4..eca72e0c3b6c 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev) * before PCI client drivers. */ pdev = of_find_device_by_node(dn); - if (pdev && of_pci_supply_present(dn)) { + if (dn && pdev && of_pci_supply_present(dn)) { if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) pci_err(dev, "failed to add device link to power control device %s\n",
A PCI device normally does not have a device node, since the bus is fully enumerable. Assuming that a device node is presence is likely bad. The newly added pwrctl code assumes such and crashes with a NULL pointer dereference. Besides that, of_find_device_by_node(NULL) is likely going to return some random device. Reported-by: Klara Modin <klarasmodin@gmail.com> Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/ Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers") Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Cc: Krzysztof Wilczyński <kwilczynski@kernel.org> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Cc: stable+noautosel@kernel.org # Depends on power supply check Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- drivers/pci/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)