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

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

[Malgorzata Kukiello]

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);
-- 
2.18.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

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

[Malgorzata Kukiello]

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>
---
 MdePkg/Include/Uefi/UefiSpec.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 05b82e0be1..2b1b72d862 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -113,7 +113,8 @@ typedef enum {
 // Attributes bitmasks, grouped by type
 //
 #define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
-#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
+#define EFI_MEMORY_ACCESS_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
+#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_ACCESS_MASK | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
 
 ///
 /// Memory descriptor version number.
-- 
2.18.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

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

[Laszlo Ersek]

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>
> ---
>  MdePkg/Include/Uefi/UefiSpec.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> index 05b82e0be1..2b1b72d862 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -113,7 +113,8 @@ typedef enum {
>  // Attributes bitmasks, grouped by type
>  //
>  #define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
> -#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
> +#define EFI_MEMORY_ACCESS_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
> +#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_ACCESS_MASK | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
>  
>  ///
>  /// Memory descriptor version number.
> 

(1) The subject line of this patch is wrong. The patch modifies MdePkg,
not MdeModulePkg. I suggest:

  MdePkg/UefiSpec: separate page access bitmask from SP and CRYPTO caps

(2) My points (1) through (4) that I made under the other patch in this
series apply here as well, wrt. CC'ing maintainers / reviewers, commit
hash reference, structuring this posting etc. Note that this patch will
have a different set of required CC's.

(3) The commit message is lacking, in my opinion.

The point is that our workaround, in the UEFI memmap construction, must
not clear the SP and CRYPTO bits, because OSes do correctly interpret SP
and CRYPTO as capabilities. For this reason, the SP and CRYPTO bits
should be separated from the bitmask that we use for hiding the
page-access attributes.

Thanks
Laszlo

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

[Laszlo Ersek]

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