[PATCH 0/3] usb: gadget: f_hid: three GET_REPORT fixes

[PATCH 0/3] usb: gadget: f_hid: three GET_REPORT fixes

[Shunbo Shi]

Cover-letter:

Hi Greg,

I found these three bugs while looking into the Sashiko AI review
report of a patch series of mine that touched f_hid.c [1]: it
flagged a data race in the GET_REPORT userspace ioctl code, and
digging into it revealed two more issues in the same feature
(a139c98f760e, v6.11):

  1. sleep-in-atomic-context: the second copy_from_user() in
     f_hidg_get_report() runs under spin_lock_irqsave(); a page
     fault on the user buffer deadlocks the scheduler.

  2. memory leak: report_list entries allocated by the ioctl are
     never freed in hidg_release().

  3. data race: the GET_REPORT response is served by pointing
     get_req->buf directly at a report_list entry's data; the
     request completes asynchronously after the spinlock is
     dropped, so a concurrent ioctl updating the same entry
     corrupts the data on the wire (a torn write).

All three were introduced by the same commit and are fixable
deterministically from the code.  The series is on top of v7.4-rc1
and applies cleanly to usb.git testing.

[1] Sashiko review: https://sashiko.dev/ (patchset 20261110064105.20335-1)

Shunbo Shi (3):
  usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()
  usb: gadget: f_hid: fix report_list entries leak
  usb: gadget: f_hid: fix race between userspace report update and
  GET_REPORT response

 drivers/usb/gadget/function/f_hid.c | 42 ++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 10 deletions(-)

--
2.53.0

[PATCH 1/3] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[Shunbo Shi]

The second copy_from_user() in f_hidg_get_report() is called while
holding hidg->get_report_spinlock with interrupts disabled.  If the
user page backing the ioctl buffer is not resident, copy_from_user()
takes a page fault and sleeps, which is not allowed with a spinlock
held: it triggers might_sleep() warnings with
CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
CPUs spinning on the lock.

The first copy_from_user() into the freshly allocated entry is
already done before the spinlock is taken; only the "report already
exists, update it" branch still copies inside the critical section.

Since the data has already been fully copied into entry->report_data
before taking the lock, updating an existing report only needs a
plain assignment between two kernel-memory objects, which is safe
inside the spinlock.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..b778eb633b79 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -654,6 +654,11 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 	if (!entry)
 		return -ENOMEM;
 
+	/*
+	 * Copy the report from userspace into the entry before taking the
+	 * spinlock: copy_from_user() may sleep, so it must not be called
+	 * with the spinlock held (and interrupts disabled).
+	 */
 	if (copy_from_user(&entry->report_data, buffer,
 				sizeof(struct usb_hidg_report))) {
 		ERROR(cdev, "copy_from_user error\n");
@@ -668,13 +673,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 */
-- 
2.53.0

Re: [PATCH 1/3] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[sashiko-bot]

> The second copy_from_user() in f_hidg_get_report() is called while
> holding hidg->get_report_spinlock with interrupts disabled.  If the
> user page backing the ioctl buffer is not resident, copy_from_user()
> takes a page fault and sleeps, which is not allowed with a spinlock
> held: it triggers might_sleep() warnings with
> CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
> CPUs spinning on the lock.
> 
> The first copy_from_user() into the freshly allocated entry is
> already done before the spinlock is taken; only the "report already
> exists, update it" branch still copies inside the critical section.
> 
> Since the data has already been fully copied into entry->report_data
> before taking the lock, updating an existing report only needs a
> plain assignment between two kernel-memory objects, which is safe
> inside the spinlock.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111021655.37851-1-shishunbo@cmss.chinamobile.com?part=1

[PATCH 2/3] usb: gadget: f_hid: fix report_list entries leak

[Shunbo Shi]

Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
yet in hidg->report_list allocates a report_entry and adds it to
the list.  hidg_release() frees report_desc and set_report_buf, but
never walks report_list, so every accumulated entry leaks on
function destroy (e.g. when the HID gadget is unbound and the
device is removed).

Free the entries in hidg_release().  This is safe without extra
locking: at release time the function is unbound (the workqueue is
already destroyed and userspace has no way to reach the ioctl).

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index b778eb633b79..d7ba30323414 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -121,6 +121,12 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 static void hidg_release(struct device *dev)
 {
 	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+	struct report_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
 
 	kfree(hidg->report_desc);
 	kfree(hidg->set_report_buf);
-- 
2.53.0

Re: [PATCH 2/3] usb: gadget: f_hid: fix report_list entries leak

[sashiko-bot]

> Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
> yet in hidg->report_list allocates a report_entry and adds it to
> the list.  hidg_release() frees report_desc and set_report_buf, but
> never walks report_list, so every accumulated entry leaks on
> function destroy (e.g. when the HID gadget is unbound and the
> device is removed).
> 
> Free the entries in hidg_release().  This is safe without extra
> locking: at release time the function is unbound (the workqueue is
> already destroyed and userspace has no way to reach the ioctl).
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111021655.37851-1-shishunbo@cmss.chinamobile.com?part=2

[PATCH 3/3] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[Shunbo Shi]

The GET_REPORT response is served by pointing get_req->buf at the
data[] array of a report_list entry and calling usb_ep_queue().
The request completes asynchronously, so the controller may still
be reading the entry's buffer after get_report_workqueue_handler()
drops get_report_spinlock.

At the same time, userspace can update that same entry via the
GADGET_HID_WRITE_GET_REPORT ioctl: f_hidg_get_report() copies new
report data into the entry while holding the spinlock, but the
transfer reading the entry is not serialized against that update
(the lock was already released by the work handler).

The result is that the host can receive a GET_REPORT response that
mixes bytes of the old and the new report (a torn write), with no
memory-safety violation but corrupted report data on the wire.

Serve the response from a dedicated buffer allocated at bind time:
the handler now copies the entry data into get_req_buf before
queueing the request, so userspace updates to the entry can no
longer race with the asynchronous read of the previous response.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index d7ba30323414..1810e3163518 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -94,6 +94,7 @@ struct f_hidg {
 
 	/* get report */
 	struct usb_request		*get_req;
+	void				*get_req_buf;
 	struct usb_hidg_report		get_report;
 	bool				get_report_returned;
 	int				get_report_req_report_id;
@@ -587,7 +588,8 @@ static void get_report_workqueue_handler(struct work_struct *work)
 	ptr = f_hidg_search_for_report(hidg, hidg->get_report_req_report_id);
 	if (ptr && !ptr->report_data.userspace_req) {
 		/* Report exists in list and it is to be used for immediate response */
-		req->buf = ptr->report_data.data;
+		memcpy(req->buf, ptr->report_data.data,
+		       min(req->length, (unsigned int)ptr->report_data.length));
 		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
 		hidg->get_report_returned = true;
 		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
@@ -622,10 +624,10 @@ static void get_report_workqueue_handler(struct work_struct *work)
 			 * Either get an updated response just serviced by userspace
 			 * or send the latest response in the list
 			 */
-			req->buf = ptr->report_data.data;
+			memcpy(req->buf, ptr->report_data.data,
+			       min(req->length, (unsigned int)ptr->report_data.length));
 		} else {
 			/* If there are no prevoiusly sent reports send empty report */
-			req->buf = hidg->get_report.data;
 			memset(req->buf, 0x0, req->length);
 		}
 
@@ -1007,6 +1009,8 @@ static void hidg_disable(struct usb_function *f)
 		hidg->get_req = NULL;
 		hidg->get_report_returned = true;
 	}
+	kfree(hidg->get_req_buf);
+	hidg->get_req_buf = NULL;
 	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 
 	spin_lock_irqsave(&hidg->read_spinlock, flags);
@@ -1160,6 +1164,19 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (!hidg->get_req)
 		return -ENOMEM;
 
+	/*
+	 * The GET_REPORT response is served from a dedicated buffer, not
+	 * from the report_list entries: userspace may update an entry
+	 * (f_hidg_get_report()) while the controller is still reading the
+	 * previous response, which would corrupt the data on the wire.
+	 */
+	hidg->get_req_buf = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
+	if (!hidg->get_req_buf) {
+		usb_ep_free_request(c->cdev->gadget->ep0, hidg->get_req);
+		return -ENOMEM;
+	}
+	hidg->get_req->buf = hidg->get_req_buf;
+
 	hidg->get_req->zero = 0;
 	hidg->get_req->complete = hidg_get_report_complete;
 	hidg->get_req->context = hidg;
-- 
2.53.0

Re: [PATCH 3/3] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] usb: gadget: f_hid: use-after-free of get_req_buf after
  hidg_disable()
- [Medium] usb: gadget: f_hid: leak of get_req_buf in hidg_bind() and
  hidg_unbind()
- [Medium] usb: gadget: f_hid: uninitialized memory leak in
  get_report_workqueue_handler()
- [Low] usb: gadget: f_hid: unused get_report field in struct f_hidg
--

commit d402239570ba7e6af5cac6cc1d8f6985f6ff4af7
Author: Shunbo Shi <shishunbo@cmss.chinamobile.com>

usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

This commit allocates a dedicated get_req_buf buffer at bind time and
copies report_list data into it in get_report_workqueue_handler() before
queueing GET_REPORT responses on ep0.

> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index d7ba30323414c..1810e31635187 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -94,6 +94,7 @@ struct f_hidg {
>  
>  	/* get report */
>  	struct usb_request		*get_req;
> +	void				*get_req_buf;
>  	struct usb_hidg_report		get_report;
>  	bool				get_report_returned;
>  	int				get_report_req_report_id;

[Severity: Low]
Since get_report_workqueue_handler() no longer points req->buf at
hidg->get_report.data, can the unused get_report member be removed from
struct f_hidg?

> @@ -587,7 +588,8 @@ static void get_report_workqueue_handler(struct work_struct *work)
>  	ptr = f_hidg_search_for_report(hidg, hidg->get_report_req_report_id);
>  	if (ptr && !ptr->report_data.userspace_req) {
>  		/* Report exists in list and it is to be used for immediate response */
> -		req->buf = ptr->report_data.data;
> +		memcpy(req->buf, ptr->report_data.data,
> +		       min(req->length, (unsigned int)ptr->report_data.length));
>  		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
>  		hidg->get_report_returned = true;
>  		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
> @@ -622,10 +624,10 @@ static void get_report_workqueue_handler(struct work_struct *work)
>  			 * Either get an updated response just serviced by userspace
>  			 * or send the latest response in the list
>  			 */
> -			req->buf = ptr->report_data.data;
> +			memcpy(req->buf, ptr->report_data.data,
> +			       min(req->length, (unsigned int)ptr->report_data.length));
>  		} else {
>  			/* If there are no prevoiusly sent reports send empty report */
> -			req->buf = hidg->get_report.data;
>  			memset(req->buf, 0x0, req->length);
>  		}
>  

[Severity: Medium]
Can this send uninitialized slab memory or stale report bytes to the USB
host when ptr->report_data.length is smaller than req->length?

hidg->get_req_buf is allocated with kmalloc(), and req->length is set to
min(hidg->get_report_req_report_length, hidg->report_length,
MAX_REPORT_LENGTH).  Both paths copy only min(req->length,
ptr->report_data.length) bytes into req->buf without clamping req->length
or zeroing the remainder before usb_ep_queue() transmits req->length
bytes.

> @@ -1007,6 +1009,8 @@ static void hidg_disable(struct usb_function *f)
>  		hidg->get_req = NULL;
>  		hidg->get_report_returned = true;
>  	}
> +	kfree(hidg->get_req_buf);
> +	hidg->get_req_buf = NULL;
>  	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
>  
>  	spin_lock_irqsave(&hidg->read_spinlock, flags);

[Severity: High]
Does freeing hidg->get_req_buf here cause a use-after-free on subsequent
GET_REPORT requests?

hidg->get_req and hidg->get_req_buf are allocated only in hidg_bind(),
where hidg->get_req->buf is set to hidg->get_req_buf.  When hidg_disable()
runs on a USB reset or disconnect while hidg->get_report_returned is true,
hidg->get_req is kept alive, but hidg->get_req_buf is unconditionally
freed and never reallocated in hidg_set_alt().  The next GET_REPORT
request after re-enumeration then runs get_report_workqueue_handler(),
which writes into the dangling req->buf via memcpy() or memset() and
queues it to ep0.  Also, because get_report_workqueue_handler() sets
hidg->get_report_returned = true as soon as usb_ep_queue() is called,
hidg_disable() can free hidg->get_req_buf while the controller is still
reading from req->buf.

> @@ -1160,6 +1164,19 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  	if (!hidg->get_req)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The GET_REPORT response is served from a dedicated buffer, not
> +	 * from the report_list entries: userspace may update an entry
> +	 * (f_hidg_get_report()) while the controller is still reading the
> +	 * previous response, which would corrupt the data on the wire.
> +	 */
> +	hidg->get_req_buf = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
> +	if (!hidg->get_req_buf) {
> +		usb_ep_free_request(c->cdev->gadget->ep0, hidg->get_req);
> +		return -ENOMEM;
> +	}
> +	hidg->get_req->buf = hidg->get_req_buf;
> +
>  	hidg->get_req->zero = 0;
>  	hidg->get_req->complete = hidg_get_report_complete;
>  	hidg->get_req->context = hidg;

[Severity: Medium]
Does this leak hidg->get_req_buf on hidg_bind() failure or when
hidg_unbind() runs without a prior hidg_disable()?

In hidg_bind(), the early error return when usb_gstrings_attach() fails
and the fail label both omit kfree(hidg->get_req_buf), and
usb_ep_free_request() only frees the usb_request itself.  Likewise,
hidg_unbind() does not free hidg->get_req_buf, so unbinding a function
that was never configured and disabled by a host leaks the buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111021655.37851-1-shishunbo@cmss.chinamobile.com?part=3

[PATCH v2 0/4] usb: gadget: f_hid: three GET_REPORT fixes

[Shunbo Shi]

Cover-letter:

Hi Greg,

v2 of this series.  Changes from v1, driven by the Sashiko AI
review and further self-review:

  - the dedicated response buffer introduced in patch 3 is now
    freed in hidg_unbind() and on the bind() error paths instead
    of hidg_disable(): hidg_set_alt() never reallocates it, so the
    v1 placement caused a use-after-free after a USB reset;
  - the tail of the response buffer is zeroed so no uninitialized
    slab memory is sent to the host;
  - the now unused get_report member is removed from struct f_hidg;
  - a pre-existing leak of get_req when usb_gstrings_attach()
    fails, found while auditing the bind() error paths, is split
    out into its own patch (4).

Patch 1 and 2 are unchanged from v1.

While reviewing the f_hid GET_REPORT userspace ioctl feature
(a139c98f760e, v6.11) I found these bugs in it:

  1. sleep-in-atomic-context: the second copy_from_user() in
     f_hidg_get_report() runs under spin_lock_irqsave(); a page
     fault on the user buffer deadlocks the scheduler.

  2. memory leak: report_list entries allocated by the ioctl are
     never freed in hidg_release().

  3. data race: the GET_REPORT response is served by pointing
     get_req->buf directly at a report_list entry's data; the
     request completes asynchronously after the spinlock is
     dropped, so a concurrent ioctl updating the same entry
     corrupts the data on the wire (a torn write).

  4. memory leak: get_req is leaked when usb_gstrings_attach()
     fails in hidg_bind().

All four were introduced by the same commit and are fixable
deterministically from the code.

Shunbo Shi (4):
  usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()
  usb: gadget: f_hid: fix report_list entries leak
  usb: gadget: f_hid: fix race between userspace report update and
  GET_REPORT response
  usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

 drivers/usb/gadget/function/f_hid.c | 61 +++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 13 deletions(-)

--
2.53.0

[PATCH v2 1/4] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[Shunbo Shi]

The second copy_from_user() in f_hidg_get_report() is called while
holding hidg->get_report_spinlock with interrupts disabled.  If the
user page backing the ioctl buffer is not resident, copy_from_user()
takes a page fault and sleeps, which is not allowed with a spinlock
held: it triggers might_sleep() warnings with
CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
CPUs spinning on the lock.

The first copy_from_user() into the freshly allocated entry is
already done before the spinlock is taken; only the "report already
exists, update it" branch still copies inside the critical section.

Since the data has already been fully copied into entry->report_data
before taking the lock, updating an existing report only needs a
plain assignment between two kernel-memory objects, which is safe
inside the spinlock.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..b778eb633b79 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -654,6 +654,11 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 	if (!entry)
 		return -ENOMEM;
 
+	/*
+	 * Copy the report from userspace into the entry before taking the
+	 * spinlock: copy_from_user() may sleep, so it must not be called
+	 * with the spinlock held (and interrupts disabled).
+	 */
 	if (copy_from_user(&entry->report_data, buffer,
 				sizeof(struct usb_hidg_report))) {
 		ERROR(cdev, "copy_from_user error\n");
@@ -668,13 +673,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 */
-- 
2.53.0

Re: [PATCH v2 1/4] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[sashiko-bot]

> The second copy_from_user() in f_hidg_get_report() is called while
> holding hidg->get_report_spinlock with interrupts disabled.  If the
> user page backing the ioctl buffer is not resident, copy_from_user()
> takes a page fault and sleeps, which is not allowed with a spinlock
> held: it triggers might_sleep() warnings with
> CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
> CPUs spinning on the lock.
> 
> The first copy_from_user() into the freshly allocated entry is
> already done before the spinlock is taken; only the "report already
> exists, update it" branch still copies inside the critical section.
> 
> Since the data has already been fully copied into entry->report_data
> before taking the lock, updating an existing report only needs a
> plain assignment between two kernel-memory objects, which is safe
> inside the spinlock.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111081313.16626-1-shishunbo@cmss.chinamobile.com?part=1

[PATCH v2 2/4] usb: gadget: f_hid: fix report_list entries leak

[Shunbo Shi]

Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
yet in hidg->report_list allocates a report_entry and adds it to
the list.  hidg_release() frees report_desc and set_report_buf, but
never walks report_list, so every accumulated entry leaks on
function destroy (e.g. when the HID gadget is unbound and the
device is removed).

Free the entries in hidg_release().  This is safe without extra
locking: at release time the function is unbound (the workqueue is
already destroyed and userspace has no way to reach the ioctl).

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index b778eb633b79..d7ba30323414 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -121,6 +121,12 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 static void hidg_release(struct device *dev)
 {
 	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+	struct report_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
 
 	kfree(hidg->report_desc);
 	kfree(hidg->set_report_buf);
-- 
2.53.0

Re: [PATCH v2 2/4] usb: gadget: f_hid: fix report_list entries leak

[sashiko-bot]

> Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
> yet in hidg->report_list allocates a report_entry and adds it to
> the list.  hidg_release() frees report_desc and set_report_buf, but
> never walks report_list, so every accumulated entry leaks on
> function destroy (e.g. when the HID gadget is unbound and the
> device is removed).
> 
> Free the entries in hidg_release().  This is safe without extra
> locking: at release time the function is unbound (the workqueue is
> already destroyed and userspace has no way to reach the ioctl).
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111081313.16626-1-shishunbo@cmss.chinamobile.com?part=2

[PATCH v2 3/4] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[Shunbo Shi]

The GET_REPORT response is served by pointing get_req->buf at the
data[] array of a report_list entry and calling usb_ep_queue().
The request completes asynchronously, so the controller may still
be reading the entry's buffer after get_report_workqueue_handler()
drops get_report_spinlock.

At the same time, userspace can update that same entry via the
GADGET_HID_WRITE_GET_REPORT ioctl: f_hidg_get_report() copies new
report data into the entry while holding the spinlock, but the
transfer reading the entry is not serialized against that update
(the lock was already released by the work handler).

The result is that the host can receive a GET_REPORT response that
mixes bytes of the old and the new report (a torn write), with no
memory-safety violation but corrupted report data on the wire.

Serve the response from a dedicated buffer allocated at bind time:
the handler now copies the entry data into get_req_buf and zeroes
the rest of the buffer before queueing the request, so userspace
updates to the entry can no longer race with the asynchronous read
of the previous response, and no uninitialized slab memory is
exposed to the host.

The buffer lives from bind() to unbind(); hidg_disable() must not
free it, because hidg_set_alt() does not reallocate it and the next
GET_REPORT after a USB reset would use freed memory.

This also removes the now unused get_report member from struct
f_hidg.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 36 +++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index d7ba30323414..426bc1743cbb 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -94,7 +94,7 @@ struct f_hidg {
 
 	/* get report */
 	struct usb_request		*get_req;
-	struct usb_hidg_report		get_report;
+	void				*get_req_buf;
 	bool				get_report_returned;
 	int				get_report_req_report_id;
 	int				get_report_req_report_length;
@@ -568,6 +568,7 @@ static void get_report_workqueue_handler(struct work_struct *work)
 	struct usb_request		*req;
 	struct report_entry *ptr;
 	unsigned long	flags;
+	unsigned int copied;
 
 	int status = 0;
 
@@ -587,7 +588,10 @@ static void get_report_workqueue_handler(struct work_struct *work)
 	ptr = f_hidg_search_for_report(hidg, hidg->get_report_req_report_id);
 	if (ptr && !ptr->report_data.userspace_req) {
 		/* Report exists in list and it is to be used for immediate response */
-		req->buf = ptr->report_data.data;
+		copied = min_t(unsigned int, req->length, ptr->report_data.length);
+		memcpy(req->buf, ptr->report_data.data, copied);
+		if (copied < MAX_REPORT_LENGTH)
+			memset(req->buf + copied, 0x0, MAX_REPORT_LENGTH - copied);
 		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
 		hidg->get_report_returned = true;
 		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
@@ -622,10 +626,12 @@ static void get_report_workqueue_handler(struct work_struct *work)
 			 * Either get an updated response just serviced by userspace
 			 * or send the latest response in the list
 			 */
-			req->buf = ptr->report_data.data;
+			copied = min_t(unsigned int, req->length, ptr->report_data.length);
+			memcpy(req->buf, ptr->report_data.data, copied);
+			if (copied < MAX_REPORT_LENGTH)
+				memset(req->buf + copied, 0x0, MAX_REPORT_LENGTH - copied);
 		} else {
 			/* If there are no prevoiusly sent reports send empty report */
-			req->buf = hidg->get_report.data;
 			memset(req->buf, 0x0, req->length);
 		}
 
@@ -1160,6 +1166,23 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (!hidg->get_req)
 		return -ENOMEM;
 
+	/*
+	 * The GET_REPORT response is served from a dedicated buffer, not
+	 * from the report_list entries: userspace may update an entry
+	 * (f_hidg_get_report()) while the controller is still reading the
+	 * previous response, which would corrupt the data on the wire.
+	 *
+	 * The buffer lives from bind() to unbind(); hidg_disable() must
+	 * not free it, because hidg_set_alt() does not reallocate it and
+	 * the next GET_REPORT after a USB reset would use freed memory.
+	 */
+	hidg->get_req_buf = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
+	if (!hidg->get_req_buf) {
+		status = -ENOMEM;
+		goto fail_free_request;
+	}
+	hidg->get_req->buf = hidg->get_req_buf;
+
 	hidg->get_req->zero = 0;
 	hidg->get_req->complete = hidg_get_report_complete;
 	hidg->get_req->context = hidg;
@@ -1303,6 +1326,9 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (hidg->req != NULL)
 		free_ep_req(hidg->in_ep, hidg->req);
 
+	kfree(hidg->get_req_buf);
+	hidg->get_req_buf = NULL;
+fail_free_request:
 	usb_ep_free_request(c->cdev->gadget->ep0, hidg->get_req);
 	hidg->get_req = NULL;
 
@@ -1593,6 +1619,8 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 
 	cdev_device_del(hidg->cdev, &hidg->dev);
 	destroy_workqueue(hidg->workqueue);
+	kfree(hidg->get_req_buf);
+	hidg->get_req_buf = NULL;
 	usb_free_all_descriptors(f);
 }
 
-- 
2.53.0

Re: [PATCH v2 3/4] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: f_hid: use-after-free of get_req_buf in
  hidg_unbind()
--

commit e3eb22ae89791d0f4572b69a78a39ec433f7be8d
Author: Shunbo Shi <shishunbo@cmss.chinamobile.com>

usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

This commit allocates a dedicated buffer (get_req_buf) at bind time and
copies GET_REPORT response data into it before queueing hidg->get_req on
ep0, avoiding a race with userspace report updates.

> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index d7ba30323414c..426bc1743cbb9 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c

[ ... ]

> @@ -1593,6 +1619,8 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
>  
>  	cdev_device_del(hidg->cdev, &hidg->dev);
>  	destroy_workqueue(hidg->workqueue);
> +	kfree(hidg->get_req_buf);
> +	hidg->get_req_buf = NULL;
>  	usb_free_all_descriptors(f);
>  }

[Severity: Medium]
Can freeing hidg->get_req_buf here cause a use-after-free if hidg->get_req
is still queued on cdev->gadget->ep0?

get_report_workqueue_handler() queues hidg->get_req (with req->buf set to
hidg->get_req_buf) asynchronously on ep0 and sets hidg->get_report_returned
to true before the transfer completes.  During gadget unbind,
hidg_disable() skips hidg->get_req when hidg->get_report_returned is true,
and destroy_workqueue(hidg->workqueue) in hidg_unbind() can also drain a
pending work item that queues hidg->get_req onto ep0 right before
kfree(hidg->get_req_buf) runs.  Because hidg_unbind() neither dequeues nor
frees hidg->get_req, the UDC can access freed slab memory via req->buf and
hidg->get_req is leaked with a dangling buffer pointer.

Should hidg_unbind() dequeue and free hidg->get_req on ep0 before freeing
hidg->get_req_buf?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111081313.16626-1-shishunbo@cmss.chinamobile.com?part=3

[PATCH v2 4/4] usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

[Shunbo Shi]

In hidg_bind(), hidg->get_req is allocated early; if
usb_gstrings_attach() subsequently fails, the function returns
directly without freeing the request, leaking it.  Unwind through
the fail path instead.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 426bc1743cbb..861b43071daa 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -1191,8 +1191,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,
 				 ARRAY_SIZE(ct_func_string_defs));
-	if (IS_ERR(us))
-		return PTR_ERR(us);
+	if (IS_ERR(us)) {
+		status = PTR_ERR(us);
+		goto fail;
+	}
 	hidg_interface_desc.iInterface = us[CT_FUNC_HID_IDX].id;
 
 	/* allocate instance-specific interface IDs, and patch descriptors */
-- 
2.53.0

Re: [PATCH v2 4/4] usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

[sashiko-bot]

> In hidg_bind(), hidg->get_req is allocated early; if
> usb_gstrings_attach() subsequently fails, the function returns
> directly without freeing the request, leaking it.  Unwind through
> the fail path instead.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111081313.16626-1-shishunbo@cmss.chinamobile.com?part=4

[PATCH v3 0/4] usb: gadget: f_hid: three GET_REPORT fixes

[Shunbo Shi]

Cover-letter:

Hi Greg,

v3 of this series.  The only change from v2 is in patch 3: the
dedicated GET_REPORT response buffer is now allocated in
hidg_alloc() and freed in hidg_release() instead of being managed
across bind/unbind.  This eliminates the use-after-free window on
unbind that the Sashiko AI review found in v2 (the buffer can
no longer be freed while a request is still queued on ep0), and
it also removes the need for buffer cleanup on the bind() error
paths.  Patches 1, 2 and 4 are unchanged.

Original description:

While reviewing the f_hid GET_REPORT userspace ioctl feature
(a139c98f760e, v6.11) I found these bugs in it:

  1. sleep-in-atomic-context: the second copy_from_user() in
     f_hidg_get_report() runs under spin_lock_irqsave(); a page
     fault on the user buffer deadlocks the scheduler.

  2. memory leak: report_list entries allocated by the ioctl are
     never freed in hidg_release().

  3. data race: the GET_REPORT response is served by pointing
     get_req->buf directly at a report_list entry's data; the
     request completes asynchronously after the spinlock is
     dropped, so a concurrent ioctl updating the same entry
     corrupts the data on the wire (a torn write).

  4. memory leak: get_req is leaked when usb_gstrings_attach()
     fails in hidg_bind().

All four were introduced by the same commit and are fixable
deterministically from the code.

Shunbo Shi (4):
  usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()
  usb: gadget: f_hid: fix report_list entries leak
  usb: gadget: f_hid: fix race between userspace report update and
  GET_REPORT response
  usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

 drivers/usb/gadget/function/f_hid.c | 61 +++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 13 deletions(-)

--
2.53.0

[PATCH v3 1/4] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[Shunbo Shi]

The second copy_from_user() in f_hidg_get_report() is called while
holding hidg->get_report_spinlock with interrupts disabled.  If the
user page backing the ioctl buffer is not resident, copy_from_user()
takes a page fault and sleeps, which is not allowed with a spinlock
held: it triggers might_sleep() warnings with
CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
CPUs spinning on the lock.

The first copy_from_user() into the freshly allocated entry is
already done before the spinlock is taken; only the "report already
exists, update it" branch still copies inside the critical section.

Since the data has already been fully copied into entry->report_data
before taking the lock, updating an existing report only needs a
plain assignment between two kernel-memory objects, which is safe
inside the spinlock.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..b778eb633b79 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -654,6 +654,11 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 	if (!entry)
 		return -ENOMEM;
 
+	/*
+	 * Copy the report from userspace into the entry before taking the
+	 * spinlock: copy_from_user() may sleep, so it must not be called
+	 * with the spinlock held (and interrupts disabled).
+	 */
 	if (copy_from_user(&entry->report_data, buffer,
 				sizeof(struct usb_hidg_report))) {
 		ERROR(cdev, "copy_from_user error\n");
@@ -668,13 +673,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 */
-- 
2.53.0

Re: [PATCH v3 1/4] usb: gadget: f_hid: fix sleep-in-atomic-context in f_hidg_get_report()

[sashiko-bot]

> The second copy_from_user() in f_hidg_get_report() is called while
> holding hidg->get_report_spinlock with interrupts disabled.  If the
> user page backing the ioctl buffer is not resident, copy_from_user()
> takes a page fault and sleeps, which is not allowed with a spinlock
> held: it triggers might_sleep() warnings with
> CONFIG_DEBUG_ATOMIC_SLEEP and can deadlock the scheduler on other
> CPUs spinning on the lock.
> 
> The first copy_from_user() into the freshly allocated entry is
> already done before the spinlock is taken; only the "report already
> exists, update it" branch still copies inside the critical section.
> 
> Since the data has already been fully copied into entry->report_data
> before taking the lock, updating an existing report only needs a
> plain assignment between two kernel-memory objects, which is safe
> inside the spinlock.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112010928.23716-1-shishunbo@cmss.chinamobile.com?part=1

[PATCH v3 2/4] usb: gadget: f_hid: fix report_list entries leak

[Shunbo Shi]

Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
yet in hidg->report_list allocates a report_entry and adds it to
the list.  hidg_release() frees report_desc and set_report_buf, but
never walks report_list, so every accumulated entry leaks on
function destroy (e.g. when the HID gadget is unbound and the
device is removed).

Free the entries in hidg_release().  This is safe without extra
locking: at release time the function is unbound (the workqueue is
already destroyed and userspace has no way to reach the ioctl).

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index b778eb633b79..d7ba30323414 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -121,6 +121,12 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 static void hidg_release(struct device *dev)
 {
 	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+	struct report_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
 
 	kfree(hidg->report_desc);
 	kfree(hidg->set_report_buf);
-- 
2.53.0

Re: [PATCH v3 2/4] usb: gadget: f_hid: fix report_list entries leak

[sashiko-bot]

> Each GADGET_HID_WRITE_GET_REPORT ioctl for a report ID that is not
> yet in hidg->report_list allocates a report_entry and adds it to
> the list.  hidg_release() frees report_desc and set_report_buf, but
> never walks report_list, so every accumulated entry leaks on
> function destroy (e.g. when the HID gadget is unbound and the
> device is removed).
> 
> Free the entries in hidg_release().  This is safe without extra
> locking: at release time the function is unbound (the workqueue is
> already destroyed and userspace has no way to reach the ioctl).
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112010928.23716-1-shishunbo@cmss.chinamobile.com?part=2

[PATCH v3 3/4] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[Shunbo Shi]

The GET_REPORT response is served by pointing get_req->buf at the
data[] array of a report_list entry and calling usb_ep_queue().
The request completes asynchronously, so the controller may still
be reading the entry's buffer after get_report_workqueue_handler()
drops get_report_spinlock.

At the same time, userspace can update that same entry via the
GADGET_HID_WRITE_GET_REPORT ioctl: f_hidg_get_report() copies new
report data into the entry while holding the spinlock, but the
transfer reading the entry is not serialized against that update
(the lock was already released by the work handler).

The result is that the host can receive a GET_REPORT response that
mixes bytes of the old and the new report (a torn write), with no
memory-safety violation but corrupted report data on the wire.

Serve the response from a dedicated buffer allocated once in
hidg_alloc() and freed in hidg_release(): the handler copies the
entry data into get_req_buf and zeroes the rest before queueing,
so userspace updates to the entry can no longer race with the
asynchronous read of the previous response, and no uninitialized
slab memory is exposed to the host.  Placing the buffer at the
function-instance level (rather than bind/unbind) means it is only
freed after all USB activity has ceased and all userspace file
references are gone, so no disable/set_alt/unbind ordering
constraints apply.

This also removes the now unused get_report member from struct
f_hidg.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 36 +++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index d7ba30323414..8c2adf392389 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -94,7 +94,7 @@ struct f_hidg {
 
 	/* get report */
 	struct usb_request		*get_req;
-	struct usb_hidg_report		get_report;
+	void				*get_req_buf;
 	bool				get_report_returned;
 	int				get_report_req_report_id;
 	int				get_report_req_report_length;
@@ -128,6 +128,7 @@ static void hidg_release(struct device *dev)
 		kfree(entry);
 	}
 
+	kfree(hidg->get_req_buf);
 	kfree(hidg->report_desc);
 	kfree(hidg->set_report_buf);
 	kfree(hidg);
@@ -568,6 +569,7 @@ static void get_report_workqueue_handler(struct work_struct *work)
 	struct usb_request		*req;
 	struct report_entry *ptr;
 	unsigned long	flags;
+	unsigned int copied;
 
 	int status = 0;
 
@@ -587,7 +589,10 @@ static void get_report_workqueue_handler(struct work_struct *work)
 	ptr = f_hidg_search_for_report(hidg, hidg->get_report_req_report_id);
 	if (ptr && !ptr->report_data.userspace_req) {
 		/* Report exists in list and it is to be used for immediate response */
-		req->buf = ptr->report_data.data;
+		copied = min_t(unsigned int, req->length, ptr->report_data.length);
+		memcpy(req->buf, ptr->report_data.data, copied);
+		if (copied < MAX_REPORT_LENGTH)
+			memset(req->buf + copied, 0x0, MAX_REPORT_LENGTH - copied);
 		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
 		hidg->get_report_returned = true;
 		spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
@@ -622,10 +627,12 @@ static void get_report_workqueue_handler(struct work_struct *work)
 			 * Either get an updated response just serviced by userspace
 			 * or send the latest response in the list
 			 */
-			req->buf = ptr->report_data.data;
+			copied = min_t(unsigned int, req->length, ptr->report_data.length);
+			memcpy(req->buf, ptr->report_data.data, copied);
+			if (copied < MAX_REPORT_LENGTH)
+				memset(req->buf + copied, 0x0, MAX_REPORT_LENGTH - copied);
 		} else {
 			/* If there are no prevoiusly sent reports send empty report */
-			req->buf = hidg->get_report.data;
 			memset(req->buf, 0x0, req->length);
 		}
 
@@ -1160,6 +1167,15 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (!hidg->get_req)
 		return -ENOMEM;
 
+	/*
+	 * The GET_REPORT response is served from a dedicated buffer
+	 * (allocated in hidg_alloc()), not from the report_list
+	 * entries: userspace may update an entry while the controller
+	 * is still reading the previous response, which would corrupt
+	 * the data on the wire.
+	 */
+	hidg->get_req->buf = hidg->get_req_buf;
+
 	hidg->get_req->zero = 0;
 	hidg->get_req->complete = hidg_get_report_complete;
 	hidg->get_req->context = hidg;
@@ -1646,6 +1662,18 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	}
 	hidg->use_out_ep = !opts->no_out_endpoint;
 
+	/*
+	 * Dedicated buffer for GET_REPORT responses, allocated once for
+	 * the whole lifetime of this function instance: hidg_release()
+	 * is the only safe free point, as it runs after all USB
+	 * activity has ceased and all file references are gone.
+	 */
+	hidg->get_req_buf = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
+	if (!hidg->get_req_buf) {
+		ret = -ENOMEM;
+		goto err_put_device;
+	}
+
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
-- 
2.53.0

Re: [PATCH v3 3/4] usb: gadget: f_hid: fix race between userspace report update and GET_REPORT response

[sashiko-bot]

> The GET_REPORT response is served by pointing get_req->buf at the
> data[] array of a report_list entry and calling usb_ep_queue().
> The request completes asynchronously, so the controller may still
> be reading the entry's buffer after get_report_workqueue_handler()
> drops get_report_spinlock.
> 
> At the same time, userspace can update that same entry via the
> GADGET_HID_WRITE_GET_REPORT ioctl: f_hidg_get_report() copies new
> report data into the entry while holding the spinlock, but the
> transfer reading the entry is not serialized against that update
> (the lock was already released by the work handler).
> 
> The result is that the host can receive a GET_REPORT response that
> mixes bytes of the old and the new report (a torn write), with no
> memory-safety violation but corrupted report data on the wire.
> [ ... ]
> This also removes the now unused get_report member from struct
> f_hidg.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112010928.23716-1-shishunbo@cmss.chinamobile.com?part=3

[PATCH v3 4/4] usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

[Shunbo Shi]

In hidg_bind(), hidg->get_req is allocated early; if
usb_gstrings_attach() subsequently fails, the function returns
directly without freeing the request, leaking it.  Unwind through
the fail path instead.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>
---
 drivers/usb/gadget/function/f_hid.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 8c2adf392389..af68684fa720 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -1184,8 +1184,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,
 				 ARRAY_SIZE(ct_func_string_defs));
-	if (IS_ERR(us))
-		return PTR_ERR(us);
+	if (IS_ERR(us)) {
+		status = PTR_ERR(us);
+		goto fail;
+	}
 	hidg_interface_desc.iInterface = us[CT_FUNC_HID_IDX].id;
 
 	/* allocate instance-specific interface IDs, and patch descriptors */
-- 
2.53.0

Re: [PATCH v3 4/4] usb: gadget: f_hid: fix get_req leak when usb_gstrings_attach() fails

[sashiko-bot]

> In hidg_bind(), hidg->get_req is allocated early; if
> usb_gstrings_attach() subsequently fails, the function returns
> directly without freeing the request, leaking it.  Unwind through
> the fail path instead.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Signed-off-by: Shunbo Shi <shishunbo@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112010928.23716-1-shishunbo@cmss.chinamobile.com?part=4