[PATCH 0/4] usb: fix UAF related to dynamic ID

[PATCH 0/4] usb: fix UAF related to dynamic ID

[Gary Guo]

This is the USB version of the dynamic ID UAF fix similar to that of PCI
[1]. usb_match_dynamic_id returns a pointer to field of usb_dynid, which
can be freed when dynamic ID is removed via sysfs. Fix it by making a stack
copy of the ID.
    
There're 2 existing users which stores their usb_device_id argument in
probe callback. This is a bad pattern because nothing except driver_data
inside usb_device_id is what they want. Actual idProduct information can be
retrieved from usb_device instead. I've used the following coccinelle
script to find the cases where the argument is stored and converted them to
stop storing usb_device_id.

There's an additional user, spcp8x5, which also stores usb_device_id, but
is part of USB serial which doesn't support dyn ID removal. However, there
is no reason in keeping the usb_device_id for it anyway so it is also
converted.

@store@
identifier fn;
identifier id;
expression E;
parameter list[n] ps;
@@
  fn(ps, struct usb_device_id *id, ...)
  {
    ...
*   E = id
    ...
  }

@cast@
identifier fn;
identifier id;
parameter list[n] ps;
@@
  fn(ps, struct usb_device_id *id, ...)
  {
    ...
*   (void *)id
    ...
  }

@in_struct@
identifier s, fld;
@@
  struct s {
    ...
*   struct usb_device_id *fld;
    ...
  };

Link: https://lore.kernel.org/driver-core/20260630-pci_id_fix-v2-0-b834a98c0af2@garyguo.net [1]

Signed-off-by: Gary Guo <gary@garyguo.net>
---
Gary Guo (4):
      wifi: ath9k_htc: don't keep usb_device_id
      usb: usbtmc: don't keep usb_device_id
      usb: serial: spcp8x5: don't keep usb_device_id
      usb: fix UAF when probe runs concurrent to dyn ID removal

 drivers/net/wireless/ath/ath9k/hif_usb.c | 12 ++++++------
 drivers/net/wireless/ath/ath9k/hif_usb.h |  2 +-
 drivers/usb/class/usbtmc.c               |  2 --
 drivers/usb/core/driver.c                | 33 ++++++++++++++++----------------
 drivers/usb/serial/spcp8x5.c             |  6 +++---
 include/linux/usb.h                      |  3 ++-
 6 files changed, 29 insertions(+), 29 deletions(-)
---
base-commit: 7de6ae9e12207ec146f2f3f1e58d1a99317e88bc
change-id: 20260629-usb_dyn_id_uaf-9d5f415387d4

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

[PATCH 1/4] wifi: ath9k_htc: don't keep usb_device_id

[Gary Guo]

usb_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 usb_device, so just store driver_data.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 12 ++++++------
 drivers/net/wireless/ath/ath9k/hif_usb.h |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 515267f48d80..d65c09c2b7fc 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1087,7 +1087,7 @@ static int ath9k_hif_usb_download_fw(struct hif_device_usb *hif_dev)
 	}
 	kfree(buf);
 
-	if (IS_AR7010_DEVICE(hif_dev->usb_device_id->driver_info))
+	if (IS_AR7010_DEVICE(hif_dev->id_info))
 		firm_offset = AR7010_FIRMWARE_TEXT;
 	else
 		firm_offset = AR9271_FIRMWARE_TEXT;
