From f736e625598eefe3ad1373e227f6d893826f6735 Mon Sep 17 00:00:00 2001
From: Star Zeng <star.zeng@intel.com>
Date: Mon, 25 Jun 2018 16:50:01 +0800
Subject: [PATCH 1/2] MdeModulePkg UsbBusPei: Fix wrong buffer length used to
 read hub desc

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

Bug 973 just mentions UsbBusDxe, but UsbBusPei has similar issue.

HUB descriptor has variable length.
But the code uses stack (HubDescriptor in PeiDoHubConfig) with fixed
length sizeof(EFI_USB_HUB_DESCRIPTOR) to hold HUB descriptor data.
It uses hard code length value (12) for SuperSpeed path.
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.

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>
Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>
(cherry picked from commit 72750e3bf9174f15c17e78f0f117b5e7311bb49f)
---
 MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c | 108 ++++++++---------------
 MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h |   4 +-
 2 files changed, 38 insertions(+), 74 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c
index 16a7b589c1c2..ac930ee8ea00 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c
@@ -1,7 +1,7 @@
 /** @file
 Usb Hub Request Support In PEI Phase
 
-Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
   
 This program and the accompanying materials
 are licensed and made available under the terms and conditions
@@ -276,9 +276,10 @@ PeiHubClearHubFeature (
 }
 
 /**
-  Get a given hub descriptor.
+  Get a given (SuperSpeed) hub descriptor.
 
   @param  PeiServices    General-purpose services that are available to every PEIM.
+  @param  PeiUsbDevice   Indicates the hub controller device.
   @param  UsbIoPpi       Indicates the PEI_USB_IO_PPI instance.
   @param  DescriptorSize The length of Hub Descriptor buffer.
   @param  HubDescriptor  Caller allocated buffer to store the hub descriptor if
@@ -292,21 +293,28 @@ PeiHubClearHubFeature (
 EFI_STATUS
 PeiGetHubDescriptor (
   IN  EFI_PEI_SERVICES          **PeiServices,
+  IN  PEI_USB_DEVICE            *PeiUsbDevice,
   IN  PEI_USB_IO_PPI            *UsbIoPpi,
   IN  UINTN                     DescriptorSize,
   OUT EFI_USB_HUB_DESCRIPTOR    *HubDescriptor
   )
 {
   EFI_USB_DEVICE_REQUEST      DevReq;
+  UINT8                       DescType;
+
   ZeroMem (&DevReq, sizeof (EFI_USB_DEVICE_REQUEST));
 
+  DescType = (PeiUsbDevice->DeviceSpeed == EFI_USB_SPEED_SUPER) ?
+             USB_DT_SUPERSPEED_HUB :
+             USB_DT_HUB;
+
   //
   // Fill Device request packet
   //
   DevReq.RequestType = USB_RT_HUB | 0x80;
   DevReq.Request     = USB_HUB_GET_DESCRIPTOR;
-  DevReq.Value       = USB_DT_HUB << 8;
-  DevReq.Length      = (UINT16)DescriptorSize;
+  DevReq.Value       = (UINT16) (DescType << 8);
+  DevReq.Length      = (UINT16) DescriptorSize;
 
   return  UsbIoPpi->UsbControlTransfer (
                       PeiServices,
@@ -319,48 +327,6 @@ PeiGetHubDescriptor (
                       );
 }
 
-/**
-  Get a given SuperSpeed hub descriptor.
-
-  @param  PeiServices       General-purpose services that are available to every PEIM.
-  @param  UsbIoPpi          Indicates the PEI_USB_IO_PPI instance.
-  @param  HubDescriptor     Caller allocated buffer to store the hub descriptor if
-                            successfully returned.
-
-  @retval EFI_SUCCESS       Hub descriptor is obtained successfully.
-  @retval EFI_DEVICE_ERROR  Cannot get the hub descriptor due to a hardware error.
-  @retval Others            Other failure occurs.
-
-**/
-EFI_STATUS
-PeiGetSuperSpeedHubDesc (
-  IN  EFI_PEI_SERVICES          **PeiServices,
-  IN  PEI_USB_IO_PPI            *UsbIoPpi,
-  OUT EFI_USB_HUB_DESCRIPTOR    *HubDescriptor
-  )
-{
-  EFI_USB_DEVICE_REQUEST        DevReq;
-  ZeroMem (&DevReq, sizeof (EFI_USB_DEVICE_REQUEST));
-
-  //
-  // Fill Device request packet
-  //
-  DevReq.RequestType = USB_RT_HUB | 0x80;
-  DevReq.Request     = USB_HUB_GET_DESCRIPTOR;
-  DevReq.Value       = USB_DT_SUPERSPEED_HUB << 8;
-  DevReq.Length      = 12;
-
-  return  UsbIoPpi->UsbControlTransfer (
-                      PeiServices,
-                      UsbIoPpi,
-                      &DevReq,
-                      EfiUsbDataIn,
-                      PcdGet32 (PcdUsbTransferTimeoutValue),
-                      HubDescriptor,
-                      12
-                      );
-}
-
 /**
   Read the whole usb hub descriptor. It is necessary
   to do it in two steps because hub descriptor is of
@@ -387,29 +353,19 @@ PeiUsbHubReadDesc (
 {
   EFI_STATUS Status;
 
-  if (PeiUsbDevice->DeviceSpeed == EFI_USB_SPEED_SUPER) {
-    //
-    // Get the super speed hub descriptor
-    //
-    Status = PeiGetSuperSpeedHubDesc (PeiServices, UsbIoPpi, HubDescriptor);
-  } else {
+  //
+  // First get the hub descriptor length
+  //
+  Status = PeiGetHubDescriptor (PeiServices, PeiUsbDevice, UsbIoPpi, 2, HubDescriptor);
 
-    //
-    // First get the hub descriptor length
-    //
-    Status = PeiGetHubDescriptor (PeiServices, UsbIoPpi, 2, HubDescriptor);
-
-    if (EFI_ERROR (Status)) {
-      return Status;
-    }
-
-    //
-    // Get the whole hub descriptor
-    //
-    Status = PeiGetHubDescriptor (PeiServices, UsbIoPpi, HubDescriptor->Length, HubDescriptor);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
-  return Status;
+  //
+  // Get the whole hub descriptor
+  //
+  return PeiGetHubDescriptor (PeiServices, PeiUsbDevice, UsbIoPpi, HubDescriptor->Length, HubDescriptor);
 }
 
 /**
@@ -468,15 +424,21 @@ PeiDoHubConfig (
   IN PEI_USB_DEVICE      *PeiUsbDevice
   )
 {
-  EFI_USB_HUB_DESCRIPTOR  HubDescriptor;
+  UINT8                   HubDescBuffer[256];
+  EFI_USB_HUB_DESCRIPTOR  *HubDescriptor;
   EFI_STATUS              Status;
   EFI_USB_HUB_STATUS      HubStatus;
   UINTN                   Index;
   PEI_USB_IO_PPI          *UsbIoPpi;
 
-  ZeroMem (&HubDescriptor, sizeof (HubDescriptor));
   UsbIoPpi = &PeiUsbDevice->UsbIoPpi;
 
+  //
+  // The length field of descriptor is UINT8 type, so the buffer
+  // with 256 bytes is enough to hold the descriptor data.
+  //
+  HubDescriptor = (EFI_USB_HUB_DESCRIPTOR *) HubDescBuffer;
+
   //
   // Get the hub descriptor 
   //
@@ -484,13 +446,13 @@ PeiDoHubConfig (
             PeiServices,
             PeiUsbDevice,
             UsbIoPpi,
-            &HubDescriptor
+            HubDescriptor
             );
   if (EFI_ERROR (Status)) {
     return EFI_DEVICE_ERROR;
   }
 
-  PeiUsbDevice->DownStreamPortNo = HubDescriptor.NbrPorts;
+  PeiUsbDevice->DownStreamPortNo = HubDescriptor->NbrPorts;
 
   if (PeiUsbDevice->DeviceSpeed == EFI_USB_SPEED_SUPER) {
     DEBUG ((EFI_D_INFO, "PeiDoHubConfig: Set Hub Depth as 0x%x\n", PeiUsbDevice->Tier));
@@ -516,9 +478,9 @@ PeiDoHubConfig (
       }
     }
 
-    DEBUG (( EFI_D_INFO, "PeiDoHubConfig: HubDescriptor.PwrOn2PwrGood: 0x%x\n", HubDescriptor.PwrOn2PwrGood));
-    if (HubDescriptor.PwrOn2PwrGood > 0) {
-      MicroSecondDelay (HubDescriptor.PwrOn2PwrGood * USB_SET_PORT_POWER_STALL);
+    DEBUG (( EFI_D_INFO, "PeiDoHubConfig: HubDescriptor.PwrOn2PwrGood: 0x%x\n", HubDescriptor->PwrOn2PwrGood));
+    if (HubDescriptor->PwrOn2PwrGood > 0) {
+      MicroSecondDelay (HubDescriptor->PwrOn2PwrGood * USB_SET_PORT_POWER_STALL);
     }
 
     //
diff --git a/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h
index f50bc6350156..341f6f32e3d0 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h
+++ b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h
@@ -1,7 +1,7 @@
 /** @file
 Constants definitions for Usb Hub Peim
 
-Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
   
 This program and the accompanying materials
 are licensed and made available under the terms and conditions
@@ -227,6 +227,7 @@ PeiHubClearHubFeature (
   Get a given hub descriptor.
 
   @param  PeiServices    General-purpose services that are available to every PEIM.
+  @param  PeiUsbDevice   Indicates the hub controller device.
   @param  UsbIoPpi       Indicates the PEI_USB_IO_PPI instance.
   @param  DescriptorSize The length of Hub Descriptor buffer.
   @param  HubDescriptor  Caller allocated buffer to store the hub descriptor if
@@ -240,6 +241,7 @@ PeiHubClearHubFeature (
 EFI_STATUS
 PeiGetHubDescriptor (
   IN EFI_PEI_SERVICES         **PeiServices,
+  IN PEI_USB_DEVICE           *PeiUsbDevice,
   IN PEI_USB_IO_PPI           *UsbIoPpi,
   IN UINTN                    DescriptorSize,
   OUT EFI_USB_HUB_DESCRIPTOR  *HubDescriptor
-- 
2.21.0


From 4e88575ebed2291b0b43fa8bcee16f1dc65942bf Mon Sep 17 00:00:00 2001
From: Star Zeng <star.zeng@intel.com>
Date: Mon, 25 Jun 2018 16:44:33 +0800
Subject: [PATCH 2/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to
 read hub desc

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>
Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>
(cherry picked from commit acebdf14c985c5c9f50b37ece0b15ada87767359)
---
 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);
 
-    //
-    // First get the hub descriptor length
-    //
-    Status = UsbHubCtrlGetHubDesc (HubDev, HubDesc, 2);
-
-    if (EFI_ERROR (Status)) {
-      return Status;
-    }
-
-    //
-    // 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()
 
 
-- 
2.21.0

