[PATCH v2 0/2] PCI: Fix dev_set_name() error handling in pci_setup_device()

[PATCH v2 0/2] PCI: Fix dev_set_name() error handling in pci_setup_device()

[Rihyeon Kim]

Hi,

pci_setup_device() calls dev_set_name() without checking it.  syzbot hits
this with fault injection, where it turns into a WARNING and then a NULL
pointer dereference.  Patch 1 checks the return value and propagates it.

Patch 2 is the neighboring leak on the "unknown header type" path, where
the name that dev_set_name() allocated is never freed.  It was reported by
Yang Yingliang in 2022, and Krzysztof suggested picking it up here.

I am not sure about the attribution on patch 2, since most of it is Yang
Yingliang's fix with the NULL assignment added.  If it should look
different, or if that patch would be better sent on its own, please let me
know and I will respin.

This is my first patch to the kernel, so please let me know if I got
anything wrong about the form.

v1: https://lore.kernel.org/linux-pci/20260725104747.226575-1-rihyeon8648@gmail.com/

v2:
 - 1/2: Fixes: points at 1fa5ae857bb1 rather than eebfcfb52ce7, since
   dev_set_name() could not fail before that commit (Krzysztof)
 - 1/2: update the kernel-doc for the new return value (Krzysztof)
 - 1/2: say kvasprintf_const() rather than kvasprintf() in the commit log
   (Krzysztof)
 - 1/2: pick up Reviewed-by
 - 2/2: new patch (Krzysztof)

Thanks,
Rihyeon

Rihyeon Kim (2):
  PCI: Handle dev_set_name() failure in pci_setup_device()
  PCI: Free the device name on the pci_setup_device() error path

 drivers/pci/probe.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

[PATCH v2 1/2] 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() calls kobject_set_name_vargs(), which in turn allocates the
name with kvasprintf_const() and can fail, leaving dev->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 added to 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 finds it and passes it to __device_attach(), which
dereferences dev->p.  device_add() freed dev->p on its error path, so this
is a NULL pointer 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

Check the return value and propagate the error instead.  pci_setup_device()
already returns int, and both callers, pci_scan_device() and
pci_iov_scan_device(), already release the device when it fails.  Release
the OF node first, like the existing unknown header type path does, and
update the kernel-doc for the new return value.

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Reported-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=87bb32e345aa80c0554b
Tested-by: syzbot+87bb32e345aa80c0554b@syzkaller.appspotmail.com
Reviewed-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
Assisted-by: Claude:claude-opus-5 sparse
Signed-off-by: Rihyeon Kim <rihyeon8648@gmail.com>
---
The reproducer is the one from the syzbot 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 it removes and rescans exists.  Without the patch it gives the
WARNING and then the panic quoted above; with it the allocation still fails
in dev_set_name(), enumeration skips the device, and the device comes back
on the next rescan.  syzbot tested it as well.

The report also has an arm64 crash at 0000:00:01.0 with a virtio device, so
this is not specific to that controller.

v2:
 - Fixes: points at 1fa5ae857bb1 rather than eebfcfb52ce7 (Krzysztof)
 - update the kernel-doc for the new return value (Krzysztof)
 - say kvasprintf_const() rather than kvasprintf() in the commit log
   (Krzysztof)
 - pick up Reviewed-by

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

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd0abbc63e18..3e6f0eb7d456 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2010,8 +2010,9 @@ static const char *pci_type_str(struct pci_dev *dev)
  * Initialize the device structure with information about the device's
  * vendor,class,memory and IO-space addresses, IRQ lines etc.
  * Called at initialisation of the PCI subsystem and by CardBus services.
