Good idea. I will do that in V3. > -----Original Message----- > From: Ni, Ray <ray...@intel.com> > Sent: Thursday, November 7, 2019 12:42 PM > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > Lou, Yun <yun....@intel.com> > Subject: RE: [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support. > > > > > -----Original Message----- > > From: Yao, Jiewen <jiewen....@intel.com> > > Sent: Thursday, October 31, 2019 8:30 PM > > To: devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > > Ni, Ray <ray...@intel.com>; Lou, Yun <yun....@intel.com> > > Subject: [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2303 > > > > Whenever a PCI device is discovered, PCI bus calls the > > EDKII_DEVICE_SECURITY_PROTOCOL to authenticate it. > > If the function returns success, the PCI bus allocates the resource and > > installs > > the PCI_IO for the device. > > If the function returns fail, the PCI bus skips the device. > > > > It is similar to EFI_SECURITY_ARCH_PROTOCOL, which is used to verify an EFI > > image. > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Hao A Wu <hao.a...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Yun Lou <yun....@intel.com> > > Signed-off-by: Jiewen Yao <jiewen....@intel.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c | 12 +++- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 + > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 4 +- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 63 > > +++++++++++++++++++- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 4 +- > > 5 files changed, 77 insertions(+), 7 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c > > index b020ce50ce..64284ac825 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c > > @@ -8,7 +8,7 @@ > > PCI Root Bridges. So it means platform needs install PCI Root Bridge IO > > protocol for each > > PCI Root Bus and install PCI Host Bridge Resource Allocation Protocol. > > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -37,7 +37,7 @@ UINT64 gAllZero > > = 0; > > EFI_PCI_PLATFORM_PROTOCOL *gPciPlatformProtocol; > > EFI_PCI_OVERRIDE_PROTOCOL *gPciOverrideProtocol; > > EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; > > - > > +EDKII_DEVICE_SECURITY_PROTOCOL *mDeviceSecurityProtocol; > > > > GLOBAL_REMOVE_IF_UNREFERENCED > > EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest = { > > PciHotPlugRequestNotify > > @@ -293,6 +293,14 @@ PciBusDriverBindingStart ( > > ); > > } > > > > + if (mDeviceSecurityProtocol == NULL) { > > + gBS->LocateProtocol ( > > + &gEdkiiDeviceSecurityProtocolGuid, > > + NULL, > > + (VOID **) &mDeviceSecurityProtocol > > + ); > > + } > > + > > if (PcdGetBool (PcdPciDisableBusEnumeration)) { > > gFullEnumeration = FALSE; > > } else { > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > > index 504a1b1c12..d4113993c8 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h > > @@ -27,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include > > <Protocol/PciOverride.h> #include <Protocol/PciEnumerationComplete.h> > > #include <Protocol/IoMmu.h> > > +#include <Protocol/DeviceSecurity.h> > > > > #include <Library/DebugLib.h> > > #include <Library/UefiDriverEntryPoint.h> diff --git > > a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > index 05c22025b8..9284998f36 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > @@ -2,7 +2,7 @@ > > # The PCI bus driver will probe all PCI devices and allocate MMIO and IO > > space for these devices. > > # Please use PCD feature flag PcdPciBusHotplugDeviceSupport to enable > > hot plug supporting. > > # > > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights > > +reserved.<BR> > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -90,6 +90,8 @@ > > gEfiIncompatiblePciDeviceSupportProtocolGuid ## > > SOMETIMES_CONSUMES > > gEfiLoadFile2ProtocolGuid ## SOMETIMES_PRODUCES > > gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES > > + gEdkiiDeviceSecurityProtocolGuid ## SOMETIMES_CONSUMES > > + gEdkiiDeviceIdentifierTypePciGuid ## SOMETIMES_CONSUMES > > gEfiLoadedImageDevicePathProtocolGuid ## CONSUMES > > > > [FeaturePcd] > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > index c7eafff593..df3d1c8fcc 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > @@ -10,6 +10,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include > > "PciBus.h" > > > > extern CHAR16 *mBarTypeStr[]; > > +extern EDKII_DEVICE_SECURITY_PROTOCOL > > *mDeviceSecurityProtocol; > > > > #define OLD_ALIGN 0xFFFFFFFFFFFFFFFFULL > > #define EVEN_ALIGN 0xFFFFFFFFFFFFFFFEULL @@ -2092,9 +2093,10 @@ > > CreatePciIoDevice ( > > IN UINT8 Func > > ) > > { > > - PCI_IO_DEVICE *PciIoDevice; > > - EFI_PCI_IO_PROTOCOL *PciIo; > > - EFI_STATUS Status; > > + PCI_IO_DEVICE *PciIoDevice; > > + EFI_PCI_IO_PROTOCOL *PciIo; > > + EFI_STATUS Status; > > + EDKII_DEVICE_IDENTIFIER DeviceIdentifier; > > > > PciIoDevice = AllocateZeroPool (sizeof (PCI_IO_DEVICE)); > > if (PciIoDevice == NULL) { > > @@ -2156,6 +2158,61 @@ CreatePciIoDevice ( > > PciIoDevice->IsPciExp = TRUE; > > } > > > > + // > > + // Now we can do the authentication check for the device. > > + // > > + if (mDeviceSecurityProtocol != NULL) { > > + // > > + // Prepare the parameter > > + // > > + DeviceIdentifier.Version = EDKII_DEVICE_IDENTIFIER_REVISION; > > + CopyGuid (&DeviceIdentifier.DeviceType, > > &gEdkiiDeviceIdentifierTypePciGuid); > > + DeviceIdentifier.DeviceHandle = NULL; > > + Status = gBS->InstallMultipleProtocolInterfaces ( > > + &DeviceIdentifier.DeviceHandle, > > + &gEfiDevicePathProtocolGuid, > > + PciIoDevice->DevicePath, > > + &gEdkiiDeviceIdentifierTypePciGuid, > > + &PciIoDevice->PciIo, > > + NULL > > + ); > > + if (EFI_ERROR(Status)) { > > + if (PciIoDevice->DevicePath != NULL) { > > + FreePool (PciIoDevice->DevicePath); > > + } > > + FreePool (PciIoDevice); > > + return NULL; > > + } > > + > > + // > > + // Do DeviceAuthentication > > + // > > + Status = mDeviceSecurityProtocol->DeviceAuthenticate > > (mDeviceSecurityProtocol, &DeviceIdentifier); > > + // > > + // Always uninstall, because they are only for Authentication. > > + // No need to check return Status. > > + // > > + gBS->UninstallMultipleProtocolInterfaces ( > > + DeviceIdentifier.DeviceHandle, > > + &gEfiDevicePathProtocolGuid, > > + PciIoDevice->DevicePath, > > + &gEdkiiDeviceIdentifierTypePciGuid, > > + &PciIoDevice->PciIo, > > + NULL > > + ); > > + > > + // > > + // If authentication fails, skip this device. > > + // > > + if (EFI_ERROR(Status)) { > > + if (PciIoDevice->DevicePath != NULL) { > > + FreePool (PciIoDevice->DevicePath); > > + } > > + FreePool (PciIoDevice); > > + return NULL; > > + } > > + } > > + > > Jiewen, > I have no other comments with the logic except one minor request: > Can you please create a standalone function like PciDeviceAuthenticate() and > move the new code to that function then call it from CreatePciIoDevice? > With that, Reviewed-by: Ray Ni <ray...@intel.com>
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50179): https://edk2.groups.io/g/devel/message/50179 Mute This Topic: https://groups.io/mt/40117504/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
