[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@xxxxxxxxxxxxxxxxxxxx>
---
 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