[PATCH] PCI: Handle dev_set_name() failure in pci_setup_device()

[PATCH] PCI: Handle dev_set_name() failure in pci_setup_device()

[Rihyeon Kim]

pci_setup_device() calls dev_set_name() but ignores its return value.
dev_set_name() allocates the name with kvasprintf() and can fail, leaving
dev->kobj.name NULL.

Enumeration then continues with an unnamed device.  device_add() rejects
it with -EINVAL because dev_name() returns NULL and pci_bus_type has no
->dev_name callback, but pci_device_add() only WARNs about that failure:

  pci (null): [8086:2934] type 00 class 0x0c0300 conventional PCI endpoint
  pci (null): BAR 4 [io  0xc0e0-0xc0ff]
  ------------[ cut here ]------------
  ret < 0
  WARNING: drivers/pci/probe.c:2768 at pci_device_add+0x133b/0x1810
  Call Trace:
   pci_scan_single_device+0x1d0/0x240
   pci_scan_slot+0x1c9/0x7c0
   pci_scan_child_bus_extend+0x6b/0x7b0
   pci_rescan_bus+0x18/0x40
   rescan_store+0xfb/0x130

The device was already put on bus->devices before device_add() ran and is
not removed from it, so pci_bus_add_devices() later in the same
pci_rescan_bus() call picks it up and hands it to __device_attach(), which
dereferences dev->p.  device_add() freed dev->p on its error path, so this
is a NULL dereference:

  Oops: general protection fault, probably for non-canonical address ...
  KASAN: null-ptr-deref in range [0x0000000000000108-0x000000000000010f]
  RIP: 0010:__device_attach+0xb0/0x4d0
  Call Trace:
   device_initial_probe+0xaf/0xd0
   pci_bus_add_device+0xc5/0x100
   pci_bus_add_devices+0x9a/0x1f0
   pci_rescan_bus+0x2a/0x40
   rescan_store+0xfb/0x130
  Kernel panic - not syncing: Fatal exception

Propagate the error instead.  pci_setup_device() already returns int and
both callers, pci_scan_device() and pci_iov_scan_device(), release the
device on failure, so no caller changes are needed.  Release the OF node
first, matching the existing error path for an unknown header type.

Fixes: eebfcfb52ce7 ("PCI: handle pci_name() being const")
Reported-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=87bb32e345aa80c0554b
Tested-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com
Signed-off-by: Rihyeon Kim <rihyeon8648@gmail.com>
---
Found by syzbot.  The reproducer is the one from the report, unmodified.

Tested on 48a5a7ab8d6a with syzbot's config for this bug (x86_64, KASAN,
CONFIG_FAILSLAB), under QEMU with "-machine q35 -usb" so that the
0000:00:1d.0 the reproducer removes and rescans exists:

  before  fault injected in dev_set_name() -> "pci (null): [8086:2934] ..."
          -> WARNING at probe.c:2768 -> GPF in __device_attach() -> panic
  after   fault still injected at the same call site, no unnamed device,
          no WARNING, no oops.  The device is skipped for that scan and
          comes back, driver bound, on the next "echo 1 > .../rescan".

syzbot reproduces this on arm64 too, at 0000:00:01.0 with a virtio device,
so it is not specific to that controller.

Two things I noticed while working on this and deliberately left alone,
since they look like separate changes:

- pci_scan_device() releases the pci_dev with kfree() rather than
  put_device(), so on the existing "unknown header type" -EIO path the
  name allocated by dev_set_name() is leaked.  That was reported in 2022
  [1] but never applied.

- pci_device_add() ignores device_add() failure entirely; the WARN_ON() is
  all there is and the half-added device stays on bus->devices.  I built a
  kernel with [2] applied (it stops device_add() from freeing dev->p) and
  the NULL dereference above does go away, but the WARNING and the stale
  bus->devices entry remain.  Happy to follow up on that separately if you
  think it is worth doing.

[1] https://lore.kernel.org/linux-pci/20220817084444.2876059-1-yangyingliang@huawei.com/
[2] https://lore.kernel.org/all/20260716230411.2767394-2-tarunsahu@google.com/

 drivers/pci/probe.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd0abbc63e18..474c6cb327be 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2052,9 +2052,13 @@ int pci_setup_device(struct pci_dev *dev)
 	 */
 	dev->msi_addr_mask = DMA_BIT_MASK(64);
 
