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.web10.2728.1582248202283984243 for ; Thu, 20 Feb 2020 17:23:22 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Feb 2020 17:23:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,466,1574150400"; d="scan'208";a="436794777" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 20 Feb 2020 17:23:21 -0800 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 20 Feb 2020 17:23:21 -0800 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 20 Feb 2020 17:23:21 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.46]) with mapi id 14.03.0439.000; Fri, 21 Feb 2020 09:23:18 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "gaurav.jain@nxp.com" , "Gao, Liming" , "afish@apple.com" , "lersek@redhat.com" , "leif@nuviainc.com" , "Kinney, Michael D" CC: "Wang, Jian J" , "Ni, Ray" , "Ard Biesheuvel" , Pankaj Bansal Subject: Re: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Topic: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Index: AdXoVX1UfqTUDarfQqSjaIalQXdrhg== Date: Fri, 21 Feb 2020 01:23:18 +0000 Message-ID: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable A couple of inline comments below. Please help to handle them in the next version of patch. With them addressed, Reviewed-by: Hao A Wu Hello Liming and Stewards, I would like to confirm with you for whether the patch should catch the=20 upcoming stable tag. My personal take is that the patch is more like a code refinement rather t= han a bug fix. Could you help to make a final call for this one? Thanks in advance. Best Regards, Hao Wu > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Gaurav Jain > Sent: Thursday, February 20, 2020 11:40 PM > To: devel@edk2.groups.io > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Ard Biesheuvel; Pankaj Bansal; Gau= rav > Jain > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in > SCT PCIIO Protocol Test. >=20 > ASSERT in PollMem_Conf, CopyMem_Conf, SetBarAttributes_Conf > Conformance Test. > SCT Test expect return as Invalid Parameter or Unsupported. > Added Checks for Function Parameters. > return Invalid or Unsupported if Check fails. >=20 > Added Checks in PciIoPollIo(), PciIoIoRead() > PciIoIoWrite() >=20 > Signed-off-by: Gaurav Jain > --- >=20 > Notes: > v2 > - Reverted ASSERT(FALSE) code. > - Added Checks for Width, BarIndex, Buffer, > Address range in PciIoIoRead, PciIoIoWrite. > - Added Checks for Width, BarIndex, Result, > Address range in PciIoPollIo, PciIoPollMem, > PciIoCopyMem. > - Added Checks for Attributes, BarIndex, > Address range in PciIoSetBarAttributes. >=20 > .../NonDiscoverablePciDeviceIo.c | 180 ++++++++++++++++++ > 1 file changed, 180 insertions(+) >=20 > diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > ciDeviceIo.c > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > ciDeviceIo.c > index 2d55c9699322..4dd804356021 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > ciDeviceIo.c > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > ciDeviceIo.c > @@ -93,6 +93,35 @@ PciIoPollMem ( > OUT UINT64 *Result > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > + UINTN Count; > + EFI_STATUS Status; > + > + if ((UINT32)Width > EfiPciIoWidthUint64) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (BarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + if (Result =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + Count =3D 1; > + > + Status =3D GetBarResource (Dev, BarIndex, &Desc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > @@ -126,6 +155,35 @@ PciIoPollIo ( > OUT UINT64 *Result > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > + UINTN Count; > + EFI_STATUS Status; > + > + if ((UINT32)Width > EfiPciIoWidthUint64) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (BarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + if (Result =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + Count =3D 1; > + > + Status =3D GetBarResource (Dev, BarIndex, &Desc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > @@ -396,6 +454,33 @@ PciIoIoRead ( > IN OUT VOID *Buffer > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > + EFI_STATUS Status; > + > + if ((UINT32)Width > EfiPciIoWidthUint64) { > + return EFI_INVALID_PARAMETER; > + } For PciIoIoRead(), I think enum values smaller than EfiPciIoWidthMaximum a= re all valid. The above check seems to strict. > + > + if (BarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + if (Buffer =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Status =3D GetBarResource (Dev, BarIndex, &Desc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > @@ -425,6 +510,33 @@ PciIoIoWrite ( > IN OUT VOID *Buffer > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > + EFI_STATUS Status; > + > + if ((UINT32)Width > EfiPciIoWidthUint64) { > + return EFI_INVALID_PARAMETER; > + } For PciIoIoWrite(), I think enum values smaller than EfiPciIoWidthMaximum = are all valid. The above check seems to strict. > + > + if (BarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + if (Buffer =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Status =3D GetBarResource (Dev, BarIndex, &Desc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > @@ -556,6 +668,40 @@ PciIoCopyMem ( > IN UINTN Count > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *DestDesc; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *SrcDesc; > + EFI_STATUS Status; > + > + if ((UINT32)Width > EfiPciIoWidthUint64) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (DestBarIndex >=3D PCI_MAX_BAR || > + SrcBarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Status =3D GetBarResource (Dev, DestBarIndex, &DestDesc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (DestOffset + (Count << (Width & 0x3)) > DestDesc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > + Status =3D GetBarResource (Dev, SrcBarIndex, &SrcDesc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (SrcOffset + (Count << (Width & 0x3)) > SrcDesc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > @@ -1414,6 +1560,40 @@ PciIoSetBarAttributes ( > IN OUT UINT64 *Length > ) > { > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > + UINTN Count; > + EFI_STATUS Status; > + > + #define DEV_SUPPORTED_ATTRIBUTES \ > + (EFI_PCI_DEVICE_ENABLE | > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) > + > + if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) !=3D 0) { > + return EFI_UNSUPPORTED; > + } I think the above check for 'Attributes' can be dropped. I found that the implementation of the PciIoGetBarAttributes() function do= es not expose any configurable attributes. So the logic can fall through to t= he ASSERT (for DEBUG images) and then returns EFI_UNSUPPORTED. Best Regards, HaoWu > + > + if (BarIndex >=3D PCI_MAX_BAR) { > + return EFI_UNSUPPORTED; > + } > + > + if (Offset =3D=3D NULL || Length =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + Width =3D EfiPciIoWidthUint8; > + Count =3D (UINT32) *Length; > + > + Status =3D GetBarResource(Dev, BarIndex, &Desc); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (*Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > + return EFI_UNSUPPORTED; > + } > + > ASSERT (FALSE); > return EFI_UNSUPPORTED; > } > -- > 2.17.1 >=20 >=20 >=20