[PATCH 0/2] usb: fix hub_configure() error handling

[PATCH 0/2] usb: fix hub_configure() error handling

[Krzysztof Mazur]

Hi,

this series fixes hub_configure() error handling that causes hub->ports[i]
NULL pointer dereferences that were previously reported at:

"[PATCH] Prevent USB hub remove oops"
http://marc.info/?l=linux-kernel&m=136189072520909&w=4
https://bugzilla.redhat.com/show_bug.cgi?id=926907

This bug still exists in 3.11-rc6 and I've got an Oops during startup
caused by hub->ports[i] dereference in hub_quiesce().

The first patch implements what Alan Stern suggested in the
"[PATCH] Prevent USB hub remove oops" thread:
  "All of these problems can be fixed in hub_configure by setting
  hub->maxchild to the total number of allocated ports (or 0 if hub_ports
  can't be allocated)."
http://marc.info/?l=linux-usb&m=136189486922963&w=4

The second patch uses slightly different approach because some
users ignore hub->maxchild and use hub->descriptor->bNbrPorts.

Krzysiek

Krzysztof Mazur (2):
  usb: fix cleanup after failure in hub_configure()
  usb: fail on usb_hub_create_port_device() errors

 drivers/usb/core/hub.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
1.8.4.rc1.409.gbd48715

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

[Krzysztof Mazur]

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.

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);
@@ -1466,7 +1466,7 @@ static int hub_configure(struct usb_hub *hub,
 	ret = usb_get_status(hdev, USB_RECIP_DEVICE, 0, &hubstatus);
 	if (ret < 2) {
 		message = "can't get hub status";
-		goto fail;
+		goto fail_maxchild;
 	}
 	le16_to_cpus(&hubstatus);
 	hcd = bus_to_hcd(hdev->bus);
@@ -1512,14 +1512,14 @@ static int hub_configure(struct usb_hub *hub,
 				&hub->tt, GFP_KERNEL);
 		if (ret < 0) {
 			message = "can't update HCD hub info";
-			goto fail;
+			goto fail_maxchild;
 		}
 	}
 
 	ret = hub_hub_status(hub, &hubstatus, &hubchange);
 	if (ret < 0) {
 		message = "can't get hub status";
-		goto fail;
+		goto fail_maxchild;
 	}
 
 	/* local power status reports aren't always correct */
@@ -1547,7 +1547,7 @@ static int hub_configure(struct usb_hub *hub,
 	hub->urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!hub->urb) {
 		ret = -ENOMEM;
-		goto fail;
+		goto fail_maxchild;
 	}
 
 	usb_fill_int_urb(hub->urb, hdev, pipe, *hub->buffer, maxp, hub_irq,
@@ -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);
-- 
1.8.4.rc1.409.gbd48715

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

[Alan Stern]

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.

Alan Stern

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 <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

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

[Alan Stern]

On Tue, 20 Aug 2013, Krzysztof Mazur wrote:

> > 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.

The second patch can either clean up the port devices by hand, or else 
jump to a new label after the line that sets maxchild to 0.

Alan Stern

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

[Krzysztof Mazur]

On Tue, Aug 20, 2013 at 02:59:18PM -0400, Alan Stern wrote:
> On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
> 
> > > 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.
> 
> The second patch can either clean up the port devices by hand, or else 
> jump to a new label after the line that sets maxchild to 0.
> Alan Stern

I think that new label is better, it's equivalent to previous version
except for ugly rename of original fail label.

Thanks,
Krzysiek
-- >8 --
Subject: [PATCH 2/2 v2] usb: fail on usb_hub_create_port_device() errors

Ignoring usb_hub_create_port_device() errors cause later NULL pointer
deference when uninitialized hub->ports[i] entries are dereferenced
after port memory allocation error.

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index affed11..292ffa8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1557,10 +1557,15 @@ static int hub_configure(struct usb_hub *hub,
 	if (hub->has_indicators && blinkenlights)
 		hub->indicator [0] = INDICATOR_CYCLE;
 
-	for (i = 0; i < hdev->maxchild; i++)
-		if (usb_hub_create_port_device(hub, i + 1) < 0)
+	for (i = 0; i < hdev->maxchild; i++) {
+		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_keep_maxchild;
+		}
+	}
 
 	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
 
