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

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

On Thu, 21 Jan 2021 14:06:38 +1300 Kai Huang wrote:
> 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.
> */
>

Actually I take it back. This is only valid for bare-metal driver. For KVM
guest, it should be:

/*
* Update the SGX_LEPUBKEYHASH MSRs according to guest's *virtual*
* SGX_LEPUBKEYHASH MSRs values, to make EINIT from guest consistent
* with hardware behavior.
*/

So like I said below, the comment is actually more reasonable for the logic of
caller of this function.

Makes sense?

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