* [PATCH 00/11] Need to validate data from HW
@ 2018-10-15 6:38 Ruiyu Ni
2018-10-15 6:38 ` [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
` (10 more replies)
0 siblings, 11 replies; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1250
The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.
Hao Wu (1):
MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
Ruiyu Ni (10):
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/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 | 59 ++++-
MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 +
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 269 +++++----------------
.../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, 219 insertions(+), 294 deletions(-)
--
2.16.1.windows.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 3:06 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
` (9 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng
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..581571ab45 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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
2018-10-15 6:38 ` [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 3:19 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
` (8 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Steven Shi
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 581571ab45..37fbeedbeb 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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
2018-10-15 6:38 ` [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
2018-10-15 6:38 ` [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 3:38 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
` (7 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao
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 | 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)) {
+ 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));
return NULL;
}
@@ -212,7 +238,7 @@ UsbCreateDesc (
CopyMem (Desc, Head, (UINTN) DescLen);
- *Consumed = Offset + Head->Len;
+ *Consumed = Offset;
return Desc;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (2 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 3:42 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
` (6 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao
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>
---
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 a93060deea..d9bc1f9e28 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -767,6 +767,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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (3 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-15 11:29 ` Wu, Hao A
2018-10-16 4:26 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
` (5 subsequent siblings)
10 siblings, 2 replies; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Star Zeng, Hao A Wu
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>
---
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (4 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-15 11:31 ` Wu, Hao A
2018-10-15 6:38 ` [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
` (4 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao A Wu
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>
---
.../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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (5 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 4:52 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 08/11] MdeModulePkg/AbsPointer: " Ruiyu Ni
` (3 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Steven Shi
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..384d7e2f07 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 ((Data != NULL) && (DataLength < 8)) {
+ return EFI_DEVICE_ERROR;
+ }
+
CurKeyCodeBuffer = (UINT8 *) Data;
OldKeyCodeBuffer = UsbKeyboardDevice->LastKeyCodeArray;
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 08/11] MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (6 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 5:01 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 09/11] MdeModulePkg/UsbMouse: " Ruiyu Ni
` (2 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Steven Shi
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..b4638961d9 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 ((Data != NULL) && (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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 09/11] MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (7 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 08/11] MdeModulePkg/AbsPointer: " Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 5:03 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
2018-10-15 6:38 ` [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Steven Shi
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..d4bcf15929 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 ((Data != NULL) && (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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (8 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 09/11] MdeModulePkg/UsbMouse: " Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 5:11 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Star Zeng
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index d9bc1f9e28..182a3f97c9 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -650,7 +650,7 @@ UsbGetOneString (
//
Status = UsbCtrlGetDesc (UsbDev, USB_DESC_TYPE_STRING, Index, LangId, &Desc, 2);
- if (EFI_ERROR (Status)) {
+ if (EFI_ERROR (Status) || (Desc.Length % 2 != 0)) {
return NULL;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
` (9 preceding siblings ...)
2018-10-15 6:38 ` [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
@ 2018-10-15 6:38 ` Ruiyu Ni
2018-10-16 5:27 ` Zeng, Star
10 siblings, 1 reply; 25+ messages in thread
From: Ruiyu Ni @ 2018-10-15 6:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Star Zeng
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>
---
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected
2018-10-15 6:38 ` [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
@ 2018-10-15 11:29 ` Wu, Hao A
2018-10-16 4:26 ` Zeng, Star
1 sibling, 0 replies; 25+ messages in thread
From: Wu, Hao A @ 2018-10-15 11:29 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Zeng, Star
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Monday, October 15, 2018 2:38 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Yao, Jiewen; Zeng, Star
> Subject: [edk2] [PATCH 05/11] MdeModulePkg/Usb: Make sure data from
> HW is no more than expected
>
> 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>
> ---
> 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
2018-10-15 6:38 ` [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
@ 2018-10-15 11:31 ` Wu, Hao A
0 siblings, 0 replies; 25+ messages in thread
From: Wu, Hao A @ 2018-10-15 11:31 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, October 15, 2018 2:38 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from
> HW can fit in buffer
>
> 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>
> ---
> .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c |
> 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugComm
> unicationLibUsb3Transfer.c
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugComm
> unicationLibUsb3Transfer.c
> index fb48010a9a..fda43279a3 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugComm
> unicationLibUsb3Transfer.c
> +++
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugComm
> unicationLibUsb3Transfer.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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
2018-10-15 6:38 ` [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
@ 2018-10-16 3:06 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 3:06 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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>
Ray,
Thanks for the patch.
I have a very very minor comment below.
Reviewed-by: 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..581571ab45 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
I saw the patch kept the ending '.' for UsbBootReadWriteBlocks, but
removed the ending '.' for this function.
Is it better to be consistent? :)
Thanks,
Star
>
> - @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)) {
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
2018-10-15 6:38 ` [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
@ 2018-10-16 3:19 ` Zeng, Star
2018-10-16 3:32 ` Ni, Ruiyu
0 siblings, 1 reply; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 3:19 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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
Ray,
Thanks for the patch.
Is it possible that the BlockSize == 0 or > USB_BOOT_MAX_CARRY_SIZE?
Thanks,
Star
> 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 581571ab45..37fbeedbeb 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;
> }
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
2018-10-16 3:19 ` Zeng, Star
@ 2018-10-16 3:32 ` Ni, Ruiyu
0 siblings, 0 replies; 25+ messages in thread
From: Ni, Ruiyu @ 2018-10-16 3:32 UTC (permalink / raw)
To: Zeng, Star, edk2-devel
On 10/16/2018 11:19 AM, Zeng, Star wrote:
> On 2018/10/15 14:38, Ruiyu Ni wrote:
>> 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
>
> Ray,
>
> Thanks for the patch.
> Is it possible that the BlockSize == 0 or > USB_BOOT_MAX_CARRY_SIZE?
With a buggy HW, everything is possible:(
I will re-post V2 patch to handle this case.
>
>
> Thanks,
> Star
>
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
2018-10-15 6:38 ` [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
@ 2018-10-16 3:38 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 3:38 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
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 <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
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 <star.zeng@intel.com>
> ---
> 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;
> }
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad
2018-10-15 6:38 ` [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
@ 2018-10-16 3:42 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 3:42 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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>
Ray,
Thanks for the patch.
Reviewed-by: Star Zeng <star.zeng@intel.com>
Star
> ---
> 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 a93060deea..d9bc1f9e28 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> @@ -767,6 +767,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) {
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected
2018-10-15 6:38 ` [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
2018-10-15 11:29 ` Wu, Hao A
@ 2018-10-16 4:26 ` Zeng, Star
1 sibling, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 4:26 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Hao A Wu, Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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>
Ray,
Thanks for the patch.
Reviewed-by: Star Zeng <star.zeng@intel.com>
Star
> ---
> 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);
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong
2018-10-15 6:38 ` [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
@ 2018-10-16 4:52 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 4:52 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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..384d7e2f07 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 ((Data != NULL) && (DataLength < 8)) {
> + return EFI_DEVICE_ERROR;
> + }
Ray,
Thanks for the patch.
The NULL check to Data has been done by code piece
//
// If no error and no data, just return EFI_SUCCESS.
//
if (DataLength == 0 || Data == NULL) {
return EFI_SUCCESS;
}
And do you think whether the code can use DataLength != 8 instead of
DataLength < 8?
Thanks,
Star
> +
> CurKeyCodeBuffer = (UINT8 *) Data;
> OldKeyCodeBuffer = UsbKeyboardDevice->LastKeyCodeArray;
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/11] MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
2018-10-15 6:38 ` [PATCH 08/11] MdeModulePkg/AbsPointer: " Ruiyu Ni
@ 2018-10-16 5:01 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 5:01 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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..b4638961d9 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 ((Data != NULL) && (DataLength < 3)) {
> + return EFI_DEVICE_ERROR;
> + }
Ray,
Thanks for the patch.
Data is impossible to be NULL here as the NULL check to Data has been
done by code piece
//
// If no error and no data, just return EFI_SUCCESS.
//
if (DataLength == 0 || Data == NULL) {
return EFI_SUCCESS;
}
Thanks,
Star
> +
> + UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
> +
> UsbMouseAbsolutePointerDevice->State.ActiveButtons = *(UINT8 *) Data & (BIT0 | BIT1 | BIT2);
>
> UsbMouseAbsolutePointerDevice->State.CurrentX =
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 09/11] MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
2018-10-15 6:38 ` [PATCH 09/11] MdeModulePkg/UsbMouse: " Ruiyu Ni
@ 2018-10-16 5:03 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 5:03 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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..d4bcf15929 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 ((Data != NULL) && (DataLength < 3)) {
> + return EFI_DEVICE_ERROR;
> + }
Ray,
Thanks for the patch.
Data is impossible to be NULL here as the NULL check to Data has been
done by code piece
//
// If no error and no data, just return EFI_SUCCESS.
//
if (DataLength == 0 || Data == NULL) {
return EFI_SUCCESS;
}
Thanks,
Star
> +
> + 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);
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
2018-10-15 6:38 ` [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
@ 2018-10-16 5:11 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 5:11 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> index d9bc1f9e28..182a3f97c9 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> @@ -650,7 +650,7 @@ UsbGetOneString (
> //
> Status = UsbCtrlGetDesc (UsbDev, USB_DESC_TYPE_STRING, Index, LangId, &Desc, 2);
>
> - if (EFI_ERROR (Status)) {
> + if (EFI_ERROR (Status) || (Desc.Length % 2 != 0)) {
Ray,
Thanks for the patch.
Does the code need check (Desc.Length < 2) first as Desc.Length may be 0
or 1, right?
Thanks,
Star
> return NULL;
> }
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
2018-10-15 6:38 ` [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
@ 2018-10-16 5:27 ` Zeng, Star
0 siblings, 0 replies; 25+ messages in thread
From: Zeng, Star @ 2018-10-16 5:27 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Hao Wu, Jiewen Yao, star.zeng
On 2018/10/15 14:38, Ruiyu Ni wrote:
> 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>
Thanks for the patch.
Reviewed-by: Star Zeng <star.zeng@intel.com>
Star
> ---
> 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;
> + }
> }
>
> //
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-10-16 5:28 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-15 6:38 [PATCH 00/11] Need to validate data from HW Ruiyu Ni
2018-10-15 6:38 ` [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
2018-10-16 3:06 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
2018-10-16 3:19 ` Zeng, Star
2018-10-16 3:32 ` Ni, Ruiyu
2018-10-15 6:38 ` [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
2018-10-16 3:38 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 04/11] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
2018-10-16 3:42 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 05/11] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
2018-10-15 11:29 ` Wu, Hao A
2018-10-16 4:26 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 06/11] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
2018-10-15 11:31 ` Wu, Hao A
2018-10-15 6:38 ` [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
2018-10-16 4:52 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 08/11] MdeModulePkg/AbsPointer: " Ruiyu Ni
2018-10-16 5:01 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 09/11] MdeModulePkg/UsbMouse: " Ruiyu Ni
2018-10-16 5:03 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 10/11] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
2018-10-16 5:11 ` Zeng, Star
2018-10-15 6:38 ` [PATCH 11/11] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
2018-10-16 5:27 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox