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. :-/ - Bret From: Laszlo Ersek via groups.io Sent: Wednesday, November 25, 2020 1:17 PM To: Ard Biesheuvel; jejb@linux.ibm.com; devel@edk2.groups.io Cc: Bret Barkelew; Liming Gao (Byosoft address) 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 > > Thanks James > > Acked-by: Ard Biesheuvel > >> --- >> 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