On Tue, 2022-04-05 at 11:59 -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 4/5/2022 11:39 AM, Jarkko Sakkinen wrote: > > 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. > > No problem, I understand. > > I plan to replace the current "secinfo" field in struct sgx_enclave_restrict_permissions > with a new "permissions" field that contain only the permissions. Please let > me know if you have concerns with this (I also discuss this more in reply to > your other message related to the page type change ioctl()). I'm cool with it but if it is named as "permissions", then it is already software-defined entity, i.e. meaning just that have this check in place in the ioctl: if (addp->permissions & !(PROT_READ | PROT_WRITE | PROT_EXEC)) return -EINVAL; BR, Jarkko