-	dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
-		     dev->bus->number, PCI_SLOT(dev->devfn),
-		     PCI_FUNC(dev->devfn));
+	err = dev_set_name(&dev->dev, "%04x:%02x:%02x.%d",
+			   pci_domain_nr(dev->bus), dev->bus->number,
+			   PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+	if (err) {
+		pci_release_of_node(dev);
+		return err;
+	}
 
 	class = pci_class(dev);
 
-- 
2.43.0

Re: [PATCH] PCI: Handle dev_set_name() failure in pci_setup_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled device_add() failure in pci_device_add() leaves a corrupt device on bus->devices, triggering NULL pointer dereferences.
--

--- Patch [1]: [PATCH] PCI: Handle dev_set_name() failure in pci_setup_device() ---
commit a0f24b9a341960f58d26143c84bb644d8d2b737e
Author: Rihyeon Kim <rihyeon8648@gmail.com>

PCI: Handle dev_set_name() failure in pci_setup_device()

This commit checks the return value of dev_set_name() during
pci_setup_device() to prevent enumeration of unnamed devices. By
propagating the error early, it avoids a later failure in device_add()
that would trigger a NULL pointer dereference.

Reported-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=87bb32e345aa80c0554b
Tested-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com

> Two things I noticed while working on this and deliberately left alone,
> since they look like separate changes:

[ ... ]

> - pci_device_add() ignores device_add() failure entirely; the WARN_ON() is
>   all there is and the half-added device stays on bus->devices.  I built a
>   kernel with [2] applied (it stops device_add() from freeing dev->p) and
>   the NULL dereference above does go away, but the WARNING and the stale
>   bus->devices entry remain.  Happy to follow up on that separately if you
>   think it is worth doing.

[Severity: High]
This is a pre-existing issue, but should the failure of device_add() in
pci_device_add() be handled with explicit error paths instead of just
emitting a warning?

When device_add() fails, it cleans up and frees dev->p:

drivers/pci/probe.c:pci_device_add() {
    ...
    ret = device_add(&dev->dev);
    WARN_ON(ret < 0);
    ...
}

Leaving the initialized device on the globally visible bus->devices list
allows pci_bus_add_devices() to iterate over it and pass it to
__device_attach().

Could this result in a kernel panic when __device_attach() unconditionally
dereferences the freed dev->p?

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18d..474c6cb327be5 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725104747.226575-1-rihyeon8648@gmail.com?part=1

Re: [PATCH] PCI: Handle dev_set_name() failure in pci_setup_device()

[Krzysztof Wilczyński]

Hello,

[...]
> Propagate the error instead.  pci_setup_device() already returns int and
> both callers, pci_scan_device() and pci_iov_scan_device(), release the
> device on failure, so no caller changes are needed.  Release the OF node
> first, matching the existing error path for an unknown header type.

The kernel-doc for pci_setup_device() will probably need to be updated to
reflect changes in what the function returns on failure, since now you have
the -EIO and also -ENOMEM, potentially.

> Fixes: eebfcfb52ce7 ("PCI: handle pci_name() being const")

Probably:

  Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")

The commit you have references a state of the code, a much older code base,
where the implementation was fundamentally different, per:

  va_start(vargs, fmt);
  vsnprintf(dev->bus_id, sizeof(dev->bus_id), fmt, vargs);
  va_end(vargs);
  return 0;

The kvasprintf(), the kobject_set_name_vargs() uses internally, has been
replaced with kvasprintf_const() since commit f773f32d71a4 ("lib/kobject.c:
use kvasprintf_const for formatting ->name"), so the commit log is not
strictly correct.

[...]
> Two things I noticed while working on this and deliberately left alone,
> since they look like separate changes:
> 
> - pci_scan_device() releases the pci_dev with kfree() rather than
>   put_device(), so on the existing "unknown header type" -EIO path the
>   name allocated by dev_set_name() is leaked.  That was reported in 2022
>   [1] but never applied.

The fix there is valid and would be nice to also pick it up.  Might use
a little...

  dev->dev.kobj.name = NULL;

After the kfree_const().

Feel free to pick it up, and include here as a second patch, so a small
series.  Don't forget to credit Yang Yingliang, if you do decide to
follow-up.

> - pci_device_add() ignores device_add() failure entirely; the WARN_ON() is
>   all there is and the half-added device stays on bus->devices.  I built a
>   kernel with [2] applied (it stops device_add() from freeing dev->p) and
>   the NULL dereference above does go away, but the WARNING and the stale
>   bus->devices entry remain.  Happy to follow up on that separately if you
>   think it is worth doing.

This one I am not sure.  The NULL-assignment move looks awkward there.

[...]
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18..474c6cb327be 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2052,9 +2052,13 @@ int pci_setup_device(struct pci_dev *dev)
>  	 */
>  	dev->msi_addr_mask = DMA_BIT_MASK(64);
>  
> -	dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
> -		     dev->bus->number, PCI_SLOT(dev->devfn),
> -		     PCI_FUNC(dev->devfn));
> +	err = dev_set_name(&dev->dev, "%04x:%02x:%02x.%d",
> +			   pci_domain_nr(dev->bus), dev->bus->number,
> +			   PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> +	if (err) {
> +		pci_release_of_node(dev);
> +		return err;
> +	}
>  
>  	class = pci_class(dev);

Looks good!

  Reviewed-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Thank you for the fix!

	Krzysztof