[PATCH v2 00/12] Need to validate data from HW

[PATCH v2 00/12] Need to validate data from HW

[Ruiyu Ni]

The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.

V2: adopt all comments from Star. The block size 0/64K handle is in a
separate patch
"MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K"

Hao Wu (1):
  MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected

Ruiyu Ni (11):
  MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
  MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
  MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
  MdeModulePkg/UsbBus: Reject descriptor whose length is bad
  MdeModulePkg/Usb: Make sure data from HW is no more than expected
  SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
  MdeModulePkg/UsbKb: Don't access key codes when length is wrong
  MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
  MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
  MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
  MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K

 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c           |   9 +-
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c           |   7 +-
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c           |   9 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c        |  19 +-
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        |  30 ++-
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c           |  70 +++++-
 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c           |   4 +
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 276 ++++++---------------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  76 ++----
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        |   8 +-
 .../UsbMouseAbsolutePointer.c                      |   8 +-
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c        |   8 +-
 .../DebugCommunicationLibUsb3Transfer.c            |   7 +
 13 files changed, 237 insertions(+), 294 deletions(-)

-- 
2.16.1.windows.1

[PATCH v2 01/12] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)

[Ruiyu Ni]

The change doesn't have functionality impact.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 248 +++++----------------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  76 ++-----
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        |   8 +-
 3 files changed, 80 insertions(+), 252 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index dd5950c54c..07a376440c 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -792,32 +792,34 @@ UsbBootDetectMedia (
 
 
 /**
-  Read some blocks from the device.
+  Read or write some blocks from the device.
 
-  @param  UsbMass                The USB mass storage device to read from
+  @param  UsbMass                The USB mass storage device to access
+  @param  Write                  TRUE for write operation.
   @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to read
-  @param  Buffer                 The buffer to read to
+  @param  TotalBlock             Total block number to read or write
+  @param  Buffer                 The buffer to read to or write from
 
-  @retval EFI_SUCCESS            Data are read into the buffer
-  @retval Others                 Failed to read all the data
+  @retval EFI_SUCCESS            Data are read into the buffer or writen into the device.
+  @retval Others                 Failed to read or write all the data
 
 **/
 EFI_STATUS
-UsbBootReadBlocks (
+UsbBootReadWriteBlocks (
   IN  USB_MASS_DEVICE       *UsbMass,
+  IN  BOOLEAN               Write,
   IN  UINT32                Lba,
   IN  UINTN                 TotalBlock,
-  OUT UINT8                 *Buffer
+  IN OUT UINT8              *Buffer
   )
 {
-  USB_BOOT_READ10_CMD       ReadCmd;
-  EFI_STATUS                Status;
-  UINT16                    Count;
-  UINT16                    CountMax;
-  UINT32                    BlockSize;
-  UINT32                    ByteSize;
-  UINT32                    Timeout;
+  USB_BOOT_READ_WRITE_10_CMD Cmd;
+  EFI_STATUS                 Status;
+  UINT16                     Count;
+  UINT16                     CountMax;
+  UINT32                     BlockSize;
+  UINT32                     ByteSize;
+  UINT32                     Timeout;
 
   BlockSize = UsbMass->BlockIoMedia.BlockSize;
   CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
@@ -840,17 +842,17 @@ UsbBootReadBlocks (
     //
     // Fill in the command then execute
     //
-    ZeroMem (&ReadCmd, sizeof (USB_BOOT_READ10_CMD));
+    ZeroMem (&Cmd, sizeof (USB_BOOT_READ_WRITE_10_CMD));
 
-    ReadCmd.OpCode  = USB_BOOT_READ10_OPCODE;
-    ReadCmd.Lun     = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
-    WriteUnaligned32 ((UINT32 *) ReadCmd.Lba, SwapBytes32 (Lba));
-    WriteUnaligned16 ((UINT16 *) ReadCmd.TransferLen, SwapBytes16 (Count));
+    Cmd.OpCode  = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
+    Cmd.Lun     = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
+    WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
+    WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
 
     Status = UsbBootExecCmdWithRetry (
                UsbMass,
-               &ReadCmd,
-               (UINT8) sizeof (USB_BOOT_READ10_CMD),
+               &Cmd,
+               (UINT8) sizeof (USB_BOOT_READ_WRITE_10_CMD),
                EfiUsbDataIn,
                Buffer,
                ByteSize,
@@ -859,86 +861,11 @@ UsbBootReadBlocks (
     if (EFI_ERROR (Status)) {
       return Status;
     }
-    DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count));
-    Lba        += Count;
-    Buffer     += Count * BlockSize;
-    TotalBlock -= Count;
-  }
-
-  return Status;
-}
-
-
-/**
-  Write some blocks to the device.
-
-  @param  UsbMass                The USB mass storage device to write to
-  @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to write
-  @param  Buffer                 Pointer to the source buffer for the data.
-
-  @retval EFI_SUCCESS            Data are written into the buffer
-  @retval Others                 Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks (
-  IN  USB_MASS_DEVICE         *UsbMass,
-  IN  UINT32                  Lba,
-  IN  UINTN                   TotalBlock,
-  IN  UINT8                   *Buffer
-  )
-{
-  USB_BOOT_WRITE10_CMD  WriteCmd;
-  EFI_STATUS            Status;
-  UINT16                Count;
-  UINT16                CountMax;
-  UINT32                BlockSize;
-  UINT32                ByteSize;
-  UINT32                Timeout;
-
-  BlockSize = UsbMass->BlockIoMedia.BlockSize;
-  CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
-  Status    = EFI_SUCCESS;
-
-  while (TotalBlock > 0) {
-    //
-    // Split the total blocks into smaller pieces to ease the pressure
-    // on the device. We must split the total block because the WRITE10
-    // command only has 16 bit transfer length (in the unit of block).
-    //
-    Count     = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-    ByteSize  = (UINT32)Count * BlockSize;
-
-    //
-    // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
-    //
-    Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT;
-
-    //
-    // Fill in the write10 command block
-    //
-    ZeroMem (&WriteCmd, sizeof (USB_BOOT_WRITE10_CMD));
-
-    WriteCmd.OpCode = USB_BOOT_WRITE10_OPCODE;
-    WriteCmd.Lun    = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
-    WriteUnaligned32 ((UINT32 *) WriteCmd.Lba, SwapBytes32 (Lba));
-    WriteUnaligned16 ((UINT16 *) WriteCmd.TransferLen, SwapBytes16 (Count));
-
-    Status = UsbBootExecCmdWithRetry (
-               UsbMass,
-               &WriteCmd,
-               (UINT8) sizeof (USB_BOOT_WRITE10_CMD),
-               EfiUsbDataOut,
-               Buffer,
-               ByteSize,
-               Timeout
-               );
-    if (EFI_ERROR (Status)) {
-      return Status;
-    }
-    DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count));
-
+    DEBUG ((
+      DEBUG_BLKIO, "UsbBoot%sBlocks: LBA (0x%lx), Blk (0x%x)\n",
+      Write ? L"Write" : L"Read",
+      Lba, Count
+      ));
     Lba        += Count;
     Buffer     += Count * BlockSize;
     TotalBlock -= Count;
@@ -948,26 +875,27 @@ UsbBootWriteBlocks (
 }
 
 /**
-  Read some blocks from the device by SCSI 16 byte cmd.
+  Read or write some blocks from the device by SCSI 16 byte cmd.
 
-  @param  UsbMass                The USB mass storage device to read from
+  @param  UsbMass                The USB mass storage device to access
+  @param  Write                  TRUE for write operation.
   @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to read
-  @param  Buffer                 The buffer to read to
-
-  @retval EFI_SUCCESS            Data are read into the buffer
-  @retval Others                 Failed to read all the data
+  @param  TotalBlock             Total block number to read or write
+  @param  Buffer                 The buffer to read to or write from
 
+  @retval EFI_SUCCESS            Data are read into the buffer or writen into the device.
+  @retval Others                 Failed to read or write all the data
 **/
 EFI_STATUS
-UsbBootReadBlocks16 (
+UsbBootReadWriteBlocks16 (
   IN  USB_MASS_DEVICE       *UsbMass,
+  IN  BOOLEAN               Write,
   IN  UINT64                Lba,
   IN  UINTN                 TotalBlock,
-  OUT UINT8                 *Buffer
+  IN OUT UINT8              *Buffer
   )
 {
-  UINT8                     ReadCmd[16];
+  UINT8                     Cmd[16];
   EFI_STATUS                Status;
   UINT16                    Count;
   UINT16                    CountMax;
@@ -994,17 +922,17 @@ UsbBootReadBlocks16 (
     //
     // Fill in the command then execute
     //
-    ZeroMem (ReadCmd, sizeof (ReadCmd));
+    ZeroMem (Cmd, sizeof (Cmd));
 
-    ReadCmd[0]  = EFI_SCSI_OP_READ16;
-    ReadCmd[1]  = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
-    WriteUnaligned64 ((UINT64 *) &ReadCmd[2], SwapBytes64 (Lba));
-    WriteUnaligned32 ((UINT32 *) &ReadCmd[10], SwapBytes32 (Count));
+    Cmd[0]  = Write ? EFI_SCSI_OP_WRITE16 : EFI_SCSI_OP_READ16;
+    Cmd[1]  = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
+    WriteUnaligned64 ((UINT64 *) &Cmd[2], SwapBytes64 (Lba));
+    WriteUnaligned32 ((UINT32 *) &Cmd[10], SwapBytes32 (Count));
 
     Status = UsbBootExecCmdWithRetry (
                UsbMass,
-               ReadCmd,
-               (UINT8) sizeof (ReadCmd),
+               Cmd,
+               (UINT8) sizeof (Cmd),
                EfiUsbDataIn,
                Buffer,
                ByteSize,
@@ -1013,83 +941,11 @@ UsbBootReadBlocks16 (
     if (EFI_ERROR (Status)) {
       return Status;
     }
-    DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks16: LBA (0x%lx), Blk (0x%x)\n", Lba, Count));
-    Lba        += Count;
-    Buffer     += Count * BlockSize;
-    TotalBlock -= Count;
-  }
-
-  return Status;
-}
-
-
-/**
-  Write some blocks to the device by SCSI 16 byte cmd.
-
-  @param  UsbMass                The USB mass storage device to write to
-  @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to write
-  @param  Buffer                 Pointer to the source buffer for the data.
-
-  @retval EFI_SUCCESS            Data are written into the buffer
-  @retval Others                 Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks16 (
-  IN  USB_MASS_DEVICE         *UsbMass,
-  IN  UINT64                  Lba,
-  IN  UINTN                   TotalBlock,
-  IN  UINT8                   *Buffer
-  )
-{
-  UINT8                 WriteCmd[16];
-  EFI_STATUS            Status;
-  UINT16                Count;
-  UINT16                CountMax;
-  UINT32                BlockSize;
-  UINT32                ByteSize;
-  UINT32                Timeout;
-
-  BlockSize = UsbMass->BlockIoMedia.BlockSize;
-  CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
-  Status    = EFI_SUCCESS;
-
-  while (TotalBlock > 0) {
-    //
-    // Split the total blocks into smaller pieces.
-    //
-    Count     = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-    ByteSize  = (UINT32)Count * BlockSize;
-
-    //
-    // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
-    //
-    Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT;
-
-    //
-    // Fill in the write16 command block
-    //
-    ZeroMem (WriteCmd, sizeof (WriteCmd));
-
-    WriteCmd[0]  = EFI_SCSI_OP_WRITE16;
-    WriteCmd[1]  = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
-    WriteUnaligned64 ((UINT64 *) &WriteCmd[2], SwapBytes64 (Lba));
-    WriteUnaligned32 ((UINT32 *) &WriteCmd[10], SwapBytes32 (Count));
-
-    Status = UsbBootExecCmdWithRetry (
-               UsbMass,
-               WriteCmd,
-               (UINT8) sizeof (WriteCmd),
-               EfiUsbDataOut,
-               Buffer,
-               ByteSize,
-               Timeout
-               );
-    if (EFI_ERROR (Status)) {
-      return Status;
-    }
-    DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%lx), Blk (0x%x)\n", Lba, Count));
+    DEBUG ((
+      DEBUG_BLKIO, "UsbBoot%sBlocks16: LBA (0x%lx), Blk (0x%x)\n",
+      Write ? L"Write" : L"Read",
+      Lba, Count
+      ));
     Lba        += Count;
     Buffer     += Count * BlockSize;
     TotalBlock -= Count;
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
index d4042e320d..3f8213b3d7 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
@@ -161,17 +161,7 @@ typedef struct {
   UINT8             TransferLen[2]; ///< Transfer length
   UINT8             Reserverd1;
   UINT8             Pad[2];
-} USB_BOOT_READ10_CMD;
-
-typedef struct {
-  UINT8             OpCode;
-  UINT8             Lun;
-  UINT8             Lba[4];
-  UINT8             Reserved0;
-  UINT8             TransferLen[2];
-  UINT8             Reserverd1;
-  UINT8             Pad[2];
-} USB_BOOT_WRITE10_CMD;
+} USB_BOOT_READ_WRITE_10_CMD;
 
 typedef struct {
   UINT8             OpCode;
@@ -292,66 +282,48 @@ UsbBootReadBlocks (
   );
 
 /**
-  Write some blocks to the device.
+  Read or write some blocks from the device.
 
-  @param  UsbMass                The USB mass storage device to write to
+  @param  UsbMass                The USB mass storage device to access
+  @param  Write                  TRUE for write operation.
   @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to write
-  @param  Buffer                 Pointer to the source buffer for the data.
+  @param  TotalBlock             Total block number to read or write
+  @param  Buffer                 The buffer to read to or write from
 
-  @retval EFI_SUCCESS            Data are written into the buffer
-  @retval Others                 Failed to write all the data
+  @retval EFI_SUCCESS            Data are read into the buffer or writen into the device.
+  @retval Others                 Failed to read or write all the data
 
 **/
 EFI_STATUS
-UsbBootWriteBlocks (
-  IN  USB_MASS_DEVICE         *UsbMass,
-  IN  UINT32                  Lba,
-  IN  UINTN                   TotalBlock,
-  IN  UINT8                   *Buffer
+UsbBootReadWriteBlocks (
+  IN  USB_MASS_DEVICE       *UsbMass,
+  IN  BOOLEAN               Write,
+  IN  UINT32                Lba,
+  IN  UINTN                 TotalBlock,
+  IN OUT UINT8              *Buffer
   );
 
 /**
-  Read some blocks from the device by SCSI 16 byte cmd.
+  Read or write some blocks from the device by SCSI 16 byte cmd
 
-  @param  UsbMass                The USB mass storage device to read from
+  @param  UsbMass                The USB mass storage device to access
+  @param  Write                  TRUE for write operation.
   @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to read
-  @param  Buffer                 The buffer to read to
-
-  @retval EFI_SUCCESS            Data are read into the buffer
-  @retval Others                 Failed to read all the data
+  @param  TotalBlock             Total block number to read or write
+  @param  Buffer                 The buffer to read to or write from
 
+  @retval EFI_SUCCESS            Data are read into the buffer or writen into the device.
+  @retval Others                 Failed to read or write all the data
 **/
 EFI_STATUS
-UsbBootReadBlocks16 (
+UsbBootReadWriteBlocks16 (
   IN  USB_MASS_DEVICE       *UsbMass,
+  IN  BOOLEAN               Write,
   IN  UINT64                Lba,
   IN  UINTN                 TotalBlock,
-  OUT UINT8                 *Buffer
+  IN OUT UINT8              *Buffer
   );
 
-/**
-  Write some blocks to the device by SCSI 16 byte cmd.
-
-  @param  UsbMass                The USB mass storage device to write to
-  @param  Lba                    The start block number
-  @param  TotalBlock             Total block number to write
-  @param  Buffer                 Pointer to the source buffer for the data.
-
-  @retval EFI_SUCCESS            Data are written into the buffer
-  @retval Others                 Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks16 (
-  IN  USB_MASS_DEVICE         *UsbMass,
-  IN  UINT64                  Lba,
-  IN  UINTN                   TotalBlock,
-  IN  UINT8                   *Buffer
-  );
-
-
 /**
   Use the USB clear feature control transfer to clear the endpoint stall condition.
 
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
index 62bf3c5883..9413b00680 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
@@ -172,9 +172,9 @@ UsbMassReadBlocks (
   }
 
   if (UsbMass->Cdb16Byte) {
-    Status = UsbBootReadBlocks16 (UsbMass, Lba, TotalBlock, Buffer);
+    Status = UsbBootReadWriteBlocks16 (UsbMass, FALSE, Lba, TotalBlock, Buffer);
   } else {
-    Status = UsbBootReadBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer);
+    Status = UsbBootReadWriteBlocks (UsbMass, FALSE, (UINT32) Lba, TotalBlock, Buffer);
   }
 
   if (EFI_ERROR (Status)) {
@@ -292,9 +292,9 @@ UsbMassWriteBlocks (
   // and clear the status should the write succeed.
   //
   if (UsbMass->Cdb16Byte) {
-    Status = UsbBootWriteBlocks16 (UsbMass, Lba, TotalBlock, Buffer);
+    Status = UsbBootReadWriteBlocks16 (UsbMass, TRUE, Lba, TotalBlock, Buffer);
   } else {
-    Status = UsbBootWriteBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer);
+    Status = UsbBootReadWriteBlocks (UsbMass, TRUE, (UINT32) Lba, TotalBlock, Buffer);
   }
 
   if (EFI_ERROR (Status)) {
-- 
2.16.1.windows.1

[PATCH v2 02/12] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1

[Ruiyu Ni]

UsbBootReadWriteBlocks() and UsbBootReadWriteBlocks16() use a UINT16
local variable to hold the value of
USB_BOOT_MAX_CARRY_SIZE (=0x10000) / BlockSize.
When BlockSize is 1, the UINT16 local variable is set to 0x10000
but the high-16 bits are truncated resulting the final value be 0.

It causes the while-loop in the two functions accesses 0 block in
each loop, resulting the loop never ends.

The patch fixes the two functions to make sure no integer overflow
happens.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
---
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 27 +++++++++++-----------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 07a376440c..0b35cbacf0 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -815,14 +815,14 @@ UsbBootReadWriteBlocks (
 {
   USB_BOOT_READ_WRITE_10_CMD Cmd;
   EFI_STATUS                 Status;
-  UINT16                     Count;
-  UINT16                     CountMax;
+  UINT32                     Count;
+  UINT32                     CountMax;
   UINT32                     BlockSize;
   UINT32                     ByteSize;
   UINT32                     Timeout;
 
   BlockSize = UsbMass->BlockIoMedia.BlockSize;
-  CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+  CountMax  = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
   Status    = EFI_SUCCESS;
 
   while (TotalBlock > 0) {
@@ -831,8 +831,9 @@ UsbBootReadWriteBlocks (
     // on the device. We must split the total block because the READ10
     // command only has 16 bit transfer length (in the unit of block).
     //
-    Count     = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-    ByteSize  = (UINT32)Count * BlockSize;
+    Count    = (UINT32)MIN (TotalBlock, CountMax);
+    Count    = MIN (MAX_UINT16, Count);
+    ByteSize = Count * BlockSize;
 
     //
     // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -847,7 +848,7 @@ UsbBootReadWriteBlocks (
     Cmd.OpCode  = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
     Cmd.Lun     = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
     WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
-    WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
+    WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 ((UINT16)Count));
 
     Status = UsbBootExecCmdWithRetry (
                UsbMass,
@@ -867,7 +868,7 @@ UsbBootReadWriteBlocks (
       Lba, Count
       ));
     Lba        += Count;
-    Buffer     += Count * BlockSize;
+    Buffer     += ByteSize;
     TotalBlock -= Count;
   }
 
@@ -897,22 +898,22 @@ UsbBootReadWriteBlocks16 (
 {
   UINT8                     Cmd[16];
   EFI_STATUS                Status;
-  UINT16                    Count;
-  UINT16                    CountMax;
+  UINT32                    Count;
+  UINT32                    CountMax;
   UINT32                    BlockSize;
   UINT32                    ByteSize;
   UINT32                    Timeout;
 
   BlockSize = UsbMass->BlockIoMedia.BlockSize;
-  CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+  CountMax  = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
   Status    = EFI_SUCCESS;
 
   while (TotalBlock > 0) {
     //
     // Split the total blocks into smaller pieces.
     //
-    Count     = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
-    ByteSize  = (UINT32)Count * BlockSize;
+    Count    = (UINT32)MIN (TotalBlock, CountMax);
+    ByteSize = Count * BlockSize;
 
     //
     // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -947,7 +948,7 @@ UsbBootReadWriteBlocks16 (
       Lba, Count
       ));
     Lba        += Count;
-    Buffer     += Count * BlockSize;
+    Buffer     += ByteSize;
     TotalBlock -= Count;
   }
 
-- 
2.16.1.windows.1

[PATCH v2 03/12] MdeModulePkg/UsbBus: 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.

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/UsbBusDxe/UsbDesc.c | 55 +++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 061e4622e8..70442c57da 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -177,6 +177,18 @@ 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)) {
+    DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, total length = %d!\n", Len));
+    return NULL;
   }
 
   //
@@ -184,24 +196,43 @@ 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;
+    }
+  }
+
+  //
+  // Head->Len is invalid resulting data beyond boundary, or
+  // Descriptor cannot be found: No such type.
+  //
+  if (Len < Offset) {
+    DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Offset/Len = %d/%d!\n", Offset, Len));
   }
 
-  if ((Len <= Offset)      || (Len < Offset + Head->Len) ||
-      (Head->Type != Type) || (Head->Len < DescLen)) {
-    DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor\n"));
+  if ((Head->Type != Type) || (Head->Len < DescLen)) {
+    DEBUG ((DEBUG_ERROR, "UsbCreateDesc: descriptor cannot be found, Header(T/L) = %d/%d!\n", Head->Type, Head->Len));
     return NULL;
   }
 
@@ -212,7 +243,7 @@ UsbCreateDesc (
 
   CopyMem (Desc, Head, (UINTN) DescLen);
 
-  *Consumed = Offset + Head->Len;
+  *Consumed = Offset;
 
   return Desc;
 }
-- 
2.16.1.windows.1

[PATCH v2 04/12] MdeModulePkg/UsbBus: 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.

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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 70442c57da..9fc6422ab1 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -772,6 +772,13 @@ UsbGetOneConfig (
 
   DEBUG (( EFI_D_INFO, "UsbGetOneConfig: total length is %d\n", Desc.TotalLength));
 
+  //
+  // Reject if TotalLength even cannot cover itself.
+  //
+  if (Desc.TotalLength < OFFSET_OF (EFI_USB_CONFIG_DESCRIPTOR, TotalLength) + sizeof (Desc.TotalLength)) {
+    return NULL;
+  }
+
   Buf = AllocateZeroPool (Desc.TotalLength);
 
   if (Buf == NULL) {
-- 
2.16.1.windows.1

[PATCH v2 05/12] MdeModulePkg/Usb: Make sure data from HW is no more than expected

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 ++++++---
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 ++++---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++++++---
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index fea6f47f4c..168280be81 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -1009,9 +1009,12 @@ EhcMonitorAsyncRequests (
     ProcBuf = NULL;
 
     if (Urb->Result == EFI_USB_NOERROR) {
-      ASSERT (Urb->Completed <= Urb->DataLen);
-
-      ProcBuf = AllocatePool (Urb->Completed);
+      //
+      // Make sure the data received from HW is no more than expected.
+      //
+      if (Urb->Completed <= Urb->DataLen) {
+        ProcBuf = AllocatePool (Urb->Completed);
+      }
 
       if (ProcBuf == NULL) {
         EhcUpdateAsyncRequest (Ehc, Urb);
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
index 90f010c998..f7510f3ec0 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
@@ -2,7 +2,7 @@
 
   The EHCI register operation routines.
 
-Copyright (c) 2007 - 2013, 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
@@ -1001,11 +1001,12 @@ UhciMonitorAsyncReqList (
 
     //
     // Copy the data to temporary buffer if there are some
-    // data transferred. We may have zero-length packet
+    // data transferred. We may have zero-length packet.
+    // Make sure the data received from HW is no more than expected.
     //
     Data = NULL;
 
-    if (QhResult.Complete != 0) {
+    if ((QhResult.Complete != 0) && (QhResult.Complete <= AsyncReq->DataLen)) {
       Data = AllocatePool (QhResult.Complete);
 
       if (Data == NULL) {
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 6a2ef4cd5d..166c44bf5e 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1556,9 +1556,12 @@ XhcMonitorAsyncRequests (
     //
     ProcBuf = NULL;
     if (Urb->Result == EFI_USB_NOERROR) {
-      ASSERT (Urb->Completed <= Urb->DataLen);
-
-      ProcBuf = AllocateZeroPool (Urb->Completed);
+      //
+      // Make sure the data received from HW is no more than expected.
+      //
+      if (Urb->Completed <= Urb->DataLen) {
+        ProcBuf = AllocateZeroPool (Urb->Completed);
+      }
 
       if (ProcBuf == NULL) {
         XhcUpdateAsyncRequest (Xhc, Urb);
-- 
2.16.1.windows.1

[PATCH v2 06/12] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
---
 .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c  | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
index fb48010a9a..fda43279a3 100644
--- a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
+++ b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
@@ -556,6 +556,13 @@ XhcDataTransfer (
 
   XhcExecTransfer (Handle, Urb, Timeout);
 
+  //
+  // Make sure the data received from HW can fit in the received buffer.
+  //
+  if (Urb->Completed > *DataLength) {
+    return EFI_DEVICE_ERROR;
+  }
+
   *DataLength     = Urb->Completed;
 
   Status = EFI_TIMEOUT;
-- 
2.16.1.windows.1

[PATCH v2 07/12] MdeModulePkg/UsbKb: Don't access key codes when length is wrong

[Ruiyu Ni]

Per USB HID spec, the buffer holding key codes should be 8-byte
long.
Today's code assumes that the key codes buffer length is 8-byte
long and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 8-byte.

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>
Cc: Steven Shi <steven.shi@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
index 9cb4b5db6b..7505951c82 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -1059,6 +1059,10 @@ KeyboardHandler (
   // Byte 1 is reserved.
   // Bytes 2 to 7 are keycodes.
   //
+  if (DataLength < 8) {
+    return EFI_DEVICE_ERROR;
+  }
+
   CurKeyCodeBuffer  = (UINT8 *) Data;
   OldKeyCodeBuffer  = UsbKeyboardDevice->LastKeyCodeArray;
 
-- 
2.16.1.windows.1

[PATCH v2 08/12] MdeModulePkg/AbsPointer: Don't access key codes when length is wrong

[Ruiyu Ni]

Per USB HID spec, the buffer holding key codes should at least 3-byte
long.
Today's code assumes that the key codes buffer length is longer than
3-byte and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 3-byte.

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>
Cc: Steven Shi <steven.shi@intel.com>
---
 .../Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c  | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
index 965195ca34..42b9147ad8 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
@@ -813,8 +813,6 @@ OnMouseInterruptComplete (
     return EFI_SUCCESS;
   }
 
-  UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
-
   //
   // Check mouse Data
   // USB HID Specification specifies following data format:
@@ -827,6 +825,12 @@ OnMouseInterruptComplete (
   // 2       0 to 7  Y displacement
   // 3 to n  0 to 7  Device specific (optional)
   //
+  if (DataLength < 3) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
+
   UsbMouseAbsolutePointerDevice->State.ActiveButtons = *(UINT8 *) Data & (BIT0 | BIT1 | BIT2);
 
   UsbMouseAbsolutePointerDevice->State.CurrentX =
-- 
2.16.1.windows.1

[PATCH v2 09/12] MdeModulePkg/UsbMouse: Don't access key codes when length is wrong

[Ruiyu Ni]

Per USB HID spec, the buffer holding key codes should at least 3-byte
long.
Today's code assumes that the key codes buffer length is longer than
3-byte and unconditionally accesses the key codes buffer.
It's incorrect.
The patch fixes the issue by returning Device Error when the
length is less than 3-byte.

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>
Cc: Steven Shi <steven.shi@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 9324994975..f4448bffe6 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -811,8 +811,6 @@ OnMouseInterruptComplete (
     return EFI_SUCCESS;
   }
 
-  UsbMouseDevice->StateChanged = TRUE;
-
   //
   // Check mouse Data
   // USB HID Specification specifies following data format:
@@ -825,6 +823,12 @@ OnMouseInterruptComplete (
   // 2       0 to 7  Y displacement
   // 3 to n  0 to 7  Device specific (optional)
   //
+  if (DataLength < 3) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  UsbMouseDevice->StateChanged = TRUE;
+
   UsbMouseDevice->State.LeftButton  = (BOOLEAN) ((*(UINT8 *) Data & BIT0) != 0);
   UsbMouseDevice->State.RightButton = (BOOLEAN) ((*(UINT8 *) Data & BIT1) != 0);
   UsbMouseDevice->State.RelativeMovementX += *((INT8 *) Data + 1);
-- 
2.16.1.windows.1

[PATCH v2 10/12] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd

[Ruiyu Ni]

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

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 9fc6422ab1..22b6a9d661 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -655,7 +655,13 @@ UsbGetOneString (
   //
   Status = UsbCtrlGetDesc (UsbDev, USB_DESC_TYPE_STRING, Index, LangId, &Desc, 2);
 
-  if (EFI_ERROR (Status)) {
+  //
+  // Reject if Length even cannot cover itself, or odd because Unicode string byte length should be even.
+  //
+  if (EFI_ERROR (Status) || 
+      (Desc.Length < OFFSET_OF (EFI_USB_STRING_DESCRIPTOR, Length) + sizeof (Desc.Length)) ||
+      (Desc.Length % 2 != 0)
+    ) {
     return NULL;
   }
 
-- 
2.16.1.windows.1

[PATCH v2 11/12] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected

[Ruiyu Ni]

From: Hao Wu <hao.a.wu@intel.com>

This commit adds checks to make sure the UFS devices do not return more
data than the driver expected.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c        | 19 ++++++++++++--
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 30 +++++++++++++++++++---
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
index 936f25da5e..9b87835ed8 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
@@ -857,6 +857,14 @@ UfsRwDeviceDesc (
     SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
 
     if (Read) {
+      //
+      // Make sure the hardware device does not return more data than expected.
+      //
+      if (ReturnDataSize > Packet.InTransferLength) {
+        Status = EFI_DEVICE_ERROR;
+        goto Exit;
+      }
+
       CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
       Packet.InTransferLength = ReturnDataSize;
     } else {
@@ -1170,8 +1178,15 @@ UfsExecScsiCmds (
   SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
 
   if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
-    CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
-    Packet->SenseDataLength = (UINT8)SenseDataLen;
+    //
+    // Make sure the hardware device does not return more data than expected.
+    //
+    if (SenseDataLen <= Packet->SenseDataLength) {
+      CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+      Packet->SenseDataLength = (UINT8)SenseDataLen;
+    } else {
+      Packet->SenseDataLength = 0;
+    }
   }
 
   //
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 5756f637fd..0238264878 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -833,6 +833,7 @@ UfsStopExecCmd (
   @param[in] QueryResp  Pointer to the query response.
 
   @retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or opcode is invalid.
+  @retval EFI_DEVICE_ERROR      Data returned from device is invalid.
   @retval EFI_SUCCESS           Data extracted.
 
 **/
@@ -853,6 +854,13 @@ UfsGetReturnDataFromQueryResponse (
     case UtpQueryFuncOpcodeRdDesc:
       ReturnDataSize = QueryResp->Tsf.Length;
       SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
+      //
+      // Make sure the hardware device does not return more data than expected.
+      //
+      if (ReturnDataSize > Packet->TransferLength) {
+        return EFI_DEVICE_ERROR;
+      }
+
       CopyMem (Packet->DataBuffer, (QueryResp + 1), ReturnDataSize);
       Packet->TransferLength = ReturnDataSize;
       break;
@@ -1469,8 +1477,15 @@ UfsExecScsiCmds (
   SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
 
   if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
-    CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
-    Packet->SenseDataLength = (UINT8)SenseDataLen;
+    //
+    // Make sure the hardware device does not return more data than expected.
+    //
+    if (SenseDataLen <= Packet->SenseDataLength) {
+      CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+      Packet->SenseDataLength = (UINT8)SenseDataLen;
+    } else {
+      Packet->SenseDataLength = 0;
+    }
   }
 
   //
@@ -2226,8 +2241,15 @@ ProcessAsyncTaskList (
         SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
 
         if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
-          CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
-          Packet->SenseDataLength = (UINT8)SenseDataLen;
+          //
+          // Make sure the hardware device does not return more data than expected.
+          //
+          if (SenseDataLen <= Packet->SenseDataLength) {
+            CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+            Packet->SenseDataLength = (UINT8)SenseDataLen;
+          } else {
+            Packet->SenseDataLength = 0;
+          }
         }
 
         //
-- 
2.16.1.windows.1

[PATCH v2 12/12] MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K

[Ruiyu Ni]

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

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 0b35cbacf0..c35c7bdc12 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -736,6 +736,13 @@ UsbBootDetectMedia (
     return Status;
   }
 
+  //
+  // Simply reject device whose block size is unacceptable small (==0) or large (>64K).
+  //
+  if ((Media->BlockSize == 0) || (Media->BlockSize > USB_BOOT_MAX_CARRY_SIZE)) {
+    return EFI_DEVICE_ERROR;
+  }
+
   //
   // Detect whether it is necessary to reinstall the Block I/O Protocol.
   //
-- 
2.16.1.windows.1

Re: [PATCH v2 00/12] Need to validate data from HW

[Zeng, Star]

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


Thanks,
Star

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, October 16, 2018 1:43 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v2 00/12] Need to validate data from HW

The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.

V2: adopt all comments from Star. The block size 0/64K handle is in a separate patch
"MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K"

Hao Wu (1):
  MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected

Ruiyu Ni (11):
  MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
  MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
  MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
  MdeModulePkg/UsbBus: Reject descriptor whose length is bad
  MdeModulePkg/Usb: Make sure data from HW is no more than expected
  SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
  MdeModulePkg/UsbKb: Don't access key codes when length is wrong
  MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
  MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
  MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
  MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K

 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c           |   9 +-
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c           |   7 +-
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c           |   9 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c        |  19 +-
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        |  30 ++-
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c           |  70 +++++-
 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c           |   4 +
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 276 ++++++---------------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  76 ++----
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        |   8 +-
 .../UsbMouseAbsolutePointer.c                      |   8 +-
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c        |   8 +-
 .../DebugCommunicationLibUsb3Transfer.c            |   7 +
 13 files changed, 237 insertions(+), 294 deletions(-)

--
2.16.1.windows.1

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

Re: [PATCH v2 00/12] Need to validate data from HW

[Zeng, Star]

Oh, Remember the missing update in UsbMassBoot.h about adding back '.', you can do that when pushing patches.

-  Read some blocks from the device by SCSI 16 byte cmd.
+  Read or write some blocks from the device by SCSI 16 byte cmd


Thanks,
Star

-----Original Message-----
From: Zeng, Star 
Sent: Wednesday, October 17, 2018 10:44 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2 00/12] Need to validate data from HW

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


Thanks,
Star

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, October 16, 2018 1:43 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v2 00/12] Need to validate data from HW

The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.

V2: adopt all comments from Star. The block size 0/64K handle is in a separate patch
"MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K"

Hao Wu (1):
  MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected

Ruiyu Ni (11):
  MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
  MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
  MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
  MdeModulePkg/UsbBus: Reject descriptor whose length is bad
  MdeModulePkg/Usb: Make sure data from HW is no more than expected
  SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
  MdeModulePkg/UsbKb: Don't access key codes when length is wrong
  MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
  MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
  MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
  MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K

 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c           |   9 +-
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c           |   7 +-
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c           |   9 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c        |  19 +-
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        |  30 ++-
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c           |  70 +++++-
 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c           |   4 +
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 276 ++++++---------------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  76 ++----
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        |   8 +-
 .../UsbMouseAbsolutePointer.c                      |   8 +-
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c        |   8 +-
 .../DebugCommunicationLibUsb3Transfer.c            |   7 +
 13 files changed, 237 insertions(+), 294 deletions(-)

--
2.16.1.windows.1

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