currently most gadget driver have a pointer to save
struct usb_gadget_driver from upper layer,
it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first,
but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.
Signed-off-by: Linyu Yuan <[email protected]>
---
v3: no change
v2: no change
drivers/usb/gadget/udc/core.c | 47 ++++++++++++++++++++++++++++++++++-
include/linux/usb/gadget.h | 6 +++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index b7f0b1ebaaa8..44af6ff281d3 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1052,6 +1052,45 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
}
EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
+void usb_gadget_udc_disconnect(struct usb_gadget *gadget)
+{
+ struct usb_udc *udc = gadget->udc;
+
+ if (udc && udc->driver && udc->driver->disconnect)
+ udc->driver->disconnect(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_disconnect);
+
+void usb_gadget_udc_suspend(struct usb_gadget *gadget)
+{
+ struct usb_udc *udc = gadget->udc;
+
+ if (udc && udc->driver && udc->driver->suspend)
+ udc->driver->suspend(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_suspend);
+
+void usb_gadget_udc_resume(struct usb_gadget *gadget)
+{
+ struct usb_udc *udc = gadget->udc;
+
+ if (udc && udc->driver && udc->driver->resume)
+ udc->driver->resume(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_resume);
+
+int usb_gadget_udc_setup(struct usb_gadget *gadget,
+ const struct usb_ctrlrequest *ctrl)
+{
+ struct usb_udc *udc = gadget->udc;
+
+ if (udc && udc->driver)
+ return udc->driver->setup(gadget, ctrl);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_setup);
+
/**
* usb_gadget_udc_reset - notifies the udc core that bus reset occurs
* @gadget: The gadget which bus reset occurs
@@ -1064,7 +1103,13 @@ EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
void usb_gadget_udc_reset(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
- driver->reset(gadget);
+ struct usb_udc *udc = gadget->udc;
+
+ if (driver)
+ driver->reset(gadget);
+ else if (udc && udc->driver)
+ udc->driver->reset(gadget);
+
usb_gadget_set_state(gadget, USB_STATE_DEFAULT);
}
EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 75c7538e350a..85f4c87ba5b3 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -881,6 +881,12 @@ extern void usb_gadget_set_state(struct usb_gadget *gadget,
/*-------------------------------------------------------------------------*/
+extern void usb_gadget_udc_disconnect(struct usb_gadget *gadget);
+extern void usb_gadget_udc_suspend(struct usb_gadget *gadget);
+extern void usb_gadget_udc_resume(struct usb_gadget *gadget);
+extern int usb_gadget_udc_setup(struct usb_gadget *gadget,
+ const struct usb_ctrlrequest *ctrl);
+
/* utility to tell udc core that the bus reset occurs */
extern void usb_gadget_udc_reset(struct usb_gadget *gadget,
struct usb_gadget_driver *driver);
--
2.25.1
On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
> currently most gadget driver have a pointer to save
> struct usb_gadget_driver from upper layer,
> it allow upper layer set and unset of the pointer.
>
> there is race that upper layer unset the pointer first,
> but gadget driver use the pointer later,
> and it cause system crash due to NULL pointer access.
This race has already been fixed in Greg's usb-next branch. See commit
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
following commits 04145a03db9d ("USB: UDC: Implement
udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").
You just need to write a corresponding patch implementing the
async_callbacks op for dwc3.
Alan Stern
On 2021-06-20 10:13, Alan Stern wrote:
> On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
>> currently most gadget driver have a pointer to save
>> struct usb_gadget_driver from upper layer,
>> it allow upper layer set and unset of the pointer.
>>
>> there is race that upper layer unset the pointer first,
>> but gadget driver use the pointer later,
>> and it cause system crash due to NULL pointer access.
>
> This race has already been fixed in Greg's usb-next branch. See commit
> 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
> following commits 04145a03db9d ("USB: UDC: Implement
> udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
> Implement udc_async_callbacks in net2280").
>
thanks, this is better, lower driver only need change several places.
> You just need to write a corresponding patch implementing the
> async_callbacks op for dwc3.
yes, i will do.
>
> Alan Stern
On 2021-06-20 11:46, [email protected] wrote:
> On 2021-06-20 10:13, Alan Stern wrote:
>> On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
>>> currently most gadget driver have a pointer to save
>>> struct usb_gadget_driver from upper layer,
>>> it allow upper layer set and unset of the pointer.
>>>
>>> there is race that upper layer unset the pointer first,
>>> but gadget driver use the pointer later,
>>> and it cause system crash due to NULL pointer access.
>>
>> This race has already been fixed in Greg's usb-next branch. See
>> commit
>> 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
>> following commits 04145a03db9d ("USB: UDC: Implement
>> udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
>> Implement udc_async_callbacks in net2280").
>>
> thanks, this is better, lower driver only need change several places.
>> You just need to write a corresponding patch implementing the
>> async_callbacks op for dwc3.
> yes, i will do.
>>
Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB:
UDC:
Implement udc_async_callbacks in net2280")
+ if (dev->async_callbacks) { ----> if CPU1 saw
this is true
+ spin_unlock(&dev->lock); ---> CPU2 get
lock after this unlock,
it will set async_callbacks to false, then follow call also crash, right
?
+ tmp = dev->driver->setup(&dev->gadget,
&u.r);
+ spin_lock(&dev->lock);
+ }
>> Alan Stern
On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected] wrote:
> On 2021-06-20 11:46, [email protected] wrote:
> > On 2021-06-20 10:13, Alan Stern wrote:
> > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
> > > > currently most gadget driver have a pointer to save
> > > > struct usb_gadget_driver from upper layer,
> > > > it allow upper layer set and unset of the pointer.
> > > >
> > > > there is race that upper layer unset the pointer first,
> > > > but gadget driver use the pointer later,
> > > > and it cause system crash due to NULL pointer access.
> > >
> > > This race has already been fixed in Greg's usb-next branch. See
> > > commit
> > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
> > > following commits 04145a03db9d ("USB: UDC: Implement
> > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
> > > Implement udc_async_callbacks in net2280").
> > >
> > thanks, this is better, lower driver only need change several places.
> > > You just need to write a corresponding patch implementing the
> > > async_callbacks op for dwc3.
> > yes, i will do.
> > >
> Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: UDC:
> Implement udc_async_callbacks in net2280")
>
> + if (dev->async_callbacks) { ----> if CPU1 saw this
> is true
> + spin_unlock(&dev->lock); ---> CPU2 get lock
> after this unlock,
> it will set async_callbacks to false, then follow call also crash, right ?
> + tmp = dev->driver->setup(&dev->gadget,
> &u.r);
> + spin_lock(&dev->lock);
> + }
No, this is okay. The reason is because usb_gadget_remove_driver (CPU2
in your example) does this:
usb_gadget_disable_async_callbacks(udc);
if (udc->gadget->irq)
synchronize_irq(udc->gadget->irq);
udc->driver->unbind(udc->gadget);
usb_gadget_udc_stop(udc);
The synchronize_irq call will make CPU2 wait until CPU1 has finished
handling the interrupt for the setup packet. The system won't crash,
because dev->driver->setup will be called before unbind and udc_stop
instead of after.
Alan Stern
On 2021-06-20 21:47, Alan Stern wrote:
> On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected]
> wrote:
>> On 2021-06-20 11:46, [email protected] wrote:
>> > On 2021-06-20 10:13, Alan Stern wrote:
>> > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
>> > > > currently most gadget driver have a pointer to save
>> > > > struct usb_gadget_driver from upper layer,
>> > > > it allow upper layer set and unset of the pointer.
>> > > >
>> > > > there is race that upper layer unset the pointer first,
>> > > > but gadget driver use the pointer later,
>> > > > and it cause system crash due to NULL pointer access.
>> > >
>> > > This race has already been fixed in Greg's usb-next branch. See
>> > > commit
>> > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
>> > > following commits 04145a03db9d ("USB: UDC: Implement
>> > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
>> > > Implement udc_async_callbacks in net2280").
>> > >
>> > thanks, this is better, lower driver only need change several places.
>> > > You just need to write a corresponding patch implementing the
>> > > async_callbacks op for dwc3.
>> > yes, i will do.
>> > >
>> Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB:
>> UDC:
>> Implement udc_async_callbacks in net2280")
>>
>> + if (dev->async_callbacks) { ----> if CPU1 saw
>> this
>> is true
>> + spin_unlock(&dev->lock); ---> CPU2 get
>> lock
>> after this unlock,
>> it will set async_callbacks to false, then follow call also crash,
>> right ?
>> + tmp = dev->driver->setup(&dev->gadget,
>> &u.r);
>> + spin_lock(&dev->lock);
>> + }
>
> No, this is okay. The reason is because usb_gadget_remove_driver (CPU2
> in your example) does this:
>
> usb_gadget_disable_async_callbacks(udc);
> if (udc->gadget->irq)
> synchronize_irq(udc->gadget->irq);
> udc->driver->unbind(udc->gadget);
> usb_gadget_udc_stop(udc);
>
> The synchronize_irq call will make CPU2 wait until CPU1 has finished
> handling the interrupt for the setup packet. The system won't crash,
> because dev->driver->setup will be called before unbind and udc_stop
> instead of after.
still several question,
1. how about suspend calll dev->driver->suspend ?
2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in
dummy-hcd") backport to LTS branch ?
3. how about coding style ? so following code
if (foo->gadget_driver && foo->gadget_driver->resume)
change to
if (foo->asnyc_callbacks && foo->gadget_driver->resume)
>
> Alan Stern
On Mon, Jun 21, 2021 at 09:37:34AM +0800, [email protected] wrote:
> On 2021-06-20 21:47, Alan Stern wrote:
> > On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected] wrote:
> > > On 2021-06-20 11:46, [email protected] wrote:
> > > > On 2021-06-20 10:13, Alan Stern wrote:
> > > > > On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:
> > > > > > currently most gadget driver have a pointer to save
> > > > > > struct usb_gadget_driver from upper layer,
> > > > > > it allow upper layer set and unset of the pointer.
> > > > > >
> > > > > > there is race that upper layer unset the pointer first,
> > > > > > but gadget driver use the pointer later,
> > > > > > and it cause system crash due to NULL pointer access.
> > > > >
> > > > > This race has already been fixed in Greg's usb-next branch. See
> > > > > commit
> > > > > 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
> > > > > following commits 04145a03db9d ("USB: UDC: Implement
> > > > > udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
> > > > > Implement udc_async_callbacks in net2280").
> > > > >
> > > > thanks, this is better, lower driver only need change several places.
> > > > > You just need to write a corresponding patch implementing the
> > > > > async_callbacks op for dwc3.
> > > > yes, i will do.
> > > > >
> > > Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB:
> > > UDC:
> > > Implement udc_async_callbacks in net2280")
> > >
> > > + if (dev->async_callbacks) { ----> if CPU1
> > > saw this
> > > is true
> > > + spin_unlock(&dev->lock); ---> CPU2
> > > get lock
> > > after this unlock,
> > > it will set async_callbacks to false, then follow call also crash,
> > > right ?
> > > + tmp = dev->driver->setup(&dev->gadget,
> > > &u.r);
> > > + spin_lock(&dev->lock);
> > > + }
> >
> > No, this is okay. The reason is because usb_gadget_remove_driver (CPU2
> > in your example) does this:
> >
> > usb_gadget_disable_async_callbacks(udc);
> > if (udc->gadget->irq)
> > synchronize_irq(udc->gadget->irq);
> > udc->driver->unbind(udc->gadget);
> > usb_gadget_udc_stop(udc);
> >
> > The synchronize_irq call will make CPU2 wait until CPU1 has finished
> > handling the interrupt for the setup packet. The system won't crash,
> > because dev->driver->setup will be called before unbind and udc_stop
> > instead of after.
> still several question,
> 1. how about suspend calll dev->driver->suspend ?
The same reasoning applies. The synchronize_irq call will make CPU2
wait until CPU1 has finished handling the interrupt for the USB bus
suspend. The system won't crash, because dev->driver->suspend will be
called before unbind and udc_stop instead of after.
> 2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in
> dummy-hcd") backport to LTS branch ?
None of these commits are marked for back-porting to the -stable
kernels. The race they fix does not occur often.
If you the commits to be applied to the LTS stable kernels, you can ask
Greg KH to do it.
> 3. how about coding style ? so following code
> if (foo->gadget_driver && foo->gadget_driver->resume)
> change to
> if (foo->asnyc_callbacks && foo->gadget_driver->resume)
I don't understand this question.
Alan Stern