From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DE19221A02937 for ; Mon, 24 Sep 2018 20:04:36 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2018 20:04:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,300,1534834800"; d="scan'208";a="91609254" Received: from shzintpr03.sh.intel.com (HELO [10.253.24.48]) ([10.239.4.100]) by fmsmga004.fm.intel.com with ESMTP; 24 Sep 2018 20:02:31 -0700 To: "Ni, Ruiyu" , edk2-devel@lists.01.org References: <20180921072539.268068-1-ruiyu.ni@intel.com> <20180921072539.268068-2-ruiyu.ni@intel.com> <73f95487-2316-401a-3129-aee0e52eed9f@intel.com> <33222c6c-74d8-1efb-6656-79c3ef75de5d@Intel.com> From: "Zeng, Star" Cc: star.zeng@intel.com Message-ID: Date: Tue, 25 Sep 2018 11:02:00 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <33222c6c-74d8-1efb-6656-79c3ef75de5d@Intel.com> Subject: Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 03:04:37 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >>> Cc: Star Zeng >>> --- >>>   .../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; >>>     } >>> >> > >