@@ -1569,6 +1574,7 @@ static int hub_configure(struct usb_hub *hub,
 
 fail:
 	hdev->maxchild = 0;
+fail_keep_maxchild:
 	dev_err (hub_dev, "config failed, %s (err %d)\n",
 			message, ret);
 	/* hub_disconnect() frees urb and descriptor */
-- 
1.8.4.rc1.409.gbd48715

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

[Alan Stern]

On Tue, 20 Aug 2013, Krzysztof Mazur wrote:

> > The second patch can either clean up the port devices by hand, or else 
> > jump to a new label after the line that sets maxchild to 0.
> > Alan Stern
> 
> I think that new label is better, it's equivalent to previous version
> except for ugly rename of original fail label.
> 
> Thanks,
> Krzysiek
> -- >8 --
> Subject: [PATCH 2/2 v2] usb: fail on usb_hub_create_port_device() errors
> 
> Ignoring usb_hub_create_port_device() errors cause later NULL pointer
> deference when uninitialized hub->ports[i] entries are dereferenced
> after port memory allocation error.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
>  drivers/usb/core/hub.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index affed11..292ffa8 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1557,10 +1557,15 @@ static int hub_configure(struct usb_hub *hub,
>  	if (hub->has_indicators && blinkenlights)
>  		hub->indicator [0] = INDICATOR_CYCLE;
>  
> -	for (i = 0; i < hdev->maxchild; i++)
> -		if (usb_hub_create_port_device(hub, i + 1) < 0)
> +	for (i = 0; i < hdev->maxchild; i++) {
> +		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_keep_maxchild;
> +		}
> +	}
>  
>  	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
>  
> @@ -1569,6 +1574,7 @@ static int hub_configure(struct usb_hub *hub,
>  
>  fail:
>  	hdev->maxchild = 0;
> +fail_keep_maxchild:
>  	dev_err (hub_dev, "config failed, %s (err %d)\n",
>  			message, ret);
>  	/* hub_disconnect() frees urb and descriptor */

Much better.

Alan Stern

[PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Krzysztof Mazur]

Ignoring usb_hub_create_port_device() errors cause later NULL pointer
deference when uninitialized hub->ports[i] entries are dereferenced
after port memory allocation error.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
I'm not sure if failing in that case is a good idea, but other solutions
are more complex.

I tried also changing hdev->maxchild and skiping initialization
of later ports, but it didn't work because in some cases
hub->descriptor->bNbrPorts is used instead of hdev->maxchild.
With simulated usb_hub_create_port_device() failure I have an Oops
in hub_power_on().

Another possible solution is allowing for uninitialized ports and
checking for hub->ports[i] == NULL.

 drivers/usb/core/hub.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 588c3a3..afd334b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1557,10 +1557,15 @@ static int hub_configure(struct usb_hub *hub,
 	if (hub->has_indicators && blinkenlights)
 		hub->indicator [0] = INDICATOR_CYCLE;
 
-	for (i = 0; i < hdev->maxchild; i++)
-		if (usb_hub_create_port_device(hub, i + 1) < 0)
+	for (i = 0; i < hdev->maxchild; i++) {
+		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;
+		}
+	}
 
 	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
 
-- 
1.8.4.rc1.409.gbd48715

Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Alan Stern]

On Tue, 20 Aug 2013, Krzysztof Mazur wrote:

> Ignoring usb_hub_create_port_device() errors cause later NULL pointer
> deference when uninitialized hub->ports[i] entries are dereferenced
> after port memory allocation error.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
> I'm not sure if failing in that case is a good idea, but other solutions
> are more complex.
> 
> I tried also changing hdev->maxchild and skiping initialization
> of later ports, but it didn't work because in some cases
> hub->descriptor->bNbrPorts is used instead of hdev->maxchild.
> With simulated usb_hub_create_port_device() failure I have an Oops
> in hub_power_on().

I think this patch is correct.  However, we also should change the 
other places that refer to bNbrPorts; make them use maxchild instead.

> Another possible solution is allowing for uninitialized ports and
> checking for hub->ports[i] == NULL.

No, if we can't allocate memory for all the ports then the whole thing 
should fail.

Alan Stern

Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Krzysztof Mazur]

