Re: [PATCH v2 1/2] PCI: Handle dev_set_name() failure in pci_setup_device()

[tarunsahu@google.com]

Hi,

Thanks for this resolution!
Sashiko's comment seems valid and related to what we are resolving
in this series. So We can add one more to resolve the other issue.

Feel free to add:
Reviewed-by: Tarun Sahu <tarunsahu@google.com>

Rihyeon Kim <rihyeon8648@gmail.com> writes:

> 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