| Message ID | 20200225130846.20236-1-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: hub: Fix unhandled return value of usb_autopm_get_interface() | expand |
On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > Address below Coverity complaint (Feb 25, 2020, 8:06 AM CET): > > *** CID 1458999: Error handling issues (CHECKED_RETURN) > /drivers/usb/core/hub.c: 1869 in hub_probe() > 1863 > 1864 if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND) > 1865 hub->quirk_check_port_auto_suspend = 1; > 1866 > 1867 if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) { > 1868 hub->quirk_disable_autosuspend = 1; > >>> CID 1458999: Error handling issues (CHECKED_RETURN) > >>> Calling "usb_autopm_get_interface" without checking return value (as is done elsewhere 97 out of 111 times). > 1869 usb_autopm_get_interface(intf); > 1870 } > 1871 > 1872 if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) > 1873 return 0; > 1874 > > Fixes: 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub") > Cc: Hardik Gajjar <hgajjar@de.adit-jv.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Reported-by: scan-admin@coverity.com > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > drivers/usb/core/hub.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 1d212f82c69b..ff04ca28970d 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -1865,8 +1865,12 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id) > hub->quirk_check_port_auto_suspend = 1; > > if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) { > - hub->quirk_disable_autosuspend = 1; > - usb_autopm_get_interface(intf); > + int r = usb_autopm_get_interface(intf); > + > + if (!r) > + hub->quirk_disable_autosuspend = 1; > + else > + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r); > } > > if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) This change is not necessary, because the resume operation cannot fail at this point (interfaces are always powered-up during probe). A better solution would be to call usb_autpm_get_interface_no_resume() instead. On the other hand, the other places that call usb_autopm_get_interface() without checking should be audited. Some of them almost certainly need to be fixed. Alan Stern
Hi Alan, Many thanks for the prompt review. On Tue, Feb 25, 2020 at 10:32:32AM -0500, Alan Stern wrote: > On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > > + int r = usb_autopm_get_interface(intf); > > + > > + if (!r) > > + hub->quirk_disable_autosuspend = 1; > > + else > > + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r); > > } > > > > if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) > > This change is not necessary, because the resume operation cannot fail > at this point (interfaces are always powered-up during probe). Agreed to avoid unneeded complexity. > A better solution would be to call usb_autpm_get_interface_no_resume() > instead. Pushed to https://lore.kernel.org/lkml/20200225183057.31953-1-erosca@de.adit-jv.com > > On the other hand, the other places that call > usb_autopm_get_interface() without checking should be audited. Some of > them almost certainly need to be fixed. A quick 'git grep' outputs below list of auditable candidates [1]. With no relevant devices at hand, I would avoid touching these drivers, since oftentimes even legitimate patches introduce regressions w/o testing. If anybody volunteers with testing, I guess it should be quick to either convert usb_autpm_get_interface to *_no_resume variant or handle the return value in place in below instances. [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\(" drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf); drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface); drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface); drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf); sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface);
On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > Hi Alan, > > Many thanks for the prompt review. > > On Tue, Feb 25, 2020 at 10:32:32AM -0500, Alan Stern wrote: > > On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > > > + int r = usb_autopm_get_interface(intf); > > > + > > > + if (!r) > > > + hub->quirk_disable_autosuspend = 1; > > > + else > > > + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r); > > > } > > > > > > if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) > > > > This change is not necessary, because the resume operation cannot fail > > at this point (interfaces are always powered-up during probe). > > Agreed to avoid unneeded complexity. > > > A better solution would be to call usb_autpm_get_interface_no_resume() > > instead. > > Pushed to https://lore.kernel.org/lkml/20200225183057.31953-1-erosca@de.adit-jv.com > > > > > On the other hand, the other places that call > > usb_autopm_get_interface() without checking should be audited. Some of > > them almost certainly need to be fixed. > > A quick 'git grep' outputs below list of auditable candidates [1]. > > With no relevant devices at hand, I would avoid touching these drivers, > since oftentimes even legitimate patches introduce regressions w/o > testing. > > If anybody volunteers with testing, I guess it should be quick to > either convert usb_autpm_get_interface to *_no_resume variant or > handle the return value in place in below instances. > > [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\(" > drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf); > drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface); > drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface); > drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf); > sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface); Your regular expression isn't right because it doesn't match lines where the usb_autopm_get_interface() is preceded only by one whitespace character (i.e., a tab). It also will match lines where there are two space characters between the = sign and the function name. I think what you want is more like "(^|[^=[:space:]])\s*" at the start of the RE. A revised search finds line 997 in drivers/usb/core/hub.c and lines 216, 269 in drivers/usb/core/port.c. (I didn't try looking in any other directories.) AFAICT all three of these should check the return value, although a error message in the kernel log probably isn't needed. Do you want to fix those instances up also, maybe merging them in with the existing patch? Alan Stern
Hi Alan, On Tue, Feb 25, 2020 at 02:39:23PM -0500, Alan Stern wrote: > On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > > [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\(" > > drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf); > > drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface); > > drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface); > > drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf); > > sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface); > > Your regular expression isn't right because it doesn't match lines > where the usb_autopm_get_interface() is preceded only by one whitespace > character (i.e., a tab). It also will match lines where there are two > space characters between the = sign and the function name. I think > what you want is more like "(^|[^=[:space:]])\s*" at the start of the > RE. Agreed. My version filters out some true positives. > > A revised search finds line 997 in drivers/usb/core/hub.c and lines > 216, 269 in drivers/usb/core/port.c. (I didn't try looking in any > other directories.) AFAICT all three of these should check the return > value, although a error message in the kernel log probably isn't > needed. > > Do you want to fix those instances up also, maybe merging them in with > the existing patch? I wonder if squashing all these fixes into one patch is the best option. There are three commits fixed by the proposed changes in usb core: - v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub") - v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device") - v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute") I assume a single fix will create some pain when applying it to the stable branches. Do you have any preference/inputs about that?
On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > Hi Alan, > > On Tue, Feb 25, 2020 at 02:39:23PM -0500, Alan Stern wrote: > > On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > > > [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\(" > > > drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf); > > > drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface); > > > drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface); > > > drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf); > > > sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface); > > > > Your regular expression isn't right because it doesn't match lines > > where the usb_autopm_get_interface() is preceded only by one whitespace > > character (i.e., a tab). It also will match lines where there are two > > space characters between the = sign and the function name. I think > > what you want is more like "(^|[^=[:space:]])\s*" at the start of the > > RE. > > Agreed. My version filters out some true positives. > > > > > A revised search finds line 997 in drivers/usb/core/hub.c and lines > > 216, 269 in drivers/usb/core/port.c. (I didn't try looking in any > > other directories.) AFAICT all three of these should check the return > > value, although a error message in the kernel log probably isn't > > needed. > > > > Do you want to fix those instances up also, maybe merging them in with > > the existing patch? > > I wonder if squashing all these fixes into one patch is the best option. > > There are three commits fixed by the proposed changes in usb core: > - v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub") > - v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device") > - v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute") > > I assume a single fix will create some pain when applying it to the > stable branches. Do you have any preference/inputs about that? If you prefer to split this up into multiple patches, that's fine with me. Alan Stern
Hi Alan, On Tue, Feb 25, 2020 at 03:54:20PM -0500, Alan Stern wrote: > On Tue, 25 Feb 2020, Eugeniu Rosca wrote: > > There are three commits fixed by the proposed changes in usb core: > > - v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub") > > - v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device") > > - v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute") > > > > I assume a single fix will create some pain when applying it to the > > stable branches. Do you have any preference/inputs about that? > > If you prefer to split this up into multiple patches, that's fine with > me. Please, check https://lore.kernel.org/lkml/20200226175036.14946-1-erosca@de.adit-jv.com/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 1d212f82c69b..ff04ca28970d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1865,8 +1865,12 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id) hub->quirk_check_port_auto_suspend = 1; if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) { - hub->quirk_disable_autosuspend = 1; - usb_autopm_get_interface(intf); + int r = usb_autopm_get_interface(intf); + + if (!r) + hub->quirk_disable_autosuspend = 1; + else + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r); } if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
Address below Coverity complaint (Feb 25, 2020, 8:06 AM CET): *** CID 1458999: Error handling issues (CHECKED_RETURN) /drivers/usb/core/hub.c: 1869 in hub_probe() 1863 1864 if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND) 1865 hub->quirk_check_port_auto_suspend = 1; 1866 1867 if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) { 1868 hub->quirk_disable_autosuspend = 1; >>> CID 1458999: Error handling issues (CHECKED_RETURN) >>> Calling "usb_autopm_get_interface" without checking return value (as is done elsewhere 97 out of 111 times). 1869 usb_autopm_get_interface(intf); 1870 } 1871 1872 if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) 1873 return 0; 1874 Fixes: 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub") Cc: Hardik Gajjar <hgajjar@de.adit-jv.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reported-by: scan-admin@coverity.com Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- drivers/usb/core/hub.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)