[PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[James Bottomley]

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>
---
 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;
     }
-- 
2.26.2

Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Laszlo Ersek]

On 11/25/20 21:13, 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>
> ---
>  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;
>      }
>

(1) CC'ing Jian and Hao:

$ python BaseTools/Scripts/GetMaintainer.py \
    -l MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c

  Jian J Wang <jian.j.wang@intel.com>
  Hao A Wu <hao.a.wu@intel.com>
  Liming Gao <gaoliming@byosoft.com.cn>
  devel@edk2.groups.io


(2) My feedback:

Fixes: 355b181f74050cdf2f09b1755c1a5ee4affb1faf
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

(I tested the actual bugfix with SMM-less OVMF.  I also
regression-tested the patch, namely with SMM OVMF, and ArmVirtQemu too.)


(3) I suggest updating the subject line as follows:

  MdeModulePkg/VariablePolicyLib: Fix runtime panic in ValidateSetVariable()

74 characters, so it's not overlong.

No need to repost because of this.


Liming, can you please pick up my feedback tags from (2), in addition to
your own review, and refresh the subject as requested in (3), and then
merge this patch -- before releasing edk2-stable202011?

Thank you all,
Laszlo

回复: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[gaoliming]

Laszlo and James:
  Thanks for your root cause. The fix is clear. It works on runtime version and smm version. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
  
  I agree to merge this hot fix for this stable tag 202011. I will add Laszlo tag and update subject title when I merge this patch. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Laszlo Ersek <lersek@redhat.com>
> 发送时间: 2020年11月26日 5:01
> 收件人: jejb@linux.ibm.com; Liming Gao (Byosoft address)
> <gaoliming@byosoft.com.cn>
> 抄送: devel@edk2.groups.io; Bret Barkelew <brbarkel@microsoft.com>; Ard
> Biesheuvel (ARM address) <ard.biesheuvel@arm.com>; Hao A Wu
> <hao.a.wu@intel.com>; Jian J Wang <jian.j.wang@intel.com>
> 主题: Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()
> 
> On 11/25/20 21:13, 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>
> > ---
> >  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;
> >      }
> >
> 
> (1) CC'ing Jian and Hao:
> 
> $ python BaseTools/Scripts/GetMaintainer.py \
>     -l MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> 
>   Jian J Wang <jian.j.wang@intel.com>
>   Hao A Wu <hao.a.wu@intel.com>
>   Liming Gao <gaoliming@byosoft.com.cn>
>   devel@edk2.groups.io
> 
> 
> (2) My feedback:
> 
> Fixes: 355b181f74050cdf2f09b1755c1a5ee4affb1faf
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (I tested the actual bugfix with SMM-less OVMF.  I also
> regression-tested the patch, namely with SMM OVMF, and ArmVirtQemu too.)
> 
> 
> (3) I suggest updating the subject line as follows:
> 
>   MdeModulePkg/VariablePolicyLib: Fix runtime panic in
> ValidateSetVariable()
> 
> 74 characters, so it's not overlong.
> 
> No need to repost because of this.
> 
> 
> Liming, can you please pick up my feedback tags from (2), in addition to
> your own review, and refresh the subject as requested in (3), and then
> merge this patch -- before releasing edk2-stable202011?
> 
> Thank you all,
> Laszlo

回复: [edk2-devel] 回复: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[gaoliming]

PR https://github.com/tianocore/edk2/pull/1148 is created. 

Thanks
Liming

