Re: [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 6/25/2018 6:38 PM, Star Zeng wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=973
> 
> HUB descriptor has variable length.
> But the code uses stack (HubDesc in UsbHubInit) with fixed length
> sizeof(EFI_USB_HUB_DESCRIPTOR) to hold HUB descriptor data.
> It uses hard code length value (32 that is greater than
> sizeof(EFI_USB_HUB_DESCRIPTOR)) for SuperSpeed path, then there will
> be stack overflow when IOMMU is enabled because the Unmap operation
> will copy the data from device buffer to host buffer.
> And it uses HubDesc->Length for none SuperSpeed path, then there will
> be stack overflow when HubDesc->Length is greater than
> sizeof(EFI_USB_HUB_DESCRIPTOR).
> 
> The patch updates the code to use a big enough buffer to hold the
> descriptor data.
> The definition EFI_USB_SUPER_SPEED_HUB_DESCRIPTOR is wrong (HubDelay
> field should be UINT16 type) and no code is using it, the patch
> removes it.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Bret Barkelew <bret.barkelew@microsoft.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 96 +++++++++++----------------------
>   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h | 14 +----
>   2 files changed, 32 insertions(+), 78 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
> index fabb44157037..a962f76638e8 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
> @@ -2,7 +2,7 @@
>   
>       Unified interface for RootHub and Hub.
>   
> -Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2018, 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
> @@ -201,42 +201,7 @@ UsbHubCtrlClearTTBuffer (
>   }
>   
>   /**
> -  Usb hub control transfer to get the super speed hub descriptor.
> -
> -  @param  HubDev                The hub device.
> -  @param  Buf                   The buffer to hold the descriptor.
> -
> -  @retval EFI_SUCCESS           The hub descriptor is retrieved.
> -  @retval Others                Failed to retrieve the hub descriptor.
> -
> -**/
> -EFI_STATUS
> -UsbHubCtrlGetSuperSpeedHubDesc (
> -  IN  USB_DEVICE          *HubDev,
> -  OUT VOID                *Buf
> -  )
> -{
> -  EFI_STATUS              Status;
> -
> -  Status = EFI_INVALID_PARAMETER;
> -
> -  Status = UsbCtrlRequest (
> -             HubDev,
> -             EfiUsbDataIn,
> -             USB_REQ_TYPE_CLASS,
> -             USB_HUB_TARGET_HUB,
> -             USB_HUB_REQ_GET_DESC,
> -             (UINT16) (USB_DESC_TYPE_HUB_SUPER_SPEED << 8),
> -             0,
> -             Buf,
> -             32
> -             );
> -
> -  return Status;
> -}
> -
> -/**
> -  Usb hub control transfer to get the hub descriptor.
> +  Usb hub control transfer to get the (super speed) hub descriptor.
>   
>     @param  HubDev                The hub device.
>     @param  Buf                   The buffer to hold the descriptor.
> @@ -254,6 +219,11 @@ UsbHubCtrlGetHubDesc (
>     )
>   {
>     EFI_STATUS              Status;
> +  UINT8                   DescType;
> +
> +  DescType = (HubDev->Speed == EFI_USB_SPEED_SUPER) ?
> +             USB_DESC_TYPE_HUB_SUPER_SPEED :
> +             USB_DESC_TYPE_HUB;
>   
>     Status = UsbCtrlRequest (
>                HubDev,
> @@ -261,7 +231,7 @@ UsbHubCtrlGetHubDesc (
>                USB_REQ_TYPE_CLASS,
>                USB_HUB_TARGET_HUB,
>                USB_HUB_REQ_GET_DESC,
> -             (UINT16) (USB_DESC_TYPE_HUB << 8),
> +             (UINT16) (DescType << 8),
>                0,
>                Buf,
>                Len
> @@ -475,29 +445,19 @@ UsbHubReadDesc (
>   {
>     EFI_STATUS              Status;
>   
> -  if (HubDev->Speed == EFI_USB_SPEED_SUPER) {
> -    //
> -    // Get the super speed hub descriptor
> -    //
> -    Status = UsbHubCtrlGetSuperSpeedHubDesc (HubDev, HubDesc);
> -  } else {
> -
> -    //
> -    // First get the hub descriptor length
> -    //
> -    Status = UsbHubCtrlGetHubDesc (HubDev, HubDesc, 2);
> -
> -    if (EFI_ERROR (Status)) {
> -      return Status;
> -    }
> +  //
> +  // First get the hub descriptor length
> +  //
> +  Status = UsbHubCtrlGetHubDesc (HubDev, HubDesc, 2);
>   
> -    //
> -    // Get the whole hub descriptor
> -    //
> -    Status = UsbHubCtrlGetHubDesc (HubDev, HubDesc, HubDesc->Length);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>     }
>   
> -  return Status;
> +  //
> +  // Get the whole hub descriptor
> +  //
> +  return UsbHubCtrlGetHubDesc (HubDev, HubDesc, HubDesc->Length);
>   }
>   
>   
> @@ -690,7 +650,8 @@ UsbHubInit (
>     IN USB_INTERFACE        *HubIf
>     )
>   {
> -  EFI_USB_HUB_DESCRIPTOR  HubDesc;
> +  UINT8                   HubDescBuffer[256];
> +  EFI_USB_HUB_DESCRIPTOR  *HubDesc;
>     USB_ENDPOINT_DESC       *EpDesc;
>     USB_INTERFACE_SETTING   *Setting;
>     EFI_USB_IO_PROTOCOL     *UsbIo;
> @@ -725,14 +686,19 @@ UsbHubInit (
>       return EFI_DEVICE_ERROR;
>     }
>   
> -  Status = UsbHubReadDesc (HubDev, &HubDesc);
> +  //
> +  // The length field of descriptor is UINT8 type, so the buffer
> +  // with 256 bytes is enough to hold the descriptor data.
> +  //
> +  HubDesc = (EFI_USB_HUB_DESCRIPTOR *) HubDescBuffer;
> +  Status = UsbHubReadDesc (HubDev, HubDesc);
>   
>     if (EFI_ERROR (Status)) {
>       DEBUG (( EFI_D_ERROR, "UsbHubInit: failed to read HUB descriptor %r\n", Status));
>       return Status;
>     }
>   
> -  HubIf->NumOfPort = HubDesc.NumPorts;
> +  HubIf->NumOfPort = HubDesc->NumPorts;
>   
>     DEBUG (( EFI_D_INFO, "UsbHubInit: hub %d has %d ports\n", HubDev->Address,HubIf->NumOfPort));
>   
> @@ -751,7 +717,7 @@ UsbHubInit (
>       DEBUG ((EFI_D_INFO, "UsbHubInit: Set Hub Depth as 0x%x\n", Depth));
>       UsbHubCtrlSetHubDepth (HubIf->Device, Depth);
>       
> -    for (Index = 0; Index < HubDesc.NumPorts; Index++) {
> +    for (Index = 0; Index < HubDesc->NumPorts; Index++) {
>         UsbHubCtrlSetPortFeature (HubIf->Device, Index, USB_HUB_PORT_REMOTE_WAKE_MASK);
>       }
>     } else {
> @@ -759,15 +725,15 @@ UsbHubInit (
>       // Feed power to all the hub ports. It should be ok
>       // for both gang/individual powered hubs.
>       //
> -    for (Index = 0; Index < HubDesc.NumPorts; Index++) {
> +    for (Index = 0; Index < HubDesc->NumPorts; Index++) {
>         UsbHubCtrlSetPortFeature (HubIf->Device, Index, (EFI_USB_PORT_FEATURE) USB_HUB_PORT_POWER);
>       }
>   
>       //
>       // Update for the usb hub has no power on delay requirement
>       //
> -    if (HubDesc.PwrOn2PwrGood > 0) {
> -      gBS->Stall (HubDesc.PwrOn2PwrGood * USB_SET_PORT_POWER_STALL);
> +    if (HubDesc->PwrOn2PwrGood > 0) {
> +      gBS->Stall (HubDesc->PwrOn2PwrGood * USB_SET_PORT_POWER_STALL);
>       }
>       UsbHubAckHubStatus (HubIf->Device);
>     }
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h
> index 4e5fcd85e0af..fe9f1f74c751 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h
> @@ -2,7 +2,7 @@
>   
>       The definition for USB hub.
>   
> -Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2018, 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
> @@ -115,18 +115,6 @@ typedef struct {
>     UINT8           Filler[16];
>   } EFI_USB_HUB_DESCRIPTOR;
>   
> -typedef struct {
> -  UINT8           Length;
> -  UINT8           DescType;
> -  UINT8           NumPorts;
> -  UINT16          HubCharacter;
> -  UINT8           PwrOn2PwrGood;
> -  UINT8           HubContrCurrent;
> -  UINT8           HubHdrDecLat;
> -  UINT8           HubDelay;
> -  UINT8           DeviceRemovable;
> -} EFI_USB_SUPER_SPEED_HUB_DESCRIPTOR;
> -
>   #pragma pack()
>   
>   
> 

How about change structure definition as below?
typedef struct {
   UINT8           Length;
   UINT8           DescType;
   UINT8           NumPorts;
   UINT16          HubCharacter;
   UINT8           PwrOn2PwrGood;
   UINT8           HubContrCurrent;
   UINT8           Filler[248];
} EFI_USB_HUB_DESCRIPTOR;

This can avoid using
 > +  UINT8                   HubDescBuffer[256];

-- 
Thanks,
Ray