Re: [PATCH] x86/sgx: Enable PROT_EXEC for EAUG'd pages

[Jarkko Sakkinen]

On Mon, Mar 07, 2022 at 05:35:04PM +0200, Jarkko Sakkinen wrote:
> vm_max_permissions was created to control the pre-initialization content
> that contributes to MRSIGNATURE. It was never meant to be as a limit to
> dynamically added pages.
> 
> E.g. static content could be used as a hook for LSM's to decide whether
> certain signature is qualified for EINIT. Dynamic content has nothing to
> do with that. The current mechanisms only add to the complexity on how
> to control PTE and EPCM permissions, and do not add anything else than
> obfuscity to security side of things.
> 
> Thus add PROT_EXEC to the permissions assigned by the #PF handler.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 79e39bd99c09..0256918b2c2f 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -160,12 +160,11 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma,
>  	encl_page->encl = encl;
>  
>  	/*
> -	 * Adding a regular page that is architecturally allowed to only
> -	 * be created with RW permissions.
> -	 * TBD: Interface with user space policy to support max permissions
> -	 * of RWX.
> +	 * Dynamic pages do not contribute to MRSIGNATURE, i.e. they are
> +	 * controlled only by PTE and EPCM permissions. Thus, the no limit
> +	 * is set here.
>  	 */
> -	prot = PROT_READ | PROT_WRITE;
> +	prot = PROT_READ | PROT_WRITE | PROT_EXEC;
>  	encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
>  	encl_page->vm_max_prot_bits = encl_page->vm_run_prot_bits;
>  
> -- 
> 2.35.1
> 

This is really a show stopper. I think here's a logical mistake on for what
purpose vm_max_prot_bits are used for. They are meant for the static and
also signed content of the enclave.

These changes in the patch set that are related to vm_max_prot_bits only
messes up what already exists, and make incredibly hard to implement
anything decent on top of SGX2 features.

BR, Jarkko