[PATCH] usb: gadget: fix f_printer ep0 overflow/list race and f_hid/f_tcm/f_eem bounds

[PATCH] usb: gadget: fix f_printer ep0 overflow/list race and f_hid/f_tcm/f_eem bounds

[Hui Peng]

Fix multiple memory corruption bugs in USB gadget function drivers:

1. In printer_func_setup() and printer_reset_interface()
   (drivers/usb/gadget/function/f_printer.c), bound GET_DEVICE_ID copies
   to USB_COMP_EP0_BUFSIZ (1024 bytes) under lock, and dequeue from
   dev->rx_reqs_active instead of dev->rx_buffers in
   printer_reset_interface().
2. In drivers/usb/gadget/function/f_hid.c, f_tcm.c, and f_eem.c,
   validate setup wLength, command lengths, and skb bounds.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
index ac37d7c1d168..85dc67bc6be1 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -457,7 +457,7 @@ static int eem_unwrap(struct gether *port,
 					goto next;
 				}
 
-				ctx = kmalloc_obj(*ctx);
+				ctx = kmalloc_obj(*ctx, GFP_ATOMIC);
 				if (!ctx) {
 					kfree(req->buf);
 					usb_ep_free_request(ep, req);
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..9febf853a918 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -426,6 +426,9 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 {
 	struct f_hidg *hidg = file->private_data;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	if (hidg->use_out_ep)
 		return f_hidg_intout_read(file, buffer, count, ptr);
 	else
@@ -437,7 +440,7 @@ static void f_hidg_req_complete(struct usb_ep *ep, struct usb_request *req)
 	struct f_hidg *hidg = (struct f_hidg *)ep->driver_data;
 	unsigned long flags;
 
-	if (req->status != 0) {
+	if (req->status != 0 && hidg->func.config && hidg->func.config->cdev) {
 		ERROR(hidg->func.config->cdev,
 			"End Point Request ERROR: %d\n", req->status);
 	}
@@ -456,6 +459,9 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	unsigned long flags;
 	ssize_t status = -ENOMEM;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 	if (!hidg->req) {
@@ -644,12 +650,16 @@ static int f_hidg_get_report_id(struct file *file, __u8 __user *buffer)
 static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *buffer)
 {
 	struct f_hidg			*hidg = file->private_data;
-	struct usb_composite_dev	*cdev = hidg->func.config->cdev;
+	struct usb_composite_dev	*cdev;
 	unsigned long	flags;
 	struct report_entry *entry;
 	struct report_entry *ptr;
 	__u8 report_id;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+	cdev = hidg->func.config->cdev;
+
 	entry = kmalloc_obj(*entry);
 	if (!entry)
 		return -ENOMEM;
@@ -668,13 +678,7 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 
 	if (ptr) {
 		/* Report already exists in list - update it */
-		if (copy_from_user(&ptr->report_data, buffer,
-				sizeof(struct usb_hidg_report))) {
-			spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
-			ERROR(cdev, "copy_from_user error\n");
-			kfree(entry);
-			return -EINVAL;
-		}
+		ptr->report_data = entry->report_data;
 		kfree(entry);
 	} else {
 		/* Report does not exist in list - add it */
@@ -998,11 +1002,12 @@ static void hidg_disable(struct usb_function *f)
 
 	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
 	if (!hidg->get_report_returned) {
-		usb_ep_free_request(f->config->cdev->gadget->ep0, hidg->get_req);
-		hidg->get_req = NULL;
 		hidg->get_report_returned = true;
+		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
+		usb_ep_dequeue(f->config->cdev->gadget->ep0, hidg->get_req);
+	} else {
+		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 	}
-	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 
 	spin_lock_irqsave(&hidg->read_spinlock, flags);
 	hidg->disabled = true;
@@ -1100,6 +1105,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 
 	if (hidg->in_ep != NULL) {
 		spin_lock_irqsave(&hidg->write_spinlock, flags);
+		if (hidg->req && !hidg->write_pending)
+			free_ep_req(hidg->in_ep, hidg->req);
 		hidg->req = req_in;
 		hidg->write_pending = 0;
 		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -1585,10 +1592,19 @@ static void hidg_free(struct usb_function *f)
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
+	struct report_entry *entry, *tmp;
+	unsigned long flags;
 
 	cdev_device_del(hidg->cdev, &hidg->dev);
 	destroy_workqueue(hidg->workqueue);
+	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
+	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 	usb_free_all_descriptors(f);
+	hidg->func.config = NULL;
 }
 
 static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 1857d786110b..7709a20fa4cb 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -940,7 +940,7 @@ static void printer_soft_reset(struct printer_dev *dev)
 	}
 
 	while (likely(!(list_empty(&dev->rx_reqs_active)))) {
-		req = container_of(dev->rx_buffers.next, struct usb_request,
+		req = container_of(dev->rx_reqs_active.next, struct usb_request,
 				list);
 		list_del_init(&req->list);
 		list_add(&req->list, &dev->rx_reqs);
@@ -1030,16 +1030,19 @@ static int printer_func_setup(struct usb_function *f,
 			if ((wIndex>>8) != dev->interface)
 				break;
 
-			if (!*dev->pnp_string) {
+			spin_lock(&dev->lock);
+			if (!dev->pnp_string || !*dev->pnp_string) {
+				spin_unlock(&dev->lock);
 				value = 0;
 				break;
 			}
-			value = strlen(*dev->pnp_string);
-			buf[0] = (value >> 8) & 0xFF;
-			buf[1] = value & 0xFF;
+			value = min_t(size_t, strlen(*dev->pnp_string),
+				      USB_COMP_EP0_BUFSIZ - 2);
+			buf[0] = ((value + 2) >> 8) & 0xFF;
+			buf[1] = (value + 2) & 0xFF;
 			memcpy(buf + 2, *dev->pnp_string, value);
-			DBG(dev, "1284 PNP String: %x %s\n", value,
-			    *dev->pnp_string);
+			spin_unlock(&dev->lock);
+			value = min_t(u16, wLength, value + 2);
 			break;
 
 		case GET_PORT_STATUS: /* Get Port Status */
@@ -1265,11 +1268,17 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
 {
 	struct f_printer_opts *opts = to_f_printer_opts(item);
 	char *new_pnp;
+	size_t copy_len;
 	int result;
 
 	mutex_lock(&opts->lock);
+	if (opts->refcnt) {
+		result = -EBUSY;
+		goto unlock;
+	}
 
-	new_pnp = kstrndup(page, len, GFP_KERNEL);
+	copy_len = min_t(size_t, len, USB_COMP_EP0_BUFSIZ - 2);
+	new_pnp = kstrndup(page, copy_len, GFP_KERNEL);
 	if (!new_pnp) {
 		result = -ENOMEM;
 		goto unlock;
@@ -1277,7 +1286,6 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
 
 	if (opts->pnp_string_allocated)
 		kfree(opts->pnp_string);
-
 	opts->pnp_string_allocated = true;
 	opts->pnp_string = new_pnp;
 	result = len;
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a..6d67ec153b24 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -136,6 +136,8 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
 			req->length = cmd->data_len;
 			cmd->data_len = 0;
 		}
+		if (cmd->is_read)
+			memset(fu->cmd[0].buf, 0, req->length);
 		req->complete = bot_err_compl;
 		req->context = cmd;
 		req->buf = fu->cmd[0].buf;
@@ -330,7 +332,8 @@ static int bot_prepare_reqs(struct f_uas *fu)
 	fu->bot_status.req->complete = bot_status_complete;
 	fu->bot_status.csw.Signature = cpu_to_le32(US_BULK_CS_SIGN);
 
-	fu->cmd[0].buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
+	fu->cmd[0].buf = kzalloc(max(fu->ep_out->maxpacket,
+				     fu->ep_in->maxpacket), GFP_KERNEL);
 	if (!fu->cmd[0].buf)
 		goto err_buf;
 
@@ -528,7 +531,10 @@ static void uasp_free_cmdreq(struct f_uas *fu)
 	int i;
 
 	for (i = 0; i < USBG_NUM_CMDS; i++) {
-		usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
+		if (fu->cmd[i].req) {
+			usb_ep_dequeue(fu->ep_cmd, fu->cmd[i].req);
+			usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
+		}
 		kfree(fu->cmd[i].buf);
 		fu->cmd[i].req = NULL;
 		fu->cmd[i].buf = NULL;
@@ -871,7 +877,8 @@ static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 		return;
 	}
 
-	usbg_submit_command(fu, req);
+	if (usbg_submit_command(fu, req) < 0)
+		usb_ep_queue(fu->ep_cmd, req, GFP_ATOMIC);
 }
 
 static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
@@ -904,11 +911,14 @@ static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
 
 static int uasp_alloc_cmd(struct f_uas *fu, int i)
 {
+	size_t alloc_len = max_t(size_t, fu->ep_cmd->maxpacket,
+				 sizeof(struct command_iu) + USBG_MAX_CMD);
+
 	fu->cmd[i].req = usb_ep_alloc_request(fu->ep_cmd, GFP_KERNEL);
 	if (!fu->cmd[i].req)
 		goto err;
 
-	fu->cmd[i].buf = kmalloc(fu->ep_cmd->maxpacket, GFP_KERNEL);
+	fu->cmd[i].buf = kzalloc(alloc_len, GFP_KERNEL);
 	if (!fu->cmd[i].buf)
 		goto err_buf;
 
@@ -920,6 +930,7 @@ static int uasp_alloc_cmd(struct f_uas *fu, int i)
 
 err_buf:
 	usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
+	fu->cmd[i].req = NULL;
 err:
 	return -ENOMEM;
 }
@@ -949,6 +960,7 @@ static int uasp_prepare_reqs(struct f_uas *fu)
 
 err_free_stream:
 	uasp_free_cmdreq(fu);
+	i = USBG_NUM_CMDS;
 
 err_cleanup:
 	if (i) {
@@ -1382,6 +1394,9 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 		return -EINVAL;
 	}
 
+	if (req->actual < sizeof(struct command_iu))
+		return -EINVAL;
+
 	scsi_tag = be16_to_cpup(&iu->tag);
 	cmd = usbg_get_cmd(fu, tv_nexus, scsi_tag);
 	if (IS_ERR(cmd)) {
@@ -1434,7 +1449,8 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 	}
 
 	cmd_len = (cmd_iu->len & ~0x3) + 16;
-	if (cmd_len > USBG_MAX_CMD) {
+	if (cmd_len > USBG_MAX_CMD ||
+	    req->actual < offsetof(struct command_iu, cdb) + cmd_len) {
 		target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd);
 		hash_del(&stream->node);
 		return -EINVAL;
@@ -1740,6 +1756,9 @@ static struct se_portal_group *usbg_make_tpg(struct se_wwn *wwn,
 
 static int tcm_usbg_drop_nexus(struct usbg_tpg *);
 
+static int usbg_attach(struct usbg_tpg *);
+static void usbg_detach(struct usbg_tpg *);
+
 static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 {
 	struct usbg_tpg *tpg = container_of(se_tpg,
@@ -1747,6 +1766,11 @@ static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 	unsigned i;
 	struct f_tcm_opts *opts;
 
+	if (tpg->gadget_connect) {
+		usbg_detach(tpg);
+		tpg->gadget_connect = false;
+	}
+
 	tcm_usbg_drop_nexus(tpg);
 	core_tpg_deregister(se_tpg);
 	destroy_workqueue(tpg->workqueue);
@@ -1760,6 +1784,7 @@ static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 		opts = container_of(tpg_instances[i].func_inst,
 			struct f_tcm_opts, func_inst);
 		mutex_lock(&opts->dep_lock);
+		opts->can_attach = false;
 		if (opts->has_dep)
 			module_put(opts->dependent);
 		else
@@ -1816,9 +1841,6 @@ static struct configfs_attribute *usbg_wwn_attrs[] = {
 	NULL,
 };
 
-static int usbg_attach(struct usbg_tpg *);
-static void usbg_detach(struct usbg_tpg *);
-
 static int usbg_enable_tpg(struct se_portal_group *se_tpg, bool enable)
 {
 	struct usbg_tpg  *tpg = container_of(se_tpg, struct usbg_tpg, se_tpg);

Re: [PATCH] usb: gadget: fix f_printer ep0 overflow/list race and f_hid/f_tcm/f_eem bounds

[ivy lopez]

On Fri, Sep 19, 2026 at 10:35 PM UTC, Hui Peng wrote:
> Fix multiple memory corruption bugs in USB gadget function drivers:
>
> 1. In printer_func_setup() and printer_reset_interface()
>    (drivers/usb/gadget/function/f_printer.c), bound GET_DEVICE_ID copies
>    to USB_COMP_EP0_BUFSIZ (1024 bytes) under lock, and dequeue from
>    dev->rx_reqs_active instead of dev->rx_buffers in
>    printer_reset_interface().
> 2. In drivers/usb/gadget/function/f_hid.c, f_tcm.c, and f_eem.c,
>    validate setup wLength, command lengths, and skb bounds.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>

This touches four unrelated drivers (f_printer, f_hid, f_tcm, f_eem) 
under one Fixes tag, and 1da177e4c3f4 isn't a real Fixes tag for any
of it, it's what you get when nobody runs git blame per hunk.
Please split this into one patch per driver, each with its own
actual introducing commit.

> -		while (likely(!(list_empty(&dev->rx_reqs_active)))) {
> -			req = container_of(dev->rx_buffers.next, struct usb_request,
> +			req = container_of(dev->rx_reqs_active.next, struct usb_request,

This is genuinely bad. After the loop above it drains rx_buffers,
rx_buffers.next points back to rx_buffers itself, so this second loop
computes a fake usb_request via container_of on a list_head embedded
in printer_dev, then writes through it via list_del_init/list_add.
It also never drains rx_reqs_active since it's dequeuing from the
wrong list, so this spins corrupting memory each iteration.

git blame puts the actual introducing commit at b185f01a9ab7a ("usb: gadget:
Restructure printer gadget", 2015-03-03), not 1da177e4c3f4. Please
use that as the Fixes tag when you split this out.

> -			value = strlen(*dev->pnp_string);
> -			buf[0] = (value >> 8) & 0xFF;
> -			buf[1] = value & 0xFF;
> +			value = min_t(size_t, strlen(*dev->pnp_string),
> +				      USB_COMP_EP0_BUFSIZ - 2);
> +			buf[0] = ((value + 2) >> 8) & 0xFF;
> +			buf[1] = (value + 2) & 0xFF;

Nothing bounds strlen(*dev->pnp_string) against buf's
capacity before the memcpy below it, and pnp_string is configfs
settable with no length cap in f_printer_opts_pnp_string_store either,
so this is a genuine overflow path.

Separately from the overflow, the length prefix per IEEE 1284.3 is supposed to
include the two length bytes themselves, which the original strlen()
value doesn't. Probably worth its own patch too.

Separately, in f_hid.c:

> +	if (!hidg->func.config || !hidg->func.config->cdev)
> +		return -ENODEV;
>
>  	if (hidg->use_out_ep)
>  		return f_hidg_intout_read(file, buffer, count, ptr);

This hunk (and the matching ones in f_hidg_write() and
f_hidg_get_report()) doesn't close the race it's aimed at.
hidg_unbind() does:

> +	usb_free_all_descriptors(f);
> +	hidg->func.config = NULL;

with no lock shared with the checks above, so this is an
unsynchronized check followed by an unsynchronized use, racing an
unsynchronized write. It shrinks the window, it doesn't close it. If
this is worth fixing, it needs whatever synchronization already
coordinates unbind against the fops paths elsewhere in the driver, not
a bare pointer check with no lock behind it.

And:

> 	if (ptr) {
> 		/* Report already exists in list - update it */
> -		if (copy_from_user(&ptr->report_data, buffer,
> -				sizeof(struct usb_hidg_report))) {
> -			spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
> -			ERROR(cdev, "copy_from_user error\n");
> -			kfree(entry);
> -			return -EINVAL;
> -		}
> +		ptr->report_data = entry->report_data;

This one looks good to me and worth keeping. The existing code does
two copy_from_user() calls against the same user buffer for no reason
(once into entry->report_data unconditionally at the top of the
function, again into ptr->report_data if an entry already existed),
and this removes the redundant, racy second read. That one's real,
keep it.

Also please skip the func.config hunk unless you're going to actually
synchronize it against unbind.

ivy