From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.2615.1606339051004241547 for ; Wed, 25 Nov 2020 13:17:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Fv3YOwMu; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606339050; 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=exDWLuf7vAyQ3tRbgaKMz6luyf/D/bcCw3HAHzKvRX8=; b=Fv3YOwMuiDT91Dau0uo3QHGKrzPOUc3C6ZURzdyYM0IycocDQm3m4KfENnYM2CTL/WcXhU 0R8bI4VQEAYj/1Ba4+obJiMwg4L0b9cVdulO6g7q7Dli3eq6vnl8KogziFuUJ7L+hR5HON WbW+HGlNzvdW7wo+FCBZLw0hCeNH1Ok= 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-553-KH3X6yFMMVOuPPzF959V8w-1; Wed, 25 Nov 2020 16:17:28 -0500 X-MC-Unique: KH3X6yFMMVOuPPzF959V8w-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0F56C107B462; Wed, 25 Nov 2020 21:17:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-239.ams2.redhat.com [10.36.112.239]) by smtp.corp.redhat.com (Postfix) with ESMTP id C32809CA0; Wed, 25 Nov 2020 21:17:22 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() To: Ard Biesheuvel , jejb@linux.ibm.com, devel@edk2.groups.io Cc: Bret Barkelew , "Liming Gao (Byosoft address)" References: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com> <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com> From: "Laszlo Ersek" Message-ID: <0f6e92d4-f600-8495-53af-5898a2cdbe2a@redhat.com> Date: Wed, 25 Nov 2020 22:17:21 +0100 MIME-Version: 1.0 In-Reply-To: <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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://bugzilla.tianocore.org/show_bug.cgi?id=3092 >> 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