[edk2-devel] [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support.
Yao, Jiewen
jiewen.yao at intel.com
Thu Nov 7 07:05:35 UTC 2019
Good idea.
I will do that in V3.
> -----Original Message-----
> From: Ni, Ray <ray.ni at intel.com>
> Sent: Thursday, November 7, 2019 12:42 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
> Lou, Yun <yun.lou at intel.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support.
>
>
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao at intel.com>
> > Sent: Thursday, October 31, 2019 8:30 PM
> > To: devel at edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
> > Ni, Ray <ray.ni at intel.com>; Lou, Yun <yun.lou at 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.wang at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Yun Lou <yun.lou at intel.com>
> > Signed-off-by: Jiewen Yao <jiewen.yao at 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.ni at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list