[PATCH v3 0/9] pci: fix UAF and TOCTOU related to dynamic ID

[PATCH v3 0/9] pci: fix UAF and TOCTOU related to dynamic ID

[Gary Guo]

While working on improving the Rust abstractions [1], Sashiko reported that
an existing UAF issue related to dynamic ID, which I find to be genuine.
When taking a look at the code I also find a TOCTOU issue where the
existence check of dynamic ID happens in a separate critical section as the
actual insertion. This series fix both issues.

There are two exported functions "pci_match_id" and "pci_add_dynid" which I
have to tweak to implement this cleanly; I created separate "do_xxx"
functions to keep the existing APIs because they all have multiple users.

There're a few existing users which stores their pci_device_id argument in
probe callback. This is a bad pattern because nothing except driver_data
inside pci_device_id is what they want; actual ID information can be
retrieved from pci_dev instead.

There are two users that performs pointer arithmetic on the pci_device_id;
these are also problematic with dynamic ID and driver_override, so fix them
as well.

I've used the following coccinelle script to flag all cases where the
pci_device_id is used other than reading its fields.

@usage@
identifier fn, id;
position p;
@@
  fn(..., struct pci_device_id *id, ...)
  {
    ...
    id@p
    ...
  }

// Due to cocci isomorphism this needs to be explicit
@bad@
identifier fn, id;
type T;
position usage.p;
@@
  fn(..., struct pci_device_id *id, ...)
  {
    ...
    (T*)id@p
    ...
  }

// Good use cases
@good@
identifier fn, id, fld;
expression E;
position usage.p;
@@
  fn(..., struct pci_device_id *id, ...)
  {
    ...
(
    id@p->fld
|
    E(..., id@p, ...)
|
// Redundant checks, but ignore
    !id@p
|
// Redundant checks, but ignore
    id ? ... : ...
)
    ...
  }

@script:python depends on usage && (bad || !good)@
p << usage.p;
@@
coccilib.report.print_report(p[0], "suspicious use of pci_device_id")

Link: https://lore.kernel.org/all/20260618-id_info-v1-0-96af1e559ef9@garyguo.net/ [1]
Link: https://lore.kernel.org/all/20260619170503.518F61F00A3A@smtp.kernel.org/ [2]

---
Changes in v3:
- Fix users which uses pci_device_id for pointer arithmetic. (Sashiko)
- Convert to scoped_guard. (Danilo)
- For static IDs, still give out static pointers and avoid making a copy.
- Link to v2: https://patch.msgid.link/20260630-pci_id_fix-v2-0-b834a98c0af2@garyguo.net

Changes in v2:
- Fix users which store pci_device_id.
- Clarify in probe documentation about the lifetime of pci_device_id
  parameter.
- Dynamic ID conflict check now ignores override_only. (Sashiko)
- Link to v1: https://patch.msgid.link/20260626-pci_id_fix-v1-0-a35c803f1b95@garyguo.net

---
Gary Guo (9):
      ata: don't store pci_device_id
      nsp32: don't store pci_device_id
      ipack: tpci200: don't store pci_device_id
      mlxsw: don't store pci_device_id
      agp/via: don't rely on address of pci_device_id
      agp/amd-k7: don't rely on address of pci_device_id
      pci: make pci_match_one_device match on ID instead of device
      pci: fix dyn_id add TOCTOU
      pci: fix UAF when probe runs concurrent to dyn ID removal

 drivers/ata/ata_generic.c                 |   6 +-
 drivers/char/agp/amd-k7-agp.c             |  26 +--
 drivers/char/agp/via-agp.c                | 308 +++++++-----------------------
 drivers/ipack/carriers/tpci200.c          |   1 -
 drivers/ipack/carriers/tpci200.h          |   1 -
 drivers/net/ethernet/mellanox/mlxsw/pci.c |  11 +-
 drivers/pci/pci-driver.c                  | 193 ++++++++++---------
 drivers/pci/pci.h                         |  36 +++-
 drivers/pci/search.c                      |   6 +-
 drivers/scsi/nsp32.c                      |   8 +-
 drivers/scsi/nsp32.h                      |   8 +-
 include/linux/pci.h                       |   1 +
 12 files changed, 230 insertions(+), 375 deletions(-)
---
base-commit: 2b763db0c2763d6bf73d7d3e69665222d1f377cf
change-id: 20260626-pci_id_fix-83eaec007674