> -----邮件原件-----
> 发件人: bounce+27952+68010+4905953+8761045@groups.io
> <bounce+27952+68010+4905953+8761045@groups.io> 代表 gaoliming
> 发送时间: 2020年11月26日 9:23
> 收件人: 'Laszlo Ersek' <lersek@redhat.com>; jejb@linux.ibm.com
> 抄送: devel@edk2.groups.io; 'Bret Barkelew' <brbarkel@microsoft.com>;
> 'Ard Biesheuvel (ARM address)' <ard.biesheuvel@arm.com>; 'Hao A Wu'
> <hao.a.wu@intel.com>; 'Jian J Wang' <jian.j.wang@intel.com>
> 主题: [edk2-devel] 回复: [PATCH] MdeModulePkg: Fix runtime panic in
> ValidateSetVariable()
> 
> Laszlo and James:
>   Thanks for your root cause. The fix is clear. It works on runtime version and
> smm version. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> 
>   I agree to merge this hot fix for this stable tag 202011. I will add Laszlo tag
> and update subject title when I merge this patch.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: Laszlo Ersek <lersek@redhat.com>
> > 发送时间: 2020年11月26日 5:01
> > 收件人: jejb@linux.ibm.com; Liming Gao (Byosoft address)
> > <gaoliming@byosoft.com.cn>
> > 抄送: devel@edk2.groups.io; Bret Barkelew <brbarkel@microsoft.com>;
> Ard
> > Biesheuvel (ARM address) <ard.biesheuvel@arm.com>; Hao A Wu
> > <hao.a.wu@intel.com>; Jian J Wang <jian.j.wang@intel.com>
> > 主题: Re: [PATCH] MdeModulePkg: Fix runtime panic in
> ValidateSetVariable()
> >
> > On 11/25/20 21:13, 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>
> > > ---
> > >  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;
> > >      }
> > >
> >
> > (1) CC'ing Jian and Hao:
> >
> > $ python BaseTools/Scripts/GetMaintainer.py \
> >     -l MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> >
> >   Jian J Wang <jian.j.wang@intel.com>
> >   Hao A Wu <hao.a.wu@intel.com>
> >   Liming Gao <gaoliming@byosoft.com.cn>
> >   devel@edk2.groups.io
> >
> >
> > (2) My feedback:
> >
> > Fixes: 355b181f74050cdf2f09b1755c1a5ee4affb1faf
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Tested-by: Laszlo Ersek <lersek@redhat.com>
> >
> > (I tested the actual bugfix with SMM-less OVMF.  I also
> > regression-tested the patch, namely with SMM OVMF, and ArmVirtQemu
> too.)
> >
> >
> > (3) I suggest updating the subject line as follows:
> >
> >   MdeModulePkg/VariablePolicyLib: Fix runtime panic in
> > ValidateSetVariable()
> >
> > 74 characters, so it's not overlong.
> >
> > No need to repost because of this.
> >
> >
> > Liming, can you please pick up my feedback tags from (2), in addition to
> > your own review, and refresh the subject as requested in (3), and then
> > merge this patch -- before releasing edk2-stable202011?
> >
> > Thank you all,
> > Laszlo
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Ard Biesheuvel]

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.

Re: [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Laszlo Ersek]

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

Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

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<mailto:lersek=redhat.com@groups.io>
Sent: Wednesday, November 25, 2020 1:17 PM
To: Ard Biesheuvel<mailto:ard.biesheuvel@arm.com>; jejb@linux.ibm.com<mailto:jejb@linux.ibm.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Liming Gao (Byosoft address)<mailto:gaoliming@byosoft.com.cn>
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&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C2e91993035204bbd307d08d891878686%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637419358545184416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=uYtJTRY5RRS5XJ7j%2Bo%2B75qH12ROX9%2FQ4v1GMdUbLk3I%3D&amp;reserved=0
>> 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







[-- Attachment #2: Type: text/html, Size: 7503 bytes --]

Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Laszlo Ersek]

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<mailto:lersek=redhat.com@groups.io>
> Sent: Wednesday, November 25, 2020 1:17 PM
> To: Ard Biesheuvel<mailto:ard.biesheuvel@arm.com>; jejb@linux.ibm.com<mailto:jejb@linux.ibm.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Liming Gao (Byosoft address)<mailto:gaoliming@byosoft.com.cn>
> 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&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C2e91993035204bbd307d08d891878686%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637419358545184416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=uYtJTRY5RRS5XJ7j%2Bo%2B75qH12ROX9%2FQ4v1GMdUbLk3I%3D&amp;reserved=0
>>> 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
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Ard Biesheuvel]

On 12/3/20 11:39 AM, Laszlo Ersek wrote:
> 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.
> 

I thought traditional SMM drivers could call DXE protocols before end of
DXE, no? Some boot services return pool allocated memory that has to be
freed by the caller (e.g., LocateHandleBuffer())

Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable()

[Laszlo Ersek]

On 12/03/20 12:25, Ard Biesheuvel wrote:
> On 12/3/20 11:39 AM, Laszlo Ersek wrote:
>> 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.
>>
> 
> I thought traditional SMM drivers could call DXE protocols before end of
> DXE, no? Some boot services return pool allocated memory that has to be
> freed by the caller (e.g., LocateHandleBuffer())

Yes, and in that case, the SMM driver should release the memory with
gBS->FreePool(), not FreePool(). The fact that FreePool() in the SMM
instance of MemoryAllocationLib copes, since commit 1d733f731f968, is a
misfeature, IMO.

(Or even better, the SMM driver should release the memory with
SystemTable->BootServices->FreePool(), as having the "gBS" global
variable in the SMM module is apparently sometimes seen as a liability;
see SmmServicesTableLibConstructor() in
"MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.c".)

Thanks
Laszlo