[edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Michael Kubacki
mikuback at linux.microsoft.com
Thu Aug 19 00:13:39 UTC 2021
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.kubacki at outlook.com>
> Sent: Wednesday, August 18, 2021 11:47 AM
> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; mikuback at linux.microsoft.com; Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Yao, Jiewen <jiewen.yao at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
>> Kubacki
>> Sent: Tuesday, August 10, 2021 11:36 PM
>> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Chaganty,
>> Rangasai V <rangasai.v.chaganty at intel.com>
>> Cc: Yao, Jiewen <jiewen.yao at 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.chaganty at intel.com>
>>>> Sent: Tuesday, August 10, 2021 6:46 AM
>>>> To: mikuback at linux.microsoft.com; devel at edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni at intel.com>
>>>> Subject: RE: [edk2-platforms][PATCH v1 1/1]
>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
>>>>
>>>> Reviewed-by: Sai Chaganty <rangasai.v.chaganty at intel.com>
>>>>
>>>> -----Original Message-----
>>>> From: mikuback at linux.microsoft.com <mikuback at linux.microsoft.com>
>>>> Sent: Monday, August 09, 2021 6:40 AM
>>>> To: devel at edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni at intel.com>; Chaganty, Rangasai V
>>>> <rangasai.v.chaganty at intel.com>
>>>> Subject: [edk2-platforms][PATCH v1 1/1]
>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
>>>>
>>>> From: Michael Kubacki <michael.kubacki at 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.ni at intel.com>
>>>> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
>>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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/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