[PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict_permissions

[PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict_permissions

[Jarkko Sakkinen]

The reasoning to change SECINFO to simply flags is stated in this inline
comment:

/*
 * 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.
 */

It is better to simply change the parameter type than require to use
a malformed version of a data structure.

Link: https://lore.kernel.org/linux-sgx/26ab773de8842d03b40caf8645ca86884b195901.camel@kernel.org/T/#u
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
Only compile-tested.
 arch/x86/include/uapi/asm/sgx.h |  5 ++-
 arch/x86/kernel/cpu/sgx/ioctl.c | 57 ++++++---------------------------
 2 files changed, 12 insertions(+), 50 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index feda7f85b2ce..627136798f2a 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -88,15 +88,14 @@ struct sgx_enclave_provision {
  * @offset:	starting page offset (page aligned relative to enclave base
  *		address defined in SECS)
  * @length:	length of memory (multiple of the page size)
- * @secinfo:	address for the SECINFO data containing the new permission bits
- *		for pages in range described by @offset and @length
+ * @flags:	flags field of the SECINFO data
  * @result:	(output) SGX result code of ENCLS[EMODPR] function
  * @count:	(output) bytes successfully changed (multiple of page size)
  */
 struct sgx_enclave_restrict_permissions {
 	__u64 offset;
 	__u64 length;
-	__u64 secinfo;
+	__u64 flags;
 	__u64 result;
 	__u64 count;
 };
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index f88bc1236276..3c334e0bd4d9 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -676,41 +676,6 @@ static int sgx_ioc_sgx2_ready(struct sgx_encl *encl)
 	return 0;
 }
 
-/*
- * 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)
-{
-	struct sgx_secinfo secinfo;
-	u64 perm;
-
-	if (copy_from_user(&secinfo, (void __user *)_secinfo,
-			   sizeof(secinfo)))
-		return -EFAULT;
-
-	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;
-
-	/*
-	 * Read access is required for the enclave to be able to use the page.
-	 * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] require
-	 * read access.
-	 */
-	if (!(perm & SGX_SECINFO_R))
-		return -EINVAL;
-
-	*secinfo_perm = perm;
-
-	return 0;
-}
-
 /*
  * Some SGX functions require that no cached linear-to-physical address
  * mappings are present before they can succeed. Collaborate with
@@ -753,7 +718,6 @@ static int sgx_enclave_etrack(struct sgx_encl *encl)
  * sgx_enclave_restrict_permissions() - Restrict EPCM permissions
  * @encl:	Enclave to which the pages belong.
  * @modp:	Checked parameters from user on which pages need modifying.
- * @secinfo_perm: New (validated) permission bits.
  *
  * Return:
  * - 0:		Success.
@@ -761,8 +725,7 @@ static int sgx_enclave_etrack(struct sgx_encl *encl)
  */
 static long
 sgx_enclave_restrict_permissions(struct sgx_encl *encl,
-				 struct sgx_enclave_restrict_permissions *modp,
-				 u64 secinfo_perm)
+				 struct sgx_enclave_restrict_permissions *modp)
 {
 	struct sgx_encl_page *entry;
 	struct sgx_secinfo secinfo;
@@ -772,7 +735,7 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl,
 	int ret;
 
 	memset(&secinfo, 0, sizeof(secinfo));
-	secinfo.flags = secinfo_perm;
+	secinfo.flags = modp->flags;
 
 	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
 		addr = encl->base + modp->offset + c;
@@ -871,7 +834,6 @@ static long sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl,
 						 void __user *arg)
 {
 	struct sgx_enclave_restrict_permissions params;
-	u64 secinfo_perm;
 	long ret;
 
 	ret = sgx_ioc_sgx2_ready(encl);
@@ -884,15 +846,16 @@ static long sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl,
 	if (sgx_validate_offset_length(encl, params.offset, params.length))
 		return -EINVAL;
 
-	ret = sgx_perm_from_user_secinfo((void __user *)params.secinfo,
-					 &secinfo_perm);
-	if (ret)
-		return ret;
-
-	if (params.result || params.count)
+	/*
+	 * Read access is required for the enclave to be able to use the page.
+	 * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] require read
+	 * access.
+	 */
+	if (params.flags & ~SGX_SECINFO_PERMISSION_MASK || !(params.flags & SGX_SECINFO_R) ||
+	    params.result || params.count)
 		return -EINVAL;
 
-	ret = sgx_enclave_restrict_permissions(encl, &params, secinfo_perm);
+	ret = sgx_enclave_restrict_permissions(encl, &params);
 
 	if (copy_to_user(arg, &params, sizeof(params)))
 		return -EFAULT;
-- 
2.35.1

Re: [PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict_permissions

[Reinette Chatre]

Hi Jarkko,

On 4/5/2022 8:16 AM, Jarkko Sakkinen wrote:
> The reasoning to change SECINFO to simply flags is stated in this inline
> comment:
> 
> /*
>  * 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.
>  */
> 
> It is better to simply change the parameter type than require to use
> a malformed version of a data structure.

Could you please elaborate what is malformed?

> 
> Link: https://lore.kernel.org/linux-sgx/26ab773de8842d03b40caf8645ca86884b195901.camel@kernel.org/T/#u
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> Only compile-tested.
>  arch/x86/include/uapi/asm/sgx.h |  5 ++-
>  arch/x86/kernel/cpu/sgx/ioctl.c | 57 ++++++---------------------------
>  2 files changed, 12 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index feda7f85b2ce..627136798f2a 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -88,15 +88,14 @@ struct sgx_enclave_provision {
>   * @offset:	starting page offset (page aligned relative to enclave base
>   *		address defined in SECS)
>   * @length:	length of memory (multiple of the page size)
> - * @secinfo:	address for the SECINFO data containing the new permission bits
> - *		for pages in range described by @offset and @length
> + * @flags:	flags field of the SECINFO data
>   * @result:	(output) SGX result code of ENCLS[EMODPR] function
>   * @count:	(output) bytes successfully changed (multiple of page size)
>   */
>  struct sgx_enclave_restrict_permissions {
>  	__u64 offset;
>  	__u64 length;
> -	__u64 secinfo;
> +	__u64 flags;
>  	__u64 result;
>  	__u64 count;
>  };

What is the motivation for using the flags field of the SECINFO data? If
the goal is to only provide necessary information, why not just provide the
permission bits since none of the other bits are used? If the goal is
to make room for future SECINFO changes/demands, why not include the
reserved field of SECINFO where future changes are most likely to reside? 

Reinette

Re: [PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict_permissions

[Jarkko Sakkinen]

On Tue, 2022-04-05 at 10:21 -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 4/5/2022 8:16 AM, Jarkko Sakkinen wrote:
> > The reasoning to change SECINFO to simply flags is stated in this inline
> > comment:
> > 
> > /*
> >  * 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.
> >  */
> > 
> > It is better to simply change the parameter type than require to use
> > a malformed version of a data structure.
> 
> Could you please elaborate what is malformed?

The structure that is accepted by the API. According to SDM permission
changes are done with a structure where PT_REG is set, which gives
-EINVAL. I categorize it as a bug.

BR, Jarkko

Re: [PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict_permissions

[Reinette Chatre]

Hi Jarkko,

On 4/5/2022 11:30 AM, Jarkko Sakkinen wrote:
> On Tue, 2022-04-05 at 10:21 -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 4/5/2022 8:16 AM, Jarkko Sakkinen wrote:
>>> The reasoning to change SECINFO to simply flags is stated in this inline
>>> comment:
>>>
>>> /*
>>>  * 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.
>>>  */
>>>
>>> It is better to simply change the parameter type than require to use
>>> a malformed version of a data structure.
>>
>> Could you please elaborate what is malformed?
> 
> The structure that is accepted by the API. According to SDM permission
> changes are done with a structure where PT_REG is set, which gives
> -EINVAL. I categorize it as a bug.

I assume that you are referring to this line from the SDM:

IF (EPCM(DS:RCX).PT is not PT_REG)
    THEN #PF(DS:RCX); FI;

Please note that the above tests the PT bit of the EPCM
entry, not the PT field in the provided SECINFO.

Reinette