USB: core: replace kmalloc + copy_from_user with memdup_user

PATCH In review Thorsten Blum 3 participants 3 messages Sep 19, 2025 – Oct 8, 2025 USB

Share & tools

Thorsten Blum

author

2025-09-19 11:57 UTC #0 lore raw

PATCH 1/1 USB: core: replace kmalloc + copy_from_user with memdup_user

Replace kmalloc() followed by copy_from_user() with memdup_user() to simplify and improve proc_do_submiturb(). Replace the hard-coded 8 bytes with the size of 'struct usb_ctrlrequest'.

Return early if an error occurs, and avoid manually setting 'ret' and using 'goto error'.

No functional changes intended.

Signed-off-by Thorsten Blum <[email protected]>

---
 drivers/usb/core/devio.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

drivers/usb/core/devio.c +3 -7

index f6ce6e26e0d4..3bc54a5c59ff 100644
@@ -1670,13 +1670,9 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
 		/* min 8 byte setup packet */
 		if (uurb->buffer_length < 8)
 			return -EINVAL;
-		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
-		if (!dr)
-			return -ENOMEM;
-		if (copy_from_user(dr, uurb->buffer, 8)) {
-			ret = -EFAULT;
-			goto error;
-		}
+		dr = memdup_user(uurb->buffer, sizeof(struct usb_ctrlrequest));
+		if (IS_ERR(dr))
+			return PTR_ERR(dr);
 		if (uurb->buffer_length < (le16_to_cpu(dr->wLength) + 8)) {
 			ret = -EINVAL;
 			goto error;

Signature · 1 line

2.51.0

Greg Kroah-Hartman

maintainer

2025-10-08 11:05 UTC #1 lore raw

Quoted 8 lines · Thorsten Blum

Replace kmalloc() followed by copy_from_user() with memdup_user() to simplify and improve proc_do_submiturb(). Replace the hard-coded 8 bytes with the size of 'struct usb_ctrlrequest'.
Return early if an error occurs, and avoid manually setting 'ret' and using 'goto error'.
No functional changes intended.

Same comments as the other patch that you submitted doing the same thing for a different part of the same function.

thanks,

greg k-h

Oliver Neukum

2025-10-08 21:24 UTC #2 lore raw

In reply to Thorsten Blum

Hi,

Quoted 20 lines · Thorsten Blum

Replace kmalloc() followed by copy_from_user() with memdup_user() to simplify and improve proc_do_submiturb(). Replace the hard-coded 8 bytes with the size of 'struct usb_ctrlrequest'.
Return early if an error occurs, and avoid manually setting 'ret' and using 'goto error'.
No functional changes intended.
Signed-off-by Thorsten Blum <[email protected]>
---
  drivers/usb/core/devio.c | 10 +++-------
  1 file changed, 3 insertions(+), 7 deletions(-)
drivers/usb/core/devio.c +0 -0
index f6ce6e26e0d4..3bc54a5c59ff 100644
@@ -1670,13 +1670,9 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
  		/* min 8 byte setup packet */

drivers/usb/core/devio.c static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb · line 1673

Quoted 10 lines

drivers/usb/core/devio.c +1 -7

  		if (uurb->buffer_length < 8)
  			return -EINVAL;
-		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
-		if (!dr)
-			return -ENOMEM;
-		if (copy_from_user(dr, uurb->buffer, 8)) {
-			ret = -EFAULT;
-			goto error;
-		}
+		dr = memdup_user(uurb->buffer, sizeof(struct usb_ctrlrequest));

You cannot do this. User space cannot and must not know or care how long struct usb_ctrlrequest is. It is a data structure internal to the kernel. For the purpose of this API we copy 8 bytes. That is set in stone. If the kernel's data structure ever changes length, we will have to define a new ioctl.

You have to leave the literal 8.

Regards
Oliver