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

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

Posted by Malgorzata Kukiello 3 years, 7 months ago

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.
 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65519): https://edk2.groups.io/g/devel/message/65519
Mute This Topic: https://groups.io/mt/77031257/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 3 years, 7 months ago

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65525): https://edk2.groups.io/g/devel/message/65525
Mute This Topic: https://groups.io/mt/77031257/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-