Re: [Patch] MdeModulePkg/UsbMassStorageDxe: Verify Get Max LUN value

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Star,

I may drop this patch.  The range check is valid, but 
may want to change to an error condition instead of 
forcing to 0 if out of range.

I just found that this device is returning different
values for the max LUN.  I saw examples today of a
single LUN device returning 4.  This appears to be due to 
the request for max LUN returning a buffer with 0
DataLength, and the 1 byte data buffer is being returned
with random data.

I believe the root cause is a USB Control Transfer
with Direction of EfiUsbDataIn and the length of 
data written is less than the amount of data requested.

In this case 1 byte is requested, and 0 bytes are written.

The USB I/O Protocol ControlTransfer() API has a DataLength
parameter as an IN, but there is no method to know how 
much data was written.  The USB HC Protocol ControlTransfer()
API has DataLength as an IN/OUT, so the amount of data 
written is known at the HC level.

I am thinking the USB HC ControlTransfer() API needs to
return an error if the amount of data written is less than
requested.

I will work on a new patch with this approach.

Best regards,

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 13, 2017 4:48 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [Patch] MdeModulePkg/UsbMassStorageDxe:
> Verify Get Max LUN value
> 
> That is fine.
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> Thanks,
> Star
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Tuesday, November 14, 2017 1:08 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-
> devel@lists.01.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [Patch] MdeModulePkg/UsbMassStorageDxe:
> Verify Get Max LUN value
> 
> Star,
> 
> I do not recall the exact value I saw.
> 
> However, the spec states that one byte is returned with
> the max LUN value.  It does not state that the byte is
> composed of bitfields with the lower bits being the max
> LUN, so I prefer the value check instead if the and mask.
> 
> Thanks,
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org]
> > On Behalf Of Zeng, Star
> > Sent: Monday, November 13, 2017 2:03 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> > devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> > Subject: Re: [edk2] [Patch]
> > MdeModulePkg/UsbMassStorageDxe: Verify Get Max LUN
> value
> >
> > Mike,
> >
> > Just curious about what is the value returned for
> MaxLun in the case
> > you met before this patch?
> >
> > Could *MaxLun  &= USB_BOT_MAX_LUN work?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Monday, November 13, 2017 5:45 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> > devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> > Subject: RE: [Patch] MdeModulePkg/UsbMassStorageDxe:
> > Verify Get Max LUN value
> >
> > Reviewed-by: Star Zeng <star.zeng@intel.com>
> >
> > How about also adding the compatibility info(now in
> commit log) to the
> > code comments?
> > For example,
> >
> > +    //
> > +    // If MaxLun is larger than the maximum LUN value
> > (0x0f) supported by the
> > +    // USB Mass Storage Class Bulk-Only Transport
> Spec,
> > then set MaxLun to 0
> > +    // which means no LUN is associated with the
> device.
> > +    // It improves compatibility with USB FLASH drives
> > that have a single LUN,
> > +    // but return invalid maximum LUN values.
> > +    //
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Thursday, November 9, 2017 6:19 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> > Subject: [Patch] MdeModulePkg/UsbMassStorageDxe: Verify
> Get Max LUN
> > value
> >
> > The USB Mass Storage Class Specification states that a
> maximum LUN
> > value larger than 0x0F is invalid.  Add a check to make
> sure this
> > maximum LUN value is in this valid range, and if it is
> not, then
> > assume that the device does not support multiple LUNs
> and return a
> > maximum LUN value of 0.
> >
> > This change improves compatibility with USB FLASH
> drives that have a
> > single LUN, but return invalid maximum LUN values.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Michael D Kinney
> > <michael.d.kinney@intel.com>
> > ---
> >  MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c |
> 10
> > +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
> > b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
> > index 4bb7222b89..c7436cf036 100644
> > ---
> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
> > +++
> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
> > @@ -2,7 +2,7 @@
> >    Implementation of the USB mass storage Bulk-Only
> Transport
> > protocol,
> >    according to USB Mass Storage Class Bulk-Only
> Transport, Revision
> > 1.0.
> >
> > -Copyright (c) 2007 - 2011, Intel Corporation. All
> rights
> > reserved.<BR>
> > +Copyright (c) 2007 - 2017, Intel Corporation. All
> rights
> > reserved.<BR>
> >  This program and the accompanying materials  are
> licensed and made
> > available under the terms and conditions of the BSD
> License  which
> > accompanies this distribution.  The full text of the
> license may be
> > found at @@ -576,6 +576,14 @@ UsbBotGetMaxLun (
> >                              1,
> >                              &Result
> >                              );
> > +  if (!EFI_ERROR (Status) && *MaxLun >
> USB_BOT_MAX_LUN)
> > {
> > +    //
> > +    // If MaxLun is larger than the maximum LUN value
> > (0x0f) supported by the
> > +    // USB Mass Storage Class Bulk-Only Transport
> Spec,
> > then set MaxLun to 0
> > +    // which means no LUN is associated with the
> device.
> > +    //
> > +    *MaxLun = 0;
> > +  }
> >
> >    return Status;
> >  }
> > --
> > 2.14.2.windows.3
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel