[PATCH 0/2] MdeModulePkg/UsbBusPei: validate HW data before using

[PATCH 0/2] MdeModulePkg/UsbBusPei: validate HW data before using

[Ruiyu Ni]

The patches sync the similar fixes to UsbBusDxe to UsbBusPei.

Ruiyu Ni (2):
  MdeModulePkg/UsbBusPei: Fix out-of-bound read access to descriptors
  MdeModulePkg/UsbBusPei: Reject descriptor whose length is bad

 MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c | 93 +++++++++++++++++++-------------
 MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h | 11 ++++
 2 files changed, 67 insertions(+), 37 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/2] MdeModulePkg/UsbBusPei: Fix out-of-bound read access to descriptors

[Ruiyu Ni]

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 <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
 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 <IndustryStandard/Usb.h>
 
+//
+// 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

[PATCH 2/2] MdeModulePkg/UsbBusPei: Reject descriptor whose length is bad

[Ruiyu Ni]

Today's implementation doesn't check whether the length of
descriptor is valid before using it.

The patch fixes this issue by syncing the similar fix to UsbBusDxe.
70c3c2370a2aefe71cf0f6c1a1e063f7d74e1d79
*MdeModulePkg/UsbBus: Reject descriptor whose length is bad

Additionally the patch also rejects the data when length is
larger than sizeof (PeiUsbDevice->ConfigurationData).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c
index 86734f2f73..c31247abfe 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c
@@ -816,6 +816,20 @@ PeiUsbGetAllConfiguration (
   ConfigDesc        = (EFI_USB_CONFIG_DESCRIPTOR *) PeiUsbDevice->ConfigurationData;
   ConfigDescLength  = ConfigDesc->TotalLength;
 
+  //
+  // Reject if TotalLength even cannot cover itself.
+  //
+  if (ConfigDescLength < OFFSET_OF (EFI_USB_CONFIG_DESCRIPTOR, TotalLength) + sizeof (ConfigDesc->TotalLength)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  //
+  // Reject if TotalLength exceeds the PeiUsbDevice->ConfigurationData.
+  //
+  if (ConfigDescLength > sizeof (PeiUsbDevice->ConfigurationData)) {
+    return EFI_DEVICE_ERROR;
+  }
+
   //
   // Then we get the total descriptors for this configuration
   //
-- 
2.16.1.windows.1

Re: [PATCH 0/2] MdeModulePkg/UsbBusPei: validate HW data before using

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

As I know, you did Recovery test with USB disk for these patches.
I think you can add the test information into the commit message of patches.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Thursday, October 25, 2018 6:11 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/2] MdeModulePkg/UsbBusPei: validate HW data before using

The patches sync the similar fixes to UsbBusDxe to UsbBusPei.

Ruiyu Ni (2):
  MdeModulePkg/UsbBusPei: Fix out-of-bound read access to descriptors
  MdeModulePkg/UsbBusPei: Reject descriptor whose length is bad

 MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c | 93 +++++++++++++++++++-------------  MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.h | 11 ++++
 2 files changed, 67 insertions(+), 37 deletions(-)

--
2.16.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel