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

[Jarkko Sakkinen]

On Mon, Mar 07, 2022 at 09:13:48AM -0800, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 3/7/2022 8:09 AM, Jarkko Sakkinen wrote:
> > On Mon, Mar 07, 2022 at 06:02:03PM +0200, Jarkko Sakkinen wrote:
> >> 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.
> 
> Linux has mechanisms to enforce what can be executed. For example, with SELinux
> a process can be required to have PROCESS__EXECHEAP or PROCESS__EXECSTACK 
> before it can be allowed to execute writable memory.
> 
> A few SGX runtimes enables unmodified executables to be run within SGX enclaves.
> 
> Does a change like this not enable executables prevented by existing 
> security mechanisms to circumvent such restrictions by running within
> a SGX enclave?

It does not open any extra exposure as the existing policies apply for
the enclave content created before initialization.

And I'm not sure what kind of circumvention scenario we are talking
about.

> >>> 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.
> > 
> > I.e. once signed content has passed EINIT ioctl, and whatever checks
> > there are now or in future (e.g. LSM hooks), the system has accepted
> > the enclave behaviour, and it includes also the use of EACCEPT opcode.
> > 
> > It's the exec or no-exec decision point. The thing that these patches
> > do is making an obfuscated mess of all this. When EINIT has passed,
> > it has been decided that the enclave can do its workload. Let's not
> > throw stick in front of it, and make everyones life misserable.
> 
> A common use for these dynamically added pages is to increase the heap
> and stack. Always allowing PTEs of RWX on these pages irrespective
> whether it will be used for heap, stack, or relocatable code does
> not match with how the kernel manages protections.
> 
> As I said before I am not comfortable with such a change and cannot
> sign off on this. I would defer to the maintainers to choose the
> direction.
> 
> Reinette

My choice is to not use this existing mechanism for dynamically created
pages because otherwise the implementation overally is just crippled.

Something unusable is for sure as secure as you can get.

BR, Jarkko