[PATCH v2 1/2] usb: gadget: f_hid: don't call copy_from_user() under get_report_spinlock

[Hui Peng]

f_hidg_get_report() already copies the incoming struct usb_hidg_report
from userspace into the freshly allocated 'entry' before acquiring
hidg->get_report_spinlock.

When a report with the same report_id is already present in
hidg->report_list, the update path copies from userspace a second time,
this time directly into ptr->report_data while the spinlock is held with
interrupts disabled:

	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
	ptr = f_hidg_search_for_report(hidg, report_id);
	if (ptr) {
		if (copy_from_user(&ptr->report_data, buffer,
				sizeof(struct usb_hidg_report))) {

copy_from_user() may fault and sleep, so this is a sleeping function
called from atomic context, reported by CONFIG_DEBUG_ATOMIC_SLEEP as
"BUG: sleeping function called from invalid context". Userspace can
reach it at will by issuing GADGET_HID_WRITE_GET_REPORT twice with the
same report_id on /dev/hidgN.

The second copy is also redundant: the same user buffer has already been
copied into entry->report_data a few lines above, and report_id was
derived from that copy. Assign from the already copied data instead,
which removes the fault from the critical section and makes the update
atomic with respect to the list lookup.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
---
v2: Add the Fixes: tag Greg asked for.

    f_hidg_get_report(), the spinlock and the copy_from_user() call under
    it were all added together by a139c98f760e ("USB: gadget: f_hid: Add
    GET_REPORT via userspace IOCTL"), first released in v6.12, so that is
    where this starts. git blame on the removed lines agrees.

Found by inspection while investigating the use-after-free fixed in
patch 2/2, and build tested only; I do not have HID gadget hardware,
but the path is reachable from /dev/hidgN with dummy_hcd.

 drivers/usb/gadget/function/f_hid.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -668,13 +668,7 @@ static int f_hidg_get_report(struct file
 
 	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.43.0