Re: [PATCH v2 7/7] pci: fix UAF when probe runs concurrent to dyn ID removal

["Gary Guo" <gary@garyguo.net>]

On Wed Jul 1, 2026 at 12:10 PM BST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `id` parameter passed to PCI driver probe functions is now a pointer to a stack-allocated variable, breaking drivers that rely on pointer arithmetic to calculate indices or that cache the pointer.
> --
>
> commit 0cf7c75b08e81435f8eec9e2453a6dda8624a99b
> Author: Gary Guo <gary@garyguo.net>
> Subject: pci: fix UAF when probe runs concurrent to dyn ID removal
>
> This commit modifies the PCI matching logic to copy the matched device ID
> instead of returning a pointer to it. This ensures dynamic IDs are safely
> handled without requiring the lock to be held during probe.
>
> Link: https://lore.kernel.org/all/20260619170503.518F61F00A3A@smtp.kernel.org/
>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>
> [ ... ]
>
>> @@ -465,15 +470,14 @@ void pci_probe_flush_workqueue(void)
>>   */
>>  static int __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>>  {
>> -	const struct pci_device_id *id;
>> +	struct pci_device_id id;
>>  	int error = 0;
>>  
>>  	if (drv->probe) {
>>  		error = -ENODEV;
>>  
>> -		id = pci_match_device(drv, pci_dev);
>> -		if (id)
>> -			error = pci_call_probe(drv, pci_dev, id);
>> +		if (pci_match_device(drv, pci_dev, &id))
>> +			error = pci_call_probe(drv, pci_dev, &id);
>
> [Severity: High]
> Does passing a pointer to the stack-allocated id variable here break drivers
> that rely on the pointer belonging to their static id_table?
>
> Some drivers compute an array index using pointer arithmetic against the
> provided id pointer. For example, in drivers/char/agp/amd-k7-agp.c:
>
> agp_amdk7_probe() {
>     ...
>     j = ent - agp_amdk7_pci_table;
>     dev_info(&pdev->dev, "AMD %s chipset\n",
>              amd_agp_device_ids[j].chipset_name);
>     ...
> }

I am amused both by creative ways that drivers make use of this ID (which, btw,
is completely wrong without this change already and would be OOB access for any
dynamic ID or driver_override) and by the AI's ability to find them.

I'm now convinced that the drivers shouldn't be given the pci_device_id in the
first place but just the driver_data field, although it's going to be very
difficult to change all the drivers so that might never happen.

Best,
Gary

>
> Since the pointer now points to the stack frame in __pci_device_probe()
> rather than the static table, won't this pointer subtraction yield a
> wildly out-of-bounds index and cause a memory read fault during device
> probing?
>
>>  	}
>>  	return error;
>>  }