usb: gadget: f_hid: fixed NULL pointer dereference
PATCH v2 1/1 usb: gadget: f_hid: fixed NULL pointer dereference
From: Phil Elwell <[email protected]>
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].
--- drivers/usb/gadget/function/f_hid.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/gadget/function/f_hid.c +20 -6
@@ -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
maintainerQuoted 49 lines · Maxim Devaev
From: Phil Elwell <[email protected]>
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]>--- drivers/usb/gadget/function/f_hid.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)drivers/usb/gadget/function/f_hid.c +12
-1@@ -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" );
drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 404
Quoted 7 lines
drivers/usb/gadget/function/f_hid.c +4
-0@@ -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?
drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 409
Quoted 8 lines
drivers/usb/gadget/function/f_hid.c +3
-3+ } + 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
Quoted 7 lines
drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 404
Quoted 4 lines
drivers/usb/gadget/function/f_hid.c +4
-0+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.
Quoted 10 lines
drivers/usb/gadget/function/f_hid.c static ssize_t f_hidg_write(struct file *file, const char __user *buffer, · line 409
Quoted 8 lines
drivers/usb/gadget/function/f_hid.c +3
-3+ } + 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.