[PATCH v2 0/7] 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. It can be triggered with
the following sequence:

echo <vid> <pid> > /sys/bus/usb/drivers/<name>/new_id
<probe start>
echo <vid> <pid> > /sys/bus/usb/drivers/<name>/remove_id
<probe use ID>

Fix it by making a stack copy of the ID. This does mean that the lifetime
of ID is scoped to probe (which is already the case but never spelled out
explicitly). Drivers use these device IDs creatively, so this series also
fix these drivers.

The following coccinelle script is used to find all cases that are deemed
suspicious. Only useful case for IDs should be access its fields, or
forwarding (without type cast) to functions that do so.

@usage@
identifier fn, id;
position p;
@@
fn(..., struct usb_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 usb_device_id *id, ...)
{
...
(T*)id@p
...
}

// Good use cases
@good@
identifier fn, id, fld;
expression E;
position usage.p;
@@
fn(..., struct usb_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 device ID")

There're 3 drivers that store usb_device_id, and they're converted to just
use driver_info instead. The other fields of usb_device_id can be easily
retrieved from usb_device via descriptor.id{Vendor,Product}.

There're also a few users that rely on pointer arithmetics. Pegaus and
xusbatm are converted to use driver_info. All unusal USB mass storage
drivers rely on pointer arithemtic to index into a side table, because USB
storage subsystem is using the driver_data for flags. Luckily all these
drivers set no_dynamic_id. Ideally these could be fixed too but their
maintainers probably have a better idea of how.

Link: https://lore.kernel.org/driver-core/20260706-pci_id_fi...@garyguo.net [1]

Signed-off-by: Gary Guo <ga...@garyguo.net>
---
Changes in v2:
- Add le16_to_cpu in ath9k patch.
- Fix AS102 which relies on address comparison. (Sashiko)
- Fix pegasus, xusbatm which relies on pointer arithmetic.
- usb_device_id from static ID table is now not copied. This ensures that
USB mass storage subsystem can still perform pointer arithmetic. This
also addresses Danilo's review feedback.
- Link to v1: https://patch.msgid.link/20260630-usb_dyn_id_...@garyguo.net

---
Gary Guo (7):
wifi: ath9k_htc: don't store usb_device_id
usb: usbtmc: don't store usb_device_id
usb: serial: spcp8x5: don't store usb_device_id
media: as102: do not rely on id table address comparison
net: usb: pegasus: don't rely on id table pointer arithmetic
usb: xusbatm: don't rely on id table pointer arithmetic
usb: fix UAF when probe runs concurrent to dyn ID removal

drivers/media/usb/as102/as102_usb_drv.c | 73 +++++++++++++-------------------
drivers/net/usb/pegasus.c | 54 ++++++++++-------------
drivers/net/usb/pegasus.h | 3 --
drivers/net/wireless/ath/ath9k/hif_usb.c | 12 +++---
drivers/net/wireless/ath/ath9k/hif_usb.h | 2 +-
drivers/usb/atm/xusbatm.c | 6 ++-
drivers/usb/class/usbtmc.c | 2 -
drivers/usb/core/driver.c | 12 ++++--
drivers/usb/serial/spcp8x5.c | 6 +--
include/linux/usb.h | 3 +-
10 files changed, 76 insertions(+), 97 deletions(-)
---
base-commit: 2b763db0c2763d6bf73d7d3e69665222d1f377cf
change-id: 20260629-usb_dyn_id_uaf-9d5f415387d4

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

[Gary Guo]

The current code is broken when dynamic ID is involved; in such cases
usb_device_id parameter of probe lives on the heap and the pointer
arithmetic will get an index that is wildly out of bound. xusbatm
initialize the USB device IDs dynamically so it can just use driver_info
too.

Even with conversion, xusbatm still cannot support dynamic IDs, so also set
no_dynamic_id.

Signed-off-by: Gary Guo <ga...@garyguo.net>
---

drivers/usb/atm/xusbatm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/atm/xusbatm.c b/drivers/usb/atm/xusbatm.c
index 0befbf63d1cc..5c1e1f521555 100644
--- a/drivers/usb/atm/xusbatm.c
+++ b/drivers/usb/atm/xusbatm.c
@@ -79,7 +79,7 @@ static int xusbatm_bind(struct usbatm_data *usbatm,
struct usb_interface *intf, const struct usb_device_id *id)
{
struct usb_device *usb_dev = interface_to_usbdev(intf);
- int drv_ix = id - xusbatm_usb_ids;
+ int drv_ix = id->driver_info;
int rx_alt = rx_altsetting[drv_ix];
int tx_alt = tx_altsetting[drv_ix];
struct usb_interface *rx_intf = xusbatm_find_intf(usb_dev, rx_alt, rx_endpoint[drv_ix]);
@@ -168,7 +168,8 @@ static struct usb_driver xusbatm_usb_driver = {
.name = xusbatm_driver_name,
.probe = xusbatm_usb_probe,
.disconnect = usbatm_usb_disconnect,
- .id_table = xusbatm_usb_ids
+ .id_table = xusbatm_usb_ids,
+ .no_dynamic_id = 1,
};

static int __init xusbatm_init(void)
@@ -190,6 +191,7 @@ static int __init xusbatm_init(void)
xusbatm_usb_ids[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
xusbatm_usb_ids[i].idVendor = vendor[i];
xusbatm_usb_ids[i].idProduct = product[i];
+ xusbatm_usb_ids[i].driver_info = i;

xusbatm_drivers[i].driver_name = xusbatm_driver_name;
xusbatm_drivers[i].bind = xusbatm_bind;

--
2.54.0

[Gary Guo]

The driver info should be retrieved using the driver_info field, not by
address comparison.

Signed-off-by: Gary Guo <ga...@garyguo.net>
---

drivers/media/usb/as102/as102_usb_drv.c | 73 ++++++++++++++-------------------
1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/drivers/media/usb/as102/as102_usb_drv.c b/drivers/media/usb/as102/as102_usb_drv.c
index a11024451ceb..be2f8be560fa 100644
--- a/drivers/media/usb/as102/as102_usb_drv.c
+++ b/drivers/media/usb/as102/as102_usb_drv.c
@@ -24,35 +24,33 @@ static void as102_usb_stop_stream(struct as102_dev_t *dev);
static int as102_open(struct inode *inode, struct file *file);
static int as102_release(struct inode *inode, struct file *file);

-static const struct usb_device_id as102_usb_id_table[] = {
- { USB_DEVICE(AS102_USB_DEVICE_VENDOR_ID, AS102_USB_DEVICE_PID_0001) },
- { USB_DEVICE(PCTV_74E_USB_VID, PCTV_74E_USB_PID) },
- { USB_DEVICE(ELGATO_EYETV_DTT_USB_VID, ELGATO_EYETV_DTT_USB_PID) },
- { USB_DEVICE(NBOX_DVBT_DONGLE_USB_VID, NBOX_DVBT_DONGLE_USB_PID) },
- { USB_DEVICE(SKY_IT_DIGITAL_KEY_USB_VID, SKY_IT_DIGITAL_KEY_USB_PID) },
- { } /* Terminating entry */
+struct as102_dev_info {
+ const char *name;
+ /*
+ * eLNA configuration: devices built on the reference design work best
+ * with 0xA0, while custom designs seem to require 0xC0
+ */
+ uint8_t elna_cfg;
};

-/* Note that this table must always have the same number of entries as the
- as102_usb_id_table struct */
-static const char * const as102_device_names[] = {
- AS102_REFERENCE_DESIGN,
- AS102_PCTV_74E,
- AS102_ELGATO_EYETV_DTT_NAME,
- AS102_NBOX_DVBT_DONGLE_NAME,
- AS102_SKY_IT_DIGITAL_KEY_NAME,
- NULL /* Terminating entry */
-};
+#define DRIVER_INFO(dev_name, dev_elna_cfg) \
+ .driver_info = (kernel_ulong_t)&(const struct as102_dev_info){ \
+ .name = (dev_name), \
+ .elna_cfg = (dev_elna_cfg), \
+ }

-/* eLNA configuration: devices built on the reference design work best
- with 0xA0, while custom designs seem to require 0xC0 */
-static uint8_t const as102_elna_cfg[] = {
- 0xA0,
- 0xC0,
- 0xC0,
- 0xA0,
- 0xA0,
- 0x00 /* Terminating entry */
+static const struct usb_device_id as102_usb_id_table[] = {
+ { USB_DEVICE(AS102_USB_DEVICE_VENDOR_ID, AS102_USB_DEVICE_PID_0001),
+ DRIVER_INFO(AS102_REFERENCE_DESIGN, 0xA0) },
+ { USB_DEVICE(PCTV_74E_USB_VID, PCTV_74E_USB_PID),
+ DRIVER_INFO(AS102_PCTV_74E, 0xC0) },
+ { USB_DEVICE(ELGATO_EYETV_DTT_USB_VID, ELGATO_EYETV_DTT_USB_PID),
+ DRIVER_INFO(AS102_ELGATO_EYETV_DTT_NAME, 0xC0) },
+ { USB_DEVICE(NBOX_DVBT_DONGLE_USB_VID, NBOX_DVBT_DONGLE_USB_PID),
+ DRIVER_INFO(AS102_NBOX_DVBT_DONGLE_NAME, 0xA0) },
+ { USB_DEVICE(SKY_IT_DIGITAL_KEY_USB_VID, SKY_IT_DIGITAL_KEY_USB_PID),
+ DRIVER_INFO(AS102_SKY_IT_DIGITAL_KEY_NAME, 0xA0) },
+ { } /* Terminating entry */
};

struct usb_driver as102_usb_driver = {
@@ -336,29 +334,18 @@ static int as102_usb_probe(struct usb_interface *intf,
{
int ret;
struct as102_dev_t *as102_dev;
- int i;
-
- /* This should never actually happen */
- if (ARRAY_SIZE(as102_usb_id_table) !=
- (sizeof(as102_device_names) / sizeof(const char *))) {
- pr_err("Device names table invalid size");
- return -EINVAL;
- }
+ const struct as102_dev_info *info = (const struct as102_dev_info *)id->driver_info;

as102_dev = kzalloc_obj(struct as102_dev_t);
if (as102_dev == NULL)
return -ENOMEM;

- /* Assign the user-friendly device name */
- for (i = 0; i < ARRAY_SIZE(as102_usb_id_table); i++) {
- if (id == &as102_usb_id_table[i]) {
- as102_dev->name = as102_device_names[i];
- as102_dev->elna_cfg = as102_elna_cfg[i];
- }
- }
-
- if (as102_dev->name == NULL)
+ if (info) {
+ as102_dev->name = info->name;
+ as102_dev->elna_cfg = info->elna_cfg;
+ } else {
as102_dev->name = "Unknown AS102 device";
+ }

/* set private callback functions */
as102_dev->bus_adap.ops = &as102_priv_ops;

--
2.54.0

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

Reviewed-by: Manuel Ebner <manue...@mailbox.org>
Reviewed-by: Danilo Krummrich <da...@kernel.org>

Signed-off-by: Gary Guo <ga...@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

[Gary Guo]

The current code is broken when dynamic ID is involved; in such cases
usb_device_id parameter of probe lives on the heap and the pointer

arithmetic will get an index that is wildly out of bound. Instead of
keeping a side table for additional information, use driver_info field of
the usb_device_id.

The dynamic ID parsing code needs to be updated for this; convert it to
just write to the reserved entry for dynamic ID and remove the weird loop.

Signed-off-by: Gary Guo <ga...@garyguo.net>
---

drivers/net/usb/pegasus.c | 54 +++++++++++++++++++----------------------------
drivers/net/usb/pegasus.h | 3 ---
2 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 8700eeb8e22d..aba1a640fc26 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -43,21 +43,12 @@ static bool loopback;
static bool mii_mode;
static char *devid;

-static struct usb_eth_dev usb_dev_id[] = {
-#define PEGASUS_DEV(pn, vid, pid, flags) \
- {.name = pn, .vendor = vid, .device = pid, .private = flags},
-#define PEGASUS_DEV_CLASS(pn, vid, pid, dclass, flags) \
- PEGASUS_DEV(pn, vid, pid, flags)
-#include "pegasus.h"
-#undef PEGASUS_DEV
-#undef PEGASUS_DEV_CLASS
- {NULL, 0, 0, 0},
- {NULL, 0, 0, 0}
-};
+static struct usb_eth_dev dynamic_id_info = {};

static struct usb_device_id pegasus_ids[] = {
#define PEGASUS_DEV(pn, vid, pid, flags) \
- {.match_flags = USB_DEVICE_ID_MATCH_DEVICE, .idVendor = vid, .idProduct = pid},
+ {.match_flags = USB_DEVICE_ID_MATCH_DEVICE, .idVendor = vid, .idProduct = pid, \
+ .driver_info = (kernel_ulong_t)&(const struct usb_eth_dev) {.name = pn, .private = flags}},
/*
* The Belkin F8T012xx1 bluetooth adaptor has the same vendor and product
* IDs as the Belkin F5D5050, so we need to teach the pegasus driver to
@@ -66,7 +57,8 @@ static struct usb_device_id pegasus_ids[] = {
*/
#define PEGASUS_DEV_CLASS(pn, vid, pid, dclass, flags) \
{.match_flags = (USB_DEVICE_ID_MATCH_DEVICE | USB_DEVICE_ID_MATCH_DEV_CLASS), \
- .idVendor = vid, .idProduct = pid, .bDeviceClass = dclass},
+ .idVendor = vid, .idProduct = pid, .bDeviceClass = dclass, \
+ .driver_info = (kernel_ulong_t)&(const struct usb_eth_dev) {.name = pn, .private = flags}},
#include "pegasus.h"
#undef PEGASUS_DEV
#undef PEGASUS_DEV_CLASS
@@ -402,12 +394,12 @@ static inline int reset_mac(pegasus_t *pegasus)
if (i == REG_TIMEOUT)
return -ETIMEDOUT;

- if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
- usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
+ if (le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_LINKSYS ||
+ le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_DLINK) {
set_register(pegasus, Gpio0, 0x24);
set_register(pegasus, Gpio0, 0x26);
}
- if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_ELCON) {
+ if (le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_ELCON) {
__u16 auxmode;
ret = read_mii_word(pegasus, 3, 0x1b, &auxmode);
if (ret < 0)
@@ -445,9 +437,9 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
memcpy(pegasus->eth_regs, data, sizeof(data));
ret = set_registers(pegasus, EthCtrl0, 3, data);

- if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
- usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
- usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
+ if (le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_LINKSYS ||
+ le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_LINKSYS2 ||
+ le16_to_cpu(pegasus->usb->descriptor.idVendor) == VENDOR_DLINK) {
u16 auxmode;
ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
if (ret < 0)
@@ -1153,7 +1145,7 @@ static int pegasus_probe(struct usb_interface *intf,
struct usb_device *dev = interface_to_usbdev(intf);
struct net_device *net;
pegasus_t *pegasus;
- int dev_index = id - pegasus_ids;
+ const struct usb_eth_dev *info = (const struct usb_eth_dev *)id->driver_info;
int res = -ENOMEM;
static const u8 bulk_ep_addr[] = {
PEGASUS_USB_EP_BULK_IN | USB_DIR_IN,
@@ -1178,7 +1170,6 @@ static int pegasus_probe(struct usb_interface *intf,
goto out;

pegasus = netdev_priv(net);
- pegasus->dev_index = dev_index;
pegasus->intf = intf;

res = alloc_urbs(pegasus);
@@ -1206,7 +1197,7 @@ static int pegasus_probe(struct usb_interface *intf,
pegasus->msg_enable = netif_msg_init(msg_level, NETIF_MSG_DRV
| NETIF_MSG_PROBE | NETIF_MSG_LINK);

- pegasus->features = usb_dev_id[dev_index].private;
+ pegasus->features = info ? info->private : DEFAULT_GPIO_RESET;
res = get_interrupt_interval(pegasus);
if (res)
goto out2;
@@ -1235,7 +1226,7 @@ static int pegasus_probe(struct usb_interface *intf,
queue_delayed_work(system_long_wq, &pegasus->carrier_check,
CARRIER_CHECK_DELAY);
dev_info(&intf->dev, "%s, %s, %pM\n", net->name,
- usb_dev_id[dev_index].name, net->dev_addr);
+ info ? info->name : "(unknown)", net->dev_addr);
return 0;

out3:
@@ -1325,8 +1316,9 @@ static struct usb_driver pegasus_driver = {

static void __init parse_id(char *id)
{
- unsigned int vendor_id = 0, device_id = 0, flags = 0, i = 0;
+ unsigned int vendor_id = 0, device_id = 0, flags = 0;
char *token, *name = NULL;
+ int dyn_id_index = ARRAY_SIZE(pegasus_ids) - 2;

token = strsep(&id, ":");
if (token)
@@ -1348,14 +1340,12 @@ static void __init parse_id(char *id)
if (device_id > 0x10000 || device_id == 0)
return;

- for (i = 0; usb_dev_id[i].name; i++);
- usb_dev_id[i].name = name;
- usb_dev_id[i].vendor = vendor_id;
- usb_dev_id[i].device = device_id;
- usb_dev_id[i].private = flags;
- pegasus_ids[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
- pegasus_ids[i].idVendor = vendor_id;
- pegasus_ids[i].idProduct = device_id;
+ dynamic_id_info.name = name;
+ dynamic_id_info.private = flags;
+ pegasus_ids[dyn_id_index].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
+ pegasus_ids[dyn_id_index].idVendor = vendor_id;
+ pegasus_ids[dyn_id_index].idProduct = device_id;
+ pegasus_ids[dyn_id_index].driver_info = (kernel_ulong_t)&dynamic_id_info;
}

static int __init pegasus_init(void)
diff --git a/drivers/net/usb/pegasus.h b/drivers/net/usb/pegasus.h
index a05b143155ba..ccdedcef52e7 100644
--- a/drivers/net/usb/pegasus.h
+++ b/drivers/net/usb/pegasus.h
@@ -85,7 +85,6 @@ typedef struct pegasus {
unsigned features;
u32 msg_enable;
u32 wolopts;
- int dev_index;
int intr_interval;
struct tasklet_struct rx_tl;
struct delayed_work carrier_check;
@@ -102,8 +101,6 @@ typedef struct pegasus {

struct usb_eth_dev {
char *name;
- __u16 vendor;
- __u16 device;
__u32 private; /* LSB is gpio reset value */
};


--
2.54.0

[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 <ga...@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 47f904e7e652..d3491ff08e6e 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";
@@ -1255,9 +1255,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,
+ le16_to_cpu(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;
@@ -1369,7 +1369,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

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

Reviewed-by: Manuel Ebner <manue...@mailbox.org>
Reviewed-by: Danilo Krummrich <da...@kernel.org>

Signed-off-by: Gary Guo <ga...@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

[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 <ga...@garyguo.net>
---

drivers/usb/core/driver.c | 12 ++++++++----
include/linux/usb.h | 3 ++-
2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f63004417058..7f33fe5ba03b 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -228,14 +228,16 @@ 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)
+ const struct usb_driver *drv,
+ struct usb_device_id *id_copy)
{
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_copy = dynid->id;
+ return id_copy;
}
}
return NULL;
@@ -321,6 +323,7 @@ static int usb_probe_interface(struct device *dev)
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_copy;
int error = -ENODEV;
int lpm_disable_error = -ENODEV;

@@ -340,7 +343,7 @@ static int usb_probe_interface(struct device *dev)
return error;
}

- id = usb_match_dynamic_id(intf, driver);
+ id = usb_match_dynamic_id(intf, driver, &id_copy);
if (!id)
id = usb_match_id(intf, driver->id_table);
if (!id)
@@ -892,6 +895,7 @@ static int usb_device_match(struct device *dev, const struct device_driver *drv)
struct usb_interface *intf;
const struct usb_driver *usb_drv;
const struct usb_device_id *id;
+ struct usb_device_id id_copy;

/* device drivers never match interfaces */
if (is_usb_device_driver(drv))
@@ -904,7 +908,7 @@ static int usb_device_match(struct device *dev, const struct device_driver *drv)
if (id)
return 1;

- id = usb_match_dynamic_id(intf, usb_drv);
+ id = usb_match_dynamic_id(intf, usb_drv, &id_copy);
if (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

[Greg Kroah-Hartman]

Thanks for these. I had an old patch series that attempted to do some
locking in this area to fix this up, but this version is much nicer.
I'll go queue it up now.

greg k-h