* [PATCH 3/6] USB: hub: fix SS hub-descriptor handling [not found] <20170510125056.29155-1-johan@kernel.org> @ 2017-05-10 12:50 ` Johan Hovold 2017-05-10 14:04 ` Alan Stern 2017-05-10 12:50 ` [PATCH 4/6] USB: hub: fix non-SS " Johan Hovold 1 sibling, 1 reply; 10+ messages in thread From: Johan Hovold @ 2017-05-10 12:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alan Stern, Felipe Balbi, Mathias Nyman, linux-usb, Johan Hovold, stable, John Youn A SuperSpeed hub descriptor does not have any variable-length fields so bail out when reading a short descriptor. This avoids parsing and leaking two bytes of uninitialised slab data through sysfs removable-attributes. Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes") Cc: stable <stable@vger.kernel.org> # 2.6.39 Cc: John Youn <John.Youn@synopsys.com> Signed-off-by: Johan Hovold <johan@kernel.org> --- 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 9dca59ef18b3..3ff1e9f89f2d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, dtype << 8, 0, data, size, USB_CTRL_GET_TIMEOUT); - if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) + if (hub_is_superspeed(hdev)) { + if (ret == size) + return ret; + } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { return ret; + } } return -EINVAL; } @@ -1321,7 +1325,7 @@ static int hub_configure(struct usb_hub *hub, /* Request the entire hub descriptor. * hub->descriptor can handle USB_MAXCHILDREN ports, - * but the hub can/will return fewer bytes here. + * but a (non-SS) hub can/will return fewer bytes here. */ ret = get_hub_descriptor(hdev, hub->descriptor); if (ret < 0) { -- 2.13.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling 2017-05-10 12:50 ` [PATCH 3/6] USB: hub: fix SS hub-descriptor handling Johan Hovold @ 2017-05-10 14:04 ` Alan Stern 2017-05-10 14:15 ` Johan Hovold 0 siblings, 1 reply; 10+ messages in thread From: Alan Stern @ 2017-05-10 14:04 UTC (permalink / raw) To: Johan Hovold Cc: Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable, John Youn On Wed, 10 May 2017, Johan Hovold wrote: > A SuperSpeed hub descriptor does not have any variable-length fields so > bail out when reading a short descriptor. You mean: bail out when reading a descriptor that is not exactly the right length. The existing code already bails out when it reads a short descriptor. Alan Stern > This avoids parsing and leaking two bytes of uninitialised slab data > through sysfs removable-attributes. > > Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes") > Cc: stable <stable@vger.kernel.org> # 2.6.39 > Cc: John Youn <John.Youn@synopsys.com> > Signed-off-by: Johan Hovold <johan@kernel.org> > --- > 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 9dca59ef18b3..3ff1e9f89f2d 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > dtype << 8, 0, data, size, > USB_CTRL_GET_TIMEOUT); > - if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) > + if (hub_is_superspeed(hdev)) { > + if (ret == size) > + return ret; > + } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > return ret; > + } > } > return -EINVAL; > } > @@ -1321,7 +1325,7 @@ static int hub_configure(struct usb_hub *hub, > > /* Request the entire hub descriptor. > * hub->descriptor can handle USB_MAXCHILDREN ports, > - * but the hub can/will return fewer bytes here. > + * but a (non-SS) hub can/will return fewer bytes here. > */ > ret = get_hub_descriptor(hdev, hub->descriptor); > if (ret < 0) { > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling 2017-05-10 14:04 ` Alan Stern @ 2017-05-10 14:15 ` Johan Hovold 2017-05-10 14:28 ` Alan Stern 0 siblings, 1 reply; 10+ messages in thread From: Johan Hovold @ 2017-05-10 14:15 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable, John Youn On Wed, May 10, 2017 at 10:04:32AM -0400, Alan Stern wrote: > On Wed, 10 May 2017, Johan Hovold wrote: > > > A SuperSpeed hub descriptor does not have any variable-length fields so > > bail out when reading a short descriptor. > > You mean: bail out when reading a descriptor that is not exactly the > right length. The existing code already bails out when it reads a > short descriptor. No, the current code happily accepts a 9-byte descriptor, while an SS descriptor is always 12 bytes. And since we request 12 bytes for SS hubs, the patch description is correct. > > This avoids parsing and leaking two bytes of uninitialised slab data > > through sysfs removable-attributes. > > > > Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes") > > Cc: stable <stable@vger.kernel.org> # 2.6.39 > > Cc: John Youn <John.Youn@synopsys.com> > > Signed-off-by: Johan Hovold <johan@kernel.org> > > --- > > 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 9dca59ef18b3..3ff1e9f89f2d 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > dtype << 8, 0, data, size, > > USB_CTRL_GET_TIMEOUT); > > - if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) > > + if (hub_is_superspeed(hdev)) { > > + if (ret == size) > > + return ret; > > + } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > return ret; > > + } > > } > > return -EINVAL; > > } Thanks, Johan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling 2017-05-10 14:15 ` Johan Hovold @ 2017-05-10 14:28 ` Alan Stern 0 siblings, 0 replies; 10+ messages in thread From: Alan Stern @ 2017-05-10 14:28 UTC (permalink / raw) To: Johan Hovold Cc: Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable, John Youn On Wed, 10 May 2017, Johan Hovold wrote: > On Wed, May 10, 2017 at 10:04:32AM -0400, Alan Stern wrote: > > On Wed, 10 May 2017, Johan Hovold wrote: > > > > > A SuperSpeed hub descriptor does not have any variable-length fields so > > > bail out when reading a short descriptor. > > > > You mean: bail out when reading a descriptor that is not exactly the > > right length. The existing code already bails out when it reads a > > short descriptor. > > No, the current code happily accepts a 9-byte descriptor, while an SS > descriptor is always 12 bytes. And since we request 12 bytes for SS > hubs, the patch description is correct. My mistake; you are right. Acked-by: Alan Stern <stern@rowland.harvard.edu> > > > > This avoids parsing and leaking two bytes of uninitialised slab data > > > through sysfs removable-attributes. > > > > > > Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes") > > > Cc: stable <stable@vger.kernel.org> # 2.6.39 > > > Cc: John Youn <John.Youn@synopsys.com> > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > > --- > > > 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 9dca59ef18b3..3ff1e9f89f2d 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > > dtype << 8, 0, data, size, > > > USB_CTRL_GET_TIMEOUT); > > > - if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) > > > + if (hub_is_superspeed(hdev)) { > > > + if (ret == size) > > > + return ret; > > > + } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > > return ret; > > > + } > > > } > > > return -EINVAL; > > > } > > Thanks, > Johan > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling [not found] <20170510125056.29155-1-johan@kernel.org> 2017-05-10 12:50 ` [PATCH 3/6] USB: hub: fix SS hub-descriptor handling Johan Hovold @ 2017-05-10 12:50 ` Johan Hovold 2017-05-10 14:12 ` Alan Stern 1 sibling, 1 reply; 10+ messages in thread From: Johan Hovold @ 2017-05-10 12:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alan Stern, Felipe Balbi, Mathias Nyman, linux-usb, Johan Hovold, stable Add missing sanity check on the non-SuperSpeed hub-descriptor length in order to avoid parsing and leaking two bytes of uninitialised slab data through sysfs removable-attributes (or a compound-device debug statement). Note that we only make sure that the DeviceRemovable field is always present (and specifically ignore the unused PortPwrCtrlMask field) in order to continue support any hubs with non-compliant descriptors. As a further safeguard, the descriptor buffer is also cleared. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable <stable@vger.kernel.org> # 2.6.12 Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/usb/core/hub.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 3ff1e9f89f2d..f77a4ebde7d5 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev) } /* USB 2.0 spec Section 11.24.4.5 */ -static int get_hub_descriptor(struct usb_device *hdev, void *data) +static int get_hub_descriptor(struct usb_device *hdev, + struct usb_hub_descriptor *desc) { int i, ret, size; unsigned dtype; @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) for (i = 0; i < 3; i++) { ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, - dtype << 8, 0, data, size, + dtype << 8, 0, desc, size, USB_CTRL_GET_TIMEOUT); if (hub_is_superspeed(hdev)) { if (ret == size) return ret; - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { + /* Make sure we have the DeviceRemovable field. */ + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; + if (ret < size) + return -EMSGSIZE; return ret; } } @@ -1317,7 +1322,7 @@ static int hub_configure(struct usb_hub *hub, } mutex_init(&hub->status_mutex); - hub->descriptor = kmalloc(sizeof(*hub->descriptor), GFP_KERNEL); + hub->descriptor = kzalloc(sizeof(*hub->descriptor), GFP_KERNEL); if (!hub->descriptor) { ret = -ENOMEM; goto fail; -- 2.13.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling 2017-05-10 12:50 ` [PATCH 4/6] USB: hub: fix non-SS " Johan Hovold @ 2017-05-10 14:12 ` Alan Stern 2017-05-10 14:31 ` Johan Hovold 0 siblings, 1 reply; 10+ messages in thread From: Alan Stern @ 2017-05-10 14:12 UTC (permalink / raw) To: Johan Hovold Cc: Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable On Wed, 10 May 2017, Johan Hovold wrote: > Add missing sanity check on the non-SuperSpeed hub-descriptor length in > order to avoid parsing and leaking two bytes of uninitialised slab data > through sysfs removable-attributes (or a compound-device debug > statement). > > Note that we only make sure that the DeviceRemovable field is always > present (and specifically ignore the unused PortPwrCtrlMask field) in > order to continue support any hubs with non-compliant descriptors. As a > further safeguard, the descriptor buffer is also cleared. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable <stable@vger.kernel.org> # 2.6.12 > Signed-off-by: Johan Hovold <johan@kernel.org> > --- > drivers/usb/core/hub.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 3ff1e9f89f2d..f77a4ebde7d5 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev) > } > > /* USB 2.0 spec Section 11.24.4.5 */ > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > +static int get_hub_descriptor(struct usb_device *hdev, > + struct usb_hub_descriptor *desc) > { > int i, ret, size; > unsigned dtype; > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > for (i = 0; i < 3; i++) { > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > - dtype << 8, 0, data, size, > + dtype << 8, 0, desc, size, > USB_CTRL_GET_TIMEOUT); > if (hub_is_superspeed(hdev)) { > if (ret == size) > return ret; > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > + /* Make sure we have the DeviceRemovable field. */ > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > + if (ret < size) > + return -EMSGSIZE; The logic could be simplified a little. Since we don't really care about the return code when an error occurs, you could just do: } else if (ret >= USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1) { /* We have the entire DeviceRemovable field. */ return ret; } Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling 2017-05-10 14:12 ` Alan Stern @ 2017-05-10 14:31 ` Johan Hovold 2017-05-10 14:41 ` Johan Hovold 0 siblings, 1 reply; 10+ messages in thread From: Johan Hovold @ 2017-05-10 14:31 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote: > On Wed, 10 May 2017, Johan Hovold wrote: > > > Add missing sanity check on the non-SuperSpeed hub-descriptor length in > > order to avoid parsing and leaking two bytes of uninitialised slab data > > through sysfs removable-attributes (or a compound-device debug > > statement). > > > > Note that we only make sure that the DeviceRemovable field is always > > present (and specifically ignore the unused PortPwrCtrlMask field) in > > order to continue support any hubs with non-compliant descriptors. As a > > further safeguard, the descriptor buffer is also cleared. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Cc: stable <stable@vger.kernel.org> # 2.6.12 > > Signed-off-by: Johan Hovold <johan@kernel.org> > > --- > > drivers/usb/core/hub.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 3ff1e9f89f2d..f77a4ebde7d5 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev) > > } > > > > /* USB 2.0 spec Section 11.24.4.5 */ > > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > > +static int get_hub_descriptor(struct usb_device *hdev, > > + struct usb_hub_descriptor *desc) > > { > > int i, ret, size; > > unsigned dtype; > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > for (i = 0; i < 3; i++) { > > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > - dtype << 8, 0, data, size, > > + dtype << 8, 0, desc, size, > > USB_CTRL_GET_TIMEOUT); > > if (hub_is_superspeed(hdev)) { > > if (ret == size) > > return ret; > > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > > + /* Make sure we have the DeviceRemovable field. */ > > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > > + if (ret < size) > > + return -EMSGSIZE; > > The logic could be simplified a little. Since we don't really care > about the return code when an error occurs, you could just do: > > } else if (ret >= USB_DT_HUB_NONVAR_SIZE + > desc->bNbrPorts / 8 + 1) { > /* We have the entire DeviceRemovable field. */ > return ret; > } Sure, that would work, but I it doesn't feel right to access bNbrPorts without first verifying we got the non-variable fields. I considered dropping the +2 bit, but decided to keep it in the unlikely even that there are quirky devices out there that rely on it (e.g. first read always return 7 bytes). Spelling it out makes it sound overly conservative though. How about I drop that instead? Thanks, Johan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling 2017-05-10 14:31 ` Johan Hovold @ 2017-05-10 14:41 ` Johan Hovold 2017-05-10 15:11 ` Alan Stern 0 siblings, 1 reply; 10+ messages in thread From: Johan Hovold @ 2017-05-10 14:41 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote: > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote: > > On Wed, 10 May 2017, Johan Hovold wrote: > > > /* USB 2.0 spec Section 11.24.4.5 */ > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > +static int get_hub_descriptor(struct usb_device *hdev, > > > + struct usb_hub_descriptor *desc) > > > { > > > int i, ret, size; > > > unsigned dtype; > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > for (i = 0; i < 3; i++) { > > > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > > - dtype << 8, 0, data, size, > > > + dtype << 8, 0, desc, size, > > > USB_CTRL_GET_TIMEOUT); > > > if (hub_is_superspeed(hdev)) { > > > if (ret == size) > > > return ret; > > > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > > > + /* Make sure we have the DeviceRemovable field. */ > > > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > > > + if (ret < size) > > > + return -EMSGSIZE; > > > > The logic could be simplified a little. Since we don't really care > > about the return code when an error occurs, you could just do: > > > > } else if (ret >= USB_DT_HUB_NONVAR_SIZE + > > desc->bNbrPorts / 8 + 1) { > > /* We have the entire DeviceRemovable field. */ > > return ret; > > } > > Sure, that would work, but I it doesn't feel right to access bNbrPorts > without first verifying we got the non-variable fields. > > I considered dropping the +2 bit, but decided to keep it in the unlikely > even that there are quirky devices out there that rely on it (e.g. first > read always return 7 bytes). Spelling it out makes it sound overly > conservative though. How about I drop that instead? Then again, a non-SS hub descriptor is always at least (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes sense anyway. Johan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling 2017-05-10 14:41 ` Johan Hovold @ 2017-05-10 15:11 ` Alan Stern 2017-05-10 16:11 ` Johan Hovold 0 siblings, 1 reply; 10+ messages in thread From: Alan Stern @ 2017-05-10 15:11 UTC (permalink / raw) To: Johan Hovold Cc: Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable On Wed, 10 May 2017, Johan Hovold wrote: > On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote: > > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote: > > > On Wed, 10 May 2017, Johan Hovold wrote: > > > > > /* USB 2.0 spec Section 11.24.4.5 */ > > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > > +static int get_hub_descriptor(struct usb_device *hdev, > > > > + struct usb_hub_descriptor *desc) > > > > { > > > > int i, ret, size; > > > > unsigned dtype; > > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > > for (i = 0; i < 3; i++) { > > > > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > > > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > > > - dtype << 8, 0, data, size, > > > > + dtype << 8, 0, desc, size, > > > > USB_CTRL_GET_TIMEOUT); > > > > if (hub_is_superspeed(hdev)) { > > > > if (ret == size) > > > > return ret; > > > > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > > > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > > > > + /* Make sure we have the DeviceRemovable field. */ > > > > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > > > > + if (ret < size) > > > > + return -EMSGSIZE; > > > > > > The logic could be simplified a little. Since we don't really care > > > about the return code when an error occurs, you could just do: > > > > > > } else if (ret >= USB_DT_HUB_NONVAR_SIZE + > > > desc->bNbrPorts / 8 + 1) { > > > /* We have the entire DeviceRemovable field. */ > > > return ret; > > > } > > > > Sure, that would work, but I it doesn't feel right to access bNbrPorts > > without first verifying we got the non-variable fields. Accessing desc->bNbrPorts won't do any harm, even if it wasn't sent. After all, you now initialize the hub descriptor to all 0's. And even if the field contained garbage, it would just make this test less likely to succeed. > > I considered dropping the +2 bit, but decided to keep it in the unlikely > > even that there are quirky devices out there that rely on it (e.g. first > > read always return 7 bytes). Spelling it out makes it sound overly > > conservative though. How about I drop that instead? > > Then again, a non-SS hub descriptor is always at least > (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes > sense anyway. Personally, I wouldn't worry about it. It's your decision. Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling 2017-05-10 15:11 ` Alan Stern @ 2017-05-10 16:11 ` Johan Hovold 0 siblings, 0 replies; 10+ messages in thread From: Johan Hovold @ 2017-05-10 16:11 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, Greg Kroah-Hartman, Felipe Balbi, Mathias Nyman, linux-usb, stable On Wed, May 10, 2017 at 11:11:37AM -0400, Alan Stern wrote: > On Wed, 10 May 2017, Johan Hovold wrote: > > > On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote: > > > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote: > > > > On Wed, 10 May 2017, Johan Hovold wrote: > > > > > > > /* USB 2.0 spec Section 11.24.4.5 */ > > > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > > > +static int get_hub_descriptor(struct usb_device *hdev, > > > > > + struct usb_hub_descriptor *desc) > > > > > { > > > > > int i, ret, size; > > > > > unsigned dtype; > > > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > > > for (i = 0; i < 3; i++) { > > > > > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > > > > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > > > > - dtype << 8, 0, data, size, > > > > > + dtype << 8, 0, desc, size, > > > > > USB_CTRL_GET_TIMEOUT); > > > > > if (hub_is_superspeed(hdev)) { > > > > > if (ret == size) > > > > > return ret; > > > > > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > > > > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > > > > > + /* Make sure we have the DeviceRemovable field. */ > > > > > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > > > > > + if (ret < size) > > > > > + return -EMSGSIZE; > > > > > > > > The logic could be simplified a little. Since we don't really care > > > > about the return code when an error occurs, you could just do: > > > > > > > > } else if (ret >= USB_DT_HUB_NONVAR_SIZE + > > > > desc->bNbrPorts / 8 + 1) { > > > > /* We have the entire DeviceRemovable field. */ > > > > return ret; > > > > } > > > > > > Sure, that would work, but I it doesn't feel right to access bNbrPorts > > > without first verifying we got the non-variable fields. > > Accessing desc->bNbrPorts won't do any harm, even if it wasn't sent. > After all, you now initialize the hub descriptor to all 0's. And even > if the field contained garbage, it would just make this test less > likely to succeed. I know, but since it's not immediately obvious (and may set a bad example), I think we should avoid it. > > > I considered dropping the +2 bit, but decided to keep it in the unlikely > > > even that there are quirky devices out there that rely on it (e.g. first > > > read always return 7 bytes). Spelling it out makes it sound overly > > > conservative though. How about I drop that instead? > > > > Then again, a non-SS hub descriptor is always at least > > (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes > > sense anyway. > > Personally, I wouldn't worry about it. It's your decision. Now I remember that dropping the +2 would also mean that we start accepting descriptors without a PortPwrCtrlMask field. I'll just leave it in for now. Thanks, Johan ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-10 16:11 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170510125056.29155-1-johan@kernel.org>
2017-05-10 12:50 ` [PATCH 3/6] USB: hub: fix SS hub-descriptor handling Johan Hovold
2017-05-10 14:04 ` Alan Stern
2017-05-10 14:15 ` Johan Hovold
2017-05-10 14:28 ` Alan Stern
2017-05-10 12:50 ` [PATCH 4/6] USB: hub: fix non-SS " Johan Hovold
2017-05-10 14:12 ` Alan Stern
2017-05-10 14:31 ` Johan Hovold
2017-05-10 14:41 ` Johan Hovold
2017-05-10 15:11 ` Alan Stern
2017-05-10 16:11 ` Johan Hovold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).