Best regards,
--  
Gary Guo <gary@garyguo.net>

[PATCH v3 1/9] ata: don't store pci_device_id

[Gary Guo]

pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. All information apart from driver_data can be easily
retrieved from pci_dev, so just store driver_data.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/ata/ata_generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index e70b6c089cf1..18ea740ca582 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -51,11 +51,11 @@ enum {
 static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
 {
 	struct ata_port *ap = link->ap;
-	const struct pci_device_id *id = ap->host->private_data;
+	unsigned long driver_data = (unsigned long)ap->host->private_data;
 	int dma_enabled = 0;
 	struct ata_device *dev;
 
-	if (id->driver_data & ATA_GEN_FORCE_DMA) {
+	if (driver_data & ATA_GEN_FORCE_DMA) {
 		dma_enabled = 0xff;
 	} else if (ap->ioaddr.bmdma_addr) {
 		/* Bits 5 and 6 indicate if DMA is active on master/slave */
@@ -206,7 +206,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
 			return rc;
 		pcim_pin_device(dev);
 	}
-	return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id, 0);
+	return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id->driver_data, 0);
 }
 
 static const struct pci_device_id ata_generic[] = {

-- 
2.54.0

[PATCH v3 2/9] nsp32: don't store pci_device_id

[Gary Guo]

pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. All information apart from driver_data can be easily
retrieved from pci_dev, so just store driver_data.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/scsi/nsp32.c | 8 ++++----
 drivers/scsi/nsp32.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
index e893d5677241..9c9281222a0a 100644
--- a/drivers/scsi/nsp32.c
+++ b/drivers/scsi/nsp32.c
@@ -1470,7 +1470,7 @@ static int nsp32_show_info(struct seq_file *m, struct Scsi_Host *host)
 		   (nsp32_read2(base, INDEX_REG) >> 8) & 0xff);
 
 	mode_reg = nsp32_index_read1(base, CHIP_MODE);
