linux-kernel - Re: [PATCH RFC] x86/sgx: Simplify struct sgx_enclave_restrict

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Apr 2022 10:21:22 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>, <linux-sgx@...r.kernel.rog>
CC:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Nathaniel McCallum <nathaniel@...fian.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "open list:INTEL SGX" <linux-sgx@...r.kernel.org>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] x86/sgx: Simplify struct
 sgx_enclave_restrict_permissions

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@...nel.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