- * Returns 0 on success and negative if unknown type of device (not normal,
- * bridge or CardBus).
+ * Returns 0 on success, -ENOMEM if the device name could not be allocated,
+ * and -EIO if the device is of an unknown type (not normal, bridge or
+ * CardBus).
  */
 int pci_setup_device(struct pci_dev *dev)
 {
@@ -2052,9 +2053,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

[PATCH v2 2/2] PCI: Free the device name on the pci_setup_device() error path

[Rihyeon Kim]

pci_setup_device() sets the device name with dev_set_name(), which
allocates it.  When the header type turns out to be unknown the function
returns -EIO, and pci_scan_device() then releases the device with kfree()
rather than put_device(), because the device has not been initialized with
device_initialize() yet.  The name is never freed, so it is leaked.

Free it there and clear the pointer, so the freed name is not left behind
in the kobject.

Yang Yingliang reported the same leak in 2022.

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Suggested-by: Yang Yingliang <yangyingliang@huawei.com>
Link: https://lore.kernel.org/linux-pci/20220817084444.2876059-1-yangyingliang@huawei.com/
Assisted-by: Claude:claude-opus-5 sparse
Signed-off-by: Rihyeon Kim <rihyeon8648@gmail.com>
---
v2: new patch, picked up from Yang Yingliang's 2022 posting as suggested in
    review, with the NULL assignment added.

 drivers/pci/probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3e6f0eb7d456..74d84093943b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2192,6 +2192,8 @@ int pci_setup_device(struct pci_dev *dev)
 	default:				    /* unknown header */
 		pci_err(dev, "unknown header type %02x, ignoring device\n",
 			dev->hdr_type);
+		kfree_const(dev->dev.kobj.name);
+		dev->dev.kobj.name = NULL;
 		pci_release_of_node(dev);
 		return -EIO;
 
-- 
2.43.0

Re: [PATCH v2 2/2] PCI: Free the device name on the pci_setup_device() error path

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory allocated by early fixups (e.g., `dev->saved_cap_space` and `dev->dma_alias_mask`) is leaked when `pci_setup_device()` encounters an unknown header type.
--

--- Patch [2]: [PATCH v2 2/2] PCI: Free the device name on the pci_setup_device() error path ---
commit 25426f7c4f229419785305695a130234f0fdabc2
Author: Rihyeon Kim <rihyeon8648@gmail.com>

PCI: Free the device name on the pci_setup_device() error path
    
This commit frees the device name in the pci_setup_device() error path
for unknown header types. Since device_initialize() hasn't been called,
pci_scan_device() simply uses kfree() to release the device, which
previously leaked the allocated name.

Link: https://lore.kernel.org/linux-pci/20220817084444.2876059-1-yangyingliang@huawei.com/

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3e6f0eb7d456c..74d84093943bb 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2192,6 +2192,8 @@ int pci_setup_device(struct pci_dev *dev)
>  	default:				    /* unknown header */
>  		pci_err(dev, "unknown header type %02x, ignoring device\n",
>  			dev->hdr_type);
> +		kfree_const(dev->dev.kobj.name);
> +		dev->dev.kobj.name = NULL;
>  		pci_release_of_node(dev);
 
[Severity: High]
This isn't a bug introduced by this patch, but since we're fixing leaks on
this exact error path, does this also leak memory allocated by early fixups?

Earlier in pci_setup_device(), we execute:

    pci_fixup_device(pci_fixup_early, dev);

Some of these early fixups dynamically allocate memory and attach it to the
device structure. For example, quirk_intel_qat_vf_cap() allocates state
buffers:

drivers/pci/quirks.c:quirk_intel_qat_vf_cap() {
    ...
    state = kzalloc(sizeof(*state) + size, GFP_KERNEL);
    ...
    pci_add_saved_cap(pdev, state);
}

Similarly, pci_add_dma_alias() allocates a bitmap:

drivers/pci/pci.c:pci_add_dma_alias() {
    ...
    dev->dma_alias_mask = bitmap_zalloc(MAX_NR_DEVFNS, GFP_KERNEL);
    ...
}

Because pci_scan_device() handles the -EIO return by directly calling
kfree(dev) instead of going through standard device release, it seems these
dynamically allocated resources (dev->saved_cap_space, dev->dma_alias_mask)
will also be permanently leaked here.

>  		return -EIO;

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

Re: [PATCH v2 1/2] 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] The patch prevents a specific failure in `device_add()` (NULL name) but fails to address the systemic bug in `pci_device_add()` where `device_add()` failures are ignored, which leaves structurally invalid devices on `bus->devices` and continues to allow NULL pointer dereference panics.
--

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

PCI: Handle dev_set_name() failure in pci_setup_device()

The commit adds error handling for dev_set_name() in pci_setup_device() to
prevent it from continuing with a NULL name. This avoids a later panic where
device_add() rejects the unnamed device but pci_device_add() ignores the
failure, leaving the invalid device on bus->devices with a freed dev->p.

Link: https://syzkaller.appspot.com/bug?extid=87bb32e345aa80c0554b

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18d..3e6f0eb7d456c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
[ ... ]
> @@ -2052,9 +2053,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;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while this change prevents
one trigger for the NULL pointer dereference, does it fully fix the
underlying vulnerability in pci_device_add()?

As the commit message describes, if device_add() fails, it frees dev->p
but pci_device_add() leaves the structurally invalid device on bus->devices,
which later causes __device_attach() to panic.

Since device_add() can fail for other reasons beyond an invalid name (such
as -ENOMEM or sysfs naming conflicts), won't those other failures still
trigger the exact same NULL pointer dereference?

In drivers/pci/probe.c:pci_device_add():

	ret = device_add(&dev->dev);
	WARN_ON(ret < 0);

If device_add() fails here for a different reason, it only triggers a
warning while leaving the device on the list.

Should pci_device_add() also be updated to properly remove the device from
bus->devices when device_add() fails?

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