From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web10.2419.1606338370133867053
 for <devel@edk2.groups.io>;
 Wed, 25 Nov 2020 13:06:10 -0800
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3F60031B;
	Wed, 25 Nov 2020 13:06:07 -0800 (PST)
Received: from [192.168.1.81] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 56FA63F70D;
	Wed, 25 Nov 2020 13:06:06 -0800 (PST)
Subject: Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()
To: jejb@linux.ibm.com, devel@edk2.groups.io
Cc: Bret Barkelew <brbarkel@microsoft.com>,
 "Liming Gao (Byosoft address)" <gaoliming@byosoft.com.cn>,
 Laszlo Ersek <lersek@redhat.com>
References: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com>
From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
Message-ID: <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com>
Date: Wed, 25 Nov 2020 22:05:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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 <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.