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 5089A2117FD7A for ; Thu, 25 Oct 2018 03:10:10 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2018 03:10:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,424,1534834800"; d="scan'208";a="103176073" Received: from ray-dev.ccr.corp.intel.com ([10.239.9.11]) by orsmga002.jf.intel.com with ESMTP; 25 Oct 2018 03:10:09 -0700 From: Ruiyu Ni To: edk2-devel@lists.01.org Cc: Star Zeng , Jiewen Yao Date: Thu, 25 Oct 2018 18:11:21 +0800 Message-Id: <20181025101122.233308-2-ruiyu.ni@intel.com> X-Mailer: git-send-email 2.16.1.windows.1 In-Reply-To: <20181025101122.233308-1-ruiyu.ni@intel.com> References: <20181025101122.233308-1-ruiyu.ni@intel.com> Subject: [PATCH 1/2] MdeModulePkg/UsbBusPei: Fix out-of-bound read access to descriptors X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Oct 2018 10:10:10 -0000 Today's implementation reads the Type/Length field in the USB descriptors data without checking whether the offset to read is beyond the data boundary. The patch fixes this issue by syncing the fix in commit 4c034bf62cbc1f3c5f4b5df25de97f0f528132b2 *MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors ParsedBytes in UsbBusPei.GetExpectedDescriptor() is different from Consumed in UsbBusDxe.UsbCreateDesc(). ParsedBytes is the offset of found descriptor while Consumed is offset of next descriptor of found one. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ruiyu Ni Cc: Star Zeng Cc: Jiewen Yao --- MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c | 79 +++++++++++++++++--------------- MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h | 11 +++++ 2 files changed, 53 insertions(+), 37 deletions(-) diff --git a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c index 10d5232e59..86734f2f73 100644 --- a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c +++ b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c @@ -940,59 +940,64 @@ GetExpectedDescriptor ( OUT UINTN *ParsedBytes ) { - UINT16 DescriptorHeader; - UINT8 Len; - UINT8 *Ptr; - UINTN Parsed; + USB_DESC_HEAD *Head; + UINTN Offset; - Parsed = 0; - Ptr = Buffer; + // + // Total length is too small that cannot hold the single descriptor header plus data. + // + if (Length <= sizeof (USB_DESC_HEAD)) { + DEBUG ((DEBUG_ERROR, "GetExpectedDescriptor: met mal-format descriptor, total length = %d!\n", Length)); + return EFI_DEVICE_ERROR; + } - while (TRUE) { + // + // All the descriptor has a common LTV (Length, Type, Value) + // format. Skip the descriptor that isn't of this Type + // + Offset = 0; + Head = (USB_DESC_HEAD *)Buffer; + while (Offset < Length - sizeof (USB_DESC_HEAD)) { // - // Buffer length should not less than Desc length + // Above condition make sure Head->Len and Head->Type are safe to access // - if (Length < DescLength) { - return EFI_DEVICE_ERROR; - } - - DescriptorHeader = (UINT16) (*Ptr + ((*(Ptr + 1)) << 8)); - - Len = Buffer[0]; + Head = (USB_DESC_HEAD *)&Buffer[Offset]; - // - // Check to see if it is a start of expected descriptor - // - if (DescriptorHeader == ((DescType << 8) | DescLength)) { - break; + if (Head->Len == 0) { + DEBUG ((DEBUG_ERROR, "GetExpectedDescriptor: met mal-format descriptor, Head->Len = 0!\n")); + return EFI_DEVICE_ERROR; } - if ((UINT8) (DescriptorHeader >> 8) == DescType) { - if (Len > DescLength) { - return EFI_DEVICE_ERROR; - } - } // - // Descriptor length should be at least 2 - // and should not exceed the buffer length + // Make sure no overflow when adding Head->Len to Offset. // - if (Len < 2) { + if (Head->Len > MAX_UINTN - Offset) { + DEBUG ((DEBUG_ERROR, "GetExpectedDescriptor: met mal-format descriptor, Head->Len = %d!\n", Head->Len)); return EFI_DEVICE_ERROR; } - if (Len > Length) { - return EFI_DEVICE_ERROR; + if (Head->Type == DescType) { + break; } - // - // Skip this mismatch descriptor - // - Length -= Len; - Ptr += Len; - Parsed += Len; + + Offset += Head->Len; + } + + // + // Head->Len is invalid resulting data beyond boundary, or + // Descriptor cannot be found: No such type. + // + if (Length < Offset) { + DEBUG ((DEBUG_ERROR, "GetExpectedDescriptor: met mal-format descriptor, Offset/Len = %d/%d!\n", Offset, Length)); + return EFI_DEVICE_ERROR; } - *ParsedBytes = Parsed; + if ((Head->Type != DescType) || (Head->Len < DescLength)) { + DEBUG ((DEBUG_ERROR, "GetExpectedDescriptor: descriptor cannot be found, Header(T/L) = %d/%d!\n", Head->Type, Head->Len)); + return EFI_DEVICE_ERROR; + } + *ParsedBytes = Offset; return EFI_SUCCESS; } diff --git a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h index 1610974537..14565deb46 100644 --- a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h +++ b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h @@ -33,6 +33,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #include +// +// A common header for usb standard descriptor. +// Each stand descriptor has a length and type. +// +#pragma pack(1) +typedef struct { + UINT8 Len; + UINT8 Type; +} USB_DESC_HEAD; +#pragma pack() + #define MAX_INTERFACE 8 #define MAX_ENDPOINT 16 -- 2.16.1.windows.1