* [PATCH 0/2] UsbBus: Fix wrong buffer length used to read hub desc
@ 2018-06-25 10:38 Star Zeng
2018-06-25 10:38 ` [PATCH 1/2] MdeModulePkg UsbBusDxe: " Star Zeng
2018-06-25 10:38 ` [PATCH 2/2] MdeModulePkg UsbBusPei: " Star Zeng
0 siblings, 2 replies; 6+ messages in thread
From: Star Zeng @ 2018-06-25 10:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Bret Barkelew
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=973
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Bret Barkelew <bret.barkelew@microsoft.com>
Star Zeng (2):
MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc
MdeModulePkg UsbBusPei: Fix wrong buffer length used to read hub desc
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 96 +++++++++------------------
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.h | 14 +---
MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c | 108 ++++++++++---------------------
MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h | 4 +-
4 files changed, 70 insertions(+), 152 deletions(-)
--
2.7.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc
2018-06-25 10:38 [PATCH 0/2] UsbBus: Fix wrong buffer length used to read hub desc Star Zeng
@ 2018-06-25 10:38 ` Star Zeng
2018-06-25 17:25 ` Bret Barkelew
2018-06-26 5:40 ` Ni, Ruiyu
2018-06-25 10:38 ` [PATCH 2/2] MdeModulePkg UsbBusPei: " Star Zeng
1 sibling, 2 replies; 6+ messages in thread
From: Star Zeng @ 2018-06-25 10:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Bret Barkelew
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()
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] MdeModulePkg UsbBusPei: Fix wrong buffer length used to read hub desc
2018-06-25 10:38 [PATCH 0/2] UsbBus: Fix wrong buffer length used to read hub desc Star Zeng
2018-06-25 10:38 ` [PATCH 1/2] MdeModulePkg UsbBusDxe: " Star Zeng
@ 2018-06-25 10:38 ` Star Zeng
2018-06-25 17:27 ` Bret Barkelew
1 sibling, 1 reply; 6+ messages in thread
From: Star Zeng @ 2018-06-25 10:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni, Bret Barkelew
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>
---
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,63 +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;
- 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_HUB << 8;
- DevReq.Length = (UINT16)DescriptorSize;
-
- return UsbIoPpi->UsbControlTransfer (
- PeiServices,
- UsbIoPpi,
- &DevReq,
- EfiUsbDataIn,
- PcdGet32 (PcdUsbTransferTimeoutValue),
- HubDescriptor,
- (UINT16)DescriptorSize
- );
-}
-
-/**
- Get a given SuperSpeed hub descriptor.
+ UINT8 DescType;
- @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));
+ 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_SUPERSPEED_HUB << 8;
- DevReq.Length = 12;
+ DevReq.Value = (UINT16) (DescType << 8);
+ DevReq.Length = (UINT16) DescriptorSize;
return UsbIoPpi->UsbControlTransfer (
PeiServices,
@@ -357,7 +323,7 @@ PeiGetSuperSpeedHubDesc (
EfiUsbDataIn,
PcdGet32 (PcdUsbTransferTimeoutValue),
HubDescriptor,
- 12
+ (UINT16)DescriptorSize
);
}
@@ -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, UsbIoPpi, 2, HubDescriptor);
-
- if (EFI_ERROR (Status)) {
- return Status;
- }
+ //
+ // First get the hub descriptor length
+ //
+ Status = PeiGetHubDescriptor (PeiServices, PeiUsbDevice, UsbIoPpi, 2, HubDescriptor);
- //
- // 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,29 +424,35 @@ 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
//
Status = PeiUsbHubReadDesc (
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.7.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc
2018-06-25 10:38 ` [PATCH 1/2] MdeModulePkg UsbBusDxe: " Star Zeng
@ 2018-06-25 17:25 ` Bret Barkelew
2018-06-26 5:40 ` Ni, Ruiyu
1 sibling, 0 replies; 6+ messages in thread
From: Bret Barkelew @ 2018-06-25 17:25 UTC (permalink / raw)
To: Star Zeng, edk2-devel@lists.01.org; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni
Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>
- Bret
________________________________
From: Star Zeng <star.zeng@intel.com>
Sent: Monday, June 25, 2018 3:38:12 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng; Jiewen Yao; Ruiyu Ni; Bret Barkelew
Subject: [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc
REF: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D973&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Ca26f6d8c41174e8e643208d5da87c49e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636655199033311517&sdata=f7QwxronZfoAs9C7BLHTk9mAAiNP8ioYzKeoAbdU%2FUM%3D&reserved=0
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()
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg UsbBusPei: Fix wrong buffer length used to read hub desc
2018-06-25 10:38 ` [PATCH 2/2] MdeModulePkg UsbBusPei: " Star Zeng
@ 2018-06-25 17:27 ` Bret Barkelew
0 siblings, 0 replies; 6+ messages in thread
From: Bret Barkelew @ 2018-06-25 17:27 UTC (permalink / raw)
To: Star Zeng, edk2-devel@lists.01.org; +Cc: Star Zeng, Jiewen Yao, Ruiyu Ni
Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>
- Bret
________________________________
From: Star Zeng <star.zeng@intel.com>
Sent: Monday, June 25, 2018 3:38:13 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng; Jiewen Yao; Ruiyu Ni; Bret Barkelew
Subject: [PATCH 2/2] MdeModulePkg UsbBusPei: Fix wrong buffer length used to read hub desc
REF: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D973&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C80ae823f4723435b90f408d5da87c509%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636655199021176324&sdata=zZ1NN9uNtJLLmGt6jXOKq7WeWIWJXDY8IgO09HDpsTU%3D&reserved=0
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>
---
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,63 +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;
- 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_HUB << 8;
- DevReq.Length = (UINT16)DescriptorSize;
-
- return UsbIoPpi->UsbControlTransfer (
- PeiServices,
- UsbIoPpi,
- &DevReq,
- EfiUsbDataIn,
- PcdGet32 (PcdUsbTransferTimeoutValue),
- HubDescriptor,
- (UINT16)DescriptorSize
- );
-}
-
-/**
- Get a given SuperSpeed hub descriptor.
+ UINT8 DescType;
- @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));
+ 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_SUPERSPEED_HUB << 8;
- DevReq.Length = 12;
+ DevReq.Value = (UINT16) (DescType << 8);
+ DevReq.Length = (UINT16) DescriptorSize;
return UsbIoPpi->UsbControlTransfer (
PeiServices,
@@ -357,7 +323,7 @@ PeiGetSuperSpeedHubDesc (
EfiUsbDataIn,
PcdGet32 (PcdUsbTransferTimeoutValue),
HubDescriptor,
- 12
+ (UINT16)DescriptorSize
);
}
@@ -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, UsbIoPpi, 2, HubDescriptor);
-
- if (EFI_ERROR (Status)) {
- return Status;
- }
+ //
+ // First get the hub descriptor length
+ //
+ Status = PeiGetHubDescriptor (PeiServices, PeiUsbDevice, UsbIoPpi, 2, HubDescriptor);
- //
- // 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,29 +424,35 @@ 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
//
Status = PeiUsbHubReadDesc (
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.7.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc
2018-06-25 10:38 ` [PATCH 1/2] MdeModulePkg UsbBusDxe: " Star Zeng
2018-06-25 17:25 ` Bret Barkelew
@ 2018-06-26 5:40 ` Ni, Ruiyu
1 sibling, 0 replies; 6+ messages in thread
From: Ni, Ruiyu @ 2018-06-26 5:40 UTC (permalink / raw)
To: Star Zeng, edk2-devel; +Cc: Jiewen Yao, Bret Barkelew
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-26 5:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-25 10:38 [PATCH 0/2] UsbBus: Fix wrong buffer length used to read hub desc Star Zeng
2018-06-25 10:38 ` [PATCH 1/2] MdeModulePkg UsbBusDxe: " Star Zeng
2018-06-25 17:25 ` Bret Barkelew
2018-06-26 5:40 ` Ni, Ruiyu
2018-06-25 10:38 ` [PATCH 2/2] MdeModulePkg UsbBusPei: " Star Zeng
2018-06-25 17:27 ` Bret Barkelew
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox