On Tue, Sep 15, 2020 at 02:28:30PM +0300, Jarkko Sakkinen wrote:
> +static int sgx_validate_secs(const struct sgx_secs *secs)
> +{
> + u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
> + sgx_encl_size_max_64 : sgx_encl_size_max_32;
> +
> + if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> + return -EINVAL;
> +
> + if (secs->base & (secs->size - 1))
> + return -EINVAL;
> +
> + if (secs->miscselect & sgx_misc_reserved_mask ||
> + secs->attributes & sgx_attributes_reserved_mask ||
> + secs->xfrm & sgx_xfrm_reserved_mask)
> + return -EINVAL;
> +
> + if (secs->size > max_size)
> + return -EINVAL;
> +
> + if (!(secs->xfrm & XFEATURE_MASK_FP) ||
> + !(secs->xfrm & XFEATURE_MASK_SSE) ||
> + (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
> + ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
Let that last line stick out so that you have each statement on a single line.
> + return -EINVAL;
> +
> + if (!secs->ssa_frame_size)
> + return -EINVAL;
> +
> + if (sgx_calc_ssa_frame_size(secs->miscselect, secs->xfrm) >
> + secs->ssa_frame_size)
Let that stick out.
> + return -EINVAL;
> +
> + if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
> + memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
> + memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
> + memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> +{
> + unsigned long encl_size = secs->size + PAGE_SIZE;
You're still using secs->size before validation. I know, we will return
early if sgx_validate_secs() fails but pls move that addition after the
validation call.
...
> +/**
> + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> + * @filep: open file to /dev/sgx
Dammit, how many times do I have to type this comment here?!
"That's
@encl: enclave pointer
or so."
There's no filep - there is an encl!
> + * @arg: userspace pointer to a struct sgx_enclave_create instance
> + *
> + * Allocate kernel data structures for a new enclave and execute ECREATE after
> + * verifying the correctness of the provided SECS.
... which is done in sgx_validate_secs()."
Yes, spell it out, otherwise one has to wonder where that validation is
happening in the function *below* because the comment is over it - not
over sgx_validate_secs().
And yes, you need to spell stuff like that out because this SGX crap is
complex and it better be properly documented!
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