-	model    = data->pci_devid->driver_data;
+	model    = data->model;
 
 #ifdef CONFIG_PM
 	seq_printf(m, "Power Management:      %s\n",
@@ -2907,8 +2907,8 @@ static int nsp32_eh_host_reset(struct scsi_cmnd *SCpnt)
  */
 static int nsp32_getprom_param(nsp32_hw_data *data)
 {
-	int vendor = data->pci_devid->vendor;
-	int device = data->pci_devid->device;
+	int vendor = data->Pci->vendor;
+	int device = data->Pci->device;
 	int ret, i;
 	int __maybe_unused val;
 
@@ -3340,7 +3340,7 @@ static int nsp32_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 
 	data->Pci	  = pdev;
-	data->pci_devid   = id;
+	data->model       = id->driver_data;
 	data->IrqNumber   = pdev->irq;
 	data->BaseAddress = pci_resource_start(pdev, 0);
 	data->NumAddress  = pci_resource_len  (pdev, 0);
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index 924889f8bd37..9e65771cb592 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -564,10 +564,10 @@ typedef struct _nsp32_hw_data {
 
 	struct scsi_cmnd *CurrentSC;
 
-	struct pci_dev             *Pci;
-	const struct pci_device_id *pci_devid;
-	struct Scsi_Host           *Host;
-	spinlock_t                  Lock;
+	struct pci_dev    *Pci;
+	int                model;
+	struct Scsi_Host  *Host;
+	spinlock_t         Lock;
 
 	char info_str[100];
 

-- 
2.54.0

[PATCH v3 3/9] ipack: tpci200: don't store pci_device_id

[Gary Guo]

pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. This stored ID is unused so remove it.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/ipack/carriers/tpci200.c | 1 -
 drivers/ipack/carriers/tpci200.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/ipack/carriers/tpci200.c b/drivers/ipack/carriers/tpci200.c
index 05dcb6675cd6..1cf51f763293 100644
--- a/drivers/ipack/carriers/tpci200.c
+++ b/drivers/ipack/carriers/tpci200.c
@@ -562,7 +562,6 @@ static int tpci200_pci_probe(struct pci_dev *pdev,
 
 	/* Save struct pci_dev pointer */
 	tpci200->info->pdev = pdev;
-	tpci200->info->id_table = (struct pci_device_id *)id;
 
 	/* register the device and initialize it */
 	ret = tpci200_install(tpci200);
diff --git a/drivers/ipack/carriers/tpci200.h b/drivers/ipack/carriers/tpci200.h
index e79ac64abcff..a2bf3125794b 100644
--- a/drivers/ipack/carriers/tpci200.h
+++ b/drivers/ipack/carriers/tpci200.h
@@ -145,7 +145,6 @@ struct tpci200_slot {
  */
 struct tpci200_infos {
 	struct pci_dev			*pdev;
-	struct pci_device_id		*id_table;
 	struct tpci200_regs __iomem	*interface_regs;
 	void __iomem			*cfg_regs;
 	struct ipack_bus_device		*ipack_bus;

-- 
2.54.0

[PATCH v3 4/9] mlxsw: don't store pci_device_id

[Gary Guo]

pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. This stored ID is unused so remove it.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index 0da85d36647d..bfe3268dfdc1 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -130,7 +130,6 @@ struct mlxsw_pci {
 		} comp;
 	} cmd;
 	struct mlxsw_bus_info bus_info;
-	const struct pci_device_id *id;
 	enum mlxsw_pci_cqe_v max_cqe_ver; /* Maximal supported CQE version */
 	u8 num_cqs; /* Number of CQs */
 	u8 num_sdqs; /* Number of SDQs */
@@ -1768,7 +1767,6 @@ static void mlxsw_pci_mbox_free(struct mlxsw_pci *mlxsw_pci,
 }
 
 static int mlxsw_pci_sys_ready_wait(struct mlxsw_pci *mlxsw_pci,
-				    const struct pci_device_id *id,
 				    u32 *p_sys_status)
 {
 	unsigned long end;
@@ -1839,7 +1837,7 @@ static int mlxsw_pci_reset_sw(struct mlxsw_pci *mlxsw_pci)
 }
 
 static int
-mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)
+mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci)
 {
 	struct pci_dev *pdev = mlxsw_pci->pdev;
 	bool pci_reset_sbr_supported = false;
@@ -1848,7 +1846,7 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)
 	u32 sys_status;
 	int err;
 
-	err = mlxsw_pci_sys_ready_wait(mlxsw_pci, id, &sys_status);
+	err = mlxsw_pci_sys_ready_wait(mlxsw_pci, &sys_status);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to reach system ready status before reset. Status is 0x%x\n",
 			sys_status);
@@ -1880,7 +1878,7 @@ mlxsw_pci_reset(struct mlxsw_pci *mlxsw_pci, const struct pci_device_id *id)
 	if (err)
 		return err;
 
-	err = mlxsw_pci_sys_ready_wait(mlxsw_pci, id, &sys_status);
+	err = mlxsw_pci_sys_ready_wait(mlxsw_pci, &sys_status);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to reach system ready status after reset. Status is 0x%x\n",
 			sys_status);
@@ -1932,7 +1930,7 @@ static int mlxsw_pci_init(void *bus_priv, struct mlxsw_core *mlxsw_core,
 	if (!mbox)
 		return -ENOMEM;
 
-	err = mlxsw_pci_reset(mlxsw_pci, mlxsw_pci->id);
+	err = mlxsw_pci_reset(mlxsw_pci);
 	if (err)
 		goto err_reset;
 
@@ -2464,7 +2462,6 @@ static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mlxsw_pci->bus_info.device_name = pci_name(mlxsw_pci->pdev);
 	mlxsw_pci->bus_info.dev = &pdev->dev;
 	mlxsw_pci->bus_info.read_clock_capable = true;
-	mlxsw_pci->id = id;
 
 	err = mlxsw_core_bus_device_register(&mlxsw_pci->bus_info,
 					     &mlxsw_pci_bus, mlxsw_pci, false,

-- 
2.54.0

[PATCH v3 5/9] agp/via: don't rely on address of pci_device_id

[Gary Guo]

Address of pci_device_id cannot be relied on due to presence of dynamic ID
and driver_override. Use driver_data instead.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/char/agp/via-agp.c | 308 +++++++++++----------------------------------
 1 file changed, 72 insertions(+), 236 deletions(-)

diff --git a/drivers/char/agp/via-agp.c b/drivers/char/agp/via-agp.c
index 8b19a5d1a09b..ab3b73dd080a 100644
--- a/drivers/char/agp/via-agp.c
+++ b/drivers/char/agp/via-agp.c
@@ -221,204 +221,6 @@ static const struct agp_bridge_driver via_driver = {
 	.agp_type_to_mask_type  = agp_generic_type_to_mask_type,
 };
 
-static struct agp_device_ids via_agp_device_ids[] =
-{
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_82C597_0,
-		.chipset_name	= "Apollo VP3",
-	},
-
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_82C598_0,
-		.chipset_name	= "Apollo MVP3",
-	},
-
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8501_0,
-		.chipset_name	= "Apollo MVP4",
-	},
-
-	/* VT8601 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8601_0,
-		.chipset_name	= "Apollo ProMedia/PLE133Ta",
-	},
-
-	/* VT82C693A / VT28C694T */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_82C691_0,
-		.chipset_name	= "Apollo Pro 133",
-	},
-
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8371_0,
-		.chipset_name	= "KX133",
-	},
-
-	/* VT8633 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8633_0,
-		.chipset_name	= "Pro 266",
-	},
-
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_XN266,
-		.chipset_name	= "Apollo Pro266",
-	},
-
-	/* VT8361 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8361,
-		.chipset_name	= "KLE133",
-	},
-
-	/* VT8365 / VT8362 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8363_0,
-		.chipset_name	= "Twister-K/KT133x/KM133",
-	},
-
-	/* VT8753A */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8753_0,
-		.chipset_name	= "P4X266",
-	},
-
-	/* VT8366 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8367_0,
-		.chipset_name	= "KT266/KY266x/KT333",
-	},
-
-	/* VT8633 (for CuMine/ Celeron) */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8653_0,
-		.chipset_name	= "Pro266T",
-	},
-
-	/* KM266 / PM266 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_XM266,
-		.chipset_name	= "PM266/KM266",
-	},
-
-	/* CLE266 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_862X_0,
-		.chipset_name	= "CLE266",
-	},
-
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8377_0,
-		.chipset_name	= "KT400/KT400A/KT600",
-	},
-
-	/* VT8604 / VT8605 / VT8603
-	 * (Apollo Pro133A chipset with S3 Savage4) */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8605_0,
-		.chipset_name	= "ProSavage PM133/PL133/PN133"
-	},
-
-	/* P4M266x/P4N266 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8703_51_0,
-		.chipset_name	= "P4M266x/P4N266",
-	},
-
-	/* VT8754 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8754C_0,
-		.chipset_name	= "PT800",
-	},
-
-	/* P4X600 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8763_0,
-		.chipset_name	= "P4X600"
-	},
-
-	/* KM400 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8378_0,
-		.chipset_name	= "KM400/KM400A",
-	},
-
-	/* PT880 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_PT880,
-		.chipset_name	= "PT880",
-	},
-
-	/* PT880 Ultra */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_PT880ULTRA,
-		.chipset_name	= "PT880 Ultra",
-	},
-
-	/* PT890 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_8783_0,
-		.chipset_name	= "PT890",
-	},
-
-	/* PM800/PN800/PM880/PN880 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_PX8X0_0,
-		.chipset_name	= "PM800/PN800/PM880/PN880",
-	},
-	/* KT880 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_3269_0,
-		.chipset_name	= "KT880",
-	},
-	/* KTxxx/Px8xx */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_83_87XX_1,
-		.chipset_name	= "VT83xx/VT87xx/KTxxx/Px8xx",
-	},
-	/* P4M800 */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_3296_0,
-		.chipset_name	= "P4M800",
-	},
-	/* P4M800CE */
-	{
-		.device_id	= PCI_DEVICE_ID_VIA_P4M800CE,
-		.chipset_name	= "VT3314",
-	},
-	/* VT3324 / CX700 */
-	{
-		.device_id  = PCI_DEVICE_ID_VIA_VT3324,
-		.chipset_name   = "CX700",
-	},
-	/* VT3336 - this is a chipset for AMD Athlon/K8 CPU. Due to K8's unique
-	 * architecture, the AGP resource and behavior are different from
-	 * the traditional AGP which resides only in chipset. AGP is used
-	 * by 3D driver which wasn't available for the VT3336 and VT3364
-	 * generation until now.  Unfortunately, by testing, VT3364 works
-	 * but VT3336 doesn't. - explanation from via, just leave this as
-	 * as a placeholder to avoid future patches adding it back in.
-	 */
-#if 0
-	{
-		.device_id  = PCI_DEVICE_ID_VIA_VT3336,
-		.chipset_name   = "VT3336",
-	},
-#endif
-	/* P4M890 */
-	{
-		.device_id  = PCI_DEVICE_ID_VIA_P4M890,
-		.chipset_name   = "P4M890",
-	},
-	/* P4M900 */
-	{
-		.device_id  = PCI_DEVICE_ID_VIA_VT3364,
-		.chipset_name   = "P4M900",
-	},
-	{ }, /* dummy final entry, always present */
-};
-
 
 /*
  * VIA's AGP3 chipsets do magick to put the AGP bridge compliant
@@ -437,17 +239,14 @@ static void check_via_agp3 (struct agp_bridge_data *bridge)
 
 static int agp_via_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
-	struct agp_device_ids *devs = via_agp_device_ids;
 	struct agp_bridge_data *bridge;
-	int j = 0;
 	u8 cap_ptr;
 
 	cap_ptr = pci_find_capability(pdev, PCI_CAP_ID_AGP);
 	if (!cap_ptr)
 		return -ENODEV;
 
-	j = ent - agp_via_pci_table;
-	printk (KERN_INFO PFX "Detected VIA %s chipset\n", devs[j].chipset_name);
+	dev_info(&pdev->dev, "Detected VIA %s chipset\n", (const char *)ent->driver_data);
 
 	bridge = agp_alloc_bridge();
 	if (!bridge)
@@ -501,9 +300,8 @@ static int agp_via_resume(struct device *dev)
 	return 0;
 }
 
-/* must be the same order as name table above */
 static const struct pci_device_id agp_via_pci_table[] = {
-#define ID(x) \
+#define ID(x, name) \
 	{						\
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),	\
 	.class_mask	= ~0,				\
@@ -511,39 +309,77 @@ static const struct pci_device_id agp_via_pci_table[] = {
 	.device		= x,				\
 	.subvendor	= PCI_ANY_ID,			\
 	.subdevice	= PCI_ANY_ID,			\
+	.driver_data	= (kernel_ulong_t)name,		\
 	}
-	ID(PCI_DEVICE_ID_VIA_82C597_0),
-	ID(PCI_DEVICE_ID_VIA_82C598_0),
-	ID(PCI_DEVICE_ID_VIA_8501_0),
-	ID(PCI_DEVICE_ID_VIA_8601_0),
-	ID(PCI_DEVICE_ID_VIA_82C691_0),
-	ID(PCI_DEVICE_ID_VIA_8371_0),
-	ID(PCI_DEVICE_ID_VIA_8633_0),
-	ID(PCI_DEVICE_ID_VIA_XN266),
-	ID(PCI_DEVICE_ID_VIA_8361),
-	ID(PCI_DEVICE_ID_VIA_8363_0),
-	ID(PCI_DEVICE_ID_VIA_8753_0),
-	ID(PCI_DEVICE_ID_VIA_8367_0),
-	ID(PCI_DEVICE_ID_VIA_8653_0),
-	ID(PCI_DEVICE_ID_VIA_XM266),
-	ID(PCI_DEVICE_ID_VIA_862X_0),
-	ID(PCI_DEVICE_ID_VIA_8377_0),
-	ID(PCI_DEVICE_ID_VIA_8605_0),
-	ID(PCI_DEVICE_ID_VIA_8703_51_0),
-	ID(PCI_DEVICE_ID_VIA_8754C_0),
-	ID(PCI_DEVICE_ID_VIA_8763_0),
-	ID(PCI_DEVICE_ID_VIA_8378_0),
-	ID(PCI_DEVICE_ID_VIA_PT880),
-	ID(PCI_DEVICE_ID_VIA_PT880ULTRA),
-	ID(PCI_DEVICE_ID_VIA_8783_0),
-	ID(PCI_DEVICE_ID_VIA_PX8X0_0),
-	ID(PCI_DEVICE_ID_VIA_3269_0),
-	ID(PCI_DEVICE_ID_VIA_83_87XX_1),
-	ID(PCI_DEVICE_ID_VIA_3296_0),
-	ID(PCI_DEVICE_ID_VIA_P4M800CE),
-	ID(PCI_DEVICE_ID_VIA_VT3324),
-	ID(PCI_DEVICE_ID_VIA_P4M890),
-	ID(PCI_DEVICE_ID_VIA_VT3364),
+	ID(PCI_DEVICE_ID_VIA_82C597_0, "Apollo VP3"),
+	ID(PCI_DEVICE_ID_VIA_82C598_0, "Apollo MVP3"),
+	ID(PCI_DEVICE_ID_VIA_8501_0, "Apollo MVP4"),
+	/* VT8601 */
+	ID(PCI_DEVICE_ID_VIA_8601_0, "Apollo ProMedia/PLE133Ta"),
+	/* VT82C693A / VT28C694T */
+	ID(PCI_DEVICE_ID_VIA_82C691_0, "Apollo Pro 133"),
+	ID(PCI_DEVICE_ID_VIA_8371_0, "KX133"),
+	/* VT8633 */
+	ID(PCI_DEVICE_ID_VIA_8633_0, "Pro 266"),
+	ID(PCI_DEVICE_ID_VIA_XN266, "Apollo Pro266"),
+	/* VT8361 */
+	ID(PCI_DEVICE_ID_VIA_8361, "KLE133"),
+	/* VT8365 / VT8362 */
+	ID(PCI_DEVICE_ID_VIA_8363_0, "Twister-K/KT133x/KM133"),
+	/* VT8753A */
+	ID(PCI_DEVICE_ID_VIA_8753_0, "P4X266"),
+	/* VT8366 */
+	ID(PCI_DEVICE_ID_VIA_8367_0, "KT266/KY266x/KT333"),
+	/* VT8633 (for CuMine/ Celeron) */
+	ID(PCI_DEVICE_ID_VIA_8653_0, "Pro266T"),
+	/* KM266 / PM266 */
+	ID(PCI_DEVICE_ID_VIA_XM266, "PM266/KM266"),
+	/* CLE266 */
+	ID(PCI_DEVICE_ID_VIA_862X_0, "CLE266"),
+	ID(PCI_DEVICE_ID_VIA_8377_0, "KT400/KT400A/KT600"),
+	/* VT8604 / VT8605 / VT8603 (Apollo Pro133A chipset with S3 Savage4) */
+	ID(PCI_DEVICE_ID_VIA_8605_0, "ProSavage PM133/PL133/PN133"),
+	/* P4M266x/P4N266 */
+	ID(PCI_DEVICE_ID_VIA_8703_51_0, "P4M266x/P4N266"),
+	/* VT8754 */
+	ID(PCI_DEVICE_ID_VIA_8754C_0, "PT800"),
+	/* P4X600 */
+	ID(PCI_DEVICE_ID_VIA_8763_0, "P4X600"),
+	/* KM400 */
+	ID(PCI_DEVICE_ID_VIA_8378_0, "KM400/KM400A"),
+	/* PT880 */
+	ID(PCI_DEVICE_ID_VIA_PT880, "PT880"),
+	/* PT880 Ultra */
+	ID(PCI_DEVICE_ID_VIA_PT880ULTRA, "PT880 Ultra"),
+	/* PT890 */
+	ID(PCI_DEVICE_ID_VIA_8783_0, "PT890"),
+	/* PM800/PN800/PM880/PN880 */
+	ID(PCI_DEVICE_ID_VIA_PX8X0_0, "PM800/PN800/PM880/PN880"),
+	/* KT880 */
+	ID(PCI_DEVICE_ID_VIA_3269_0, "KT880"),
+	/* KTxxx/Px8xx */
+	ID(PCI_DEVICE_ID_VIA_83_87XX_1, "VT83xx/VT87xx/KTxxx/Px8xx"),
+	/* P4M800 */
+	ID(PCI_DEVICE_ID_VIA_3296_0, "P4M800"),
+	/* P4M800CE */
+	ID(PCI_DEVICE_ID_VIA_P4M800CE, "VT3314"),
+	/* VT3324 / CX700 */
+	ID(PCI_DEVICE_ID_VIA_VT3324, "CX700"),
+	/* VT3336 - this is a chipset for AMD Athlon/K8 CPU. Due to K8's unique
+	 * architecture, the AGP resource and behavior are different from
+	 * the traditional AGP which resides only in chipset. AGP is used
+	 * by 3D driver which wasn't available for the VT3336 and VT3364
+	 * generation until now.  Unfortunately, by testing, VT3364 works
+	 * but VT3336 doesn't. - explanation from via, just leave this as
+	 * a placeholder to avoid future patches adding it back in.
+	 */
+#if 0
+	ID(PCI_DEVICE_ID_VIA_VT3336, "VT3336"),
+#endif
+	/* P4M890 */
+	ID(PCI_DEVICE_ID_VIA_P4M890, "P4M890"),
+	/* P4M900 */
+	ID(PCI_DEVICE_ID_VIA_VT3364, "P4M900"),
 	{ }
 };
 

