Re: [edk2] [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1

[Ni, Ruiyu]

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 (=0x1) / BlockSize.
When BlockSize is 1, the UINT16 local variable is set to 0x1


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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1

[Zeng, Star]

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 (=0x1) / BlockSize.
When BlockSize is 1, the UINT16 local variable is set to 0x1


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 
Cc: Star Zeng 
Cc: Steven Shi 
---
  .../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_STATUSStatus;
-  UINT16Count;
-  UINT16CountMax;
+  UINT32Count;
+  UINT32CountMax;
UINT32BlockSize;
UINT32ByteSize;
UINT32Timeout;
  
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;
}
  



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

[edk2] [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1

[Ruiyu Ni]

UsbBootReadWriteBlocks() and UsbBootReadWriteBlocks16() use a UINT16
local variable to hold the value of
USB_BOOT_MAX_CARRY_SIZE (=0x1) / BlockSize.
When BlockSize is 1, the UINT16 local variable is set to 0x1
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 
Cc: Star Zeng 
Cc: Steven Shi 
---
 .../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_STATUSStatus;
-  UINT16Count;
-  UINT16CountMax;
+  UINT32Count;
+  UINT32CountMax;
   UINT32BlockSize;
   UINT32ByteSize;
   UINT32Timeout;
 
   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

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