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.
