Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Instead of relying on a questionable heuristic that avoids calling into
> the SetMemoryAttributes () DXE service when the old memory type and the
> new one are subjected to the same NX memory protection policy, make this
> call unconditionally. This avoids corner cases where memory region
> attributes are out of sync with the policy, either due to the fact that
> we are in the middle of ramping up the protections, or due to explicit
> invocations of SetMemoryAttributes() by drivers.
>
> This requires the architecture page table code to be able to deal with
> this, in particular, it needs to be robust against potential recursion
> due to NX policies being applied to newly allocated page tables.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>  1 file changed, 29 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 36987843f142..503feb72b5d0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>    IN  UINT64                Length
>    )
>  {
> -  UINT64      OldAttributes;
>    UINT64      NewAttributes;
> -  EFI_STATUS  Status;
>
>    //
>    // The policy configured in PcdDxeNxMemoryProtectionPolicy
> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>    //
>    NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>
> -  if (OldType != EfiMaxMemoryType) {
> -    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
> -    if (!mAfterDxeNxMemoryProtectionInit &&
> -        (OldAttributes == NewAttributes)) {
> -      return EFI_SUCCESS;
> -    }
> -

This removes some code that does not actually exist - apologies.

It comes down to just removing the conditional checks here, though,
and perform the tail call below unconditionally.

> -    //
> -    // If available, use the EFI memory attribute protocol to obtain
> -    // the current attributes of the region. If the entire region is
> -    // covered and the attributes match, we don't have to do anything.
> -    //
> -    if (mMemoryAttribute != NULL) {
> -      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
> -                                                      Memory,
> -                                                      Length,
> -                                                      &OldAttributes
> -                                                      );
> -      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
> -        return EFI_SUCCESS;
> -      }
> -    }
> -  } else if (NewAttributes == 0) {
> -    // newly added region of a type that does not require protection
> -    return EFI_SUCCESS;
> -  }
> -
>    return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>  }
> --
> 2.39.1
>