Re: [edk2] [PATCH 1/2] OvmfPkg: PlatformPei: lifecycle fixes for the LockBox area

Subject: Re: [edk2] [PATCH 1/2] OvmfPkg: PlatformPei: lifecycle fixes for the LockBox area

From: Jordan Justen <jljusten@gmail.com>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2014-03-28 04:10:47

On Thu, Mar 27, 2014 at 2:14 PM, Laszlo Ersek  wrote:
> On 03/27/14 21:51, Jordan Justen wrote:
>> On Wed, Mar 26, 2014 at 3:23 PM, Laszlo Ersek  wrote:
>>> If (mBootMode == BOOT_ON_S3_RESUME) -- that is, we are resuming --, then
>>> the patch has no observable effect.
>>>
>>> If (mBootMode != BOOT_ON_S3_RESUME && mS3Supported) -- that is, we are
>>> booting or rebooting, and S3 is supported), then the patch has no
>>> observable effect either.
>>>
>>> If (mBootMode != BOOT_ON_S3_RESUME && !mS3Supported) -- that is, we are
>>> booting or rebooting, and S3 is unsupported), then the patch effects the
>>> following two fixes:
>>>
>>> - The LockBox storage is reserved from DXE (but not the OS). Drivers in
>>>   DXE may save data in the LockBox regardless of S3 support, potentially
>>>   corrupting any overlapping allocations. Make sure there's no overlap.
>>>
>>> - The LockBox storage is cleared. A LockBox inherited across a non-resume
>>>   reboot, populated with well-known GUIDs, breaks drivers that want to
>>>   save entries with those GUIDs.
>>>
>>> Reported-by: Matt Fleming 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>  OvmfPkg/PlatformPei/MemDetect.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>>> index c1350b9..15b279e 100644
>>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>>> @@ -218,13 +218,19 @@ InitializeRamRegions (
>>>        EfiACPIMemoryNVS
>>>        );
>>>  #endif
>>> +  }
>>>
>>> +  if (mBootMode != BOOT_ON_S3_RESUME) {
>>>      //
>>>      // Reserve the lock box storage area
>>>      //
>>>      // Since this memory range will be used on S3 resume, it must be
>>>      // reserved as ACPI NVS.
>>>      //
>>> +    // If S3 is unsupported, then various drivers might still write to the
>>> +    // LockBox area. We ought to prevent DXE from serving allocation requests
>>> +    // such that they would overlap the LockBox storage.
>>> +    //
>>>      ZeroMem (
>>>        (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>>>        (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
>>> @@ -232,7 +238,7 @@ InitializeRamRegions (
>>>      BuildMemoryAllocationHob (
>>>        (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>>>        (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
>>> -      EfiACPIMemoryNVS
>>> +      mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
>>
>> Should we do the same thing with all the EfiACPIMemoryNVS allocations
>> in this function?
>
> No, we shouldn't.
>
> Yesterday I spent hours on the analysis visible in the 0/2 blurb
> *exactly* so that now I could answer this question immediately. I tried
> very hard to audit the lifecycles of all ranges in question.
>
> Any specific range you pick, you will find it in my analysis in 0/2, and
> you will also find the reason there why my 1/2 patch doesn't change the
> allocation type for it. Changes that turned out to be necessary (covered
> by patch 1/2) are marked with bug {1} and bug {2} in the analysis.
>
> Of course I may have made mistakes in my analysis (and then patch 1/2
> would be wrong too, accordingly), but in order to prove such a thing,
> you'd have to chew through my analysis in 0/2... :)
>
> So, my mental image about this patch is that it is both Sound and Complete.

Indeed. Your analysis looks correct.

(Much ado about 128KB? :)

Reviewed-by: Jordan Justen 

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel