[PATCH] usb: gadget: f_hid: avoid copy_from_user() under get_report_spinlock

[PATCH] usb: gadget: f_hid: avoid copy_from_user() under get_report_spinlock

[Yu Zhang]

f_hidg_get_report() copies the userspace report into a freshly allocated
'entry' before taking get_report_spinlock.  When the report id is already
present in the list it then copies the same userspace buffer a second
time into the existing entry, this time while holding get_report_spinlock
(acquired with spin_lock_irqsave()):

	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, which is not allowed while holding
a spinlock with interrupts disabled:

	BUG: sleeping function called from invalid context at ./include/linux/uaccess.h:169
	in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 3481, name: hidg_getreport
	 __might_fault+0x4d/0x60
	 f_hidg_get_report+0x155/0x310
	 f_hidg_ioctl+0xbd/0xe0
	 __x64_sys_ioctl+0x120/0x170
	 do_syscall_64+0xf9/0x540

This is reached by issuing GADGET_HID_WRITE_GET_REPORT twice for the same
report id (the first call inserts the entry, the second takes the update
path).  On a non-debug kernel a userspace buffer crafted to fault (e.g.
via userfaultfd) would sleep with the spinlock held and interrupts
disabled.

The data has already been copied into 'entry' before the lock is taken,
so reuse it instead of copying from userspace again under the lock.  This
also drops a redundant second read of the user buffer.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/usb/gadget/function/f_hid.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d..520892b 100644
--- 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 *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.43.0

Re: [PATCH] usb: gadget: f_hid: avoid copy_from_user() under get_report_spinlock

[sashiko-bot]

> f_hidg_get_report() copies the userspace report into a freshly allocated
> 'entry' before taking get_report_spinlock.  When the report id is already
> present in the list it then copies the same userspace buffer a second
> time into the existing entry, this time while holding get_report_spinlock
> (acquired with spin_lock_irqsave()):
> 
> 	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, which is not allowed while holding
> a spinlock with interrupts disabled:
> 
> [ ... ]
> also drops a redundant second read of the user buffer.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020051446.92179-1-yuz08559@gmail.com?part=1