From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 01648203369DC for ; Mon, 25 Jun 2018 22:40:31 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2018 22:40:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,273,1526367600"; d="scan'208";a="235609080" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by orsmga005.jf.intel.com with ESMTP; 25 Jun 2018 22:40:30 -0700 To: Star Zeng , edk2-devel@lists.01.org Cc: Jiewen Yao , Bret Barkelew References: <1529923093-156972-1-git-send-email-star.zeng@intel.com> <1529923093-156972-2-git-send-email-star.zeng@intel.com> From: "Ni, Ruiyu" Message-ID: Date: Tue, 26 Jun 2018 13:40:51 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <1529923093-156972-2-git-send-email-star.zeng@intel.com> Subject: Re: [PATCH 1/2] MdeModulePkg UsbBusDxe: Fix wrong buffer length used to read hub desc X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jun 2018 05:40:32 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ruiyu Ni > Cc: Bret Barkelew > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > 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.
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
> 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.
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
> 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