| Message ID | 1564407819-10746-3-git-send-email-saranya.gopal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Prevent requests from being queued twice | expand |
On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote: > From: Felipe Balbi <felipe.balbi@linux.intel.com> > > [Upstream commit b2b6d601365a1acb90b87c85197d79] > > Queueing the same request twice can introduce hard-to-debug > problems. At least one function driver - Android's f_mtp.c - is known > to cause this problem. > > While that function is out-of-tree, this is a problem that's easy > enough to avoid. > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > --- > drivers/usb/dwc3/gadget.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 3f337a0..a56a92a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > &req->request, req->dep->name)) > return -EINVAL; > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, > + "%s: request %pK already in flight\n", > + dep->name, &req->request)) > + return -EINVAL; So we are going to trip syzbot up on this out-of-tree driver? Brave...
> On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote: > > From: Felipe Balbi <felipe.balbi@linux.intel.com> > > > > [Upstream commit b2b6d601365a1acb90b87c85197d79] > > > > Queueing the same request twice can introduce hard-to-debug > > problems. At least one function driver - Android's f_mtp.c - is known > > to cause this problem. > > > > While that function is out-of-tree, this is a problem that's easy > > enough to avoid. > > > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > --- > > drivers/usb/dwc3/gadget.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 3f337a0..a56a92a 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct > dwc3_ep *dep, struct dwc3_request *req) > > &req->request, req->dep->name)) > > return -EINVAL; > > > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, > > + "%s: request %pK already in flight\n", > > + dep->name, &req->request)) > > + return -EINVAL; > > So we are going to trip syzbot up on this out-of-tree driver? Brave... I had retained the commit message from the upstream commit. However, without this patch, I see issues with adb as well. Adb will hang after adb root/unroot command and needs a reboot to recover. Thanks, Saranya
On Mon, Jul 29, 2019 at 05:06:13PM +0000, Gopal, Saranya wrote: > > On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote: > > > From: Felipe Balbi <felipe.balbi@linux.intel.com> > > > > > > [Upstream commit b2b6d601365a1acb90b87c85197d79] > > > > > > Queueing the same request twice can introduce hard-to-debug > > > problems. At least one function driver - Android's f_mtp.c - is known > > > to cause this problem. > > > > > > While that function is out-of-tree, this is a problem that's easy > > > enough to avoid. > > > > > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > --- > > > drivers/usb/dwc3/gadget.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 3f337a0..a56a92a 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct > > dwc3_ep *dep, struct dwc3_request *req) > > > &req->request, req->dep->name)) > > > return -EINVAL; > > > > > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, > > > + "%s: request %pK already in flight\n", > > > + dep->name, &req->request)) > > > + return -EINVAL; > > > > So we are going to trip syzbot up on this out-of-tree driver? Brave... > > I had retained the commit message from the upstream commit. > However, without this patch, I see issues with adb as well. > Adb will hang after adb root/unroot command and needs a reboot to recover. So you see huge WARN dumps in the log? That's fine, but be aware, if userspace can trigger this, then syzbot will trigger it, and any system running 'panic on warn' will crash. If this is something that we normally have to catch and handle, WARN() is not how to do it. But we should fix that upstream, not here. thanks, greg k-h
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3f337a0..a56a92a 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) &req->request, req->dep->name)) return -EINVAL; + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, + "%s: request %pK already in flight\n", + dep->name, &req->request)) + return -EINVAL; + pm_runtime_get(dwc->dev); req->request.actual = 0;