Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Mar 2022 18:09:03 +0200
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     linux-sgx@vger.kernel.org
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        Nathaniel McCallum <nathaniel@profian.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/sgx: Enable PROT_EXEC for EAUG'd pages
Message-ID: <YiYuH95wps+yHDk1@iki.fi>
References: <20220307153504.198112-1-jarkko@kernel.org>
 <YiYseF3aKI70x3d8@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YiYseF3aKI70x3d8@iki.fi>
Precedence: bulk

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.
> > 
> > Thus add PROT_EXEC to the permissions assigned by the #PF handler.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.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.

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.

BR, Jarkko