<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> 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. :-/ <o:p> </o:p> - Bret <o:p></o:p> <o:p> </o:p> <div style="mso-element:para-border-div;border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: <a href="mailto:lersek=redhat.com@groups.io">Laszlo Ersek via groups.io</a> Sent: Wednesday, November 25, 2020 1:17 PM To: <a href="mailto:ard.biesheuvel@arm.com">Ard Biesheuvel</a>; <a href="mailto:jejb@linux.ibm.com"> jejb@linux.ibm.com</a>; <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: <a href="mailto:Bret.Barkelew@microsoft.com">Bret Barkelew</a>; <a href="mailto:gaoliming@byosoft.com.cn"> Liming Gao (Byosoft address)</a> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() </div> <o:p> </o:p> 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: <a href="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"> 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</a> >> Signed-off-by: James Bottomley <jejb@linux.ibm.com> > > Thanks James > > Acked-by: Ard Biesheuvel <ard.biesheuvel@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 <o:p></o:p> <o:p> </o:p> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/68149">View/Reply Online (#68149)</a> | | <a target="_blank" href="https://groups.io/mt/78644930/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>