On Tue, Aug 20, 2013 at 02:18:57PM -0400, Alan Stern wrote:
> On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
> 
> > Ignoring usb_hub_create_port_device() errors cause later NULL pointer
> > deference when uninitialized hub->ports[i] entries are dereferenced
> > after port memory allocation error.
> > 
> > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > ---
> > I'm not sure if failing in that case is a good idea, but other solutions
> > are more complex.
> > 
> > I tried also changing hdev->maxchild and skiping initialization
> > of later ports, but it didn't work because in some cases
> > hub->descriptor->bNbrPorts is used instead of hdev->maxchild.
> > With simulated usb_hub_create_port_device() failure I have an Oops
> > in hub_power_on().
> 
> I think this patch is correct.  However, we also should change the 
> other places that refer to bNbrPorts; make them use maxchild instead.

Ok, I can try to catch all such cases and fix problems with
maxchild < bNBrPorts, but I think that changing bNBrPorts to maxchild
might be not enough. For instance:

	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
		if (hub->ports[port1 - 1]->power_is_on)
			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
		else
			usb_clear_port_feature(hub->hdev, port1,
						USB_PORT_FEAT_POWER);

in hub_power_on() should be probably changed to something like:
	
	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
		if (hub->ports[port1 - 1] && hub->ports[port1 - 1]->power_is_on)
			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
		else
			usb_clear_port_feature(hub->hdev, port1,
						USB_PORT_FEAT_POWER);

to disable uninitialized ports.

> 
> > Another possible solution is allowing for uninitialized ports and
> > checking for hub->ports[i] == NULL.
> 
> No, if we can't allocate memory for all the ports then the whole thing 
> should fail.

The usb_hub_create_port_device() might fail also in case of device_add()
error.

> 
> Alan Stern

Thanks,
Krzysiek

Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Alan Stern]

On Tue, 20 Aug 2013, Krzysztof Mazur wrote:

> On Tue, Aug 20, 2013 at 02:18:57PM -0400, Alan Stern wrote:
> > On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
> > 
> > > Ignoring usb_hub_create_port_device() errors cause later NULL pointer
> > > deference when uninitialized hub->ports[i] entries are dereferenced
> > > after port memory allocation error.
> > > 
> > > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > > ---
> > > I'm not sure if failing in that case is a good idea, but other solutions
> > > are more complex.
> > > 
> > > I tried also changing hdev->maxchild and skiping initialization
> > > of later ports, but it didn't work because in some cases
> > > hub->descriptor->bNbrPorts is used instead of hdev->maxchild.
> > > With simulated usb_hub_create_port_device() failure I have an Oops
> > > in hub_power_on().
> > 
> > I think this patch is correct.  However, we also should change the 
> > other places that refer to bNbrPorts; make them use maxchild instead.
> 
> Ok, I can try to catch all such cases and fix problems with
> maxchild < bNBrPorts, but I think that changing bNBrPorts to maxchild
> might be not enough. For instance:

We should never have maxchild < bNbrPorts (unless maxchild is 0).  But 
just in case we do, changing the code is a good idea.

Besides, "hub->maxchild" is shorter and easier to read than
"hub->descriptor->bNbrPorts".  :-)

> 	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> 		if (hub->ports[port1 - 1]->power_is_on)
> 			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
> 		else
> 			usb_clear_port_feature(hub->hdev, port1,
> 						USB_PORT_FEAT_POWER);
> 
> in hub_power_on() should be probably changed to something like:
> 	
> 	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> 		if (hub->ports[port1 - 1] && hub->ports[port1 - 1]->power_is_on)
> 			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
> 		else
> 			usb_clear_port_feature(hub->hdev, port1,
> 						USB_PORT_FEAT_POWER);
> 
> to disable uninitialized ports.