-- 
2.54.0

[PATCH v3 6/9] agp/amd-k7: don't rely on address of pci_device_id

[Gary Guo]

Address of pci_device_id cannot be relied on due to presence of dynamic ID
and driver_override. Use driver_data instead.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/char/agp/amd-k7-agp.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index 898ff30ffd46..4d201e71c517 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -387,37 +387,17 @@ static const struct agp_bridge_driver amd_irongate_driver = {
 	.agp_type_to_mask_type  = agp_generic_type_to_mask_type,
 };
 
-static struct agp_device_ids amd_agp_device_ids[] =
-{
-	{
-		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_7006,
-		.chipset_name	= "Irongate",
-	},
-	{
-		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_700E,
-		.chipset_name	= "761",
-	},
-	{
-		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_700C,
-		.chipset_name	= "760MP",
-	},
-	{ }, /* dummy final entry, always present */
-};
-
 static int agp_amdk7_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
 	struct agp_bridge_data *bridge;
 	u8 cap_ptr;
-	int j;
 
 	cap_ptr = pci_find_capability(pdev, PCI_CAP_ID_AGP);
 	if (!cap_ptr)
 		return -ENODEV;
 
-	j = ent - agp_amdk7_pci_table;
-	dev_info(&pdev->dev, "AMD %s chipset\n",
-		 amd_agp_device_ids[j].chipset_name);
+	dev_info(&pdev->dev, "AMD %s chipset\n", (const char *)ent->driver_data);
 
 	bridge = agp_alloc_bridge();
 	if (!bridge)
