* [PATCH] usb: gadget: Check for NULL pointer in disconnect @ 2015-03-20 23:40 Badhri Jagan Sridharan 2015-03-22 7:43 ` Peter Chen 0 siblings, 1 reply; 5+ messages in thread From: Badhri Jagan Sridharan @ 2015-03-20 23:40 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, Badhri Jagan Sridharan Added a safety net to make sure that composite_disconnect does not end up disconneting a NULL device. Prevents NULL pointer crash. Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> --- drivers/usb/gadget/composite.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 13adfd1..90b37bd 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -1823,6 +1823,11 @@ void composite_disconnect(struct usb_gadget *gadget) struct usb_composite_dev *cdev = get_gadget_data(gadget); unsigned long flags; + if (!cdev) { + WARN(1, "Trying to disconnect a NULL composite device\n"); + return; + } + /* REVISIT: should we have config and device level * disconnect callbacks? */ -- 2.2.0.rc0.207.ga3a616c ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: Check for NULL pointer in disconnect 2015-03-20 23:40 [PATCH] usb: gadget: Check for NULL pointer in disconnect Badhri Jagan Sridharan @ 2015-03-22 7:43 ` Peter Chen 2015-03-23 23:00 ` Badhri Jagan Sridharan 0 siblings, 1 reply; 5+ messages in thread From: Peter Chen @ 2015-03-22 7:43 UTC (permalink / raw) To: Badhri Jagan Sridharan Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel On Fri, Mar 20, 2015 at 04:40:52PM -0700, Badhri Jagan Sridharan wrote: > Added a safety net to make sure that > composite_disconnect does not end up disconneting > a NULL device. Prevents NULL pointer crash. > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> > --- > drivers/usb/gadget/composite.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 13adfd1..90b37bd 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -1823,6 +1823,11 @@ void composite_disconnect(struct usb_gadget *gadget) > struct usb_composite_dev *cdev = get_gadget_data(gadget); > unsigned long flags; > > + if (!cdev) { > + WARN(1, "Trying to disconnect a NULL composite device\n"); > + return; > + } > + Do you really see some udc drivers call it after composite_unbind is called? If it is, you may add dump_stack() to track that error. Besides this, function suspended_show is needed to add cdev NULL pointer checking. > /* REVISIT: should we have config and device level > * disconnect callbacks? > */ > -- > 2.2.0.rc0.207.ga3a616c > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: Check for NULL pointer in disconnect 2015-03-22 7:43 ` Peter Chen @ 2015-03-23 23:00 ` Badhri Jagan Sridharan 2015-03-24 8:39 ` Peter Chen 2015-03-24 16:30 ` Felipe Balbi 0 siblings, 2 replies; 5+ messages in thread From: Badhri Jagan Sridharan @ 2015-03-23 23:00 UTC (permalink / raw) To: Peter Chen; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel > Do you really see some udc drivers call it after composite_unbind is > called? If it is, you may add dump_stack() to track that error. > > Besides this, function suspended_show is needed to add cdev NULL pointer > checking. We see this happening occasionally in *not yet* upstreamed UDC code of some vendors (Yes, disconnect being called after unbind) After reviewing the entire composite.c file, I did notice that none of the functions check for NULL pointer when cdev is obtained from get_gadget_data. Is crashing/bringing down the whole kernel intentionally left to happen ? Isn't printing a WARN/ERROR msg and returning not the preferable approach ? On 03/22/2015 12:43 AM, Peter Chen wrote: > On Fri, Mar 20, 2015 at 04:40:52PM -0700, Badhri Jagan Sridharan wrote: >> Added a safety net to make sure that >> composite_disconnect does not end up disconneting >> a NULL device. Prevents NULL pointer crash. >> >> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> >> --- >> drivers/usb/gadget/composite.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >> index 13adfd1..90b37bd 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -1823,6 +1823,11 @@ void composite_disconnect(struct usb_gadget *gadget) >> struct usb_composite_dev *cdev = get_gadget_data(gadget); >> unsigned long flags; >> >> + if (!cdev) { >> + WARN(1, "Trying to disconnect a NULL composite device\n"); >> + return; >> + } >> + > > Do you really see some udc drivers call it after composite_unbind is > called? If it is, you may add dump_stack() to track that error. > > Besides this, function suspended_show is needed to add cdev NULL pointer > checking. > >> /* REVISIT: should we have config and device level >> * disconnect callbacks? >> */ >> -- >> 2.2.0.rc0.207.ga3a616c >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: Check for NULL pointer in disconnect 2015-03-23 23:00 ` Badhri Jagan Sridharan @ 2015-03-24 8:39 ` Peter Chen 2015-03-24 16:30 ` Felipe Balbi 1 sibling, 0 replies; 5+ messages in thread From: Peter Chen @ 2015-03-24 8:39 UTC (permalink / raw) To: Badhri Jagan Sridharan Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, Mar 23, 2015 at 04:00:37PM -0700, Badhri Jagan Sridharan wrote: > > Do you really see some udc drivers call it after composite_unbind is > > called? If it is, you may add dump_stack() to track that error. > > > > Besides this, function suspended_show is needed to add cdev NULL pointer > > checking. > > We see this happening occasionally in *not yet* upstreamed UDC > code of some vendors (Yes, disconnect being called after unbind) Does it a spurious interrupt? I guess if you add struct usb_gadget_driver NULL pointer check check, it will not occur: if (you_udc->driver) you_udc->driver->disconnect(&you_udc->gadget); > After reviewing the entire composite.c file, I did notice that > none of the functions check for NULL pointer when cdev is > obtained from get_gadget_data. > Is crashing/bringing down the whole kernel intentionally > left to happen ? Hmm, I don't think, the design may think it never occurs. > Isn't printing a WARN/ERROR msg and > returning not the preferable approach ? I think BUG_ON is more suitable, let's see Felipe's comment. > > On 03/22/2015 12:43 AM, Peter Chen wrote: > >On Fri, Mar 20, 2015 at 04:40:52PM -0700, Badhri Jagan Sridharan wrote: > >>Added a safety net to make sure that > >>composite_disconnect does not end up disconneting > >>a NULL device. Prevents NULL pointer crash. > >> > >>Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> > >>--- > >> drivers/usb/gadget/composite.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >>diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > >>index 13adfd1..90b37bd 100644 > >>--- a/drivers/usb/gadget/composite.c > >>+++ b/drivers/usb/gadget/composite.c > >>@@ -1823,6 +1823,11 @@ void composite_disconnect(struct usb_gadget *gadget) > >> struct usb_composite_dev *cdev = get_gadget_data(gadget); > >> unsigned long flags; > >> > >>+ if (!cdev) { > >>+ WARN(1, "Trying to disconnect a NULL composite device\n"); > >>+ return; > >>+ } > >>+ > > > >Do you really see some udc drivers call it after composite_unbind is > >called? If it is, you may add dump_stack() to track that error. > > > >Besides this, function suspended_show is needed to add cdev NULL pointer > >checking. > > > >> /* REVISIT: should we have config and device level > >> * disconnect callbacks? > >> */ > >>-- > >>2.2.0.rc0.207.ga3a616c > >> > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-usb" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: Check for NULL pointer in disconnect 2015-03-23 23:00 ` Badhri Jagan Sridharan 2015-03-24 8:39 ` Peter Chen @ 2015-03-24 16:30 ` Felipe Balbi 1 sibling, 0 replies; 5+ messages in thread From: Felipe Balbi @ 2015-03-24 16:30 UTC (permalink / raw) To: Badhri Jagan Sridharan Cc: Peter Chen, Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 568 bytes --] On Mon, Mar 23, 2015 at 04:00:37PM -0700, Badhri Jagan Sridharan wrote: > > Do you really see some udc drivers call it after composite_unbind is > > called? If it is, you may add dump_stack() to track that error. > > > > Besides this, function suspended_show is needed to add cdev NULL pointer > > checking. > > We see this happening occasionally in *not yet* upstreamed UDC > code of some vendors (Yes, disconnect being called after unbind) fix your UDC. We leave these sort of thing open so people realise they have to fix their drivers. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-03-24 16:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-20 23:40 [PATCH] usb: gadget: Check for NULL pointer in disconnect Badhri Jagan Sridharan 2015-03-22 7:43 ` Peter Chen 2015-03-23 23:00 ` Badhri Jagan Sridharan 2015-03-24 8:39 ` Peter Chen 2015-03-24 16:30 ` Felipe Balbi
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).