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