Re: [RFC PATCH v2 12/26] x86/sgx: Add helper to update SGX_LEPUBKEYHASHn MSRs

Kai Huang
2021-01-21T01:10:22Z

On Wed, 20 Jan 2021 15:50:31 -0800 Dave Hansen wrote:
> On 1/20/21 3:36 PM, Kai Huang wrote:
> > I actually feel the function name already explains what the function does
> > clearly, therefore I don't think even comment is needed. To be honest I
> > don't know how to rephrase here. Perhaps:
> >
> > /* Update SGX LEPUBKEYHASH MSRs of the platform. */
>
> Whee! I'm gonna write me a function comment!
>
> /*
> * A Launch Enclave (LE) must be signed with a public key
> * that matches this SHA256 hash. Usually overwrites Intel's
> * default signing key.
> */
>
> So, this isn't a one-liner. *But*, it tells us what "le" means, what
> "pubkey" means and implies that there need to be 4x64-bits worth of MSR
> writes to get to a SHA256 hash.

In current linux driver implementation, LE is effectively abandoned, because
the initialization of any enclave doesn't take a valid TOKEN, making
initializing enclave requires hash of enclave's signer equal to the hash in
SGX_LEPUBKEYHASH MSRs.

I written the function name based on SDM's description, to reflect the fact
that we are updating the SGX_LEPUBKEYHASH MSRs, but nothing more.

So perhaps below?

/*
* Update the SGX_LEPUBKEYHASH MSRs to the values specified by caller.
*
* EINITTOKEN is not used in enclave initialization, which requires
* hash of enclave's signer must match values in SGX_LEPUBKEYHASH MSRs
* to make EINIT be successful.
*/

It also tells what it's usually doing
> here: overwriting Intel's blasted hash.

Technically, only initial value is intel's pubkey hash. This function
overwrites whatever pubkey hash that used to sign previous enclave.

>
> It sure beats the entirely uncommented for loop that we've got today.

Agreed, although to me it seems the comment is a little bit out of the scope
of this function itself, but is more about the logic of the caller.