[edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement

[Chaganty, Rangasai V]

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 (#79516): https://edk2.groups.io/g/devel/message/79516
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]
-=-=-=-=-=-=-=-=-=-=-=-