| Message ID | 5116fdc732e8e14b3378c44e3b461a43f330ed0c.1610935432.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM SGX virtualization support | expand |
On Mon, Jan 18, 2021 at 04:28:05PM +1300, Kai Huang wrote: > Add a helper to update SGX_LEPUBKEYHASHn MSRs. SGX virtualization also > needs to update those MSRs based on guest's "virtual" SGX_LEPUBKEYHASHn > before EINIT from guest. > > Acked-by: Dave Hansen <dave.hansen@intel.com> > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 5 ++--- > arch/x86/kernel/cpu/sgx/main.c | 8 ++++++++ > arch/x86/kernel/cpu/sgx/sgx.h | 2 ++ > 3 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index e5977752c7be..1bae754268d1 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -495,7 +495,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, > void *token) > { > u64 mrsigner[4]; > - int i, j, k; > + int i, j; > void *addr; > int ret; > > @@ -544,8 +544,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, > > preempt_disable(); > > - for (k = 0; k < 4; k++) > - wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + k, mrsigner[k]); > + sgx_update_lepubkeyhash(mrsigner); > > ret = __einit(sigstruct, token, addr); > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index bdda631c975b..1cf1f0f058b8 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -697,6 +697,14 @@ static bool __init sgx_page_cache_init(void) > return true; > } > > +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) > +{ > + int i; > + > + for (i = 0; i < 4; i++) > + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); > +} Missing kdoc. > + > static void __init sgx_init(void) > { > int ret; > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index 509f2af33e1d..ccd4f145c464 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -83,4 +83,6 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page); > int sgx_unmark_page_reclaimable(struct sgx_epc_page *page); > struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim); > > +void sgx_update_lepubkeyhash(u64 *lepubkeyhash); > + > #endif /* _X86_SGX_H */ > -- > 2.29.2 > > /Jarkko
On 1/20/21 4:03 AM, Jarkko Sakkinen wrote: >> +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) >> +{ >> + int i; >> + >> + for (i = 0; i < 4; i++) >> + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); >> +} > Missing kdoc. I dunno... kdoc is nice, but I'm not sure its verbosity is useful here, even if this function is called from more than one .c file. I'd be happy with a single-line comment, personally.
On Wed, 20 Jan 2021 10:36:09 -0800 Dave Hansen wrote: > On 1/20/21 4:03 AM, Jarkko Sakkinen wrote: > >> +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < 4; i++) > >> + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); > >> +} > > Missing kdoc. > > I dunno... kdoc is nice, but I'm not sure its verbosity is useful here, > even if this function is called from more than one .c file. > > I'd be happy with a single-line comment, personally. > 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. */ ?
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. It also tells what it's usually doing here: overwriting Intel's blasted hash. It sure beats the entirely uncommented for loop that we've got today.
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.
On Wed, Jan 20, 2021 at 10:36:09AM -0800, Dave Hansen wrote: > On 1/20/21 4:03 AM, Jarkko Sakkinen wrote: > >> +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < 4; i++) > >> + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); > >> +} > > Missing kdoc. > > I dunno... kdoc is nice, but I'm not sure its verbosity is useful here, > even if this function is called from more than one .c file. > > I'd be happy with a single-line comment, personally. WFM. /Jarkko
On Thu, Jan 21, 2021 at 12:36:25PM +1300, Kai Huang wrote: > On Wed, 20 Jan 2021 10:36:09 -0800 Dave Hansen wrote: > > On 1/20/21 4:03 AM, Jarkko Sakkinen wrote: > > >> +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) > > >> +{ > > >> + int i; > > >> + > > >> + for (i = 0; i < 4; i++) > > >> + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); > > >> +} > > > Missing kdoc. > > > > I dunno... kdoc is nice, but I'm not sure its verbosity is useful here, > > even if this function is called from more than one .c file. > > > > I'd be happy with a single-line comment, personally. > > > > 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. */ > > ? WFM, thanks. /Jarkko
On 1/20/21 5:06 PM, Kai Huang wrote: > > /* > * 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. > */ I'm grumpy, but I hate it. I'll stop the bike shedding for now, though.
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.
On Wed, 20 Jan 2021 17:15:35 -0800 Dave Hansen wrote: > On 1/20/21 5:06 PM, Kai Huang wrote: > > > > /* > > * 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. > > */ > > I'm grumpy, but I hate it. > > I'll stop the bike shedding for now, though. Jarkko and Dave, I'll change to use below: /* * Update the SGX_LEPUBKEYHASH MSRs to the values specified by caller. * Bare-metal driver requires to update them to hash of enclave's signer * before EINIT. KVM needs to update them to guest's virtual MSR values * before doing EINIT from guest. */ Please let me know if are not OK with this.
On Thu, Jan 21, 2021 at 02:44:26PM +1300, Kai Huang wrote: > On Wed, 20 Jan 2021 17:15:35 -0800 Dave Hansen wrote: > > On 1/20/21 5:06 PM, Kai Huang wrote: > > > > > > /* > > > * 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. > > > */ > > > > I'm grumpy, but I hate it. > > > > I'll stop the bike shedding for now, though. > > Jarkko and Dave, > > I'll change to use below: > > /* > * Update the SGX_LEPUBKEYHASH MSRs to the values specified by caller. > * Bare-metal driver requires to update them to hash of enclave's signer > * before EINIT. KVM needs to update them to guest's virtual MSR values > * before doing EINIT from guest. > */ > > Please let me know if are not OK with this. I am. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index e5977752c7be..1bae754268d1 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -495,7 +495,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, void *token) { u64 mrsigner[4]; - int i, j, k; + int i, j; void *addr; int ret; @@ -544,8 +544,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, preempt_disable(); - for (k = 0; k < 4; k++) - wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + k, mrsigner[k]); + sgx_update_lepubkeyhash(mrsigner); ret = __einit(sigstruct, token, addr); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index bdda631c975b..1cf1f0f058b8 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -697,6 +697,14 @@ static bool __init sgx_page_cache_init(void) return true; } +void sgx_update_lepubkeyhash(u64 *lepubkeyhash) +{ + int i; + + for (i = 0; i < 4; i++) + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]); +} + static void __init sgx_init(void) { int ret; diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index 509f2af33e1d..ccd4f145c464 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -83,4 +83,6 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page); int sgx_unmark_page_reclaimable(struct sgx_epc_page *page); struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim); +void sgx_update_lepubkeyhash(u64 *lepubkeyhash); + #endif /* _X86_SGX_H */