Re: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

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

Star,

For that case both DataLength and RequestedDataLength
will be 0 and the new path will not be taken.

Are you concerned that the USB HC Protocol could return
a non zero DataLength for the EfiUsbNoData case?  Even
if it does, the new path can never be taken because 0 is
less than all UINTN values.

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:40 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 V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> Do you think it is better or not to also check the
> Direction is not EfiUsbNoData?
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> If the Direction parameter is EfiUsbNoData, Data is
> NULL, and DataLength is 0, then no data phase exists
> for the control transfer.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 9:03 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=767
> 
> The USB I/O Protocol function ControlTransfer() has a
> DataLength parameter that specifies the size of the
> Data buffer.  The UsbBusDxe module implements the USB
> I/O Protocol using the services of the USB2 Host
> Controller Protocol.  The DataLength parameter in the
> USB2 Host Controller Protocol ControlTransfer() service
> is an IN OUT parameter so the number of bytes actually
> transferred is returned.  Since the USB I/O Protocol
> ControlTransfer() service can not return the number of
> bytes actually transferred, the only option if the
> number of bytes actually transferred is less than the
> number of bytes requested is to return
> EFI_DEVICE_ERROR.
> 
> The change fixes an issue with a USB mass storage
> device that responds with 0 bytes to the Get MAX LUN
> command.
> 
> 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/UsbBusDxe/UsbBus.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index 78220222f6..b0ad7938e9 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -2,7 +2,7 @@
> 
>      Usb Bus Driver Binding and Bus IO Protocol.
> 
> -Copyright (c) 2004 - 2016, Intel Corporation. All
> rights reserved.<BR>
> +Copyright (c) 2004 - 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 @@ -76,6 +76,7 @@ UsbIoControlTransfer (
>    USB_ENDPOINT_DESC       *EpDesc;
>    EFI_TPL                 OldTpl;
>    EFI_STATUS              Status;
> +  UINTN                   RequestedDataLength;
> 
>    if (UsbStatus == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -86,6 +87,7 @@ UsbIoControlTransfer (
>    UsbIf  = USB_INTERFACE_FROM_USBIO (This);
>    Dev    = UsbIf->Device;
> 
> +  RequestedDataLength = DataLength;
>    Status = UsbHcControlTransfer (
>               Dev->Bus,
>               Dev->Address,
> @@ -99,6 +101,10 @@ UsbIoControlTransfer (
>               &Dev->Translator,
>               UsbStatus
>               );
> +  if (!EFI_ERROR (Status) && DataLength <
> RequestedDataLength) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto ON_EXIT;
> +  }
> 
>    if (EFI_ERROR (Status) || (*UsbStatus !=
> EFI_USB_NOERROR)) {
>      //
> --
> 2.14.2.windows.3