From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Ni, Ray" <ray.ni@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Wu, Hao A" <hao.a.wu@intel.com>, "Lou, Yun" <yun.lou@intel.com>
Subject: Re: [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support.
Date: Thu, 7 Nov 2019 07:05:35 +0000 [thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F841F73@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C352D31@SHSMSX104.ccr.corp.intel.com>
Good idea.
I will do that in V3.
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, November 7, 2019 12:42 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Lou, Yun <yun.lou@intel.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support.
>
>
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Thursday, October 31, 2019 8:30 PM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Lou, Yun <yun.lou@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@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Yun Lou <yun.lou@intel.com>
> > Signed-off-by: Jiewen Yao <jiewen.yao@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@intel.com>
next prev parent reply other threads:[~2019-11-07 7:05 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-31 12:30 [PATCH V2 0/4] Add SPDM device security Yao, Jiewen
2019-10-31 12:30 ` [PATCH V2 1/4] MdePkg/Include: Add DMTF SPDM definition Yao, Jiewen
2019-11-06 14:38 ` Liming Gao
2019-11-07 0:25 ` Yao, Jiewen
[not found] ` <15D4B9B24059B4F1.19610@groups.io>
2019-11-07 0:57 ` [edk2-devel] " Yao, Jiewen
2019-10-31 12:30 ` [PATCH V2 2/4] MdeModulePkg/Include: Add DeviceSecurity.h Yao, Jiewen
2019-11-06 7:55 ` [edk2-devel] " Ni, Ray
2019-11-06 8:25 ` Yao, Jiewen
2019-11-07 1:58 ` Ni, Ray
[not found] ` <15D4BEC95EBB70CB.18056@groups.io>
2019-11-07 4:31 ` Ni, Ray
2019-11-07 7:13 ` Yao, Jiewen
2019-11-07 7:16 ` Ni, Ray
2019-10-31 12:30 ` [PATCH V2 3/4] MdeModulePkg/dec: Add EdkiiDeviceSecurityProtocolGuid Yao, Jiewen
2019-10-31 12:30 ` [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support Yao, Jiewen
2019-11-07 4:42 ` Ni, Ray
2019-11-07 7:05 ` Yao, Jiewen [this message]
[not found] ` <15D2BB2D773DBDBA.23805@groups.io>
2019-11-06 6:47 ` [edk2-devel] [PATCH V2 1/4] MdePkg/Include: Add DMTF SPDM definition Yao, Jiewen
[not found] ` <15D2BB2E5CC7FD95.31603@groups.io>
2019-11-06 6:47 ` [edk2-devel] [PATCH V2 4/4] MdeModulePkg/Pci: Add DeviceSecurity support Yao, Jiewen
[not found] ` <15D2BB2DC41C838D.31603@groups.io>
2019-11-06 6:47 ` [edk2-devel] [PATCH V2 2/4] MdeModulePkg/Include: Add DeviceSecurity.h Yao, Jiewen
[not found] ` <15D2BB2E0995721D.31603@groups.io>
2019-11-06 6:47 ` [edk2-devel] [PATCH V2 3/4] MdeModulePkg/dec: Add EdkiiDeviceSecurityProtocolGuid Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=74D8A39837DF1E4DA445A8C0B3885C503F841F73@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox