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

[sashiko-bot@kernel.org]

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