Re: [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Mar 01, 2017 at 04:31:42PM +0000, Ard Biesheuvel wrote:
> Now that we have the prerequisite functionality available in ArmMmuLib,
> wire it up into ArmSetMemoryRegionNoExec, ArmClearMemoryRegionNoExec,
> ArmSetMemoryRegionReadOnly and ArmClearMemoryRegionReadOnly. This is
> used by the non-executable stack feature that is configured by DxeIpl.
> 
> NOTE: The current implementation will not combine RO and XP attributes,
>       i.e., setting/clearing a region no-exec will unconditionally
>       clear the read-only attribute, and vice versa. Currently, we
>       only use ArmSetMemoryRegionNoExec(), so for now, we should be
>       able to live with this.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 1112660b434e..55601328d93e 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -761,40 +761,40 @@ ArmSetMemoryAttributes (
>    return Status;
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS

Could these RETURN_->EFI_ fixes be folded into 1/5 instead (if you've
not already pushed it by the time you get here)?

>  ArmSetMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, 0);

I'd be slightly happier if there was a #define for that 0, throughout.
Alternatively, replace with a macro called ArmClearMemoryAttributes().

/
    Leif

>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmSetMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, 0);
>  }
>  
>  RETURN_STATUS
> -- 
> 2.7.4
>