From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.62683.1629332020695727215 for ; Wed, 18 Aug 2021 17:13:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=lh2MDREv; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.0.0.19] (c-73-27-179-174.hsd1.fl.comcast.net [73.27.179.174]) by linux.microsoft.com (Postfix) with ESMTPSA id 9420820C3344; Wed, 18 Aug 2021 17:13:39 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 9420820C3344 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1629332020; bh=amA8XYmPDmdq9PEvbKfkYhRsQxuM24S2iaxs531fE3c=; h=Subject:To:References:From:Date:In-Reply-To:From; b=lh2MDREv89vWbStCWVpVlnPotfVo5r0W7hgKzFnyAkZTZAe3kaoRAtkOLkBqarqaG Fj49/dRHj9s+Sjm0O+Bdq+2MeLet4xYdM/+3bBa+lQr9iNH4z7eY1o8KNi9YTCbTcD orBZKNbnHyEqk/sjrX8SWxsB2AkEeyZq6fiS8sSU= Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement To: "Chaganty, Rangasai V" , "devel@edk2.groups.io" , "Ni, Ray" , "Yao, Jiewen" References: <20210809133938.2430-1-mikuback@linux.microsoft.com> <9a5c7c6b-70c5-c1c0-6405-51149013c295@linux.microsoft.com> From: "Michael Kubacki" Message-ID: Date: Wed, 18 Aug 2021 20:13:39 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Sent: Wednesday, August 18, 2021 11:47 AM > To: devel@edk2.groups.io; Ni, Ray ; mikuback@linux.microsoft.com; Chaganty, Rangasai V ; Yao, Jiewen > 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 On Behalf Of Michael >> Kubacki >> Sent: Tuesday, August 10, 2021 11:36 PM >> To: devel@edk2.groups.io; Ni, Ray ; Chaganty, >> Rangasai V >> Cc: Yao, Jiewen >> 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 >>>> Sent: Tuesday, August 10, 2021 6:46 AM >>>> To: mikuback@linux.microsoft.com; devel@edk2.groups.io >>>> Cc: Ni, Ray >>>> Subject: RE: [edk2-platforms][PATCH v1 1/1] >>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement >>>> >>>> Reviewed-by: Sai Chaganty >>>> >>>> -----Original Message----- >>>> From: mikuback@linux.microsoft.com >>>> Sent: Monday, August 09, 2021 6:40 AM >>>> To: devel@edk2.groups.io >>>> Cc: Ni, Ray ; Chaganty, Rangasai V >>>> >>>> Subject: [edk2-platforms][PATCH v1 1/1] >>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement >>>> >>>> From: Michael Kubacki >>>> >>>> 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 >>>> Cc: Rangasai V Chaganty >>>> Signed-off-by: Michael Kubacki >>>> --- >>>> 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 >>> >>> >>> >>> >>> >> >> >> >> >> >> >> >> >> >>