From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web11.3837.1587138505888677264 for ; Fri, 17 Apr 2020 08:48:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.151, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: VW+qDxEkuzNp6yg1uDrQk5cvc5tPgNu18OaGOgwCZk/G3R3NVg52hHhFE9ZiqmGRnWjW6s7dBP TsGCZKrbZG4g== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Apr 2020 08:48:25 -0700 IronPort-SDR: 8e1KET0Ii3nuAHcM00yub/wrQpkaptcegbRTtaJUaL1rm9ayeDC7PyjMdKsYovGYfJUF1WAhJG a3jaXphp0HNA== X-IronPort-AV: E=Sophos;i="5.72,395,1580803200"; d="scan'208";a="299646802" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.30.64]) ([10.213.30.64]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Apr 2020 08:48:23 -0700 Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Prevent invalid PCI BAR access To: devel@edk2.groups.io, siyuan.fu@intel.com, "michael.kubacki@outlook.com" Cc: "Wu, Jiaxin" References: From: "Maciej Rabeda" Message-ID: Date: Fri, 17 Apr 2020 17:48:22 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl Reviewed-by: Maciej Rabeda On 17-Apr-20 04:51, Siyuan, Fu wrote: > Reviewed-by: Siyuan Fu > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Michael >> Kubacki >> Sent: 2020Äê4ÔÂ9ÈÕ 11:02 >> To: devel@edk2.groups.io >> Cc: Fu, Siyuan ; Maciej Rabeda >> ; Wu, Jiaxin >> Subject: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Prevent invalid PCI >> BAR access >> >> From: Michael Kubacki >> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1563 >> >> SnpDxe initializes values for MemoryBarIndex and IoBarIndex to 0 and 1 >> respectively even if calls to PciIo->GetBarAttributes never return >> success. >> >> Later, if the BAR is used to perform IO/Mem reads/writes, a potentially >> non-existent BAR index may be accessed. This change initializes the values >> to an invalid BAR index (PCI_MAX_BAR) so the condition can be explicitly >> checked to avoid an invalid BAR access. >> >> Cc: Siyuan Fu >> Cc: Maciej Rabeda >> Cc: Jiaxin Wu >> Signed-off-by: Michael Kubacki >> --- >> NetworkPkg/SnpDxe/Callback.c | 77 ++++++++++++-------- >> NetworkPkg/SnpDxe/Snp.c | 5 +- >> 2 files changed, 48 insertions(+), 34 deletions(-) >> >> diff --git a/NetworkPkg/SnpDxe/Callback.c b/NetworkPkg/SnpDxe/Callback.c >> index 0c0b81fdca8e..99b7fd3ef835 100644 >> --- a/NetworkPkg/SnpDxe/Callback.c >> +++ b/NetworkPkg/SnpDxe/Callback.c >> @@ -4,6 +4,7 @@ >> stores the interface context for the NIC that snp is trying to talk. >> >> Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
>> +Copyright (c) Microsoft Corporation.
>> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -115,47 +116,59 @@ SnpUndi32CallbackMemio ( >> >> switch (ReadOrWrite) { >> case PXE_IO_READ: >> - Snp->PciIo->Io.Read ( >> - Snp->PciIo, >> - Width, >> - Snp->IoBarIndex, // BAR 1 (for 32bit regs), IO base address >> - MemOrPortAddr, >> - 1, // count >> - (VOID *) (UINTN) BufferPtr >> - ); >> + ASSERT (Snp->IoBarIndex < PCI_MAX_BAR); >> + if (Snp->IoBarIndex < PCI_MAX_BAR) { >> + Snp->PciIo->Io.Read ( >> + Snp->PciIo, >> + Width, >> + Snp->IoBarIndex, // BAR 1 (for 32bit regs), IO base address >> + MemOrPortAddr, >> + 1, // count >> + (VOID *) (UINTN) BufferPtr >> + ); >> + } >> break; >> >> case PXE_IO_WRITE: >> - Snp->PciIo->Io.Write ( >> - Snp->PciIo, >> - Width, >> - Snp->IoBarIndex, // BAR 1 (for 32bit regs), IO base address >> - MemOrPortAddr, >> - 1, // count >> - (VOID *) (UINTN) BufferPtr >> - ); >> + ASSERT (Snp->IoBarIndex < PCI_MAX_BAR); >> + if (Snp->IoBarIndex < PCI_MAX_BAR) { >> + Snp->PciIo->Io.Write ( >> + Snp->PciIo, >> + Width, >> + Snp->IoBarIndex, // BAR 1 (for 32bit regs), IO base address >> + MemOrPortAddr, >> + 1, // count >> + (VOID *) (UINTN) BufferPtr >> + ); >> + } >> break; >> >> case PXE_MEM_READ: >> - Snp->PciIo->Mem.Read ( >> - Snp->PciIo, >> - Width, >> - Snp->MemoryBarIndex, // BAR 0, Memory base address >> - MemOrPortAddr, >> - 1, // count >> - (VOID *) (UINTN) BufferPtr >> - ); >> + ASSERT (Snp->MemoryBarIndex < PCI_MAX_BAR); >> + if (Snp->MemoryBarIndex < PCI_MAX_BAR) { >> + Snp->PciIo->Mem.Read ( >> + Snp->PciIo, >> + Width, >> + Snp->MemoryBarIndex, // BAR 0, Memory base address >> + MemOrPortAddr, >> + 1, // count >> + (VOID *) (UINTN) BufferPtr >> + ); >> + } >> break; >> >> case PXE_MEM_WRITE: >> - Snp->PciIo->Mem.Write ( >> - Snp->PciIo, >> - Width, >> - Snp->MemoryBarIndex, // BAR 0, Memory base address >> - MemOrPortAddr, >> - 1, // count >> - (VOID *) (UINTN) BufferPtr >> - ); >> + ASSERT (Snp->MemoryBarIndex < PCI_MAX_BAR); >> + if (Snp->MemoryBarIndex < PCI_MAX_BAR) { >> + Snp->PciIo->Mem.Write ( >> + Snp->PciIo, >> + Width, >> + Snp->MemoryBarIndex, // BAR 0, Memory base address >> + MemOrPortAddr, >> + 1, // count >> + (VOID *) (UINTN) BufferPtr >> + ); >> + } >> break; >> } >> >> diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c >> index 078b27cf5edd..da9fba51eff5 100644 >> --- a/NetworkPkg/SnpDxe/Snp.c >> +++ b/NetworkPkg/SnpDxe/Snp.c >> @@ -2,6 +2,7 @@ >> Implementation of driver entry point and driver binding protocol. >> >> Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.
>> +Copyright (c) Microsoft Corporation.
>> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -465,8 +466,8 @@ SimpleNetworkDriverStart ( >> // the IO BAR. Save the index of the BAR into the adapter info structure. >> // for regular 32bit BARs, 0 is memory mapped, 1 is io mapped >> // >> - Snp->MemoryBarIndex = 0; >> - Snp->IoBarIndex = 1; >> + Snp->MemoryBarIndex = PCI_MAX_BAR; >> + Snp->IoBarIndex = PCI_MAX_BAR; >> FoundMemoryBar = FALSE; >> FoundIoBar = FALSE; >> for (BarIndex = 0; BarIndex < PCI_MAX_BAR; BarIndex++) { >> -- >> 2.16.3.windows.1 >> >> >> > > >