lkml.org 
[lkml]   [2013]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/2] usb: fix cleanup after failure in hub_configure()
On Tue, Aug 20, 2013 at 02:14:42PM -0400, Alan Stern wrote:
> On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
>
> > If the hub_configure() fails after setting the hdev->maxchild
> > the hub->ports might be NULL or point to uninitialized kzallocated
> > memory causing NULL pointer dereference in hub_quiesce() during cleanup.
> >
> > Now after such error the hdev->maxchild is set to 0 to avoid cleanup
> > of uninitialized ports.
>
> The idea is good, but the implementation is a little silly...
>
> > Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > ---
> > drivers/usb/core/hub.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 558313d..588c3a3 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -1339,7 +1339,7 @@ static int hub_configure(struct usb_hub *hub,
> > GFP_KERNEL);
> > if (!hub->ports) {
> > ret = -ENOMEM;
> > - goto fail;
> > + goto fail_maxchild;
> > }
> >
> > wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics);
>
> > @@ -1567,6 +1567,8 @@ static int hub_configure(struct usb_hub *hub,
> > hub_activate(hub, HUB_INIT);
> > return 0;
> >
> > +fail_maxchild:
> > + hdev->maxchild = 0;
> > fail:
> > dev_err (hub_dev, "config failed, %s (err %d)\n",
> > message, ret);
>
> Why bother with a separate jump label? Just set maxchild to 0 whenever
> a failure occurs.
>

Initially I had just straightforward "dev->maxchild = 0;" after fail,
but I changed that to simplify the second patch and be able to
use goto fail:

ret = usb_hub_create_port_device(hub, i + 1);
if (ret < 0) {
dev_err(hub->intfdev,
"couldn't create port%d device.\n", i + 1);
hdev->maxchild = i;
goto fail;
}

and avoid "return ret" here or something like

if (hdev->maxchild == hub->descriptor->bNbrPorts)
hdev->maxchild = 0;

in the fail path.

Thanks,
Krzysiek

-- >8 --
Subject: [PATCH 1/2 v2] usb: fix cleanup after failure in hub_configure()

If the hub_configure() fails after setting the hdev->maxchild
the hub->ports might be NULL or point to uninitialized kzallocated
memory causing NULL pointer dereference in hub_quiesce() during cleanup.

Now after such error the hdev->maxchild is set to 0 to avoid cleanup
of uninitialized ports.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
drivers/usb/core/hub.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 558313d..affed11 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1568,6 +1568,7 @@ static int hub_configure(struct usb_hub *hub,
return 0;

fail:
+ hdev->maxchild = 0;
dev_err (hub_dev, "config failed, %s (err %d)\n",
message, ret);
/* hub_disconnect() frees urb and descriptor */
--
1.8.4.rc1.409.gbd48715


\
 
 \ /
  Last update: 2013-08-20 21:01    [W:0.075 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site