From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com ([209.85.128.193]:34467 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751955AbdEJOb2 (ORCPT ); Wed, 10 May 2017 10:31:28 -0400 Date: Wed, 10 May 2017 16:31:19 +0200 From: Johan Hovold To: Alan Stern Cc: Johan Hovold , Greg Kroah-Hartman , Felipe Balbi , Mathias Nyman , linux-usb@vger.kernel.org, stable Subject: Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling Message-ID: <20170510143119.GH30445@localhost> References: <20170510125056.29155-5-johan@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: stable-owner@vger.kernel.org List-ID: 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 # 2.6.12 > > Signed-off-by: Johan Hovold > > --- > > 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