No.  The ports above maxchild aren't merely uninitialized; they are 
completely unused.  We should ignore them altogether.  Just replace 
descriptor->bNbrPorts with maxchild.

> > > Another possible solution is allowing for uninitialized ports and
> > > checking for hub->ports[i] == NULL.
> > 
> > No, if we can't allocate memory for all the ports then the whole thing 
> > should fail.
> 
> The usb_hub_create_port_device() might fail also in case of device_add()
> error.

The main reason for errors in device_add() is memory allocation 
failure.  

Regardless, if the port structures can't be initialized then let's 
fail.

Alan Stern

Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Krzysztof Mazur]

On Tue, Aug 20, 2013 at 03:50:37PM -0400, Alan Stern wrote:
> On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
> 
> > On Tue, Aug 20, 2013 at 02:18:57PM -0400, Alan Stern wrote:
> > > On Tue, 20 Aug 2013, Krzysztof Mazur wrote:
> > > 
> > > > Ignoring usb_hub_create_port_device() errors cause later NULL pointer
> > > > deference when uninitialized hub->ports[i] entries are dereferenced
> > > > after port memory allocation error.
> > > > 
> > > > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > > > ---
> > > > I'm not sure if failing in that case is a good idea, but other solutions
> > > > are more complex.
> > > > 
> > > > I tried also changing hdev->maxchild and skiping initialization
> > > > of later ports, but it didn't work because in some cases
> > > > hub->descriptor->bNbrPorts is used instead of hdev->maxchild.
> > > > With simulated usb_hub_create_port_device() failure I have an Oops
> > > > in hub_power_on().
> > > 
> > > I think this patch is correct.  However, we also should change the 
> > > other places that refer to bNbrPorts; make them use maxchild instead.
> > 
> > Ok, I can try to catch all such cases and fix problems with
> > maxchild < bNBrPorts, but I think that changing bNBrPorts to maxchild
> > might be not enough. For instance:
> 
> We should never have maxchild < bNbrPorts (unless maxchild is 0).  But 
> just in case we do, changing the code is a good idea.
> 
> Besides, "hub->maxchild" is shorter and easier to read than
> "hub->descriptor->bNbrPorts".  :-)
> 

It's "hub->hdev->maxchild", but it's still shorter and
in many places it's just "hdev->maxchild" :)

I tested that with maxchild < bNbrPorts and there are no NULL pointer
dereferences anymore.

Krzysiek
-- >8 --
Subject: [PATCH 3/2] usb: don't use bNbrPorts after initialization

After successful initialization hub->descriptor->bNbrPorts and
hub->hdev->maxchild are equal, but using hub->hdev->maxchild is
preferred because that value is explicitly used for initialization
of hub->ports[].

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 292ffa8..86e353f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -451,7 +451,7 @@ static void led_work (struct work_struct *work)
 	if (hdev->state != USB_STATE_CONFIGURED || hub->quiescing)
 		return;
 
-	for (i = 0; i < hub->descriptor->bNbrPorts; i++) {
+	for (i = 0; i < hdev->maxchild; i++) {
 		unsigned	selector, mode;
 
 		/* 30%-50% duty cycle */
@@ -500,7 +500,7 @@ static void led_work (struct work_struct *work)
 	}
 	if (!changed && blinkenlights) {
 		cursor++;
-		cursor %= hub->descriptor->bNbrPorts;
+		cursor %= hdev->maxchild;
 		set_port_led(hub, cursor + 1, HUB_LED_GREEN);
 		hub->indicator[cursor] = INDICATOR_CYCLE;
 		changed++;
@@ -826,7 +826,7 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
 	else
 		dev_dbg(hub->intfdev, "trying to enable port power on "
 				"non-switchable hub\n");
-	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
+	for (port1 = 1; port1 <= hub->hdev->maxchild; port1++)
 		if (hub->ports[port1 - 1]->power_is_on)
 			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
 		else
@@ -4655,9 +4655,7 @@ static void hub_events(void)
 		hub_dev = hub->intfdev;
 		intf = to_usb_interface(hub_dev);
 		dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
-				hdev->state, hub->descriptor
-					? hub->descriptor->bNbrPorts
-					: 0,
+				hdev->state, hdev->maxchild,
 				/* NOTE: expects max 15 ports... */
 				(u16) hub->change_bits[0],
 				(u16) hub->event_bits[0]);
@@ -4702,7 +4700,7 @@ static void hub_events(void)
 		}
 
 		/* deal with port status changes */
