Re: [edk2-devel] [PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others

["Laszlo Ersek" <lersek@redhat.com>]

On 09/23/20 10:08, Malgorzata Kukiello wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2982
>
> During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed
> the way memory attributes are handled it affected a function in Page.c that
> would clear all memory attributes even security related ones
>
> Signed-off-by: Malgorzata Kukiello <jacek.kukiello@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 2c2c9cd6c3..9254afb92b 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1933,7 +1933,7 @@ CoreGetMemoryMap (
>    MemoryMapEnd = MemoryMap;
>    MemoryMap = MemoryMapStart;
>    while (MemoryMap < MemoryMapEnd) {
> -    MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
> +    MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ACCESS_MASK;
>      MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
>    }
>    MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
>

(1) Please CC the maintainers that need to review this patch.

I'm doing that for you, for now.

$ python BaseTools/Scripts/GetMaintainer.py -l \
    MdeModulePkg/Core/Dxe/Mem/Page.c

(But note that "BaseTools/Scripts/GetMaintainer.py" can operate on
patches / commits as well; it's better to use it that way, and then to
add corresponding "Cc:" tags to the commit messages themselves.)


(2) Also CC'ing Oleksiy (author of the commit in question), and Ard
(Linux EFI subsystem maintainer).


(3) Your commit reference in the commit message is *still* wrong (I
corrected it before in the BZ). The proper (upstream) commit hash is
3bd5c994c879f78e8e3d5346dc3b627f199291aa.


(4) This patch should be patch#2 in a series of two patches.

Currently this patch has been posted as a thread starter, and the more
foundational patch (the one that modifies the macros in "UefiSpec.h")
has been posted in response to this one.

Instead, there should be a cover letter 0/2, then 1/2 (in response to
the cover letter) should modify the macros, then finally this patch
should be 2/2.

Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone; it will
set up shallow threading (meaning sendemail.thread=True AND
sendemail.chainreplyto=False).


(5) A better subject line for this patch, in my opinion, would be:

  MdeModulePkg/Core/Dxe: expose SP and CRYPTO capabilities in UEFI memmap

Because, near the location that you are modifying, we have the following
helpful comment:

  //
  // Note: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
  //       set attributes and change memory paging attribute accordingly.
  //       But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
  //       value from Capabilities in GCD memory map. This might cause
  //       boot problems. Clearing all paging related capabilities can
  //       workaround it. Following code is supposed to be removed once
  //       the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in
  //       UEFI spec and adopted by both EDK-II Core and all supported
  //       OSs.
  //

Which in fact clarifies your issue report: apparently, the SP and CRYPTO
attributes *are* already treated as capabilities (and not as presently
set attributes) by operating systems. Therefore we should not hide these
capabilities from OSes -- we should not allow the workaround to cover
these capabilities.

(BTW, when Oleksiy posted the original patch and we reviewed it, I
believe this information -- i.e. how OSes would treat these new
capabilities -- was not at our disposal. So I think it was impossible to
tell whether we should hide SP and CRYPTO too, or not.)


(6) I think the comment I quoted above should be clarified too. It
currently says "Clearing all paging related capabilities". It should say
"Clearing all page-access permission capabilities", or something
similar; to better reflect the purpose of EFI_MEMORY_ACCESS_MASK.

Thanks
Laszlo