* [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference
@ 2021-07-23 9:53 Maxim Devaev
2021-07-23 10:14 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Maxim Devaev @ 2021-07-23 9:53 UTC (permalink / raw)
To: balbi; +Cc: gregkh, sandeen, linux-usb, mdevaev, Phil Elwell
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 <mdevaev@gmail.com>
Signed-off-by: Phil Elwell <phil@raspberrypi.com>
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(-)
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 02683ac07..4975bbf02 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -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;
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference
2021-07-23 9:53 [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference Maxim Devaev
@ 2021-07-23 10:14 ` Greg KH
2021-07-23 15:22 ` Maxim Devaev
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-07-23 10:14 UTC (permalink / raw)
To: Maxim Devaev; +Cc: balbi, sandeen, linux-usb, Phil Elwell
On Fri, Jul 23, 2021 at 12:53:23PM +0300, Maxim Devaev wrote:
> 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 <mdevaev@gmail.com>
> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
> 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.
> ---
> drivers/usb/gadget/function/f_hid.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 02683ac07..4975bbf02 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -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) {
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.
> + 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;
> + }
Blank line after this please.
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference
2021-07-23 10:14 ` Greg KH
@ 2021-07-23 15:22 ` Maxim Devaev
2021-07-23 15:52 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Maxim Devaev @ 2021-07-23 15:22 UTC (permalink / raw)
To: Greg KH; +Cc: balbi, sandeen, linux-usb, Phil Elwell
> Greg KH <gregkh@linuxfoundation.org> 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.
> 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.
> Blank line after this please.
Added.
> 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 <phil@raspberrypi.com>
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 <mdevaev@gmail.com>
Signed-off-by: Phil Elwell <phil@raspberrypi.com>
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(-)
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 02683ac07..08e73e812 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -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:
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference
2021-07-23 15:22 ` Maxim Devaev
@ 2021-07-23 15:52 ` Greg KH
2021-07-23 16:03 ` Maxim Devaev
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-07-23 15:52 UTC (permalink / raw)
To: Maxim Devaev; +Cc: balbi, sandeen, linux-usb, Phil Elwell
On Fri, Jul 23, 2021 at 06:22:03PM +0300, Maxim Devaev wrote:
> > Greg KH <gregkh@linuxfoundation.org> 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.
>
> > 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.
>
> > Blank line after this please.
>
> Added.
>
> > 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 <phil@raspberrypi.com>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-23 16:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 9:53 [PATCH] usb: gadget: f_hid: fixed NULL pointer dereference Maxim Devaev
2021-07-23 10:14 ` Greg KH
2021-07-23 15:22 ` Maxim Devaev
2021-07-23 15:52 ` Greg KH
2021-07-23 16:03 ` Maxim Devaev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).