[EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

Thu Dec 3 10:39:08 UTC 2020

On 12/01/20 22:12, Bret Barkelew via groups.io wrote:
> Getting caught up on emails from last week. I wanted to say thanks for the good catch and the good patch in my absence!
> Sorry for the oversight. :-/

It happens. It's not great that I didn't get to regression-testing
earlier during the HFF. It's even less great that I chalked it up to a
guest kernel issue initially.

What *is* great however is that James kept pushing and fixed it! :)

MemoryAllocationLib is tricky. The abstraction it provides is a bit
"deceptive", IMO. There's an argument for removing support, between the
existing instances of this lib class, for *runtime* DXE drivers
altogether. In runtime DXE drivers, just force programmers to spell out
gBS, and the desired memory type. In other module types (PEIMs, DXE and
UEFI drivers, and SMM drivers), there is no ambiguity whether the
allocated memory survives into OS runtime. (Well, AllocateReservedPool()
complicates matters a bit, but it's not used frequently.)

Another sign that MemoryAllocationLib is a leaky abstraction is commit
1d733f731f968 -- in the SMM instance, AllocatePool() allocates SMRAM,
but FreePool() is supposed to be able to release both SMRAM and normal
RAM :( Something seems really fishy there, as one would expect calling
FreePool() only on what AllocatePool() returned earlier in the *same
module*. Since AllocatePool() only returns SMRAM in an SMM driver, why
would FreePool() *have* to release normal RAM? Oof.

Thanks,
Laszlo

> 
> - Bret
> 
> From: Laszlo Ersek via groups.io<mailto:lersek=redhat.com at groups.io>
> Sent: Wednesday, November 25, 2020 1:17 PM
> To: Ard Biesheuvel<mailto:ard.biesheuvel at arm.com>; jejb at linux.ibm.com<mailto:jejb at linux.ibm.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
> Cc: Bret Barkelew<mailto:Bret.Barkelew at microsoft.com>; Liming Gao (Byosoft address)<mailto:gaoliming at byosoft.com.cn>
> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()
> 
> On 11/25/20 22:05, Ard Biesheuvel wrote:
>> On 11/25/20 9:13 PM, James Bottomley wrote:
>>> The current variable policy is allocated by AllocatePool(), which is
>>> boot time only.  This means that if you do any variable setting in the
>>> runtime, the policy has been freed.  Ordinarily this isn't detected
>>> because freed memory is still there, but when you boot the Linux
>>> kernel, it's been remapped so the actual memory no longer exists in
>>> the memory map causing a page fault.
>>>
>>> Fix this by making it AllocateRuntimePool().  For SMM drivers, the
>>> platform DSC is responsible for resolving the MemoryAllocationLib
>>> class to the SmmMemoryAllocationLib instance. In the
>>> SmmMemoryAllocationLib instance, AllocatePool() and
>>> AllocateRuntimePool() are implemented identically. Therefore this
>>> change is a no-op when the RegisterVariablePolicy() function is built
>>> into an SMM driver. The fix affects runtime DXE drivers only.
>>>
>>> Ref: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3092&data=04%7C01%7Cbret.barkelew%40microsoft.com%7C2e91993035204bbd307d08d891878686%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637419358545184416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uYtJTRY5RRS5XJ7j%2Bo%2B75qH12ROX9%2FQ4v1GMdUbLk3I%3D&reserved=0
>>> Signed-off-by: James Bottomley <jejb at linux.ibm.com>
>>
>> Thanks James
>>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
>>
>>> ---
>>>   MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>> b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>> index 5029ddb96adb..12944ac7ea81 100644
>>> --- a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>> @@ -411,7 +411,7 @@ RegisterVariablePolicy (
>>>       }
>>>         // Reallocate and copy the table.
>>> -    NewTable = AllocatePool( NewSize );
>>> +    NewTable = AllocateRuntimePool( NewSize );
>>>       if (NewTable == NULL) {
>>>         return EFI_OUT_OF_RESOURCES;
>>>       }
>>>
>>
>> BTW I wouldn't mind if the whitespace gets fixed up here at merge time.
>>
> 
> The coding style all over the VariablePolicy code (that I have
> investigated) is non-idiomatic for edk2 -- it should have been pointed
> out during the original patch review sessions.
> 
> The coding style can also be fixed up retro-actively whole-sale, of course.
> 
> In the present patch, James is only sticking with the (non-idiomatic)
> style that's been part of the VariablePolicy contribution.
> 
> I'm quite displeased myself with the reams of non-idiomatic coding style
> in VariablePolicy, but I don't blame that on the contribution -- IMO it
> should have been caught in review.
> 
> (
> 
> Meta-request: Ard, can you please start signing your emails? (Such as,
> in "Bye: Ard", not as in cryptographic signing.) It's quite hit-or-miss
> to know where your emails end; in the present case, I *almost* didn't
> scroll down to the bottom (because in many other cases, you insert an
> A-b, don't remove the tail, and add nothing at the bottom, so the reader
> kind of gets conditioned to stop reading after the A-b, seeing
> repeatedly how scrolling down to the bottom is a waste). Consistently
> using a manual signature does away with this problem. Another solution
> is of course to always strip the tail, when you're done responding.
> Sorry about this verbiage, I just wanted to have it said. :)
> 
> )
> 
> Thanks,
> Laszlo
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68237): https://edk2.groups.io/g/devel/message/68237
Mute This Topic: https://groups.io/mt/78644930/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-