* [PATCH 1/3] pci: make pci_match_one_device match on ID instead of device
2026-06-26 19:49 [PATCH 0/3] pci: fix UAF and TOCTOU related to dynamic ID Gary Guo
@ 2026-06-26 19:49 ` Gary Guo
2026-06-26 19:56 ` sashiko-bot
2026-06-26 19:49 ` [PATCH 2/3] pci: fix dyn_id add TOCTOU Gary Guo
2026-06-26 19:49 ` [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal Gary Guo
2 siblings, 1 reply; 8+ messages in thread
From: Gary Guo @ 2026-06-26 19:49 UTC (permalink / raw)
To: Bjorn Helgaas, Zhenzhong Duan, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich
Cc: linux-pci, driver-core, linux-kernel, Gary Guo
There is a need to match just IDs instead of against devices. Thus rename
this function to pci_match_one_id, and add a pci_id_from_device helper to
make it easy to convert users.
Similar convert pci_match_id to do_pci_match_id, however the existing API
is kept due to quite a few users.
Signed-off-by: Gary Guo <gary@garyguo.net>
---
drivers/pci/pci-driver.c | 38 ++++++++++++++++++++++++++++----------
drivers/pci/pci.h | 36 ++++++++++++++++++++++++++----------
drivers/pci/search.c | 6 ++++--
3 files changed, 58 insertions(+), 22 deletions(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f36778e62ac1..0507cb801310 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -90,6 +90,27 @@ static void pci_free_dynids(struct pci_driver *drv)
spin_unlock(&drv->dynids.lock);
}
+/**
+ * do_pci_match_id - See if a PCI ID matches a given pci_id table
+ * @ids: array of PCI device ID structures to search in
+ * @dev_id: the actual PCI device ID structure to match against.
+ *
+ * Returns the matching pci_device_id structure or
+ * %NULL if there is no match.
+ */
+static const struct pci_device_id *do_pci_match_id(const struct pci_device_id *ids,
+ const struct pci_device_id *dev_id)
+{
+ if (ids) {
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ if (pci_match_one_id(ids, dev_id))
+ return ids;
+ ids++;
+ }
+ }
+ return NULL;
+}
+
/**
* pci_match_id - See if a PCI device matches a given pci_id table
* @ids: array of PCI device ID structures to search in
@@ -105,14 +126,9 @@ static void pci_free_dynids(struct pci_driver *drv)
const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
struct pci_dev *dev)
{
- if (ids) {
- while (ids->vendor || ids->subvendor || ids->class_mask) {
- if (pci_match_one_device(ids, dev))
- return ids;
- ids++;
- }
- }
- return NULL;
+ struct pci_device_id dev_id = pci_id_from_device(dev);
+
+ return do_pci_match_id(ids, &dev_id);
}
EXPORT_SYMBOL(pci_match_id);
@@ -138,6 +154,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
{
struct pci_dynid *dynid;
const struct pci_device_id *found_id = NULL, *ids;
+ struct pci_device_id dev_id;
int ret;
/* When driver_override is set, only bind to the matching driver */
@@ -145,10 +162,11 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
if (ret == 0)
return NULL;
+ dev_id = pci_id_from_device(dev);
/* Look at the dynamic ids first, before the static ones */
spin_lock(&drv->dynids.lock);
list_for_each_entry(dynid, &drv->dynids.list, node) {
- if (pci_match_one_device(&dynid->id, dev)) {
+ if (pci_match_one_id(&dynid->id, &dev_id)) {
found_id = &dynid->id;
break;
}
@@ -158,7 +176,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
if (found_id)
return found_id;
- for (ids = drv->id_table; (found_id = pci_match_id(ids, dev));
+ for (ids = drv->id_table; (found_id = do_pci_match_id(ids, &dev_id));
ids = found_id + 1) {
/*
* The match table is split based on driver_override.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4469e1a77f3c..0567a8762baa 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -442,21 +442,37 @@ static inline int pci_setup_cardbus(char *str) { return -ENOENT; }
#endif /* CONFIG_CARDBUS */
/**
- * pci_match_one_device - Tell if a PCI device structure has a matching
- * PCI device id structure
- * @id: single PCI device id structure to match
- * @dev: the PCI device structure to match against
+ * pci_id_from_device - Obtain a pci_device_id from a PCI device
+ * @dev: the PCI device
+ *
+ * Returns a pci_device_id filled.
+ */
+static inline struct pci_device_id pci_id_from_device(const struct pci_dev *dev)
+{
+ return (struct pci_device_id) {
+ .vendor = dev->vendor,
+ .device = dev->device,
+ .subvendor = dev->subsystem_vendor,
+ .subdevice = dev->subsystem_device,
+ .class = dev->class,
+ };
+}
+
+/**
+ * pci_match_one_id - Tell if a PCI device ID matches a needle PCI device id
+ * @id: single PCI device id structure to match against (needle)
+ * @dev_id: the actual ID from the PCI device (can be created via pci_id_from_device)
*
* Returns the matching pci_device_id structure or %NULL if there is no match.
*/
static inline const struct pci_device_id *
-pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
+pci_match_one_id(const struct pci_device_id *id, const struct pci_device_id *dev_id)
{
- if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
- (id->device == PCI_ANY_ID || id->device == dev->device) &&
- (id->subvendor == PCI_ANY_ID || id->subvendor == dev->subsystem_vendor) &&
- (id->subdevice == PCI_ANY_ID || id->subdevice == dev->subsystem_device) &&
- !((id->class ^ dev->class) & id->class_mask))
+ if ((id->vendor == PCI_ANY_ID || id->vendor == dev_id->vendor) &&
+ (id->device == PCI_ANY_ID || id->device == dev_id->device) &&
+ (id->subvendor == PCI_ANY_ID || id->subvendor == dev_id->subvendor) &&
+ (id->subdevice == PCI_ANY_ID || id->subdevice == dev_id->subdevice) &&
+ !((id->class ^ dev_id->class) & id->class_mask))
return id;
return NULL;
}
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index e3d3177fce54..c8c4bfe7817b 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -245,8 +245,10 @@ static int match_pci_dev_by_id(struct device *dev, const void *data)
{
struct pci_dev *pdev = to_pci_dev(dev);
const struct pci_device_id *id = data;
+ struct pci_device_id dev_id;
- if (pci_match_one_device(id, pdev))
+ dev_id = pci_id_from_device(pdev);
+ if (pci_match_one_id(id, &dev_id))
return 1;
return 0;
}
@@ -418,7 +420,7 @@ EXPORT_SYMBOL(pci_get_class);
*
* Iterates through the list of known PCI devices. If a PCI device is found
* with a matching base class code, the reference count to the device is
- * incremented. See pci_match_one_device() to figure out how does this works.
+ * incremented. See pci_match_one_id() to figure out how does this works.
* A new search is initiated by passing %NULL as the @from argument.
* Otherwise if @from is not %NULL, searches continue from next device on the
* global list. The reference count for @from is always decremented if it is
--
2.54.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] pci: fix dyn_id add TOCTOU
2026-06-26 19:49 [PATCH 0/3] pci: fix UAF and TOCTOU related to dynamic ID Gary Guo
2026-06-26 19:49 ` [PATCH 1/3] pci: make pci_match_one_device match on ID instead of device Gary Guo
@ 2026-06-26 19:49 ` Gary Guo
2026-06-26 20:02 ` sashiko-bot
2026-06-26 19:49 ` [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal Gary Guo
2 siblings, 1 reply; 8+ messages in thread
From: Gary Guo @ 2026-06-26 19:49 UTC (permalink / raw)
To: Bjorn Helgaas, Zhenzhong Duan, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich
Cc: linux-pci, driver-core, linux-kernel, Gary Guo
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.
Fixes: 3853f9123c18 ("PCI: Avoid duplicate IDs in driver dynamic IDs list")
Signed-off-by: Gary Guo <gary@garyguo.net>
---
drivers/pci/pci-driver.c | 112 ++++++++++++++++++++++++++---------------------
1 file changed, 62 insertions(+), 50 deletions(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0507cb801310..464ee52346fa 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -29,6 +29,48 @@ struct pci_dynid {
struct pci_device_id id;
};
+/**
+ * do_pci_add_dynid - add a new PCI device ID to this driver and re-probe devices
+ * @drv: target pci driver
+ * @id: ID to be added
+ * @check_dup: whether to check if matching ID is already present
+ *
+ * Adds a new dynamic pci device ID to this driver and causes the
+ * driver to probe for all devices again. @drv must have been
+ * registered prior to calling this function.
+ *
+ * CONTEXT:
+ * Does GFP_KERNEL allocation.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
+ */
+static int do_pci_add_dynid(struct pci_driver *drv, const struct pci_device_id *id, bool check_dup)
+{
+ struct pci_dynid *dynid, *existing_dynid;
+
+ dynid = kzalloc_obj(*dynid);
+ if (!dynid)
+ return -ENOMEM;
+
+ dynid->id = *id;
+
+ {
+ guard(spinlock)(&drv->dynids.lock);
+ if (check_dup) {
+ list_for_each_entry(existing_dynid, &drv->dynids.list, node) {
+ if (pci_match_one_id(&existing_dynid->id, id)) {
+ kfree(dynid);
+ return -EEXIST;
+ }
+ }
+ }
+ list_add_tail(&dynid->node, &drv->dynids.list);
+ }
+
+ return driver_attach(&drv->driver);
+}
+
/**
* pci_add_dynid - add a new PCI device ID to this driver and re-probe devices
* @drv: target pci driver
@@ -56,25 +98,17 @@ int pci_add_dynid(struct pci_driver *drv,
unsigned int class, unsigned int class_mask,
unsigned long driver_data)
{
- struct pci_dynid *dynid;
-
- dynid = kzalloc_obj(*dynid);
- if (!dynid)
- return -ENOMEM;
-
- dynid->id.vendor = vendor;
- dynid->id.device = device;
- dynid->id.subvendor = subvendor;
- dynid->id.subdevice = subdevice;
- dynid->id.class = class;
- dynid->id.class_mask = class_mask;
- dynid->id.driver_data = driver_data;
-
- spin_lock(&drv->dynids.lock);
- list_add_tail(&dynid->node, &drv->dynids.list);
- spin_unlock(&drv->dynids.lock);
+ struct pci_device_id id = {
+ .vendor = vendor,
+ .device = device,
+ .subvendor = subvendor,
+ .subdevice = subdevice,
+ .class = class,
+ .class_mask = class_mask,
+ .driver_data = driver_data,
+ };
- return driver_attach(&drv->driver);
+ return do_pci_add_dynid(drv, &id, false);
}
EXPORT_SYMBOL_GPL(pci_add_dynid);
@@ -197,11 +231,6 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
return NULL;
}
-static void _pci_free_device(struct device *dev)
-{
- kfree(to_pci_dev(dev));
-}
-
/**
* new_id_store - sysfs frontend to pci_add_dynid()
* @driver: target device driver
@@ -215,38 +244,22 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
{
struct pci_driver *pdrv = to_pci_driver(driver);
const struct pci_device_id *ids = pdrv->id_table;
- u32 vendor, device, subvendor = PCI_ANY_ID,
- subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
- unsigned long driver_data = 0;
+ struct pci_device_id id = {
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID
+ };
int fields;
int retval = 0;
fields = sscanf(buf, "%x %x %x %x %x %x %lx",
- &vendor, &device, &subvendor, &subdevice,
- &class, &class_mask, &driver_data);
+ &id.vendor, &id.device, &id.subvendor, &id.subdevice,
+ &id.class, &id.class_mask, &id.driver_data);
if (fields < 2)
return -EINVAL;
if (fields != 7) {
- struct pci_dev *pdev = kzalloc_obj(*pdev);
- if (!pdev)
- return -ENOMEM;
-
- pdev->vendor = vendor;
- pdev->device = device;
- pdev->subsystem_vendor = subvendor;
- pdev->subsystem_device = subdevice;
- pdev->class = class;
- pdev->dev.release = _pci_free_device;
-
- device_initialize(&pdev->dev);
- if (pci_match_device(pdrv, pdev))
- retval = -EEXIST;
-
- put_device(&pdev->dev);
-
- if (retval)
- return retval;
+ if (do_pci_match_id(pdrv->id_table, &id))
+ return -EEXIST;
}
/* Only accept driver_data values that match an existing id_table
@@ -254,7 +267,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
if (ids) {
retval = -EINVAL;
while (ids->vendor || ids->subvendor || ids->class_mask) {
- if (driver_data == ids->driver_data) {
+ if (id.driver_data == ids->driver_data) {
retval = 0;
break;
}
@@ -264,8 +277,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
return retval;
}
- retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
- class, class_mask, driver_data);
+ retval = do_pci_add_dynid(pdrv, &id, fields != 7);
if (retval)
return retval;
return count;
--
2.54.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] pci: fix dyn_id add TOCTOU
2026-06-26 19:49 ` [PATCH 2/3] pci: fix dyn_id add TOCTOU Gary Guo
@ 2026-06-26 20:02 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-26 20:02 UTC (permalink / raw)
To: Gary Guo; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] new_id_store unconditionally rejects dynamic IDs that overlap with override_only static IDs (e.g., in vfio-pci), breaking device assignment.
--
commit 65f57bc2128a4f1b44e3036aee55956006935cca
Author: Gary Guo <gary@garyguo.net>
pci: fix dyn_id add TOCTOU
This patch fixes a TOCTOU issue in new_id_store() by moving the duplicate
dynamic ID check inside do_pci_add_dynid() under the lock. It also replaces
the dummy pci_dev allocation and pci_match_device() call in new_id_store()
with a direct call to do_pci_match_id() against the static table.
> 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
[ ... ]
> @@ -215,38 +244,22 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
[ ... ]
> if (fields != 7) {
> - struct pci_dev *pdev = kzalloc_obj(*pdev);
[ ... ]
> - if (pci_match_device(pdrv, pdev))
> - retval = -EEXIST;
> -
> - put_device(&pdev->dev);
> -
> - if (retval)
> - return retval;
> + if (do_pci_match_id(pdrv->id_table, &id))
> + return -EEXIST;
> }
[Severity: High]
Does this change break dynamic ID insertion for drivers that rely on
override_only static IDs (like vfio-pci)?
Previously, pci_match_device() was used, which explicitly checks the
override_only flag:
for (ids = drv->id_table; (found_id = do_pci_match_id(ids, &dev_id));
ids = found_id + 1) {
if (!found_id->override_only || ret > 0) {
*id = *found_id;
return true;
}
}
By directly calling do_pci_match_id(), which performs purely structural
matching and ignores the override_only flag, it seems like new_id_store()
will now unconditionally reject dynamic IDs for drivers whose static table
contains a catch-all override_only entry, returning -EEXIST instead of
allowing the assignment.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-pci_id_fix-v1-0-a35c803f1b95@garyguo.net?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal
2026-06-26 19:49 [PATCH 0/3] pci: fix UAF and TOCTOU related to dynamic ID Gary Guo
2026-06-26 19:49 ` [PATCH 1/3] pci: make pci_match_one_device match on ID instead of device Gary Guo
2026-06-26 19:49 ` [PATCH 2/3] pci: fix dyn_id add TOCTOU Gary Guo
@ 2026-06-26 19:49 ` Gary Guo
2026-06-26 20:00 ` sashiko-bot
2026-06-26 21:55 ` Gary Guo
2 siblings, 2 replies; 8+ messages in thread
From: Gary Guo @ 2026-06-26 19:49 UTC (permalink / raw)
To: Bjorn Helgaas, Zhenzhong Duan, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich
Cc: linux-pci, driver-core, linux-kernel, Gary Guo, Sashiko
Dynamic IDs are only guaranteed to be valid when dynids.lock is held,
as remove_id_store can free the node. Thus, make a copy in
pci_match_device.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/20260619170503.518F61F00A3A@smtp.kernel.org/
Fixes: 0994375e9614 ("PCI: add remove_id sysfs entry")
Signed-off-by: Gary Guo <gary@garyguo.net>
---
drivers/pci/pci-driver.c | 60 ++++++++++++++++++++++--------------------------
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 464ee52346fa..2c76bba9a715 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -177,14 +177,16 @@ static const struct pci_device_id pci_device_id_any = {
* pci_match_device - See if a device matches a driver's list of IDs
* @drv: the PCI driver to match against
* @dev: the PCI device structure to match against
+ * @id: Matched pci_device_id
*
* Used by a driver to check whether a PCI device is in its list of
* supported devices or in the dynids list, which may have been augmented
- * via the sysfs "new_id" file. Returns the matching pci_device_id
- * structure or %NULL if there is no match.
+ * via the sysfs "new_id" file. Returns true if there is a match, the matched
+ * ID is stored in @id.
*/
-static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
- struct pci_dev *dev)
+static bool pci_match_device(struct pci_driver *drv,
+ struct pci_dev *dev,
+ struct pci_device_id *id)
{
struct pci_dynid *dynid;
const struct pci_device_id *found_id = NULL, *ids;
@@ -194,21 +196,19 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
/* When driver_override is set, only bind to the matching driver */
ret = device_match_driver_override(&dev->dev, &drv->driver);
if (ret == 0)
- return NULL;
+ return false;
dev_id = pci_id_from_device(dev);
/* Look at the dynamic ids first, before the static ones */
- 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;
+ {
+ guard(spinlock)(&drv->dynids.lock);
+ list_for_each_entry(dynid, &drv->dynids.list, node) {
+ if (pci_match_one_id(&dynid->id, &dev_id)) {
+ *id = dynid->id;
+ return true;
+ }
}
}
- spin_unlock(&drv->dynids.lock);
-
- if (found_id)
- return found_id;
for (ids = drv->id_table; (found_id = do_pci_match_id(ids, &dev_id));
ids = found_id + 1) {
@@ -217,18 +217,19 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
* In case override_only was set, enforce driver_override
* matching.
*/
- if (found_id->override_only) {
- if (ret > 0)
- return found_id;
- } else {
- return found_id;
+ if (!found_id->override_only || ret > 0) {
+ *id = *found_id;
+ return true;
}
}
/* driver_override will always match, send a dummy id */
- if (ret > 0)
- return &pci_device_id_any;
- return NULL;
+ if (ret > 0) {
+ *id = pci_device_id_any;
+ return true;
+ }
+
+ return false;
}
/**
@@ -474,15 +475,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);
}
return error;
}
@@ -1567,17 +1567,13 @@ static int pci_bus_match(struct device *dev, const struct device_driver *drv)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *pci_drv;
- const struct pci_device_id *found_id;
+ struct pci_device_id id;
if (pci_dev_binding_disallowed(pci_dev))
return 0;
pci_drv = (struct pci_driver *)to_pci_driver(drv);
- found_id = pci_match_device(pci_drv, pci_dev);
- if (found_id)
- return 1;
-
- return 0;
+ return pci_match_device(pci_drv, pci_dev, &id);
}
/**
--
2.54.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal
2026-06-26 19:49 ` [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal Gary Guo
@ 2026-06-26 20:00 ` sashiko-bot
2026-06-26 21:55 ` Gary Guo
1 sibling, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-26 20:00 UTC (permalink / raw)
To: Gary Guo; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Passing a pointer to a stack-allocated `struct pci_device_id` to driver `probe` functions introduces a stack use-after-free vulnerability for drivers that store the pointer for later use.
--
commit 2537209ca17dda70306c583f96a659580e419b4a
Author: Gary Guo <gary@garyguo.net>
pci: fix UAF when probe runs concurrent to dyn ID removal
Dynamic IDs are only guaranteed to be valid when dynids.lock is held, as
remove_id_store can free the node. This patch copies the matched ID in
pci_match_device to avoid the UAF.
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
> index 464ee52346fa8..2c76bba9a7156 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
[ ... ]
> @@ -474,15 +475,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: Critical]
Could passing a pointer to the stack-allocated struct pci_device_id to the
driver's probe function introduce a stack use-after-free regression?
Some drivers, like nsp32 and ata_generic, save the pointer to the ID passed
into their probe function for later use. For example:
drivers/scsi/nsp32.c:nsp32_probe() {
...
data->pci_devid = id;
...
}
When normal operations later access this saved pointer, won't they dereference
invalid stack memory since the __pci_device_probe() stack frame has already
been destroyed?
For example, when reading from procfs:
drivers/scsi/nsp32.c:nsp32_show_info() {
...
model = data->pci_devid->driver_data;
...
}
Similarly in ata_generic_init_one(), the id pointer is passed as host private
data for later use in ATA operations:
drivers/ata/ata_generic.c:ata_generic_init_one() {
...
return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id, 0);
}
> }
> return error;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-pci_id_fix-v1-0-a35c803f1b95@garyguo.net?part=3
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal
2026-06-26 19:49 ` [PATCH 3/3] pci: fix UAF when probe runs concurrent to dyn ID removal Gary Guo
2026-06-26 20:00 ` sashiko-bot
@ 2026-06-26 21:55 ` Gary Guo
1 sibling, 0 replies; 8+ messages in thread
From: Gary Guo @ 2026-06-26 21:55 UTC (permalink / raw)
To: Gary Guo, Bjorn Helgaas, Zhenzhong Duan, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich
Cc: linux-pci, driver-core, linux-kernel, Sashiko
On Fri Jun 26, 2026 at 8:49 PM BST, Gary Guo wrote:
> Dynamic IDs are only guaranteed to be valid when dynids.lock is held,
> as remove_id_store can free the node. Thus, make a copy in
> pci_match_device.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/20260619170503.518F61F00A3A@smtp.kernel.org/
> Fixes: 0994375e9614 ("PCI: add remove_id sysfs entry")
> Signed-off-by: Gary Guo <gary@garyguo.net>
Sashiko reports driver UAF issue with this:
https://sashiko.dev/#/patchset/20260626-pci_id_fix-v1-0-a35c803f1b95%40garyguo.net?part=3
All of these cases are already existing UAF issues, albeit now it is much easier
to trigger because this patch converts them to heap UAF for dynamic ID only to
stack UAF for all IDs.
One obvious fix is to instead keep a copy of the ID in pci_dev. However, in
practice I think it doesn't make much sense for drivers to keep pci_device_id
pointers at all; everything except driver_data can just be retrieved from
pci_dev.
So I think it's better to fix drivers that stores these IDs. I used this
coccinelle script to find cases:
---
@store@
identifier fn;
identifier id;
expression E;
parameter list[n] ps;
@@
fn(ps, const struct pci_device_id *id, ...)
{
...
* E = id
...
}
@store_cast@
identifier fn;
identifier id;
expression E;
type T;
parameter list[n] ps;
@@
fn(ps, const struct pci_device_id *id, ...)
{
...
* E = (T)id
...
}
@struct_const@
identifier s, fld;
@@
struct s {
...
* const struct pci_device_id *fld;
...
};
@struct_mut@
identifier s, fld;
@@
struct s {
...
* struct pci_device_id *fld;
...
};
---
And it looks like there're only 4 drivers that need fixing:
- mlxsw
- ipack/tpci200
- nsp32
- ata
Best,
Gary
^ permalink raw reply [flat|nested] 8+ messages in thread