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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux