Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751576AbdIMRZm (ORCPT ); Wed, 13 Sep 2017 13:25:42 -0400 Received: from mail.skyhub.de ([5.9.137.197]:43562 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751112AbdIMRZj (ORCPT ); Wed, 13 Sep 2017 13:25:39 -0400 Date: Wed, 13 Sep 2017 19:25:10 +0200 From: Borislav Petkov To: Brijesh Singh Cc: linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org, Thomas Gleixner , Borislav Petkov , Joerg Roedel , "Michael S . Tsirkin" , Paolo Bonzini , =?utf-8?B?XCJSYWRpbSBLcsSNbcOhxZlcIg==?= , Tom Lendacky Subject: Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV LAUNCH_START command Message-ID: <20170913172510.azjkur3wsvat32aq@pd.tnic> References: <20170724200303.12197-1-brijesh.singh@amd.com> <20170724200303.12197-16-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170724200303.12197-16-brijesh.singh@amd.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5250 Lines: 199 On Mon, Jul 24, 2017 at 03:02:52PM -0500, Brijesh Singh wrote: > The command is used to bootstrap SEV guest from unencrypted boot images. > The command creates a new VM encryption key (VEK) using the guest owner's s/The command/It/ > policy, public DH certificates, and session information. > > Signed-off-by: Brijesh Singh > --- > arch/x86/kvm/svm.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 165 insertions(+) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 72f7c27..3e325578 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -329,6 +329,8 @@ static unsigned int max_sev_asid; > static unsigned long *sev_asid_bitmap; > static int sev_asid_new(void); > static void sev_asid_free(int asid); > +static void sev_deactivate_handle(struct kvm *kvm, int *error); > +static void sev_decommission_handle(struct kvm *kvm, int *error); Please move code in a way that you don't need those forward declarations. Also, I'm wondering if having all the SEV-related code could live in sev.c or so - svm.c is humongous. > static bool svm_sev_enabled(void) > { > @@ -1565,6 +1567,12 @@ static void sev_vm_destroy(struct kvm *kvm) > if (!sev_guest(kvm)) > return; > > + /* release the firmware resources for this guest */ > + if (sev_get_handle(kvm)) { > + sev_deactivate_handle(kvm, &error); > + sev_decommission_handle(kvm, &error); > + } > + > sev_asid_free(sev_get_asid(kvm)); > sev_firmware_uninit(); > } > @@ -5635,6 +5643,159 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error) > +{ > + int fd = sev_get_fd(kvm); > + struct fd f; > + int ret; > + > + f = fdget(fd); > + if (!f.file) > + return -EBADF; > + > + ret = sev_issue_cmd_external_user(f.file, id, data, error); > + fdput(f); > + > + return ret; > +} > + > +static void sev_decommission_handle(struct kvm *kvm, int *error) > +{ > + struct sev_data_decommission *data; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); Also, better on stack. Please do that for the other functions below too. > + if (!data) > + return; > + > + data->handle = sev_get_handle(kvm); > + sev_guest_decommission(data, error); > + kfree(data); > +} > + > +static void sev_deactivate_handle(struct kvm *kvm, int *error) > +{ > + struct sev_data_deactivate *data; > + int ret; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + data->handle = sev_get_handle(kvm); > + ret = sev_guest_deactivate(data, error); > + if (ret) > + goto e_free; > + > + wbinvd_on_all_cpus(); > + > + sev_guest_df_flush(error); > +e_free: > + kfree(data); > +} > + > +static int sev_activate_asid(struct kvm *kvm, unsigned int handle, int *error) > +{ > + struct sev_data_activate *data; > + int asid = sev_get_asid(kvm); > + int ret; > + > + wbinvd_on_all_cpus(); > + > + ret = sev_guest_df_flush(error); > + if (ret) > + return ret; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->handle = handle; > + data->asid = asid; > + ret = sev_guest_activate(data, error); > + if (ret) > + goto e_err; > + > + sev_set_handle(kvm, handle); > +e_err: > + kfree(data); > + return ret; > +} > + > +static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct sev_data_launch_start *start = NULL; > + struct kvm_sev_launch_start params; > + void *dh_cert_addr = NULL; > + void *session_addr = NULL; > + int *error = &argp->error; > + int ret; > + > + if (!sev_guest(kvm)) > + return -ENOTTY; > + > + ret = -EFAULT; > + if (copy_from_user(¶ms, (void *)argp->data, > + sizeof(struct kvm_sev_launch_start))) Sanity-check params. This is especially important if later we start using reserved fields. > + goto e_free; > + > + ret = -ENOMEM; > + start = kzalloc(sizeof(*start), GFP_KERNEL); > + if (!start) > + goto e_free; > + > + /* Bit 15:6 reserved, must be 0 */ > + start->policy = params.policy & ~0xffc0; > + > + if (params.dh_cert_length && params.dh_cert_address) { Yeah, we talked about this already: sanity-checking needed. But you get the idea. > + ret = -ENOMEM; > + dh_cert_addr = kmalloc(params.dh_cert_length, GFP_KERNEL); > + if (!dh_cert_addr) > + goto e_free; > + > + ret = -EFAULT; > + if (copy_from_user(dh_cert_addr, (void *)params.dh_cert_address, > + params.dh_cert_length)) > + goto e_free; > + > + start->dh_cert_address = __sme_set(__pa(dh_cert_addr)); > + start->dh_cert_length = params.dh_cert_length; > + } > + > + if (params.session_length && params.session_address) { > + ret = -ENOMEM; > + session_addr = kmalloc(params.session_length, GFP_KERNEL); > + if (!session_addr) > + goto e_free; > + > + ret = -EFAULT; > + if (copy_from_user(session_addr, (void *)params.session_address, > + params.session_length)) if (copy_from_user(session_addr, (void *)params.session_address, params.session_length)) reads better to me. Better yet if you shorten those member names into s_addr and s_len and so on... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.