-		for (i = 1; i <= hub->descriptor->bNbrPorts; i++) {
+		for (i = 1; i <= hdev->maxchild; i++) {
 			if (test_bit(i, hub->busy_bits))
 				continue;
 			connect_change = test_bit(i, hub->change_bits);
-- 
1.8.4.rc4.527.g303b16c

Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Alan Stern]

On Wed, 21 Aug 2013, Krzysztof Mazur wrote:

> > We should never have maxchild < bNbrPorts (unless maxchild is 0).  But 
> > just in case we do, changing the code is a good idea.
> > 
> > Besides, "hub->maxchild" is shorter and easier to read than
> > "hub->descriptor->bNbrPorts".  :-)
> > 
> 
> It's "hub->hdev->maxchild", but it's still shorter and
> in many places it's just "hdev->maxchild" :)
> 
> I tested that with maxchild < bNbrPorts and there are no NULL pointer
> dereferences anymore.
> 
> Krzysiek
> -- >8 --
> Subject: [PATCH 3/2] usb: don't use bNbrPorts after initialization
> 
> After successful initialization hub->descriptor->bNbrPorts and
> hub->hdev->maxchild are equal, but using hub->hdev->maxchild is
> preferred because that value is explicitly used for initialization
> of hub->ports[].
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
>  drivers/usb/core/hub.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 292ffa8..86e353f 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -451,7 +451,7 @@ static void led_work (struct work_struct *work)
>  	if (hdev->state != USB_STATE_CONFIGURED || hub->quiescing)
>  		return;
>  
> -	for (i = 0; i < hub->descriptor->bNbrPorts; i++) {
> +	for (i = 0; i < hdev->maxchild; i++) {
>  		unsigned	selector, mode;
>  
>  		/* 30%-50% duty cycle */
> @@ -500,7 +500,7 @@ static void led_work (struct work_struct *work)
>  	}
>  	if (!changed && blinkenlights) {
>  		cursor++;
> -		cursor %= hub->descriptor->bNbrPorts;
> +		cursor %= hdev->maxchild;
>  		set_port_led(hub, cursor + 1, HUB_LED_GREEN);
>  		hub->indicator[cursor] = INDICATOR_CYCLE;
>  		changed++;
> @@ -826,7 +826,7 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
>  	else
>  		dev_dbg(hub->intfdev, "trying to enable port power on "
>  				"non-switchable hub\n");
> -	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> +	for (port1 = 1; port1 <= hub->hdev->maxchild; port1++)
>  		if (hub->ports[port1 - 1]->power_is_on)
>  			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
>  		else
> @@ -4655,9 +4655,7 @@ static void hub_events(void)
>  		hub_dev = hub->intfdev;
>  		intf = to_usb_interface(hub_dev);
>  		dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
> -				hdev->state, hub->descriptor
> -					? hub->descriptor->bNbrPorts
> -					: 0,
> +				hdev->state, hdev->maxchild,
>  				/* NOTE: expects max 15 ports... */
>  				(u16) hub->change_bits[0],
>  				(u16) hub->event_bits[0]);
> @@ -4702,7 +4700,7 @@ static void hub_events(void)
>  		}
>  
>  		/* deal with port status changes */
> -		for (i = 1; i <= hub->descriptor->bNbrPorts; i++) {
> +		for (i = 1; i <= hdev->maxchild; i++) {
>  			if (test_bit(i, hub->busy_bits))
>  				continue;
>  			connect_change = test_bit(i, hub->change_bits);
> 

This is good.  Combined with your other changes, it should fix all 
the problems you described.  You can add my Acked-by to these patches 
now.

Alan Stern