On 9/25/2018 10:14 AM, Zeng, Star wrote:
Two very small comments are added below.
On 2018/9/21 15:25, Ruiyu Ni wrote:
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <[email protected]>
Cc: Star Zeng <[email protected]>
---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26
+++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f8a1239ceb..0b6b56f846 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
UINT64 Base;
UINT64 Limit;
UINT32 Size;
+ UINT64 Length;
//
// Check to see if Buffer is NULL
@@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
}
//
- // For FIFO type, the target address won't increase during the access,
+ // For FIFO type, the device address won't increase during the access,
// so treat Count as 1
//
if (Width >= EfiPciWidthFifoUint8 && Width <=
EfiPciWidthFifoUint64) {
@@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
Size = 1 << Width;
+ //
+ // Make sure (Count * Size) doesn't exceed MAX_UINT64
+ //
+ if (Count > DivU64x32 (MAX_UINT64, Size)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
Mark as "Code Block 1".
//
// Check to see if Address is aligned
//
@@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
return EFI_UNSUPPORTED;
}
+ //
+ // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
+ //
+ Length = MultU64x32 (Count, Size);
+ if (Address > MAX_UINT64 - Length) {
+ return EFI_INVALID_PARAMETER;
+ }
+
Is there some reason this code block is not put together with the "Code
Block 1"? Both are checking integer overflow.
This code block is to check whether the Address is valid.
I group the code by the parameter. If you check the original code, you
will see the checks performed on parameters: Buffer, Width, Count, Address.
How about also enhancing the function description a little to add one
line for describing the overflow invalid parameter cases?
@retval EFI_INVALID_PARAMETER XXX.
Sure, I will send V2 with the updated function description.
or just updating the line below?
@retval EFI_UNSUPPORTED The address range specified by
Address, Width,
and Count is not valid for this PI
system.
Thanks,
Star
RootBridge = ROOT_BRIDGE_FROM_THIS (This);
//
@@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy IO access
//
- if (Address + MultU64x32 (Count, Size) <= 0x1000) {
+ if (Address + Length <= 0x1000) {
if ((RootBridge->Attributes & (
EFI_PCI_ATTRIBUTE_ISA_IO |
EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO |
EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
@@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy MMIO access
//
- if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size))
<= 0xC0000) {
+ if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) !=
0) {
return EFI_SUCCESS;
}
@@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
// By comparing the Address against Limit we know which range to
be used
// for checking
//
- if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit +
1) {
+ if (Address + Length <= RootBridge->Mem.Limit + 1) {
Base = RootBridge->Mem.Base;
Limit = RootBridge->Mem.Limit;
} else {
@@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
return EFI_INVALID_PARAMETER;
}
- if (Address + MultU64x32 (Count, Size) > Limit + 1) {
+ if (Address + Length > Limit + 1) {
return EFI_INVALID_PARAMETER;
}
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
