1. Patchew
  2. linux
  3. View series

[PATCH] usb: gadget: f_hid: do not copy_from_user() under a spinlock

Linkai Gong posted 1 patch 1 month, 1 week ago 
drivers/usb/gadget/function/f_hid.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
[PATCH] usb: gadget: f_hid: do not copy_from_user() under a spinlock
Posted by Linkai Gong 1 month, 1 week ago 
f_hidg_get_report() already copied the report from userspace into a
new entry, then called copy_from_user() again under
get_report_spinlock. That can fault and sleep in atomic context.

Update the existing entry with memcpy() from the copy already taken.

Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
---
 drivers/usb/gadget/function/f_hid.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..e4621e5a0b69 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -667,14 +667,9 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 	ptr = f_hidg_search_for_report(hidg, report_id);
 
 	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;
-		}
+		/* Report already exists; data was copied before taking the lock. */
+		memcpy(&ptr->report_data, &entry->report_data,
+		       sizeof(struct usb_hidg_report));
 		kfree(entry);
 	} else {
 		/* Report does not exist in list - add it */
-- 
2.25.1
Re: [PATCH] usb: gadget: f_hid: do not copy_from_user() under a spinlock
Posted by David Laight 1 month, 1 week ago 
On Mon, 17 Aug 2026 14:11:41 +0800
Linkai Gong <gonglinkai@kylinos.cn> wrote:

> f_hidg_get_report() already copied the report from userspace into a
> new entry, then called copy_from_user() again under
> get_report_spinlock. That can fault and sleep in atomic context.
> 
> Update the existing entry with memcpy() from the copy already taken.
> 
> Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
> ---
>  drivers/usb/gadget/function/f_hid.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 3c6b43d06a6d..e4621e5a0b69 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -667,14 +667,9 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
>  	ptr = f_hidg_search_for_report(hidg, report_id);
>  
>  	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");

How about a patch to remove the ERROR() from the earlier copy_from_user()
error path?

	David

> -			kfree(entry);
> -			return -EINVAL;
> -		}
> +		/* Report already exists; data was copied before taking the lock. */
> +		memcpy(&ptr->report_data, &entry->report_data,
> +		       sizeof(struct usb_hidg_report));
>  		kfree(entry);
>  	} else {
>  		/* Report does not exist in list - add it */
Re: [PATCH] usb: gadget: f_hid: do not copy_from_user() under a spinlock
Posted by Linkai Gong 1 month, 1 week ago 
On Mon, 17 Aug 2026 15:46:04 +0100, David Laight wrote:
> How about a patch to remove the ERROR() from the earlier copy_from_user()
> error path?

Agreed. A failed copy_from_user() is a userspace error and should not
spam the kernel log. I'll send a follow-up that drops that ERROR().

Thanks,
Linkai
[PATCH] usb: gadget: f_hid: drop ERROR() on copy_from_user() failure
Posted by Linkai Gong 1 month, 1 week ago 
A failed copy_from_user() is a userspace error and should not spam the
kernel log. Just free the temporary entry and return.

Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
---
 drivers/usb/gadget/function/f_hid.c | 1 -
 1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..5c39da1ac7a6 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -656,7 +656,6 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 
 	if (copy_from_user(&entry->report_data, buffer,
 				sizeof(struct usb_hidg_report))) {
-		ERROR(cdev, "copy_from_user error\n");
 		kfree(entry);
 		return -EINVAL;
 	}
-- 
2.25.1
Re: [PATCH] usb: gadget: f_hid: drop ERROR() on copy_from_user() failure
Posted by David Laight 1 month, 1 week ago 
On Tue, 18 Aug 2026 09:21:18 +0800
Linkai Gong <gonglinkai@kylinos.cn> wrote:

> A failed copy_from_user() is a userspace error and should not spam the
> kernel log. Just free the temporary entry and return.
> 
> Suggested-by: David Laight <david.laight.linux@gmail.com>
> Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
> ---
>  drivers/usb/gadget/function/f_hid.c | 1 -
>  1 file changed, 1 deletion(-)
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 3c6b43d06a6d..5c39da1ac7a6 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -656,7 +656,6 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
>  
>  	if (copy_from_user(&entry->report_data, buffer,
>  				sizeof(struct usb_hidg_report))) {
> -		ERROR(cdev, "copy_from_user error\n");
>  		kfree(entry);
>  		return -EINVAL;

This should be -EFAULT.

David

>  	}
Re: [PATCH] usb: gadget: f_hid: drop ERROR() on copy_from_user() failure
Posted by Peter Korsgaard 1 month, 1 week ago 
>>>>> "Linkai" == Linkai Gong <gonglinkai@kylinos.cn> writes:

 > A failed copy_from_user() is a userspace error and should not spam the
 > kernel log. Just free the temporary entry and return.

 > Suggested-by: David Laight <david.laight.linux@gmail.com>
 > Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>

Acked-by: Peter Korsgaard <peter@korsgaard.com>

> ---
 >  drivers/usb/gadget/function/f_hid.c | 1 -
 >  1 file changed, 1 deletion(-)

 > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
 > index 3c6b43d06a6d..5c39da1ac7a6 100644
 > --- a/drivers/usb/gadget/function/f_hid.c
 > +++ b/drivers/usb/gadget/function/f_hid.c
 > @@ -656,7 +656,6 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 
 >  	if (copy_from_user(&entry->report_data, buffer,
 >  				sizeof(struct usb_hidg_report))) {
 > -		ERROR(cdev, "copy_from_user error\n");
 >  		kfree(entry);
 >  		return -EINVAL;
 >  	}
 > -- 

 > 2.25.1


-- 
Bye, Peter Korsgaard
[PATCH v2] usb: gadget: f_hid: drop ERROR() on copy_from_user() failure
Posted by Linkai Gong 1 month, 1 week ago 
A failed copy_from_user() is a userspace fault, not a kernel error.
Do not spam the log, and return -EFAULT instead of -EINVAL.

Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
---
v2:
- Return -EFAULT instead of -EINVAL (David Laight)

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

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3c6b43d06a6d..b8e1c0e0f1a2 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -656,9 +656,8 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 
 	if (copy_from_user(&entry->report_data, buffer,
 				sizeof(struct usb_hidg_report))) {
-		ERROR(cdev, "copy_from_user error\n");
 		kfree(entry);
-		return -EINVAL;
+		return -EFAULT;
 	}
 
 	report_id = entry->report_data.report_id;
-- 
2.25.1
Re: [PATCH] usb: gadget: f_hid: do not copy_from_user() under a spinlock
Posted by Peter Korsgaard 1 month, 1 week ago 
>>>>> "Linkai" == Linkai Gong <gonglinkai@kylinos.cn> writes:

 > f_hidg_get_report() already copied the report from userspace into a
 > new entry, then called copy_from_user() again under
 > get_report_spinlock. That can fault and sleep in atomic context.

 > Update the existing entry with memcpy() from the copy already taken.

 > Fixes: a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL")
 > Cc: stable@vger.kernel.org
 > Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>

Acked-by: Peter Korsgaard <peter@korsgaard.com>

> ---
 >  drivers/usb/gadget/function/f_hid.c | 11 +++--------
 >  1 file changed, 3 insertions(+), 8 deletions(-)

 > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
 > index 3c6b43d06a6d..e4621e5a0b69 100644
 > --- a/drivers/usb/gadget/function/f_hid.c
 > +++ b/drivers/usb/gadget/function/f_hid.c
 > @@ -667,14 +667,9 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 >  	ptr = f_hidg_search_for_report(hidg, report_id);
 
 >  	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;
 > -		}
 > +		/* Report already exists; data was copied before taking the lock. */
 > +		memcpy(&ptr->report_data, &entry->report_data,
 > +		       sizeof(struct usb_hidg_report));
 >  		kfree(entry);
 >  	} else {
 >  		/* Report does not exist in list - add it */
 > -- 

 > 2.25.1


-- 
Bye, Peter Korsgaard

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.