* [PATCH v2 01/12] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 02/12] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
` (11 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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..07a376440c 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -792,32 +792,34 @@ UsbBootDetectMedia (
/**
- Read some blocks from the device.
+ Read or write some blocks from the device.
- @param UsbMass The USB mass storage device to read from
+ @param UsbMass The USB mass storage device to access
+ @param Write TRUE for write operation.
@param Lba The start block number
- @param TotalBlock Total block number to read
- @param Buffer The buffer to read to
+ @param TotalBlock Total block number to read or write
+ @param Buffer The buffer to read to or write from
- @retval EFI_SUCCESS Data are read into the buffer
- @retval Others Failed to read all the data
+ @retval EFI_SUCCESS Data are read into the buffer or writen into the device.
+ @retval Others Failed to read or write all the data
**/
EFI_STATUS
-UsbBootReadBlocks (
+UsbBootReadWriteBlocks (
IN USB_MASS_DEVICE *UsbMass,
+ IN BOOLEAN Write,
IN UINT32 Lba,
IN UINTN TotalBlock,
- OUT UINT8 *Buffer
+ IN OUT UINT8 *Buffer
)
{
- USB_BOOT_READ10_CMD ReadCmd;
- EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
- UINT32 BlockSize;
- UINT32 ByteSize;
- UINT32 Timeout;
+ USB_BOOT_READ_WRITE_10_CMD Cmd;
+ EFI_STATUS Status;
+ UINT16 Count;
+ UINT16 CountMax;
+ UINT32 BlockSize;
+ UINT32 ByteSize;
+ UINT32 Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
@@ -840,17 +842,17 @@ UsbBootReadBlocks (
//
// Fill in the command then execute
//
- ZeroMem (&ReadCmd, sizeof (USB_BOOT_READ10_CMD));
+ ZeroMem (&Cmd, sizeof (USB_BOOT_READ_WRITE_10_CMD));
- ReadCmd.OpCode = USB_BOOT_READ10_OPCODE;
- ReadCmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
- WriteUnaligned32 ((UINT32 *) ReadCmd.Lba, SwapBytes32 (Lba));
- WriteUnaligned16 ((UINT16 *) ReadCmd.TransferLen, SwapBytes16 (Count));
+ Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
+ Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
+ WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
+ WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
Status = UsbBootExecCmdWithRetry (
UsbMass,
- &ReadCmd,
- (UINT8) sizeof (USB_BOOT_READ10_CMD),
+ &Cmd,
+ (UINT8) sizeof (USB_BOOT_READ_WRITE_10_CMD),
EfiUsbDataIn,
Buffer,
ByteSize,
@@ -859,86 +861,11 @@ UsbBootReadBlocks (
if (EFI_ERROR (Status)) {
return Status;
}
- DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count));
- Lba += Count;
- Buffer += Count * BlockSize;
- TotalBlock -= Count;
- }
-
- return Status;
-}
-
-
-/**
- Write some blocks to the device.
-
- @param UsbMass The USB mass storage device to write to
- @param Lba The start block number
- @param TotalBlock Total block number to write
- @param Buffer Pointer to the source buffer for the data.
-
- @retval EFI_SUCCESS Data are written into the buffer
- @retval Others Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks (
- IN USB_MASS_DEVICE *UsbMass,
- IN UINT32 Lba,
- IN UINTN TotalBlock,
- IN UINT8 *Buffer
- )
-{
- USB_BOOT_WRITE10_CMD WriteCmd;
- EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
- UINT32 BlockSize;
- UINT32 ByteSize;
- UINT32 Timeout;
-
- BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
- Status = EFI_SUCCESS;
-
- while (TotalBlock > 0) {
- //
- // Split the total blocks into smaller pieces to ease the pressure
- // on the device. We must split the total block because the WRITE10
- // command only has 16 bit transfer length (in the unit of block).
- //
- Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
- ByteSize = (UINT32)Count * BlockSize;
-
- //
- // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
- //
- Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT;
-
- //
- // Fill in the write10 command block
- //
- ZeroMem (&WriteCmd, sizeof (USB_BOOT_WRITE10_CMD));
-
- WriteCmd.OpCode = USB_BOOT_WRITE10_OPCODE;
- WriteCmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
- WriteUnaligned32 ((UINT32 *) WriteCmd.Lba, SwapBytes32 (Lba));
- WriteUnaligned16 ((UINT16 *) WriteCmd.TransferLen, SwapBytes16 (Count));
-
- Status = UsbBootExecCmdWithRetry (
- UsbMass,
- &WriteCmd,
- (UINT8) sizeof (USB_BOOT_WRITE10_CMD),
- EfiUsbDataOut,
- Buffer,
- ByteSize,
- Timeout
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
- DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count));
-
+ DEBUG ((
+ DEBUG_BLKIO, "UsbBoot%sBlocks: LBA (0x%lx), Blk (0x%x)\n",
+ Write ? L"Write" : L"Read",
+ Lba, Count
+ ));
Lba += Count;
Buffer += Count * BlockSize;
TotalBlock -= Count;
@@ -948,26 +875,27 @@ UsbBootWriteBlocks (
}
/**
- Read some blocks from the device by SCSI 16 byte cmd.
+ Read or write some blocks from the device by SCSI 16 byte cmd.
- @param UsbMass The USB mass storage device to read from
+ @param UsbMass The USB mass storage device to access
+ @param Write TRUE for write operation.
@param Lba The start block number
- @param TotalBlock Total block number to read
- @param Buffer The buffer to read to
-
- @retval EFI_SUCCESS Data are read into the buffer
- @retval Others Failed to read all the data
+ @param TotalBlock Total block number to read or write
+ @param Buffer The buffer to read to or write from
+ @retval EFI_SUCCESS Data are read into the buffer or writen into the device.
+ @retval Others Failed to read or write all the data
**/
EFI_STATUS
-UsbBootReadBlocks16 (
+UsbBootReadWriteBlocks16 (
IN USB_MASS_DEVICE *UsbMass,
+ IN BOOLEAN Write,
IN UINT64 Lba,
IN UINTN TotalBlock,
- OUT UINT8 *Buffer
+ IN OUT UINT8 *Buffer
)
{
- UINT8 ReadCmd[16];
+ UINT8 Cmd[16];
EFI_STATUS Status;
UINT16 Count;
UINT16 CountMax;
@@ -994,17 +922,17 @@ UsbBootReadBlocks16 (
//
// Fill in the command then execute
//
- ZeroMem (ReadCmd, sizeof (ReadCmd));
+ ZeroMem (Cmd, sizeof (Cmd));
- ReadCmd[0] = EFI_SCSI_OP_READ16;
- ReadCmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
- WriteUnaligned64 ((UINT64 *) &ReadCmd[2], SwapBytes64 (Lba));
- WriteUnaligned32 ((UINT32 *) &ReadCmd[10], SwapBytes32 (Count));
+ Cmd[0] = Write ? EFI_SCSI_OP_WRITE16 : EFI_SCSI_OP_READ16;
+ Cmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
+ WriteUnaligned64 ((UINT64 *) &Cmd[2], SwapBytes64 (Lba));
+ WriteUnaligned32 ((UINT32 *) &Cmd[10], SwapBytes32 (Count));
Status = UsbBootExecCmdWithRetry (
UsbMass,
- ReadCmd,
- (UINT8) sizeof (ReadCmd),
+ Cmd,
+ (UINT8) sizeof (Cmd),
EfiUsbDataIn,
Buffer,
ByteSize,
@@ -1013,83 +941,11 @@ UsbBootReadBlocks16 (
if (EFI_ERROR (Status)) {
return Status;
}
- DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks16: LBA (0x%lx), Blk (0x%x)\n", Lba, Count));
- Lba += Count;
- Buffer += Count * BlockSize;
- TotalBlock -= Count;
- }
-
- return Status;
-}
-
-
-/**
- Write some blocks to the device by SCSI 16 byte cmd.
-
- @param UsbMass The USB mass storage device to write to
- @param Lba The start block number
- @param TotalBlock Total block number to write
- @param Buffer Pointer to the source buffer for the data.
-
- @retval EFI_SUCCESS Data are written into the buffer
- @retval Others Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks16 (
- IN USB_MASS_DEVICE *UsbMass,
- IN UINT64 Lba,
- IN UINTN TotalBlock,
- IN UINT8 *Buffer
- )
-{
- UINT8 WriteCmd[16];
- EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
- UINT32 BlockSize;
- UINT32 ByteSize;
- UINT32 Timeout;
-
- BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
- Status = EFI_SUCCESS;
-
- while (TotalBlock > 0) {
- //
- // Split the total blocks into smaller pieces.
- //
- Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
- ByteSize = (UINT32)Count * BlockSize;
-
- //
- // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
- //
- Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT;
-
- //
- // Fill in the write16 command block
- //
- ZeroMem (WriteCmd, sizeof (WriteCmd));
-
- WriteCmd[0] = EFI_SCSI_OP_WRITE16;
- WriteCmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0));
- WriteUnaligned64 ((UINT64 *) &WriteCmd[2], SwapBytes64 (Lba));
- WriteUnaligned32 ((UINT32 *) &WriteCmd[10], SwapBytes32 (Count));
-
- Status = UsbBootExecCmdWithRetry (
- UsbMass,
- WriteCmd,
- (UINT8) sizeof (WriteCmd),
- EfiUsbDataOut,
- Buffer,
- ByteSize,
- Timeout
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
- DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%lx), Blk (0x%x)\n", Lba, Count));
+ DEBUG ((
+ DEBUG_BLKIO, "UsbBoot%sBlocks16: LBA (0x%lx), Blk (0x%x)\n",
+ Write ? L"Write" : L"Read",
+ Lba, Count
+ ));
Lba += Count;
Buffer += Count * BlockSize;
TotalBlock -= Count;
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
index d4042e320d..3f8213b3d7 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
@@ -161,17 +161,7 @@ typedef struct {
UINT8 TransferLen[2]; ///< Transfer length
UINT8 Reserverd1;
UINT8 Pad[2];
-} USB_BOOT_READ10_CMD;
-
-typedef struct {
- UINT8 OpCode;
- UINT8 Lun;
- UINT8 Lba[4];
- UINT8 Reserved0;
- UINT8 TransferLen[2];
- UINT8 Reserverd1;
- UINT8 Pad[2];
-} USB_BOOT_WRITE10_CMD;
+} USB_BOOT_READ_WRITE_10_CMD;
typedef struct {
UINT8 OpCode;
@@ -292,66 +282,48 @@ UsbBootReadBlocks (
);
/**
- Write some blocks to the device.
+ Read or write some blocks from the device.
- @param UsbMass The USB mass storage device to write to
+ @param UsbMass The USB mass storage device to access
+ @param Write TRUE for write operation.
@param Lba The start block number
- @param TotalBlock Total block number to write
- @param Buffer Pointer to the source buffer for the data.
+ @param TotalBlock Total block number to read or write
+ @param Buffer The buffer to read to or write from
- @retval EFI_SUCCESS Data are written into the buffer
- @retval Others Failed to write all the data
+ @retval EFI_SUCCESS Data are read into the buffer or writen into the device.
+ @retval Others Failed to read or write all the data
**/
EFI_STATUS
-UsbBootWriteBlocks (
- IN USB_MASS_DEVICE *UsbMass,
- IN UINT32 Lba,
- IN UINTN TotalBlock,
- IN UINT8 *Buffer
+UsbBootReadWriteBlocks (
+ IN USB_MASS_DEVICE *UsbMass,
+ IN BOOLEAN Write,
+ IN UINT32 Lba,
+ IN UINTN TotalBlock,
+ IN OUT UINT8 *Buffer
);
/**
- Read some blocks from the device by SCSI 16 byte cmd.
+ Read or write some blocks from the device by SCSI 16 byte cmd
- @param UsbMass The USB mass storage device to read from
+ @param UsbMass The USB mass storage device to access
+ @param Write TRUE for write operation.
@param Lba The start block number
- @param TotalBlock Total block number to read
- @param Buffer The buffer to read to
-
- @retval EFI_SUCCESS Data are read into the buffer
- @retval Others Failed to read all the data
+ @param TotalBlock Total block number to read or write
+ @param Buffer The buffer to read to or write from
+ @retval EFI_SUCCESS Data are read into the buffer or writen into the device.
+ @retval Others Failed to read or write all the data
**/
EFI_STATUS
-UsbBootReadBlocks16 (
+UsbBootReadWriteBlocks16 (
IN USB_MASS_DEVICE *UsbMass,
+ IN BOOLEAN Write,
IN UINT64 Lba,
IN UINTN TotalBlock,
- OUT UINT8 *Buffer
+ IN OUT UINT8 *Buffer
);
-/**
- Write some blocks to the device by SCSI 16 byte cmd.
-
- @param UsbMass The USB mass storage device to write to
- @param Lba The start block number
- @param TotalBlock Total block number to write
- @param Buffer Pointer to the source buffer for the data.
-
- @retval EFI_SUCCESS Data are written into the buffer
- @retval Others Failed to write all the data
-
-**/
-EFI_STATUS
-UsbBootWriteBlocks16 (
- IN USB_MASS_DEVICE *UsbMass,
- IN UINT64 Lba,
- IN UINTN TotalBlock,
- IN UINT8 *Buffer
- );
-
-
/**
Use the USB clear feature control transfer to clear the endpoint stall condition.
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
index 62bf3c5883..9413b00680 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
@@ -172,9 +172,9 @@ UsbMassReadBlocks (
}
if (UsbMass->Cdb16Byte) {
- Status = UsbBootReadBlocks16 (UsbMass, Lba, TotalBlock, Buffer);
+ Status = UsbBootReadWriteBlocks16 (UsbMass, FALSE, Lba, TotalBlock, Buffer);
} else {
- Status = UsbBootReadBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer);
+ Status = UsbBootReadWriteBlocks (UsbMass, FALSE, (UINT32) Lba, TotalBlock, Buffer);
}
if (EFI_ERROR (Status)) {
@@ -292,9 +292,9 @@ UsbMassWriteBlocks (
// and clear the status should the write succeed.
//
if (UsbMass->Cdb16Byte) {
- Status = UsbBootWriteBlocks16 (UsbMass, Lba, TotalBlock, Buffer);
+ Status = UsbBootReadWriteBlocks16 (UsbMass, TRUE, Lba, TotalBlock, Buffer);
} else {
- Status = UsbBootWriteBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer);
+ Status = UsbBootReadWriteBlocks (UsbMass, TRUE, (UINT32) Lba, TotalBlock, Buffer);
}
if (EFI_ERROR (Status)) {
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 02/12] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 01/12] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 03/12] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
` (10 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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 07a376440c..0b35cbacf0 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -815,14 +815,14 @@ UsbBootReadWriteBlocks (
{
USB_BOOT_READ_WRITE_10_CMD Cmd;
EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
+ UINT32 Count;
+ UINT32 CountMax;
UINT32 BlockSize;
UINT32 ByteSize;
UINT32 Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+ CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
Status = EFI_SUCCESS;
while (TotalBlock > 0) {
@@ -831,8 +831,9 @@ UsbBootReadWriteBlocks (
// on the device. We must split the total block because the READ10
// command only has 16 bit transfer length (in the unit of block).
//
- Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
- ByteSize = (UINT32)Count * BlockSize;
+ Count = (UINT32)MIN (TotalBlock, CountMax);
+ Count = MIN (MAX_UINT16, Count);
+ ByteSize = Count * BlockSize;
//
// USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -847,7 +848,7 @@ UsbBootReadWriteBlocks (
Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE;
Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun));
WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba));
- WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count));
+ WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 ((UINT16)Count));
Status = UsbBootExecCmdWithRetry (
UsbMass,
@@ -867,7 +868,7 @@ UsbBootReadWriteBlocks (
Lba, Count
));
Lba += Count;
- Buffer += Count * BlockSize;
+ Buffer += ByteSize;
TotalBlock -= Count;
}
@@ -897,22 +898,22 @@ UsbBootReadWriteBlocks16 (
{
UINT8 Cmd[16];
EFI_STATUS Status;
- UINT16 Count;
- UINT16 CountMax;
+ UINT32 Count;
+ UINT32 CountMax;
UINT32 BlockSize;
UINT32 ByteSize;
UINT32 Timeout;
BlockSize = UsbMass->BlockIoMedia.BlockSize;
- CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize);
+ CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
Status = EFI_SUCCESS;
while (TotalBlock > 0) {
//
// Split the total blocks into smaller pieces.
//
- Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax);
- ByteSize = (UINT32)Count * BlockSize;
+ Count = (UINT32)MIN (TotalBlock, CountMax);
+ ByteSize = Count * BlockSize;
//
// USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1]
@@ -947,7 +948,7 @@ UsbBootReadWriteBlocks16 (
Lba, Count
));
Lba += Count;
- Buffer += Count * BlockSize;
+ Buffer += ByteSize;
TotalBlock -= Count;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 03/12] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 01/12] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 02/12] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 04/12] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
` (9 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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 | 55 +++++++++++++++++++++++++-------
1 file changed, 43 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 061e4622e8..70442c57da 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -177,6 +177,18 @@ UsbCreateDesc (
DescLen = sizeof (EFI_USB_ENDPOINT_DESCRIPTOR);
CtrlLen = sizeof (USB_ENDPOINT_DESC);
break;
+
+ default:
+ ASSERT (FALSE);
+ return NULL;
+ }
+
+ //
+ // Total length is too small that cannot hold the single descriptor header plus data.
+ //
+ if (Len <= sizeof (USB_DESC_HEAD)) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, total length = %d!\n", Len));
+ return NULL;
}
//
@@ -184,24 +196,43 @@ UsbCreateDesc (
// format. Skip the descriptor that isn't of this Type
//
Offset = 0;
- Head = (USB_DESC_HEAD*)DescBuf;
+ Head = (USB_DESC_HEAD *)DescBuf;
+ while (Offset < Len - sizeof (USB_DESC_HEAD)) {
+ //
+ // Above condition make sure Head->Len and Head->Type are safe to access
+ //
+ Head = (USB_DESC_HEAD *)&DescBuf[Offset];
- while ((Offset < Len) && (Head->Type != Type)) {
- Offset += Head->Len;
- if (Len <= Offset) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Beyond boundary!\n"));
+ if (Head->Len == 0) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = 0!\n"));
return NULL;
}
- Head = (USB_DESC_HEAD*)(DescBuf + Offset);
- if (Head->Len == 0) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = 0!\n"));
+
+ //
+ // Make sure no overflow when adding Head->Len to Offset.
+ //
+ if (Head->Len > MAX_UINTN - Offset) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = %d!\n", Head->Len));
return NULL;
}
+
+ Offset += Head->Len;
+
+ if (Head->Type == Type) {
+ break;
+ }
+ }
+
+ //
+ // Head->Len is invalid resulting data beyond boundary, or
+ // Descriptor cannot be found: No such type.
+ //
+ if (Len < Offset) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Offset/Len = %d/%d!\n", Offset, Len));
}
- if ((Len <= Offset) || (Len < Offset + Head->Len) ||
- (Head->Type != Type) || (Head->Len < DescLen)) {
- DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor\n"));
+ if ((Head->Type != Type) || (Head->Len < DescLen)) {
+ DEBUG ((DEBUG_ERROR, "UsbCreateDesc: descriptor cannot be found, Header(T/L) = %d/%d!\n", Head->Type, Head->Len));
return NULL;
}
@@ -212,7 +243,7 @@ UsbCreateDesc (
CopyMem (Desc, Head, (UINTN) DescLen);
- *Consumed = Offset + Head->Len;
+ *Consumed = Offset;
return Desc;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 04/12] MdeModulePkg/UsbBus: Reject descriptor whose length is bad
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (2 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 03/12] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 05/12] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
` (8 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 70442c57da..9fc6422ab1 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -772,6 +772,13 @@ UsbGetOneConfig (
DEBUG (( EFI_D_INFO, "UsbGetOneConfig: total length is %d\n", Desc.TotalLength));
+ //
+ // Reject if TotalLength even cannot cover itself.
+ //
+ if (Desc.TotalLength < OFFSET_OF (EFI_USB_CONFIG_DESCRIPTOR, TotalLength) + sizeof (Desc.TotalLength)) {
+ return NULL;
+ }
+
Buf = AllocateZeroPool (Desc.TotalLength);
if (Buf == NULL) {
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 05/12] MdeModulePkg/Usb: Make sure data from HW is no more than expected
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (3 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 04/12] MdeModulePkg/UsbBus: Reject descriptor whose length is bad Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 06/12] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
` (7 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 ++++++---
MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 ++++---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++++++---
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index fea6f47f4c..168280be81 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -1009,9 +1009,12 @@ EhcMonitorAsyncRequests (
ProcBuf = NULL;
if (Urb->Result == EFI_USB_NOERROR) {
- ASSERT (Urb->Completed <= Urb->DataLen);
-
- ProcBuf = AllocatePool (Urb->Completed);
+ //
+ // Make sure the data received from HW is no more than expected.
+ //
+ if (Urb->Completed <= Urb->DataLen) {
+ ProcBuf = AllocatePool (Urb->Completed);
+ }
if (ProcBuf == NULL) {
EhcUpdateAsyncRequest (Ehc, Urb);
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
index 90f010c998..f7510f3ec0 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
@@ -2,7 +2,7 @@
The EHCI register operation routines.
-Copyright (c) 2007 - 2013, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -1001,11 +1001,12 @@ UhciMonitorAsyncReqList (
//
// Copy the data to temporary buffer if there are some
- // data transferred. We may have zero-length packet
+ // data transferred. We may have zero-length packet.
+ // Make sure the data received from HW is no more than expected.
//
Data = NULL;
- if (QhResult.Complete != 0) {
+ if ((QhResult.Complete != 0) && (QhResult.Complete <= AsyncReq->DataLen)) {
Data = AllocatePool (QhResult.Complete);
if (Data == NULL) {
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 6a2ef4cd5d..166c44bf5e 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1556,9 +1556,12 @@ XhcMonitorAsyncRequests (
//
ProcBuf = NULL;
if (Urb->Result == EFI_USB_NOERROR) {
- ASSERT (Urb->Completed <= Urb->DataLen);
-
- ProcBuf = AllocateZeroPool (Urb->Completed);
+ //
+ // Make sure the data received from HW is no more than expected.
+ //
+ if (Urb->Completed <= Urb->DataLen) {
+ ProcBuf = AllocateZeroPool (Urb->Completed);
+ }
if (ProcBuf == NULL) {
XhcUpdateAsyncRequest (Xhc, Urb);
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 06/12] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (4 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 05/12] MdeModulePkg/Usb: Make sure data from HW is no more than expected Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 07/12] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
` (6 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
---
.../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
index fb48010a9a..fda43279a3 100644
--- a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
+++ b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
@@ -556,6 +556,13 @@ XhcDataTransfer (
XhcExecTransfer (Handle, Urb, Timeout);
+ //
+ // Make sure the data received from HW can fit in the received buffer.
+ //
+ if (Urb->Completed > *DataLength) {
+ return EFI_DEVICE_ERROR;
+ }
+
*DataLength = Urb->Completed;
Status = EFI_TIMEOUT;
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 07/12] MdeModulePkg/UsbKb: Don't access key codes when length is wrong
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (5 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 06/12] SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 08/12] MdeModulePkg/AbsPointer: " Ruiyu Ni
` (5 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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..7505951c82 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -1059,6 +1059,10 @@ KeyboardHandler (
// Byte 1 is reserved.
// Bytes 2 to 7 are keycodes.
//
+ if (DataLength < 8) {
+ return EFI_DEVICE_ERROR;
+ }
+
CurKeyCodeBuffer = (UINT8 *) Data;
OldKeyCodeBuffer = UsbKeyboardDevice->LastKeyCodeArray;
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 08/12] MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (6 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 07/12] MdeModulePkg/UsbKb: Don't access key codes when length is wrong Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 09/12] MdeModulePkg/UsbMouse: " Ruiyu Ni
` (4 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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..42b9147ad8 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
@@ -813,8 +813,6 @@ OnMouseInterruptComplete (
return EFI_SUCCESS;
}
- UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
-
//
// Check mouse Data
// USB HID Specification specifies following data format:
@@ -827,6 +825,12 @@ OnMouseInterruptComplete (
// 2 0 to 7 Y displacement
// 3 to n 0 to 7 Device specific (optional)
//
+ if (DataLength < 3) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
+
UsbMouseAbsolutePointerDevice->State.ActiveButtons = *(UINT8 *) Data & (BIT0 | BIT1 | BIT2);
UsbMouseAbsolutePointerDevice->State.CurrentX =
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 09/12] MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (7 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 08/12] MdeModulePkg/AbsPointer: " Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 10/12] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
` (3 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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..f4448bffe6 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -811,8 +811,6 @@ OnMouseInterruptComplete (
return EFI_SUCCESS;
}
- UsbMouseDevice->StateChanged = TRUE;
-
//
// Check mouse Data
// USB HID Specification specifies following data format:
@@ -825,6 +823,12 @@ OnMouseInterruptComplete (
// 2 0 to 7 Y displacement
// 3 to n 0 to 7 Device specific (optional)
//
+ if (DataLength < 3) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ UsbMouseDevice->StateChanged = TRUE;
+
UsbMouseDevice->State.LeftButton = (BOOLEAN) ((*(UINT8 *) Data & BIT0) != 0);
UsbMouseDevice->State.RightButton = (BOOLEAN) ((*(UINT8 *) Data & BIT1) != 0);
UsbMouseDevice->State.RelativeMovementX += *((INT8 *) Data + 1);
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 10/12] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (8 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 09/12] MdeModulePkg/UsbMouse: " Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 11/12] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
` (2 subsequent siblings)
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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 | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
index 9fc6422ab1..22b6a9d661 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
@@ -655,7 +655,13 @@ UsbGetOneString (
//
Status = UsbCtrlGetDesc (UsbDev, USB_DESC_TYPE_STRING, Index, LangId, &Desc, 2);
- if (EFI_ERROR (Status)) {
+ //
+ // Reject if Length even cannot cover itself, or odd because Unicode string byte length should be even.
+ //
+ if (EFI_ERROR (Status) ||
+ (Desc.Length < OFFSET_OF (EFI_USB_STRING_DESCRIPTOR, Length) + sizeof (Desc.Length)) ||
+ (Desc.Length % 2 != 0)
+ ) {
return NULL;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 11/12] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (9 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 10/12] MdeModulePkg/UsbBus: Deny when the string descriptor length is odd Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-16 5:43 ` [PATCH v2 12/12] MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K Ruiyu Ni
2018-10-17 2:43 ` [PATCH v2 00/12] Need to validate data from HW Zeng, Star
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 19 ++++++++++++--
.../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 30 +++++++++++++++++++---
2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
index 936f25da5e..9b87835ed8 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
@@ -857,6 +857,14 @@ UfsRwDeviceDesc (
SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
if (Read) {
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (ReturnDataSize > Packet.InTransferLength) {
+ Status = EFI_DEVICE_ERROR;
+ goto Exit;
+ }
+
CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
Packet.InTransferLength = ReturnDataSize;
} else {
@@ -1170,8 +1178,15 @@ UfsExecScsiCmds (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
- CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
- Packet->SenseDataLength = (UINT8)SenseDataLen;
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (SenseDataLen <= Packet->SenseDataLength) {
+ CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+ Packet->SenseDataLength = (UINT8)SenseDataLen;
+ } else {
+ Packet->SenseDataLength = 0;
+ }
}
//
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 5756f637fd..0238264878 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -833,6 +833,7 @@ UfsStopExecCmd (
@param[in] QueryResp Pointer to the query response.
@retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or opcode is invalid.
+ @retval EFI_DEVICE_ERROR Data returned from device is invalid.
@retval EFI_SUCCESS Data extracted.
**/
@@ -853,6 +854,13 @@ UfsGetReturnDataFromQueryResponse (
case UtpQueryFuncOpcodeRdDesc:
ReturnDataSize = QueryResp->Tsf.Length;
SwapLittleEndianToBigEndian ((UINT8*)&ReturnDataSize, sizeof (UINT16));
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (ReturnDataSize > Packet->TransferLength) {
+ return EFI_DEVICE_ERROR;
+ }
+
CopyMem (Packet->DataBuffer, (QueryResp + 1), ReturnDataSize);
Packet->TransferLength = ReturnDataSize;
break;
@@ -1469,8 +1477,15 @@ UfsExecScsiCmds (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
- CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
- Packet->SenseDataLength = (UINT8)SenseDataLen;
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (SenseDataLen <= Packet->SenseDataLength) {
+ CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+ Packet->SenseDataLength = (UINT8)SenseDataLen;
+ } else {
+ Packet->SenseDataLength = 0;
+ }
}
//
@@ -2226,8 +2241,15 @@ ProcessAsyncTaskList (
SwapLittleEndianToBigEndian ((UINT8*)&SenseDataLen, sizeof (UINT16));
if ((Packet->SenseDataLength != 0) && (Packet->SenseData != NULL)) {
- CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
- Packet->SenseDataLength = (UINT8)SenseDataLen;
+ //
+ // Make sure the hardware device does not return more data than expected.
+ //
+ if (SenseDataLen <= Packet->SenseDataLength) {
+ CopyMem (Packet->SenseData, Response->SenseData, SenseDataLen);
+ Packet->SenseDataLength = (UINT8)SenseDataLen;
+ } else {
+ Packet->SenseDataLength = 0;
+ }
}
//
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 12/12] MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (10 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 11/12] MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected Ruiyu Ni
@ 2018-10-16 5:43 ` Ruiyu Ni
2018-10-17 2:43 ` [PATCH v2 00/12] Need to validate data from HW Zeng, Star
12 siblings, 0 replies; 15+ messages in thread
From: Ruiyu Ni @ 2018-10-16 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 0b35cbacf0..c35c7bdc12 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -736,6 +736,13 @@ UsbBootDetectMedia (
return Status;
}
+ //
+ // Simply reject device whose block size is unacceptable small (==0) or large (>64K).
+ //
+ if ((Media->BlockSize == 0) || (Media->BlockSize > USB_BOOT_MAX_CARRY_SIZE)) {
+ return EFI_DEVICE_ERROR;
+ }
+
//
// Detect whether it is necessary to reinstall the Block I/O Protocol.
//
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 00/12] Need to validate data from HW
2018-10-16 5:43 [PATCH v2 00/12] Need to validate data from HW Ruiyu Ni
` (11 preceding siblings ...)
2018-10-16 5:43 ` [PATCH v2 12/12] MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K Ruiyu Ni
@ 2018-10-17 2:43 ` Zeng, Star
2018-10-17 2:46 ` Zeng, Star
12 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-10-17 2:43 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Zeng, Star
Series Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, October 16, 2018 1:43 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v2 00/12] Need to validate data from HW
The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.
V2: adopt all comments from Star. The block size 0/64K handle is in a separate patch
"MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K"
Hao Wu (1):
MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
Ruiyu Ni (11):
MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
MdeModulePkg/UsbBus: Reject descriptor whose length is bad
MdeModulePkg/Usb: Make sure data from HW is no more than expected
SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
MdeModulePkg/UsbKb: Don't access key codes when length is wrong
MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +-
MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 +-
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 +-
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 19 +-
.../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 30 ++-
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 70 +++++-
MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 +
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 276 ++++++---------------
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h | 76 ++----
.../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 8 +-
.../UsbMouseAbsolutePointer.c | 8 +-
MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 8 +-
.../DebugCommunicationLibUsb3Transfer.c | 7 +
13 files changed, 237 insertions(+), 294 deletions(-)
--
2.16.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 00/12] Need to validate data from HW
2018-10-17 2:43 ` [PATCH v2 00/12] Need to validate data from HW Zeng, Star
@ 2018-10-17 2:46 ` Zeng, Star
0 siblings, 0 replies; 15+ messages in thread
From: Zeng, Star @ 2018-10-17 2:46 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Zeng, Star
Oh, Remember the missing update in UsbMassBoot.h about adding back '.', you can do that when pushing patches.
- Read some blocks from the device by SCSI 16 byte cmd.
+ Read or write some blocks from the device by SCSI 16 byte cmd
Thanks,
Star
-----Original Message-----
From: Zeng, Star
Sent: Wednesday, October 17, 2018 10:44 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2 00/12] Need to validate data from HW
Series Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, October 16, 2018 1:43 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v2 00/12] Need to validate data from HW
The patches contain logic to check the data from HW before using.
It can avoid corrupted data from HW causes software behave abnormally.
V2: adopt all comments from Star. The block size 0/64K handle is in a separate patch
"MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K"
Hao Wu (1):
MdeModulePkg/Bus/Ufs: Ensure device not return more data than expected
Ruiyu Ni (11):
MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16)
MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1
MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors
MdeModulePkg/UsbBus: Reject descriptor whose length is bad
MdeModulePkg/Usb: Make sure data from HW is no more than expected
SourceLevelDebugPkg/Usb3: Make sure data from HW can fit in buffer
MdeModulePkg/UsbKb: Don't access key codes when length is wrong
MdeModulePkg/AbsPointer: Don't access key codes when length is wrong
MdeModulePkg/UsbMouse: Don't access key codes when length is wrong
MdeModulePkg/UsbBus: Deny when the string descriptor length is odd
MdeModulePkg/UsbMass: Reject device whose block size is 0 or > 64K
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +-
MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 7 +-
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 +-
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 19 +-
.../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 30 ++-
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 70 +++++-
MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 +
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 276 ++++++---------------
.../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h | 76 ++----
.../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 8 +-
.../UsbMouseAbsolutePointer.c | 8 +-
MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 8 +-
.../DebugCommunicationLibUsb3Transfer.c | 7 +
13 files changed, 237 insertions(+), 294 deletions(-)
--
2.16.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread