Re: [PATCH 1/2] usb: fix cleanup after failure in hub_configure()

[Krzysztof Mazur]

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 <st...@rowland.harvard.edu>
> > Signed-off-by: Krzysztof Mazur <krzys...@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 <krzys...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html