RE: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

["Ayoun, Serge" <serge.ayoun@intel.com>]

> From: linux-sgx-owner@vger.kernel.org <linux-sgx-owner@vger.kernel.org>
> On Behalf Of Jarkko Sakkinen
> Sent: Wednesday, August 21, 2019 21:46
> To: linux-sgx@vger.kernel.org
> Cc: Christopherson, Sean J <sean.j.christopherson@intel.com>
> Subject: Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in
> sgx_validate_secinfo()
> 
> On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote:
> > The validation of TCS permissions was missing from
> > sgx_validate_secinfo(). This patch adds the validation.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 99b1b9776c3a..2415dcb20a6e 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo
> *secinfo)
> >  	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
> >  		return -EINVAL;
> >
> > +	/* CPU will silently overwrite the permissions as zero, which means
> > +	 * that we need to validate it ourselves.
> > +	 */
> > +	if (page_type == SGX_SECINFO_TCS && perm)
> > +		return -EINVAL;
> > +
> >  	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
> >  		return -EINVAL;
> >
> > --
> > 2.20.1
> >
> 
> OK, just found out that this patch did not end up to my test image and
> causes a regression.
> 
> I think this should be fixed in sgx_encl_may_map() by having the
> following special case for TCS (in addition to the change in this
> patch of course):
> 
> 1. Check if we encounter a TCS page.
> 2. If yes, we evaluate RW for the VM flags.
> 

Also replying to Sean.
Sean is right that never mind the value in secsinfo->flags, HW will reset RWX
For TCS pages.
So basically you may not enforce and and could not check those but... The signature depends
On those flags, so if you put a non-zero flag value, eadd will pass but if you
compute the signature according to this non zero value then you will have
a delta between ur signature and HW's signature: einit will fail.
So this is tricky and more a usability issue.
I vote for checking the flag is zeroed.
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.