Re: [PATCH net-next 11/12] misc: tc956x_pci: add TC956x/QPS615 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Dangling `platform_data` pointer and stateless device link lead to Use-After-Free
- [High] Devres lifecycle violation causes memory leak of device links
- [High] Reference count leak of PCI peer device
- [High] Missing `pci_free_irq_vectors()` in error paths and remove function
--

commit 7eab7fd4b209226034d88bc445ad71c833dda6b7
Author: Alex Elder <elder@riscstar.com>

misc: tc956x_pci: add TC956x/QPS615 support

This commit introduces support for the Toshiba TC956x Ethernet AVB/TSN
bridge, configuring its PCI functions and creating auxiliary devices to
support the XGMAC interfaces.

diff --git a/drivers/misc/tc956x_pci.c b/drivers/misc/tc956x_pci.c
--- /dev/null
+++ b/drivers/misc/tc956x_pci.c

[ ... ]

> +static struct tc956x_chip *chip_get(struct pci_dev *pdev)
> +{

[ ... ]

> +	/* Function 1 has to get the chip structure from function 0 */
> +	peer = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));
> +	if (!peer)
> +		return ERR_PTR(-ENXIO);

Does this leak the pci_dev reference count?

pci_get_slot() returns a device with its reference count incremented, but I
do not see a matching pci_dev_put(peer) in either the success path or when
returning -EPROBE_DEFER below.

> +
> +	/* If function 0 hasn't set up the chip yet, try again later */
> +	chip = dev_get_platdata(&peer->dev);
> +	if (!chip)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	/* Mark function 1's device as dependent on function 0 */
> +	link = device_link_add(dev, &peer->dev, DL_FLAG_STATELESS);

Is DL_FLAG_STATELESS safe to use here?

Stateless links do not enforce unbind ordering. If function 0 is unbound by
the user, its devres will free the chip structure and unmap the MMIO regions
while function 1 remains bound. Will function 1 cause a use-after-free if it
accesses the freed chip state or unmapped MMIO space?

> +	if (!link)
> +		return ERR_PTR(-ENODEV);
> +
> +	ret = devm_add_action_or_reset(&peer->dev, chip_link_del, link);

Should this cleanup action be attached to dev (function 1) instead of
peer->dev (function 0)?

If function 1's probe fails downstream, its own devres list is rolled back,
but this cleanup action remains attached to function 0. Will this leak the
device link and accumulate duplicate devres actions on function 0 each time
function 1 defers its probe?

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return chip;
> +}

[ ... ]

> +static int
> +tc956x_function_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{

[ ... ]

> +	/* We called pcim_enable_device() so this will be freed automatically */
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (ret < 1)
> +		return dev_err_probe(dev, ret ? : -EIO,
> +				     "failed to allocate IRQ vectors\n");

Is this comment accurate?

According to PCI subsystem guidelines, pci_alloc_irq_vectors() requires
explicit manual cleanup. Should the error paths below and the remove function
call pci_free_irq_vectors(pdev) to prevent leaking system IRQ resources?

> +
> +	ret = pci_irq_vector(pdev, 0);
> +	if (ret < 1)
> +		return dev_err_probe(dev, ret ? : -EIO, "failed to get IRQ\n");
> +	msigen_irq = ret;
> +
> +	ret = chip_init(chip, pdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize chip\n");
> +
> +	/* We're ready; the other function can now probe */
> +	dev->platform_data = chip;
> +
> +	ret = function_xgmac_adev_add(pdev, chip, msigen_irq);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add xgmap device\n");

If function_xgmac_adev_add() fails, devm_kzalloc() will automatically free
the chip structure upon exit, but dev->platform_data is not cleared.

Will function 1's chip_get() retrieve this dangling pointer via
dev_get_platdata() and cause a use-after-free when it probes later?

> +
> +	return 0;
> +}
> +
> +static void tc956x_function_remove(struct pci_dev *pdev)
> +{
> +	struct tc956x_chip *chip = dev_get_platdata(&pdev->dev);
> +
> +	pci_clear_master(pdev);
> +
> +	if (&pdev->dev == chip->dev)
> +		chip_msigen_disable(chip);
> +}

Does this function need a call to pci_free_irq_vectors(pdev) to clean up the
allocated IRQ vectors before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260501155421.3329862-1-elder@riscstar.com?part=11