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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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