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

["Zeng, Star" <star.zeng@intel.com>]

Mike,

I do not insist on the check that the Direction is not EfiUsbNoData, and I know the functionality should have no improvement with the check.
So, you can have my Reviewed-by: Star Zeng <star.zeng@intel.com>.

I raised the question just for consideration from literally, as according to the spec, the code could not touch DataLength at all when Direction is EfiUsbNoData.

You can decide to have / have not the check when pushing. :)


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Wednesday, November 15, 2017 10:46 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>
Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

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