From a design perspective, I disagree this function is the proper place
to try to enforce this.
The single responsibility of this function is to install the MM Access
PPI. That is it.
---
From a security perspective, the boot mode is a weak way to enforce this.
Platform code often overrides/updates the boot mode based on arbitrary
conditions several times in the boot. A bug in that messy process should
not compromise the system.
---
It is not clear what the problem is.
1. What security guarantees is this function trying to make? Why?
2. Is there a security problem or not?
2.a. If so, why is security dependent on a PI Specification PPI not
being installed?
---
As-is the function interface is broken and the boot mode dependency
makes it worse:
1. It does not say boot mode must be BOOT_ON_S3_RESUME to install the
PPI though it must.
2. It claims that a return value of EFI_SUCCESS indicates the PPI was
installed. That is incorrect conditional on boot mode.
3. The EFI_NOT_FOUND return value is documented incorrectly.
4. The function returns EFI_SUCCESS if PeiServicesInstallPpi () fails.
My point is that a simple and accurate function interface will help
platforms achieve their integration and security goals better than one
that implicitly attempts to implement ambiguous requirements.
Thanks,
Michael
On 8/18/2021 5:15 PM, Chaganty, Rangasai V wrote:
I've looked into Intel Platforms and we have atleast one platform that could
potentially get impacted. However, it can be addressed by adding BootMode
checks by the caller.
The more important question, as Ray pointed out is, are there security
implications in installing these PPIs in normal boot, that justifies
PeiSmmAccessLib to absorb the bootmode checks.
If there are then it would be interesting to see how to support rationale #1 below -
"Practical use cases exist to require this PPI in cases other than the boot mode
being set to BOOT_ON_S3_RESUME".
Regards,
Sai
-----Original Message-----
From: Michael Kubacki <michael.kuba...@outlook.com>
Sent: Wednesday, August 18, 2021 11:47 AM
To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; mikub...@linux.microsoft.com;
Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Yao, Jiewen
<jiewen....@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Jiewen/Sai, are you thinking about this?
Thanks,
Michael
On 8/12/2021 1:20 AM, Ni, Ray wrote:
Michael,
I need Jiewen's input on why MmAccess and MmCommunication PPIs were not
installed in normal boot path. Without understanding the reason, I don't have
confidence to approve the change.
Sai,
Do you see other impacts to Intel platforms with this behavior change?
Thanks,
Ray
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
Kubacki
Sent: Tuesday, August 10, 2021 11:36 PM
To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@intel.com>
Cc: Yao, Jiewen <jiewen....@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Installation is a platform decision. The buried dependency on boot mode in this
particular function is just a roadblock platforms have to work around. The role
of this API is to install the PPI.
Thanks,
Michael
On 8/9/2021 9:47 PM, Ni, Ray wrote:
Michael,
Allowing the gPeiSmmAccessPpiGuid PPI installation in normal boot
will further allow gEfiPeiSmmCommunicationPpiGuid installation in normal path,
while without your change neither of the PPIs is installed in normal boot.
+ Jiewen for potential security concern.
Thanks,
Ray
-----Original Message-----
From: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>
Sent: Tuesday, August 10, 2021 6:46 AM
To: mikub...@linux.microsoft.com; devel@edk2.groups.io
Cc: Ni, Ray <ray...@intel.com>
Subject: RE: [edk2-platforms][PATCH v1 1/1]
IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Reviewed-by: Sai Chaganty <rangasai.v.chaga...@intel.com>
-----Original Message-----
From: mikub...@linux.microsoft.com <mikub...@linux.microsoft.com>
Sent: Monday, August 09, 2021 6:40 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>
Subject: [edk2-platforms][PATCH v1 1/1]
IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
From: Michael Kubacki <michael.kuba...@microsoft.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3539
PeiInstallSmmAccessPpi() currently requires the boot mode be set to S3 to
actually install gEfiPeiMmAccessPpiGuid.
This change removes this requirement in the function implementation for two
reasons:
1. Practical use cases exist to require this PPI in cases other than
the boot mode being set to BOOT_ON_S3_RESUME.
2. It is poor API design to implicitly bury this requirement within
a function whose responsibility is to install the PPI. The caller
can easily place arbitrary constraints around whether to call
based on conditions such as the boot mode being
BOOT_ON_S3_RESUME.
Cc: Ray Ni <ray...@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com>
Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com>
---
Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
| 12 ------------
1 file changed, 12 deletions(-)
diff --git
a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
s
sLib/PeiSmmAccessLib.c
b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
s sLib/PeiSmmAccessLib.c index d9bf4fba983e..4df0d695fdaf 100644
---
a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
s
sLib/PeiSmmAccessLib.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmm
+++ A
+++ cce
+++ ssLib/PeiSmmAccessLib.c
@@ -252,19 +252,7 @@ PeiInstallSmmAccessPpi (
EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock;
SMM_ACCESS_PRIVATE_DATA *SmmAccessPrivate;
VOID *HobList;
- EFI_BOOT_MODE BootMode;
- Status = PeiServicesGetBootMode (&BootMode);
- if (EFI_ERROR (Status)) {
- //
- // If not in S3 boot path. do nothing
- //
- return EFI_SUCCESS;
- }
-
- if (BootMode != BOOT_ON_S3_RESUME) {
- return EFI_SUCCESS;
- }
//
// Initialize private data
//
--
2.28.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79544): https://edk2.groups.io/g/devel/message/79544
Mute This Topic: https://groups.io/mt/84768258/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
