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: Laszlo Ersek <lersek@redhat.com>

To: Jordan Justen <jljusten@gmail.com>

Date: 2014-03-28 18:59:48

On 03/28/14 04:10, Jordan Justen wrote:
> 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? :)

But it's such a clean, safe feeling! :)

> 
> Reviewed-by: Jordan Justen 

Thank you!

Laszlo


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