Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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 <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> ---
>>   .../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