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

[Neill Kapron]

> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Yu Zhang <yuz08559@xxxxxxxxx>
> ---
>  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 3c6b43d06a6d..520892bbfbef 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 */
LGTM, thanks!

Reviewed-by: Neill Kapron <nkapron@xxxxxxxxxx>

-- 
Neill Kapron <nkapron@xxxxxxxxxx>