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

On 5/2/26 10:56 AM, [email protected] wrote:
> 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

All issues highlighted here are legitimate, and will be
addressed in the next version of the series.

> --
> 
> commit 7eab7fd4b209226034d88bc445ad71c833dda6b7
> Author: Alex Elder <[email protected]>
> 
> 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?

Yes it does, and this will be fixed in the next version of the patch.

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

We believe this is safe to use here.  If it is created successfully
a devm action is set up (for function 1) to remove the link.

> 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)?

Yes, this was a very good catch.  The name of the peer
variable will be changed to be "slot0" to emphasize which
device we're dealing with.

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

Looking at it now, it appears the comment is not accurate.

All error paths that follow will free this IRQ vector.
And the driver remove function will do so as well.

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

This too was a good catch.  The platform_data for function 0
will not be assigned a non-null value until it has completed
its probe successfully.

> 
>> +
>> +	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?

This will be added, as mentioned above.

					-Alex