| Message ID | 20190424160942.13567-2-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD SEV guest live migration support | expand |
On Wed, Apr 24, 2019 at 04:09:59PM +0000, Singh, Brijesh wrote: > The command is used to create an outgoing SEV guest encryption context. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Borislav Petkov <bp@suse.de> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: x86@kernel.org > Cc: kvm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > .../virtual/kvm/amd-memory-encryption.rst | 24 +++++ > arch/x86/kvm/svm.c | 101 ++++++++++++++++++ > include/uapi/linux/kvm.h | 12 +++ > 3 files changed, 137 insertions(+) > > diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst > index 659bbc093b52..340ac4f87321 100644 > --- a/Documentation/virtual/kvm/amd-memory-encryption.rst > +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst > @@ -238,6 +238,30 @@ Returns: 0 on success, -negative on error > __u32 trans_len; > }; > > +10. KVM_SEV_SEND_START > +---------------------- > + > +The KVM_SEV_SEND_START command can be used by the hypervisor to create an > +outgoing guest encryption context. > + > +Parameters (in): struct kvm_sev_send_start > + > +Returns: 0 on success, -negative on error > + > +:: > + struct kvm_sev_send_start { > + __u32 policy; /* guest policy */ > + > + __u64 pdh_cert_uaddr; /* platform Diffie-Hellman certificate */ > + __u32 pdh_cert_len; > + > + __u64 plat_cert_uaddr; /* platform certificate chain */ > + __u32 plat_cert_len; > + > + __u64 amd_cert_uaddr; /* AMD certificate */ > + __u32 amd_cert_len; __u64 session_uaddr; __u32 session_len; too, right? > + }; > + > References > ========== > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 406b558abfef..4c2a225ba546 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -6955,6 +6955,104 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + void *amd_cert = NULL, *session_data = NULL; > + void *pdh_cert = NULL, *plat_cert = NULL; > + struct sev_data_send_start *data = NULL; > + struct kvm_sev_send_start params; > + int ret; > + > + if (!sev_guest(kvm)) > + return -ENOTTY; > + > + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, > + sizeof(struct kvm_sev_send_start))) > + return -EFAULT; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + /* userspace wants to query the session length */ > + if (!params.session_len) > + goto cmd; > + > + if (!params.pdh_cert_uaddr || !params.pdh_cert_len || > + !params.session_uaddr) > + return -EINVAL; > + > + /* copy the certificate blobs from userspace */ > + pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len); > + if (IS_ERR(pdh_cert)) { > + ret = PTR_ERR(pdh_cert); > + goto e_free; > + } > + > + data->pdh_cert_address = __psp_pa(pdh_cert); > + data->pdh_cert_len = params.pdh_cert_len; > + > + plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len); > + if (IS_ERR(plat_cert)) { > + ret = PTR_ERR(plat_cert); > + goto e_free_pdh; > + } > + > + data->plat_cert_address = __psp_pa(plat_cert); > + data->plat_cert_len = params.plat_cert_len; > + > + amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len); > + if (IS_ERR(amd_cert)) { > + ret = PTR_ERR(amd_cert); > + goto e_free_plat_cert; > + } > + > + data->amd_cert_address = __psp_pa(amd_cert); > + data->amd_cert_len = params.amd_cert_len; > + > + ret = -ENOMEM; > + session_data = kmalloc(params.session_len, GFP_KERNEL); If the user is supposed to query the session length first, you could save it in a global variable perhaps and use that value instead of trusting the user to give you the correct one in params.session_len for the allocation... > + if (!session_data) > + goto e_free_amd_cert; > + > + data->session_address = __psp_pa(session_data); > + data->session_len = params.session_len; > +cmd: > + data->handle = sev->handle; > + ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error); > + > + /* if we queried the session length, FW responded with expected data */ <--- ... here you have the session length from the fw.
On 4/26/19 9:10 AM, Borislav Petkov wrote: > On Wed, Apr 24, 2019 at 04:09:59PM +0000, Singh, Brijesh wrote: >> The command is used to create an outgoing SEV guest encryption context. >> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: "H. Peter Anvin" <hpa@zytor.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >> Cc: Joerg Roedel <joro@8bytes.org> >> Cc: Borislav Petkov <bp@suse.de> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Cc: x86@kernel.org >> Cc: kvm@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> .../virtual/kvm/amd-memory-encryption.rst | 24 +++++ >> arch/x86/kvm/svm.c | 101 ++++++++++++++++++ >> include/uapi/linux/kvm.h | 12 +++ >> 3 files changed, 137 insertions(+) >> >> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst >> index 659bbc093b52..340ac4f87321 100644 >> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst >> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst >> @@ -238,6 +238,30 @@ Returns: 0 on success, -negative on error >> __u32 trans_len; >> }; >> >> +10. KVM_SEV_SEND_START >> +---------------------- >> + >> +The KVM_SEV_SEND_START command can be used by the hypervisor to create an >> +outgoing guest encryption context. >> + >> +Parameters (in): struct kvm_sev_send_start >> + >> +Returns: 0 on success, -negative on error >> + >> +:: >> + struct kvm_sev_send_start { >> + __u32 policy; /* guest policy */ >> + >> + __u64 pdh_cert_uaddr; /* platform Diffie-Hellman certificate */ >> + __u32 pdh_cert_len; >> + >> + __u64 plat_cert_uaddr; /* platform certificate chain */ >> + __u32 plat_cert_len; >> + >> + __u64 amd_cert_uaddr; /* AMD certificate */ >> + __u32 amd_cert_len; > > __u64 session_uaddr; > __u32 session_len; > > too, right? Ah good catch, I will fix in next rev. thanks > >> + }; >> + >> References >> ========== >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 406b558abfef..4c2a225ba546 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -6955,6 +6955,104 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) >> return ret; >> } >> >> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp) >> +{ >> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; >> + void *amd_cert = NULL, *session_data = NULL; >> + void *pdh_cert = NULL, *plat_cert = NULL; >> + struct sev_data_send_start *data = NULL; >> + struct kvm_sev_send_start params; >> + int ret; >> + >> + if (!sev_guest(kvm)) >> + return -ENOTTY; >> + >> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, >> + sizeof(struct kvm_sev_send_start))) >> + return -EFAULT; >> + >> + data = kzalloc(sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + /* userspace wants to query the session length */ >> + if (!params.session_len) >> + goto cmd; >> + >> + if (!params.pdh_cert_uaddr || !params.pdh_cert_len || >> + !params.session_uaddr) >> + return -EINVAL; >> + >> + /* copy the certificate blobs from userspace */ >> + pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len); >> + if (IS_ERR(pdh_cert)) { >> + ret = PTR_ERR(pdh_cert); >> + goto e_free; >> + } >> + >> + data->pdh_cert_address = __psp_pa(pdh_cert); >> + data->pdh_cert_len = params.pdh_cert_len; >> + >> + plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len); >> + if (IS_ERR(plat_cert)) { >> + ret = PTR_ERR(plat_cert); >> + goto e_free_pdh; >> + } >> + >> + data->plat_cert_address = __psp_pa(plat_cert); >> + data->plat_cert_len = params.plat_cert_len; >> + >> + amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len); >> + if (IS_ERR(amd_cert)) { >> + ret = PTR_ERR(amd_cert); >> + goto e_free_plat_cert; >> + } >> + >> + data->amd_cert_address = __psp_pa(amd_cert); >> + data->amd_cert_len = params.amd_cert_len; >> + >> + ret = -ENOMEM; >> + session_data = kmalloc(params.session_len, GFP_KERNEL); > > If the user is supposed to query the session length first, you could > save it in a global variable perhaps and use that value instead of > trusting the user to give you the correct one in params.session_len for > the allocation... > Yes that's doable but I am afraid that caching the value may lead us to wrong path and also divergence from the SEV API spec. The spec says the returned length is a minimum length but its possible that caller can give a bigger buffer and FW will still work with it. >> + if (!session_data) >> + goto e_free_amd_cert; >> + >> + data->session_address = __psp_pa(session_data); >> + data->session_len = params.session_len; >> +cmd: >> + data->handle = sev->handle; >> + ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error); >> + >> + /* if we queried the session length, FW responded with expected data */ > > <--- ... here you have the session length from the fw. >
On Fri, Apr 26, 2019 at 02:29:31PM +0000, Singh, Brijesh wrote: > Yes that's doable but I am afraid that caching the value may lead us to > wrong path and also divergence from the SEV API spec. The spec says the > returned length is a minimum length but its possible that caller can > give a bigger buffer and FW will still work with it. Does the caller even have a valid reason to give a bigger buffer len? I mean I'm still thinking defensively here but maybe the only thing that would happen here with a bigger buffer is if the kmalloc() would fail, leading to eventual failure of the migration. If the code limits the allocation to some sane max length, the migration won't fail even if userspace gives it too big values...
On 4/26/19 3:43 PM, Borislav Petkov wrote: > On Fri, Apr 26, 2019 at 02:29:31PM +0000, Singh, Brijesh wrote: >> Yes that's doable but I am afraid that caching the value may lead us to >> wrong path and also divergence from the SEV API spec. The spec says the >> returned length is a minimum length but its possible that caller can >> give a bigger buffer and FW will still work with it. > > Does the caller even have a valid reason to give a bigger buffer len? > Practically I don't see any reason why caller would do that but theoretically it can. If we cache the len then we also need to consider adding another flag to hint whether userspace ever requested length. e.g an application can compute the length of session blob by looking at the API version and spec and may never query the length. > I mean I'm still thinking defensively here but maybe the only thing that > would happen here with a bigger buffer is if the kmalloc() would fail, > leading to eventual failure of the migration. > > If the code limits the allocation to some sane max length, the migration > won't fail even if userspace gives it too big values... >
On Mon, Apr 29, 2019 at 03:01:24PM +0000, Singh, Brijesh wrote: > Practically I don't see any reason why caller would do that but > theoretically it can. If we cache the len then we also need to consider > adding another flag to hint whether userspace ever requested length. > e.g an application can compute the length of session blob by looking at > the API version and spec and may never query the length. > > > I mean I'm still thinking defensively here but maybe the only thing that > > would happen here with a bigger buffer is if the kmalloc() would fail, > > leading to eventual failure of the migration. > > > > If the code limits the allocation to some sane max length, the migration > > won't fail even if userspace gives it too big values... So what about this? Limiting to a sane length...
On 4/29/19 11:36 AM, Borislav Petkov wrote:
> So what about this? Limiting to a sane length...
Sure, we have defined a SEV_FW_BLOB_MAX_SIZE and can use it to limit
the blob copy size. I will do this in next rev. thanks
-Brijesh
diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst index 659bbc093b52..340ac4f87321 100644 --- a/Documentation/virtual/kvm/amd-memory-encryption.rst +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst @@ -238,6 +238,30 @@ Returns: 0 on success, -negative on error __u32 trans_len; }; +10. KVM_SEV_SEND_START +---------------------- + +The KVM_SEV_SEND_START command can be used by the hypervisor to create an +outgoing guest encryption context. + +Parameters (in): struct kvm_sev_send_start + +Returns: 0 on success, -negative on error + +:: + struct kvm_sev_send_start { + __u32 policy; /* guest policy */ + + __u64 pdh_cert_uaddr; /* platform Diffie-Hellman certificate */ + __u32 pdh_cert_len; + + __u64 plat_cert_uaddr; /* platform certificate chain */ + __u32 plat_cert_len; + + __u64 amd_cert_uaddr; /* AMD certificate */ + __u32 amd_cert_len; + }; + References ========== diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 406b558abfef..4c2a225ba546 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -6955,6 +6955,104 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; } +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; + void *amd_cert = NULL, *session_data = NULL; + void *pdh_cert = NULL, *plat_cert = NULL; + struct sev_data_send_start *data = NULL; + struct kvm_sev_send_start params; + int ret; + + if (!sev_guest(kvm)) + return -ENOTTY; + + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, + sizeof(struct kvm_sev_send_start))) + return -EFAULT; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + /* userspace wants to query the session length */ + if (!params.session_len) + goto cmd; + + if (!params.pdh_cert_uaddr || !params.pdh_cert_len || + !params.session_uaddr) + return -EINVAL; + + /* copy the certificate blobs from userspace */ + pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, params.pdh_cert_len); + if (IS_ERR(pdh_cert)) { + ret = PTR_ERR(pdh_cert); + goto e_free; + } + + data->pdh_cert_address = __psp_pa(pdh_cert); + data->pdh_cert_len = params.pdh_cert_len; + + plat_cert = psp_copy_user_blob(params.plat_cert_uaddr, params.plat_cert_len); + if (IS_ERR(plat_cert)) { + ret = PTR_ERR(plat_cert); + goto e_free_pdh; + } + + data->plat_cert_address = __psp_pa(plat_cert); + data->plat_cert_len = params.plat_cert_len; + + amd_cert = psp_copy_user_blob(params.amd_cert_uaddr, params.amd_cert_len); + if (IS_ERR(amd_cert)) { + ret = PTR_ERR(amd_cert); + goto e_free_plat_cert; + } + + data->amd_cert_address = __psp_pa(amd_cert); + data->amd_cert_len = params.amd_cert_len; + + ret = -ENOMEM; + session_data = kmalloc(params.session_len, GFP_KERNEL); + if (!session_data) + goto e_free_amd_cert; + + data->session_address = __psp_pa(session_data); + data->session_len = params.session_len; +cmd: + data->handle = sev->handle; + ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error); + + /* if we queried the session length, FW responded with expected data */ + if (!params.session_len) + goto done; + + if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr, + session_data, params.session_len)) { + ret = -EFAULT; + goto e_free_session; + } + + params.policy = data->policy; + +done: + params.session_len = data->session_len; + if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms, + sizeof(struct kvm_sev_send_start))) + ret = -EFAULT; + +e_free_session: + kfree(session_data); +e_free_amd_cert: + kfree(amd_cert); +e_free_plat_cert: + kfree(plat_cert); +e_free_pdh: + kfree(pdh_cert); +e_free: + kfree(data); + return ret; +} + static int svm_mem_enc_op(struct kvm *kvm, void __user *argp) { struct kvm_sev_cmd sev_cmd; @@ -6996,6 +7094,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp) case KVM_SEV_LAUNCH_SECRET: r = sev_launch_secret(kvm, &sev_cmd); break; + case KVM_SEV_SEND_START: + r = sev_send_start(kvm, &sev_cmd); + break; default: r = -EINVAL; goto out; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 6d4ea4b6c922..f425418bec13 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1520,6 +1520,18 @@ struct kvm_sev_dbg { __u32 len; }; +struct kvm_sev_send_start { + __u32 policy; + __u64 pdh_cert_uaddr; + __u32 pdh_cert_len; + __u64 plat_cert_uaddr; + __u32 plat_cert_len; + __u64 amd_cert_uaddr; + __u32 amd_cert_len; + __u64 session_uaddr; + __u32 session_len; +}; + #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
The command is used to create an outgoing SEV guest encryption context. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: Joerg Roedel <joro@8bytes.org> Cc: Borislav Petkov <bp@suse.de> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: x86@kernel.org Cc: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- .../virtual/kvm/amd-memory-encryption.rst | 24 +++++ arch/x86/kvm/svm.c | 101 ++++++++++++++++++ include/uapi/linux/kvm.h | 12 +++ 3 files changed, 137 insertions(+)