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.
