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@xxxxxxxxxx/T/#u
> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> ---
> 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