@@ -1182,7 +1182,7 @@ static int ath9k_hif_request_firmware(struct hif_device_usb *hif_dev,
 	if (MAJOR_VERSION_REQ == 1 && hif_dev->fw_minor_index == 3) {
 		const char *filename;
 
-		if (IS_AR7010_DEVICE(hif_dev->usb_device_id->driver_info))
+		if (IS_AR7010_DEVICE(hif_dev->id_info))
 			filename = FIRMWARE_AR7010_1_1;
 		else
 			filename = FIRMWARE_AR9271;
@@ -1198,7 +1198,7 @@ static int ath9k_hif_request_firmware(struct hif_device_usb *hif_dev,
 
 		return -ENOENT;
 	} else {
-		if (IS_AR7010_DEVICE(hif_dev->usb_device_id->driver_info))
+		if (IS_AR7010_DEVICE(hif_dev->id_info))
 			chip = "7010";
 		else
 			chip = "9271";
@@ -1260,9 +1260,9 @@ static void ath9k_hif_usb_firmware_cb(const struct firmware *fw, void *context)
 
 	ret = ath9k_htc_hw_init(hif_dev->htc_handle,
 				&hif_dev->interface->dev,
-				hif_dev->usb_device_id->idProduct,
+				hif_dev->udev->descriptor.idProduct,
 				hif_dev->udev->product,
-				hif_dev->usb_device_id->driver_info);
+				hif_dev->id_info);
 	if (ret) {
 		ret = -EINVAL;
 		goto err_htc_hw_init;
@@ -1374,7 +1374,7 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface,
 
 	hif_dev->udev = udev;
 	hif_dev->interface = interface;
-	hif_dev->usb_device_id = id;
+	hif_dev->id_info = id->driver_info;
 #ifdef CONFIG_PM
 	udev->reset_resume = 1;
 #endif
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
index dc0b0fa5c325..b3e7b0fb54b8 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -115,7 +115,7 @@ struct cmd_buf {
 struct hif_device_usb {
 	struct usb_device *udev;
 	struct usb_interface *interface;
-	const struct usb_device_id *usb_device_id;
+	int id_info;
 	const void *fw_data;
 	size_t fw_size;
 	struct completion fw_done;

-- 
2.54.0

[PATCH 2/4] usb: usbtmc: don't keep usb_device_id

[Gary Guo]

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

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/usb/class/usbtmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index af9ae55dae14..51cd9320a736 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -71,7 +71,6 @@ struct usbtmc_dev_capabilities {
  * allocated for each USBTMC device in the driver's probe function.
  */
 struct usbtmc_device_data {
-	const struct usb_device_id *id;
 	struct usb_device *usb_dev;
 	struct usb_interface *intf;
 	struct list_head file_list;
@@ -2394,7 +2393,6 @@ static int usbtmc_probe(struct usb_interface *intf,
 		return -ENOMEM;
 
 	data->intf = intf;
-	data->id = id;
 	data->usb_dev = usb_get_dev(interface_to_usbdev(intf));
 	usb_set_intfdata(intf, data);
 	kref_init(&data->kref);

-- 
2.54.0

[PATCH 3/4] usb: serial: spcp8x5: don't keep usb_device_id

[Gary Guo]

USB probe functions should not keep usb_device_id for longer than probe due
to presence of dynamic ID removal. USB serial does not support ID removal,
however in this case only driver_data is ever needed, there is no reason
keeping the usb_device_id in the first place, so convert it as well.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/usb/serial/spcp8x5.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/spcp8x5.c b/drivers/usb/serial/spcp8x5.c
index c11d64bf08fb..0e7715a02df4 100644
--- a/drivers/usb/serial/spcp8x5.c
+++ b/drivers/usb/serial/spcp8x5.c
@@ -133,14 +133,14 @@ struct spcp8x5_private {
 static int spcp8x5_probe(struct usb_serial *serial,
 						const struct usb_device_id *id)
 {
-	usb_set_serial_data(serial, (void *)id);
+	usb_set_serial_data(serial, (void *)id->driver_info);
 
 	return 0;
 }
 
 static int spcp8x5_port_probe(struct usb_serial_port *port)
 {
-	const struct usb_device_id *id = usb_get_serial_data(port->serial);
+	unsigned int quirks = (unsigned int)(unsigned long)usb_get_serial_data(port->serial);
 	struct spcp8x5_private *priv;
 
 	priv = kzalloc_obj(*priv);
@@ -148,7 +148,7 @@ static int spcp8x5_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&priv->lock);
-	priv->quirks = id->driver_info;
+	priv->quirks = quirks;
 
 	usb_set_serial_port_data(port, priv);
 

-- 
2.54.0

[PATCH 4/4] usb: fix UAF when probe runs concurrent to dyn ID removal

[Gary Guo]

Dynamic IDs are only guaranteed to be valid when usb_dynids_lock is held,
as remove_id_store can free the node. Thus, make a copy in
usb_probe_interface. Clarify the documentation that the id parameter is
only valid during the probe.

USB serial has the same pattern, but it does not need fixing as the IDs
cannot be removed via sysfs.

Fixes: 0c7a2b72746a ("USB: add remove_id sysfs attr for usb drivers")
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/usb/core/driver.c | 33 +++++++++++++++++----------------
 include/linux/usb.h       |  3 ++-
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f63004417058..c26410cabdfe 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -227,18 +227,19 @@ static void usb_free_dynids(struct usb_driver *usb_drv)
 	}
 }
 
-static const struct usb_device_id *usb_match_dynamic_id(struct usb_interface *intf,
-							const struct usb_driver *drv)
+static bool usb_match_dynamic_id(struct usb_interface *intf, const struct usb_driver *drv,
+				 struct usb_device_id *id)
 {
 	struct usb_dynid *dynid;
 
 	guard(mutex)(&usb_dynids_lock);
 	list_for_each_entry(dynid, &drv->dynids.list, node) {
 		if (usb_match_one_id(intf, &dynid->id)) {
-			return &dynid->id;
+			*id = dynid->id;
+			return true;
 		}
 	}
-	return NULL;
+	return false;
 }
 
 
@@ -320,7 +321,8 @@ static int usb_probe_interface(struct device *dev)
 	struct usb_driver *driver = to_usb_driver(dev->driver);
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_device *udev = interface_to_usbdev(intf);
-	const struct usb_device_id *id;
+	struct usb_device_id id;
+	const struct usb_device_id *matched_id;
 	int error = -ENODEV;
 	int lpm_disable_error = -ENODEV;
 
@@ -340,11 +342,12 @@ static int usb_probe_interface(struct device *dev)
 		return error;
 	}
 
-	id = usb_match_dynamic_id(intf, driver);
-	if (!id)
-		id = usb_match_id(intf, driver->id_table);
-	if (!id)
-		return error;
+	if (!usb_match_dynamic_id(intf, driver, &id)) {
+		matched_id = usb_match_id(intf, driver->id_table);
+		if (!matched_id)
+			return error;
+		id = *matched_id;
+	}
 
 	dev_dbg(dev, "%s - got id\n", __func__);
 
@@ -393,7 +396,7 @@ static int usb_probe_interface(struct device *dev)
 		intf->needs_altsetting0 = 0;
 	}
 
-	error = driver->probe(intf, id);
+	error = driver->probe(intf, &id);
 	if (error)
 		goto err;
 
@@ -891,7 +894,7 @@ static int usb_device_match(struct device *dev, const struct device_driver *drv)
 	} else if (is_usb_interface(dev)) {
 		struct usb_interface *intf;
 		const struct usb_driver *usb_drv;
-		const struct usb_device_id *id;
+		struct usb_device_id id;
 
 		/* device drivers never match interfaces */
 		if (is_usb_device_driver(drv))
@@ -900,12 +903,10 @@ static int usb_device_match(struct device *dev, const struct device_driver *drv)
 		intf = to_usb_interface(dev);
 		usb_drv = to_usb_driver(drv);
 
-		id = usb_match_id(intf, usb_drv->id_table);
-		if (id)
+		if (usb_match_id(intf, usb_drv->id_table))
 			return 1;
 
-		id = usb_match_dynamic_id(intf, usb_drv);
-		if (id)
+		if (usb_match_dynamic_id(intf, usb_drv, &id))
 			return 1;
 	}
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 1da4ad1610bc..49ab8dbb885f 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1185,7 +1185,8 @@ extern ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf);
  *	interface.  It may also use usb_set_interface() to specify the
  *	appropriate altsetting.  If unwilling to manage the interface,
  *	return -ENODEV, if genuine IO errors occurred, an appropriate
- *	negative errno value.
+ *	negative errno value.  The usb_device_id parameter is only valid during
+ *	probe.
  * @disconnect: Called when the interface is no longer accessible, usually
  *	because its device has been (or is being) disconnected or the
  *	driver module is being unloaded.

-- 
2.54.0

Re: [PATCH 0/4] usb: fix UAF related to dynamic ID

[Manuel Ebner]

On Tue, 2026-06-30 at 12:38 +0100, Gary Guo wrote:
> [...]
> 
> Signed-off-by: Gary Guo <gary@garyguo.net>

For all four patches:
LGTM.

I guess I'm not in a position to Acked-by or Reviewed-by.

 Manuel


> ---
> Gary Guo (4):
>       wifi: ath9k_htc: don't keep usb_device_id
>       usb: usbtmc: don't keep usb_device_id
>       usb: serial: spcp8x5: don't keep usb_device_id
>       usb: fix UAF when probe runs concurrent to dyn ID removal
> 
>  drivers/net/wireless/ath/ath9k/hif_usb.c | 12 ++++++------
>  drivers/net/wireless/ath/ath9k/hif_usb.h |  2 +-
>  drivers/usb/class/usbtmc.c               |  2 --
>  drivers/usb/core/driver.c                | 33 ++++++++++++++++----------------
>  drivers/usb/serial/spcp8x5.c             |  6 +++---
>  include/linux/usb.h                      |  3 ++-
>  6 files changed, 29 insertions(+), 29 deletions(-)
> ---
> base-commit: 7de6ae9e12207ec146f2f3f1e58d1a99317e88bc
> change-id: 20260629-usb_dyn_id_uaf-9d5f415387d4
> 
> Best regards,
> --  
> Gary Guo <gary@garyguo.net>

Re: [PATCH 0/4] usb: fix UAF related to dynamic ID

[Danilo Krummrich]

Hi Manuel,

On Tue Jun 30, 2026 at 3:26 PM CEST, Manuel Ebner wrote:
> On Tue, 2026-06-30 at 12:38 +0100, Gary Guo wrote:
>> [...]
>> 
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>
> For all four patches:
> LGTM.
>
> I guess I'm not in a position to Acked-by or Reviewed-by.

I don't know your background and I don't want to make any assumptions, so please
don't interpret anything in my attempt to clarify a bit.

As documented in [1] "any interested reviewer (who has done the work and is a
person with known identity) can offer a Reviewed-by tag for a patch".

An offer from anyone who has done this work is much appreciated.

Acked-by is usually used by stakeholders of the code (most commonly maintainers)
to indicate acceptance, e.g. for patch routing purposes. There are more details
in [2].

[1] https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight
[2] https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Re: [PATCH 1/4] wifi: ath9k_htc: don't keep usb_device_id

[Danilo Krummrich]

On Tue Jun 30, 2026 at 1:38 PM CEST, Gary Guo wrote:
> usb_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 usb_device, so just store driver_data.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH 2/4] usb: usbtmc: don't keep usb_device_id

[Danilo Krummrich]

On Tue Jun 30, 2026 at 1:38 PM CEST, Gary Guo wrote:
> usb_device_id is not guaranteed to live longer than probe due to presence
> of dynamic ID. This stored ID is unused so remove it.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH 3/4] usb: serial: spcp8x5: don't keep usb_device_id

[Danilo Krummrich]

On Tue Jun 30, 2026 at 1:38 PM CEST, Gary Guo wrote:
> USB probe functions should not keep usb_device_id for longer than probe due
> to presence of dynamic ID removal. USB serial does not support ID removal,
> however in this case only driver_data is ever needed, there is no reason
> keeping the usb_device_id in the first place, so convert it as well.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH 4/4] usb: fix UAF when probe runs concurrent to dyn ID removal

[Danilo Krummrich]

On Tue Jun 30, 2026 at 1:38 PM CEST, Gary Guo wrote:
> @@ -320,7 +321,8 @@ static int usb_probe_interface(struct device *dev)
>  	struct usb_driver *driver = to_usb_driver(dev->driver);
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_device *udev = interface_to_usbdev(intf);
> -	const struct usb_device_id *id;
> +	struct usb_device_id id;
> +	const struct usb_device_id *matched_id;
>  	int error = -ENODEV;
>  	int lpm_disable_error = -ENODEV;
>  
> @@ -340,11 +342,12 @@ static int usb_probe_interface(struct device *dev)
>  		return error;
>  	}
>  
> -	id = usb_match_dynamic_id(intf, driver);
> -	if (!id)
> -		id = usb_match_id(intf, driver->id_table);
> -	if (!id)
> -		return error;
> +	if (!usb_match_dynamic_id(intf, driver, &id)) {
> +		matched_id = usb_match_id(intf, driver->id_table);
> +		if (!matched_id)
> +			return error;
> +		id = *matched_id;
> +	}

I think this could just be:

	struct usb_device_id id_copy;

	if (usb_match_dynamic_id(intf, driver, &id_copy)) {
		id = &id_copy;
	} else {
		id = usb_match_id(intf, driver->id_table);
		if (!id)
			return error;
	}

Avoids the unnecessary copy and also results in a smaller diff.

Re: [PATCH 0/4] usb: fix UAF related to dynamic ID

[Manuel Ebner]

On Tue, 2026-06-30 at 15:39 +0200, Danilo Krummrich wrote:
> Hi Manuel,
> 
> On Tue Jun 30, 2026 at 3:26 PM CEST, Manuel Ebner wrote:
> > On Tue, 2026-06-30 at 12:38 +0100, Gary Guo wrote:
> > > [...]
> > > 
> > > Signed-off-by: Gary Guo <gary@garyguo.net>
> > 
> > For all four patches:
> > LGTM.
> > 
> > I guess I'm not in a position to Acked-by or Reviewed-by.
> 
> I don't know your background and I don't want to make any assumptions, so please
> don't interpret anything in my attempt to clarify a bit.
> 
> As documented in [1] "any interested reviewer (who has done the work and is a
> person with known identity) can offer a Reviewed-by tag for a patch".
> 
> An offer from anyone who has done this work is much appreciated.
> 
Then you can add my
Reviewed-by: Manuel Ebner <manuelebner@mailbox.org>
to all 4 patches.

 Manuel

> Acked-by is usually used by stakeholders of the code (most commonly maintainers)
> to indicate acceptance, e.g. for patch routing purposes. There are more details
> in [2].
> 
> [1]
> https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight
> [2]
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by