On Thu, 30 May 2013, Hans de Goede wrote:
> Hi,
>
> On 05/30/2013 04:51 PM, Alan Stern wrote:
> > On Thu, 30 May 2013, Hans de Goede wrote:
> >
> >> While reading the config parsing code I noticed this check is missing, without
> >> this check config->desc.wTotalLength can end up with a value larger then the
> >> dev->rawdescriptors length for the config, and when userspace then tries to
> >> get the rawdescriptors bad things may happen.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> drivers/usb/core/config.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> >> index 7199adc..a6b2cab 100644
> >> --- a/drivers/usb/core/config.c
> >> +++ b/drivers/usb/core/config.c
> >> @@ -424,7 +424,8 @@ static int usb_parse_configuration(struct usb_device *dev, int cfgidx,
> >>
> >> memcpy(&config->desc, buffer, USB_DT_CONFIG_SIZE);
> >> if (config->desc.bDescriptorType != USB_DT_CONFIG ||
> >> - config->desc.bLength < USB_DT_CONFIG_SIZE) {
> >> + config->desc.bLength < USB_DT_CONFIG_SIZE ||
> >> + config->desc.bLength > size) {
> >> dev_err(ddev, "invalid descriptor for config index %d: "
> >> "type = 0x%X, length = %d\n", cfgidx,
> >> config->desc.bDescriptorType, config->desc.bLength);
> >
> > Another thing that should be changed along with this is in line 710:
> >
> > } else if (result < 4) {
> >
> > really should be
> >
> > } else if (result != USB_DT_CONFIG_SIZE) {
> >
> > Otherwise we may read a garbage value for desc->wTotalLength.
>
> Erm, no wTotalLength is byte 3 and 4, so the original check covers it.
Oh, you're right. I must have had that in mind when going through this
code years ago, and then forgot about it. :-)
> I actually had a patch such as you suggested, but then thought it would
> be better to leave things as is, so as to avoid regressions, in case
> some crazy device has result >= 4 && result < USB_DT_CONFIG_SIZE on
> the first (header only) usb_get_descriptor call.
>
> For the second (full) usb_get_descriptor call, my patch already ensures
> that result >= USB_DT_CONFIG_SIZE.
Yes, okay. This is fine then.
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
