On 2018/9/25 10:43, Ni, Ruiyu wrote:
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.
Got it about the checking sequence.
But even this code block is moved to before "Check to see if Address is
aligned" and after "Make sure (Count * Size) doesn't exceed MAX_UINT64",
the checking sequence is kept. Only difference is first checking
unsupported or first checking invalid for Address parameter.
Anyway, I have no strong opinion for that. You can decide. :)
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.
Thanks. You may consider just updating the below EFI_UNSUPPORTED to
EFI_INVALID_PARAMETER.
@retval EFI_UNSUPPORTED The address range specified by
Address, Width,
and Count is not valid for this PI system.
Star
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;
}
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
