* [PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference
@ 2021-07-23 15:59 Maxim Devaev
2021-07-23 16:22 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Maxim Devaev @ 2021-07-23 15:59 UTC (permalink / raw)
To: balbi; +Cc: gregkh, sandeen, linux-usb, mdevaev, Phil Elwell
From: Phil Elwell <phil@raspberrypi.com>
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] 3+ messages in thread* Re: [PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference
2021-07-23 15:59 [PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference Maxim Devaev
@ 2021-07-23 16:22 ` Greg KH
2021-07-23 16:48 ` Maxim Devaev
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2021-07-23 16:22 UTC (permalink / raw)
To: Maxim Devaev; +Cc: balbi, sandeen, linux-usb, Phil Elwell
On Fri, Jul 23, 2021 at 06:59:30PM +0300, Maxim Devaev wrote:
> From: Phil Elwell <phil@raspberrypi.com>
>
> 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;
What prevents "enabled" from changing right after you test this? You do
not have the lock held so could it change then?
> + }
> +
> 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;
No error message if queueing up failed?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference
2021-07-23 16:22 ` Greg KH
@ 2021-07-23 16:48 ` Maxim Devaev
0 siblings, 0 replies; 3+ messages in thread
From: Maxim Devaev @ 2021-07-23 16:48 UTC (permalink / raw)
To: Greg KH; +Cc: balbi, sandeen, linux-usb, Phil Elwell
> > + if (!hidg->in_ep->enabled) {
> > + ERROR(hidg->func.config->cdev, "in_ep is disabled\n");
> > + status = -ESHUTDOWN;
> > + goto release_write_pending;
>
> What prevents "enabled" from changing right after you test this? You do
> not have the lock held so could it change then?
Inside usb_ep_queue() a similar check is used without blocking.
I suppose this is how it should be. Here it's necessary for the error message.
> > + }
> > +
> > 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;
>
> No error message if queueing up failed?
Yes. For each write from userspace, this generates one message.
This can lead to a huge spam in the log.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-23 16:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 15:59 [PATCH v2] usb: gadget: f_hid: fixed NULL pointer dereference Maxim Devaev
2021-07-23 16:22 ` Greg KH
2021-07-23 16:48 ` Maxim Devaev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.