@@ -492,7 +472,6 @@ static int agp_amdk7_resume(struct device *dev)
 	return amd_irongate_driver.configure();
 }
 
-/* must be the same order as name table above */
 static const struct pci_device_id agp_amdk7_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
@@ -501,6 +480,7 @@ static const struct pci_device_id agp_amdk7_pci_table[] = {
 	.device		= PCI_DEVICE_ID_AMD_FE_GATE_7006,
 	.subvendor	= PCI_ANY_ID,
 	.subdevice	= PCI_ANY_ID,
+	.driver_data	= (kernel_ulong_t)"Irongate",
 	},
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
@@ -509,6 +489,7 @@ static const struct pci_device_id agp_amdk7_pci_table[] = {
 	.device		= PCI_DEVICE_ID_AMD_FE_GATE_700E,
 	.subvendor	= PCI_ANY_ID,
 	.subdevice	= PCI_ANY_ID,
+	.driver_data	= (kernel_ulong_t)"761",
 	},
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
@@ -517,6 +498,7 @@ static const struct pci_device_id agp_amdk7_pci_table[] = {
 	.device		= PCI_DEVICE_ID_AMD_FE_GATE_700C,
 	.subvendor	= PCI_ANY_ID,
 	.subdevice	= PCI_ANY_ID,
+	.driver_data	= (kernel_ulong_t)"760MP",
 	},
 	{ }
 };

-- 
2.54.0

[PATCH v3 7/9] pci: make pci_match_one_device match on ID instead of device

[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.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
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

[PATCH v3 8/9] pci: fix dyn_id add TOCTOU

[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 | 139 ++++++++++++++++++++++++-----------------------
 1 file changed, 71 insertions(+), 68 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0507cb801310..2e80ae150ff4 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -29,6 +29,47 @@ 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;
+
+	scoped_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 +97,17 @@ int pci_add_dynid(struct pci_driver *drv,
 		  unsigned int class, unsigned int class_mask,
 		  unsigned long driver_data)
 {
-	struct pci_dynid *dynid;
+	struct pci_device_id id = {
+		.vendor = vendor,
+		.device = device,
+		.subvendor = subvendor,
+		.subdevice = subdevice,
+		.class = class,
+		.class_mask = class_mask,
+		.driver_data = driver_data,
+	};
 
-	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);
-
-	return driver_attach(&drv->driver);
+	return do_pci_add_dynid(drv, &id, false);
 }
 EXPORT_SYMBOL_GPL(pci_add_dynid);
 
@@ -94,16 +127,19 @@ static void pci_free_dynids(struct pci_driver *drv)
  * 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.
+ * @include_override_only: also match against device ID entries marked as override only.
  *
  * 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)
+						   const struct pci_device_id *dev_id,
+						   bool include_override_only)
 {
 	if (ids) {
 		while (ids->vendor || ids->subvendor || ids->class_mask) {
-			if (pci_match_one_id(ids, dev_id))
+			if ((!ids->override_only || include_override_only) &&
+			    pci_match_one_id(ids, dev_id))
 				return ids;
 			ids++;
 		}
@@ -128,7 +164,7 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
 {
 	struct pci_device_id dev_id = pci_id_from_device(dev);
 
-	return do_pci_match_id(ids, &dev_id);
+	return do_pci_match_id(ids, &dev_id, true);
 }
 EXPORT_SYMBOL(pci_match_id);
 
@@ -153,7 +189,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;
 	struct pci_device_id dev_id;
 	int ret;
 
@@ -176,20 +212,9 @@ 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 = do_pci_match_id(ids, &dev_id));
-	     ids = found_id + 1) {
-		/*
-		 * The match table is split based on driver_override.
-		 * 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;
-		}
-	}
+	found_id = do_pci_match_id(drv->id_table, &dev_id, ret > 0);
+	if (found_id)
+		return found_id;
 
 	/* driver_override will always match, send a dummy id */
 	if (ret > 0)
@@ -197,11 +222,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 +235,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, false))
+			return -EEXIST;
 	}
 
 	/* Only accept driver_data values that match an existing id_table
@@ -254,7 +258,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 +268,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

[PATCH v3 9/9] pci: fix UAF when probe runs concurrent to dyn ID removal

[Gary Guo]

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. Also, clarify that the id parameter is only valid during
probe.

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 | 28 +++++++++++++++-------------
 include/linux/pci.h      |  1 +
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 2e80ae150ff4..4851061babcb 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -179,6 +179,7 @@ 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_copy: Place to store copy of pci_device_id for dynamic 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
@@ -186,9 +187,9 @@ static const struct pci_device_id pci_device_id_any = {
  * structure or %NULL if there is no match.
  */
 static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
-						    struct pci_dev *dev)
+						    struct pci_dev *dev,
+						    struct pci_device_id *id_copy)
 {
-	struct pci_dynid *dynid;
 	const struct pci_device_id *found_id = NULL;
 	struct pci_device_id dev_id;
 	int ret;
@@ -200,17 +201,16 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
 
 	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;
+	scoped_guard(spinlock, &drv->dynids.lock) {
+		struct pci_dynid *dynid;
+
+		list_for_each_entry(dynid, &drv->dynids.list, node) {
+			if (pci_match_one_id(&dynid->id, &dev_id)) {
+				*id_copy = dynid->id;
+				return id_copy;
+			}
 		}
 	}
-	spin_unlock(&drv->dynids.lock);
-
-	if (found_id)
-		return found_id;
 
 	found_id = do_pci_match_id(drv->id_table, &dev_id, ret > 0);
 	if (found_id)
@@ -466,12 +466,13 @@ 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_copy;
 	int error = 0;
 
 	if (drv->probe) {
 		error = -ENODEV;
 
-		id = pci_match_device(drv, pci_dev);
+		id = pci_match_device(drv, pci_dev, &id_copy);
 		if (id)
 			error = pci_call_probe(drv, pci_dev, id);
 	}
@@ -1559,12 +1560,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_copy;
 
 	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);
+	found_id = pci_match_device(pci_drv, pci_dev, &id_copy);
 	if (found_id)
 		return 1;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 64b308b6e61c..92c17c116de6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -979,6 +979,7 @@ struct module;
  *		function returns zero when the driver chooses to
  *		take "ownership" of the device or an error code
  *		(negative number) otherwise.
+ *		The pci_device_id parameter is only valid during probe.
  *		The probe function always gets called from process
  *		context, so it can sleep.
  * @remove:	The remove() function gets called whenever a device

-- 
2.54.0

Re: [PATCH v3 1/9] ata: don't store pci_device_id

[Damien Le Moal]

On 7/6/26 23:11, Gary Guo wrote:
> pci_device_id is not guaranteed to live longer than probe due to presence
> of dynamic ID. All information apart from driver_data can be easily
> retrieved from pci_dev, so just store driver_data.
> 
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Gary Guo <gary@garyguo.net>

Looks good, but please change the commit title to:

ata: ata_generic: don't store pci_device_id

With that,

Acked-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research