On 2018/10/15 14:38, Ruiyu Ni wrote:
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.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <[email protected]>
Cc: Star Zeng <[email protected]>
Cc: Jiewen Yao <[email protected]>
Ray,
Thanks for the patch.
I have two minor comments.
You can take them as you wish as I have no strong opinion for them. :)
Reviewed-by: Star Zeng <[email protected]>
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 50 ++++++++++++++++++++++++--------
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 061e4622e8..a93060deea 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -177,6 +177,17 @@ UsbCreateDesc (
DescLen = sizeof (EFI_USB_ENDPOINT_DESCRIPTOR);
CtrlLen = sizeof (USB_ENDPOINT_DESC);
break;
+
+ default:
+ ASSERT (FALSE);
+ return NULL;
+ }
+
+ //
+ // Total length is too small that cannot hold the single descriptor header
plus data.
+ //
+ if (Len <= sizeof (USB_DESC_HEAD)) {
Add debug message here like others below for error cases?
+ return NULL;
}
//
@@ -184,24 +195,39 @@ UsbCreateDesc (
// format. Skip the descriptor that isn't of this Type
//
Offset = 0;
- Head = (USB_DESC_HEAD*)DescBuf;
+ Head = (USB_DESC_HEAD *)DescBuf;
+ while (Offset < Len - sizeof (USB_DESC_HEAD)) {
+ //
+ // Above condition make sure Head->Len and Head->Type are safe to access
+ //
+ Head = (USB_DESC_HEAD *)&DescBuf[Offset];
- while ((Offset < Len) && (Head->Type != Type)) {
- Offset += Head->Len;
- if (Len <= Offset) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Beyond
boundary!\n"));
+ if (Head->Len == 0) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len =
0!\n"));
return NULL;
}
- Head = (USB_DESC_HEAD*)(DescBuf + Offset);
- if (Head->Len == 0) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len =
0!\n"));
+
+ //
+ // Make sure no overflow when adding Head->Len to Offset.
+ //
+ if (Head->Len > MAX_UINTN - Offset) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len =
%d!\n", Head->Len));
return NULL;
}
+
+ Offset += Head->Len;
+
+ if (Head->Type == Type) {
+ break;
+ }
}
- if ((Len <= Offset) || (Len < Offset + Head->Len) ||
- (Head->Type != Type) || (Head->Len < DescLen)) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor\n"));
+ //
+ // Head->Len is invalid resulting data beyond boundary, or
+ // Descriptor cannot be found: No such type.
+ //
+ if ((Len < Offset) || (Head->Type != Type) || (Head->Len < DescLen)) {
+ DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor. Offset/Len = %d/%d,
Header(T/L) = %d/%d\n", Offset, Len, Head->Type, Head->Len));
How about splitting the condition check and debug message to two? :)
if (Len < Offset) { // It is boundary check.
if ((Head->Type != Type) || (Head->Len < DescLen)) { // It is content check.
Thanks,
Star
return NULL;
}
@@ -212,7 +238,7 @@ UsbCreateDesc (
CopyMem (Desc, Head, (UINTN) DescLen);
- *Consumed = Offset + Head->Len;
+ *Consumed = Offset;
return Desc;
}
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
