usb: gadget: f_hid: fixed NULL pointer dereference

PATCH Superseded Maxim Devaev 2 participants 5 messages Jul 23, 2021 USB

lore

Share & tools

lore

Maxim Devaev

author

2021-07-23 09:53 UTC #0 lore raw

PATCH 1/1 usb: gadget: f_hid: fixed NULL pointer dereference

Disconnecting and reconnecting the USB cable can lead to crashes and a variety of kernel log spam.

The problem was found and reproduced on the Raspberry Pi. The patch was created in Raspberry's own fork [1]. Since I was one of those who discovered and diagnosed the problem [2], I propose this patch for adoption to the kernel upstream with the consent of the author. It has been tested since January 2021 and is guaranteed to fix the described problem without breaking anything.

Signed-off-by Maxim Devaev <[email protected]>

Signed-off-by Phil Elwell <[email protected]>

Link https://github.com/raspberrypi/linux/commit/a6e47d5f4efbd2ea6a0b6565cd2f9b7bb217ded5 [1]

Link https://github.com/raspberrypi/linux/issues/3870 [2]

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

drivers/usb/gadget/function/f_hid.c +17 -3

index 02683ac07..4975bbf02 100644
@@ -338,6 +338,11 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
+	if (!hidg->req) {
+		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+		return -ESHUTDOWN;
+	}
+
 #define WRITE_COND (!hidg->write_pending)
 try_again:
 	/* write queue */

@@ -358,7 +363,13 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	count  = min_t(unsigned, count, hidg->report_length);
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
-	status = copy_from_user(req->buf, buffer, count);
+	if (req) {
+		status = copy_from_user(req->buf, buffer, count);
+	} else {
+		ERROR(hidg->func.config->cdev, "hidg->req is NULL\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}
 
 	if (status != 0) {
 		ERROR(hidg->func.config->cdev,

@@ -387,10 +398,13 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
+	if (!hidg->in_ep->enabled) {
+		ERROR(hidg->func.config->cdev, "in_ep is disabled\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}
 	status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
 	if (status < 0) {
-		ERROR(hidg->func.config->cdev,
-			"usb_ep_queue error on int endpoint %zd\n", status);
 		goto release_write_pending;
 	} else {
 		status = count;

Signature · 1 line

2.32.0

Greg KH

maintainer

2021-07-23 10:14 UTC #1 lore raw

Quoted 10 lines · Maxim Devaev

Disconnecting and reconnecting the USB cable can lead to crashes and a variety of kernel log spam.
The problem was found and reproduced on the Raspberry Pi. The patch was created in Raspberry's own fork [1]. Since I was one of those who discovered and diagnosed the problem [2], I propose this patch for adoption to the kernel upstream with the consent of the author. It has been tested since January 2021 and is guaranteed to fix the described problem without breaking anything.

Quoted 4 lines

Signed-off-by Maxim Devaev <[email protected]>
Signed-off-by Phil Elwell <[email protected]>
Link https://github.com/raspberrypi/linux/commit/a6e47d5f4efbd2ea6a0b6565cd2f9b7bb217ded5 [1]
Link https://github.com/raspberrypi/linux/issues/3870 [2]

Who is the original author here? Please put their name as the "From:" line in the changelog so we can give proper credit if it was not you.

Quoted 21 lines

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

drivers/usb/gadget/function/f_hid.c +5 -0

index 02683ac07..4975bbf02 100644
@@ -338,6 +338,11 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
+	if (!hidg->req) {
+		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+		return -ESHUTDOWN;
+	}
+
 #define WRITE_COND (!hidg->write_pending)
 try_again:
 	/* write queue */

drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 366

Quoted 5 lines

drivers/usb/gadget/function/f_hid.c +1 -1

@@ -358,7 +363,13 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	count  = min_t(unsigned, count, hidg->report_length);
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
-	status = copy_from_user(req->buf, buffer, count);
+	if (req) {

Test for !req and then abort, and then continue on. No need for moving the copy_from_user line in, right? Should make the change smaller overall.

Quoted 10 lines

drivers/usb/gadget/function/f_hid.c +6 -0

+		status = copy_from_user(req->buf, buffer, count);
+	} else {
+		ERROR(hidg->func.config->cdev, "hidg->req is NULL\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}
 
 	if (status != 0) {
 		ERROR(hidg->func.config->cdev,

drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 405

Quoted 8 lines

drivers/usb/gadget/function/f_hid.c +5 -0

@@ -387,10 +398,13 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
+	if (!hidg->in_ep->enabled) {
+		ERROR(hidg->func.config->cdev, "in_ep is disabled\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}

Blank line after this please.

drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 408

Quoted 5 lines

drivers/usb/gadget/function/f_hid.c +0 -2

 	status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
 	if (status < 0) {
-		ERROR(hidg->func.config->cdev,
-			"usb_ep_queue error on int endpoint %zd\n", status);
 		goto release_write_pending;

Are the braces still needed here?

thanks,

greg k-h

Maxim Devaev

author

2021-07-23 15:22 UTC #2 lore raw

Quoted 3 lines

Greg KH <[email protected]> writes:
Who is the original author here? Please put their name as the "From:" line in the changelog so we can give proper credit if it was not you.

Phil Elwell. Fixed.

Quoted 3 lines

Test for !req and then abort, and then continue on. No need for moving the copy_from_user line in, right? Should make the change smaller overall.

Fixed.

Quoted 1 line

Blank line after this please.

Added.

Quoted 1 line

Are the braces still needed here?

No, you're right.

Thanks for the review! Here the new version of the patch:

From 72a504af8e3df3d7a44e9954b8cf03268795c07f Mon Sep 17 00:00:00 2001
From: Phil Elwell <[email protected]>
Date: Fri, 23 Jul 2021 11:59:49 +0300
Subject: [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference

Disconnecting and reconnecting the USB cable can lead to crashes and a variety of kernel log spam.

The problem was found and reproduced on the Raspberry Pi [1] and the original fix was created in Raspberry's own fork [2].

Signed-off-by Maxim Devaev <[email protected]>

Signed-off-by Phil Elwell <[email protected]>

Link https://github.com/raspberrypi/linux/issues/3870 [1]

Link https://github.com/raspberrypi/linux/commit/a6e47d5f4efbd2ea6a0b6565cd2f9b7bb217ded5 [2]

---
 drivers/usb/gadget/function/f_hid.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/gadget/function/f_hid.c +20 -6

index 02683ac07..08e73e812 100644
@@ -338,6 +338,11 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
+	if (!hidg->req) {
+		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+		return -ESHUTDOWN;
+	}
+
 #define WRITE_COND (!hidg->write_pending)
 try_again:
 	/* write queue */

@@ -358,8 +363,14 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	count  = min_t(unsigned, count, hidg->report_length);
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
-	status = copy_from_user(req->buf, buffer, count);
 
+	if (!req) {
+		ERROR(hidg->func.config->cdev, "hidg->req is NULL\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}
+
+	status = copy_from_user(req->buf, buffer, count);
 	if (status != 0) {
 		ERROR(hidg->func.config->cdev,
 			"copy_from_user error\n");

@@ -387,14 +398,17 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
+	if (!hidg->in_ep->enabled) {
+		ERROR(hidg->func.config->cdev, "in_ep is disabled\n");
+		status = -ESHUTDOWN;
+		goto release_write_pending;
+	}
+
 	status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
-	if (status < 0) {
-		ERROR(hidg->func.config->cdev,
-			"usb_ep_queue error on int endpoint %zd\n", status);
+	if (status < 0)
 		goto release_write_pending;
-	} else {
+	else
 		status = count;
-	}
 
 	return status;
 release_write_pending:

Signature · 1 line

2.32.0

Greg KH

maintainer

2021-07-23 15:52 UTC #3 lore raw

Quoted 18 lines · Maxim Devaev

Quoted 3 lines
Greg KH <[email protected]> writes:
Who is the original author here? Please put their name as the "From:" line in the changelog so we can give proper credit if it was not you.
Phil Elwell. Fixed.
Quoted 3 lines
Test for !req and then abort, and then continue on. No need for moving the copy_from_user line in, right? Should make the change smaller overall.
Fixed.
Quoted 1 line
Blank line after this please.
Added.
Quoted 1 line
Are the braces still needed here?

Quoted 8 lines

No, you're right.
Thanks for the review! Here the new version of the patch:
From 72a504af8e3df3d7a44e9954b8cf03268795c07f Mon Sep 17 00:00:00 2001
From: Phil Elwell <[email protected]>
Date: Fri, 23 Jul 2021 11:59:49 +0300
Subject: [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference

<snip>

Please resend as a v2 in a format that I can apply it in (as a new email.)

thanks,

greg k-h

Maxim Devaev

author

2021-07-23 16:03 UTC #4 lore raw

Quoted 1 line

Greg KH <[email protected]> writes:

s a v2 in a format that I can apply it in (as a new

Quoted 1 line

email.)

Sent as ("[PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference")