[Patch V2 0/2] Add Max LUN status/value checks

[Patch V2 0/2] Add Max LUN status/value checks

[Michael D Kinney]

https://bugzilla.tianocore.org/show_bug.cgi?id=767

Add error check to USB I/O Protocol UsbControlTransfer() for the
number of bytes actually transfered.  If less than requested, then
return EFI_DEVICE_ERROR.

Check Get Max LUN status/value in USB Mass Storage Driver to handle
cases where USB device does not support Get Max LUN command or
returned an invalud Max LUN value.

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>

Michael D Kinney (2):
  MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check
  MdeModulePkg/UsbMassStorageDxe: Check Get Max LUN status/value

 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c             |  8 +++++++-
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.14.2.windows.3

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

[Michael D Kinney]

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

[Patch V2 2/2] MdeModulePkg/UsbMassStorageDxe: Check Get Max LUN status/value

[Michael D Kinney]

https://bugzilla.tianocore.org/show_bug.cgi?id=767

If a USB Mass Storage device does not support the Get
Max LUN command, then the USB I/O Protocol ControlTransfer()
service may return an error.  If an error is returned for
this command, then assume that the device does not support
multiple LUNs and return a maximum LUN value of 0.

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 o not support the Get Max LUN command or return
an invalid maximum LUN value.

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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
index 4bb7222b89..6afe2ae235 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,18 @@ UsbBotGetMaxLun (
                             1,
                             &Result
                             );
+  if (EFI_ERROR (Status) || *MaxLun > USB_BOT_MAX_LUN) {
+    //
+    // If the Get LUN request returns an error or the 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.
+    //
+    // This improves compatibility with USB FLASH drives that have a single LUN
+    // and either do not return a max LUN value or return an invalid maximum LUN
+    // value.
+    //
+    *MaxLun = 0;
+  }
 
   return Status;
 }
-- 
2.14.2.windows.3

Re: [Patch V2 2/2] MdeModulePkg/UsbMassStorageDxe: Check Get Max LUN status/value

[Zeng, Star]

Mike,

With a minor typo "that o not" fixed to "that do not" in the commit log, Reviewed-by: Star Zeng <star.zeng@intel.com>

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 2/2] MdeModulePkg/UsbMassStorageDxe: Check Get Max LUN status/value

https://bugzilla.tianocore.org/show_bug.cgi?id=767

If a USB Mass Storage device does not support the Get Max LUN command, then the USB I/O Protocol ControlTransfer() service may return an error.  If an error is returned for this command, then assume that the device does not support multiple LUNs and return a maximum LUN value of 0.

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 o not support the Get Max LUN command or return an invalid maximum LUN value.

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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
index 4bb7222b89..6afe2ae235 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,18 @@ UsbBotGetMaxLun (
                             1,
                             &Result
                             );
+  if (EFI_ERROR (Status) || *MaxLun > USB_BOT_MAX_LUN) {
+    //
+    // If the Get LUN request returns an error or the 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.
+    //
+    // This improves compatibility with USB FLASH drives that have a single LUN
+    // and either do not return a max LUN value or return an invalid maximum LUN
+    // value.
+    //
+    *MaxLun = 0;
+  }
 
   return Status;
 }
--
2.14.2.windows.3

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

[Zeng, Star]

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

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

[Kinney, Michael D]

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

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

[Zeng, Star]

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

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

[Kinney, Michael D]

Star,

I am curious.  Which spec statement are you referring to?

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:59 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,
> 
> 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

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

[Zeng, Star]

Mike,

UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in the description.

Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Wednesday, November 15, 2017 12:00 PM
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,

I am curious.  Which spec statement are you referring to?

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:59 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,
> 
> 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

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

[Kinney, Michael D]

Star,

I have read that section, and I do not see anything
that says the DataLength parameter can not be read.

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 8:23 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,
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in
> the description.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 12:00 PM
> 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,
> 
> I am curious.  Which spec statement are you referring
> to?
> 
> Mike
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:59 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,
> >
> > 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

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

[Zeng, Star]

Mike,

Yes, the DataLength could be read. And Data/DataLength is optional when Direction is EfiUsbNoData per my understanding.

  IN  OUT VOID                *Data,      OPTIONAL
  IN  UINTN                   DataLength, OPTIONAL


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Thursday, November 16, 2017 12:44 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,

I have read that section, and I do not see anything that says the DataLength parameter can not be read.

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 8:23 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,
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in the 
> description.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 12:00 PM
> 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,
> 
> I am curious.  Which spec statement are you referring to?
> 
> Mike
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:59 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,
> >
> > 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

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

[Kinney, Michael D]

Star,

I agree they are optional.  The description says
DataLength should be 0 and Data should be NULL for
the EfiUsbNoData case.  That is why I did not think
Direction needed to be evaluated if DataLength must
be 0 in this case.

In fact, the Host Controller implementations return
an error if this is not the case:

  if ((TransferDirection == EfiUsbNoData) &&
      ((Data != NULL) || (*DataLength != 0))) {
    return EFI_INVALID_PARAMETER;
  }

So I think the logic in the current patch is safe.

However, since there are questions on this topic,
I agree I can update the logic to only check for
short data if Direction is not EfiUsbNoData.  That
should help reduce questions in future reviews of
this function.  I will also add more comments.

Thanks for the feedback.

Mike
 
> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, November 15, 2017 6:26 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,
> 
> Yes, the DataLength could be read. And Data/DataLength
> is optional when Direction is EfiUsbNoData per my
> understanding.
> 
>   IN  OUT VOID                *Data,      OPTIONAL
>   IN  UINTN                   DataLength, OPTIONAL
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, November 16, 2017 12:44 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,
> 
> I have read that section, and I do not see anything
> that says the DataLength parameter can not be read.
> 
> Mike
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 8:23 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,
> >
> > UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(),
> in the
> > description.
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 12:00 PM
> > 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,
> >
> > I am curious.  Which spec statement are you referring
> to?
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Tuesday, November 14, 2017 6:59 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,
> > >
> > > 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