Re: [PATCH V3 14/30] x86/sgx: Support restricting of enclave page permissions

[Jarkko Sakkinen]

On Tue, 2022-04-05 at 09:49 -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 4/5/2022 7:52 AM, Jarkko Sakkinen wrote:
> > n Tue, 2022-04-05 at 17:27 +0300, Jarkko Sakkinen wrote:
> > > According to SDM having page type as regular is fine for EMODPR,
> > > i.e. that's why I did not care about having it in SECINFO.
> > > 
> > > Given that the opcode itself contains validation, I wonder
> > > why this needs to be done:
> > > 
> > > if (secinfo.flags & ~SGX_SECINFO_PERMISSION_MASK)
> > >                 return -EINVAL;
> > > 
> > > if (memchr_inv(secinfo.reserved, 0, sizeof(secinfo.reserved)))
> > >                 return -EINVAL;
> > > 
> > > perm = secinfo.flags & SGX_SECINFO_PERMISSION_MASK;
> > > 
> > > I.e. why duplicate validation and why does it have different
> > > invariant than the opcode?
> > 
> > Right it is done to prevent exceptions and also pseudo-code
> > has this validation:
> > 
> > IF (EPCM(DS:RCX).PT is not PT_REG) THEN #PF(DS:RCX); FI; 
> 
> The current type of the page is validated - not the page type
> provided in the parameters of the command.
> 
> > 
> > This is clearly wrong:
> 
> Could you please elaborate what is wrong? The hardware only checks
> the permission bits and that is what is provided.

I think it's for most a bit confusing that it takes a special Linux
defined SECINFO instead of what you read from spec. 

> 
> > 
> > /*
> >  * Return valid permission fields from a secinfo structure provided by
> >  * user space. The secinfo structure is required to only have bits in
> >  * the permission fields set.
> >  */
> > static int sgx_perm_from_user_secinfo(void __user *_secinfo, u64 *secinfo_perm)
> > 
> > It means that the API requires a malformed data as input.
> 
> It is not clear to me how this is malformed. The API requires that only
> the permission bits are set in the secinfo, only the permission bits in secinfo
> is provided to the hardware, and the hardware only checks the permission bits.
> 
> > 
> > Maybe it would be better idea then to replace secinfo with just the
> > permission field?
> 
> That is what I implemented in V1 [1], but was asked to change to secinfo. I could
> go back to that if you prefer.

Yeah, if I was the one saying that, I was clearly wrong. But also
perspective is now very different after using a lot of these
features.

Alternatively you could have a single "mod" ioctl given the disjoint
nature how the parameters go to SECINFO.


> Reinette
> 
> [1] https://lore.kernel.org/linux-sgx/44fe170cfd855760857660b9f56cae8c4747cc15.1638381245.git.reinette.chatre@xxxxxxxxx/

BR, Jarkko