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@xxxxxxxxxxxxxxx> # 2.6.12
> > Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> > ---
> > 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