Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote:
> The validation of TCS permissions was missing from
> sgx_validate_secinfo(). This patch adds the validation.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 99b1b9776c3a..2415dcb20a6e 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
>  		return -EINVAL;
>  
> +	/* CPU will silently overwrite the permissions as zero, which means

Comment style again.  I looked it up just to double check :)


Documentation/process/coding-style.rst says:

When commenting the kernel API functions, please use the kernel-doc format.
See the files at :ref:`Documentation/doc-guide/ <doc_guide>` and
``scripts/kernel-doc`` for details.

The preferred style for long (multi-line) comments is:

.. code-block:: c

        /*
         * This is the preferred style for multi-line
         * comments in the Linux kernel source code.
         * Please use it consistently.
         *
         * Description:  A column of asterisks on the left side,
         * with beginning and ending almost-blank lines.
         */

For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.

.. code-block:: c

        /* The preferred comment style for files in net/ and drivers/net
         * looks like this.
         *
         * It is nearly the same as the generally preferred comment style,
         * but there is no initial almost-blank line.
         */


> +	 * that we need to validate it ourselves.
> +	 */
> +	if (page_type == SGX_SECINFO_TCS && perm)
> +		return -EINVAL;

Why are we validating the TCS protection bits?  Hardware ignores them, so
why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
bits so there's no danger of putting the wrong thing in the page tables.

> +
>  	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
>  		return -EINVAL;
>  
> -- 
> 2.20.1
>