On Fri, 24 May 2013, Sarah Sharp wrote:
> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>
> usb_hub_to_struct_hub() can return NULL in the unlikely cases a hub
> without active configuration, or a hub without ports is found.
I think the proper way to fix the last part is to prevent the hub
driver from binding to a hub with no ports. Then it will be guaranteed
that usb_hub_to_struct_hub() will never return NULL whenever the hub
driver is bound to the device in question.
Even now, we are guaranteed that the return value will not be NULL if
the device has any child USB devices or any ports (i.e., if its
maxchild value is larger than 0). Therefore almost all of the checks
added in this patch are not necessary.
> Even if unlikely we need to handle those cases properly.
However, we don't have to if they are not just unlikely but impossible.
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -730,7 +730,12 @@ int usb_hub_set_port_power(struct usb_device *hdev, int port1,
> {
> int ret;
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> - struct usb_port *port_dev = hub->ports[port1 - 1];
> + struct usb_port *port_dev;
> +
> + if (!hub)
> + return -EINVAL;
This routine is called from only two places, and in both places it is
known that the return value isn't NULL. On the other hand, it might
make more sense to pass the hub pointer as an additional argument to
usb_hub_set_port_power(), so the issue doesn't arise at all.
> @@ -964,6 +969,10 @@ int usb_remove_device(struct usb_device *udev)
> if (!udev->parent) /* Can't remove a root hub */
> return -EINVAL;
> hub = usb_hub_to_struct_hub(udev->parent);
> +
> + if (!hub)
> + return -EINVAL;
Not needed since we know there is a child device.
> @@ -1733,6 +1742,9 @@ hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data)
> struct usb_device *hdev = interface_to_usbdev (intf);
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
>
> + if (!hub)
> + return -EINVAL;
This should be fixed by not binding to hubs with no ports.
> @@ -1769,15 +1781,17 @@ hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data)
> static int find_port_owner(struct usb_device *hdev, unsigned port1,
> struct dev_state ***ppowner)
> {
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> +
> if (hdev->state == USB_STATE_NOTATTACHED)
> return -ENODEV;
> - if (port1 == 0 || port1 > hdev->maxchild)
> + if (port1 == 0 || port1 > hdev->maxchild || !hub)
> return -EINVAL;
The test is not needed since the hdev->maxchild cannot be larger than 0
if the hub driver isn't bound.
> /* This assumes that devices not managed by the hub driver
> * will always have maxchild equal to 0.
> */
> - *ppowner = &(usb_hub_to_struct_hub(hdev)->ports[port1 - 1]->port_owner);
> + *ppowner = &(hub->ports[port1 - 1]->port_owner);
This change is okay. The comment should be updated as well, since the
fact it refers to is not an assumption but is guaranteed.
Most of the remaining changes in this patch aren't needed, for reasons
already mentioned.
> @@ -5323,7 +5367,8 @@ void usb_set_hub_port_connect_type(struct usb_device *hdev, int port1,
> {
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
>
> - hub->ports[port1 - 1]->connect_type = type;
> + if (hub)
> + hub->ports[port1 - 1]->connect_type = type;
> }
>
> /**
> @@ -5339,6 +5384,9 @@ usb_get_hub_port_connect_type(struct usb_device *hdev, int port1)
> {
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
>
> + if (!hub)
> + return USB_PORT_CONNECT_TYPE_UNKNOWN;
> +
> return hub->ports[port1 - 1]->connect_type;
> }
>
> @@ -5397,6 +5445,9 @@ acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev,
> {
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
>
> + if (!hub)
> + return NULL;
> +
> return DEVICE_ACPI_HANDLE(&hub->ports[port1 - 1]->dev);
> }
These three changes probably aren't needed either, but verifying it is
a little more complicated since the callers are in a different source
file. I guess it won't hurt to keep them.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
