Re: [PATCH v4 8/9] PCI: Fix dyn_id add TOCTOU

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

Pre-existing issues:
- [High] `pci_match_device()` returns a pointer to the `id` field of a dynamically allocated `struct pci_dynid` after releasing the `drv->dynids.lock` spinlock, which can result in a Use-After-Free in the probe path if the ID is concurrently removed via sysfs.
--

--- Patch [8]: [PATCH v4 8/9] PCI: Fix dyn_id add TOCTOU ---
commit b2b9cd8b65d097234cb7fc1e84df45480eb112de
Author: Gary Guo <[email protected]>

PCI: Fix dyn_id add TOCTOU

Currently there is a TOCTOU issue in new_id_store() as the dyn ID insertion
in pci_add_dynid() and the pci_match_device() are in separate critical
sections.

Fix this by moving the existing ID check to inside pci_add_dynid() and only
check against the static ID table outside the critical section.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index c9424edb45481..ab3bb756ce89a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
[ ... ]
> @@ -153,7 +190,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>  						    struct pci_dev *dev)
>  {
>  	struct pci_dynid *dynid;
> -	const struct pci_device_id *found_id = NULL, *ids;
> +	const struct pci_device_id *found_id = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does pci_match_device() have a
use-after-free risk when returning dynamic IDs?

It looks like it returns a pointer to the id field of a dynamically allocated
struct pci_dynid after releasing the drv->dynids.lock spinlock:

pci_match_device()
    spin_lock(&drv->dynids.lock);
    list_for_each_entry(dynid, &drv->dynids.list, node) {
        if (pci_match_one_id(&dynid->id, &dev_id)) {
            found_id = &dynid->id;
            break;
        }
    }
    spin_unlock(&drv->dynids.lock);

    if (found_id)
        return found_id;

If remove_id_store() is triggered via sysfs concurrently, it can acquire
dynids.lock, remove the dynamic ID, and call kfree() on the structure:

remove_id_store()
    list_del(&dynid->node);
    kfree(dynid);

If this happens while __pci_device_probe() has matched the dynamic ID but
hasn't finished executing the driver's probe() routine, could the driver
dereference a freed struct pci_device_id?

__pci_device_probe()
    id = pci_match_device(drv, pci_dev);
    if (id)
        error = pci_call_probe(drv, pci_dev, id);

>  	struct pci_device_id dev_id;
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8