[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()

[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

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

[Rihyeon Kim]

Hello,

Thanks a lot for the review.

> 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.

Good point, I missed that.  Updated in v2.

> 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:

You are right, thanks.  I had missed that dev_set_name() could not fail at
all back then, and that the commit I picked was only replacing an snprintf()
into the same fixed array.  Both patches in v2 use 1fa5ae857bb1 now, and the
commit log says kvasprintf_const().

> 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.

Thanks for the suggestion.  It is patch 2/2 in v2, with the NULL assignment
as you described, and with Suggested-by: and a Link: to Yang Yingliang's
original posting.

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

Agreed, I left pci_device_add() alone.  It also overlaps with the driver
core series that changes when device_add() frees dev->p [1], so I would
rather wait and see how that one settles before proposing anything there.
If it still looks worth doing afterwards, I can revisit it then.

[1] https://lore.kernel.org/all/20260716230411.2767394-2-tarunsahu@google.com/

Thanks again,
Rihyeon