linux-kernel - Re: [PATCH] x86/sgx: Enable PROT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YiYseF3aKI70x3d8@iki.fi>
Date:   Mon, 7 Mar 2022 18:02:00 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     linux-sgx@...r.kernel.org
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Nathaniel McCallum <nathaniel@...fian.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/sgx: Enable PROT_EXEC for EAUG'd pages

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@...nel.org>
> ---
>  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