From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.8364.1606991958703652457 for ; Thu, 03 Dec 2020 02:39:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=M/Bsy2dq; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606991957; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CNn4YOLU+3uqbg4ULeIGX8dM+9d6BwSuYFuN8TZ+H30=; b=M/Bsy2dqRoGvS5WQX3NJo4ln2SnR3PDB410iBrq+d7TucfFUfMEcbytABk7NJGzntJeZbf PU3oUpej8fSU48WDnLKv+i157U+QxzbZWxxnoUxFcVPp/C2QyMnE8ek/VS/nD/zZoPDtqM yG1P84OyKDQvf+puZhnK5jT9JMOv1GA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-586-i8Oab-U7NF6UzHEcTw4UpA-1; Thu, 03 Dec 2020 05:39:12 -0500 X-MC-Unique: i8Oab-U7NF6UzHEcTw4UpA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0936A835DE3; Thu, 3 Dec 2020 10:39:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-182.ams2.redhat.com [10.36.113.182]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9A2175D9CA; Thu, 3 Dec 2020 10:39:09 +0000 (UTC) Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() To: devel@edk2.groups.io, bret.barkelew@microsoft.com, Ard Biesheuvel , "jejb@linux.ibm.com" Cc: "Liming Gao (Byosoft address)" References: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com> <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com> <0f6e92d4-f600-8495-53af-5898a2cdbe2a@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 3 Dec 2020 11:39:08 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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 > > > > > > > > > > > >