| Message ID | 20170724200303.12197-16-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <brijesh.singh@amd.com> > --- > 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...
On 09/13/2017 12:25 PM, Borislav Petkov wrote: ... >> +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. > Yes, svm.c is humongous. ... >> + >> +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. Yes, some structures are small and I don't expect them to grow in newer API spec. We should be able to move them on the stack. I will audit the code and make the necessary changes. .... >> + 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. > Yes, I will add some upper bound check on the length field and add the sanity-check just after copying the parameters from userspace ... >> + 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. > Will do ... > > 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... > > Will use your recommendation. thanks
On Wed, Sep 13, 2017 at 01:23:08PM -0500, Brijesh Singh wrote: > Yes, I will add some upper bound check on the length field and add the > sanity-check just after copying the parameters from userspace Also, you could either fail the command if some of the reserved fields are set - picky - or zero them out - less picky :)
On 09/13/2017 01:37 PM, Borislav Petkov wrote: > On Wed, Sep 13, 2017 at 01:23:08PM -0500, Brijesh Singh wrote: >> Yes, I will add some upper bound check on the length field and add the >> sanity-check just after copying the parameters from userspace > > Also, you could either fail the command if some of the reserved fields > are set - picky - or zero them out - less picky :) > Actually reversed fields are not exposed in userspace structure. e.g a LAUNCH_UPDATE_DATE userspace structure looks like this: struct kvm_sev_launch_update_data { __u64 address; /* userspace address of memory region to encrypt */ __u32 length; /* length of memory region to encrypt */ }; But SEV firmware command structure is a slightly different (mainly it contains the reserved field and firmware handle etc). /** * struct sev_data_launch_update_data - LAUNCH_UPDATE_DATA command parameter * * @handle: firmware handle to use * @length: length of memory to be encrypted * @address: physical address of memory region to encrypt */ struct sev_data_launch_update_data { u32 handle; /* In */ u32 reserved; u64 address; /* In */ u32 length; /* In */ }; Please note that some commands require us passing the VM ASID etc -- userspace does not have VM ASID information. The current approach is -- while handling the command we copy the value from userspace structure into FW compatible structure and also populate missing fields which are not known to userspace (e.g firmware handle, VM ASID, use system physical addresses etc).
On Wed, Sep 13, 2017 at 01:58:31PM -0500, Brijesh Singh wrote: > Actually reversed fields are not exposed in userspace structure. Ok. > The current approach is -- while handling the command we copy the value > from userspace structure into FW compatible structure and also populate > missing fields which are not known to userspace (e.g firmware handle, > VM ASID, use system physical addresses etc). Yap, that makes sense. All I'm saying is, check anything that userspace can influence and make sure it is sensible. Thx.
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); 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); + 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))) + 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) { + 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)) + goto e_free; + + start->session_address = __sme_set(__pa(session_addr)); + start->session_length = params.session_length; + } + + start->handle = params.handle; + ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_START, start, error); + if (ret) + goto e_free; + + ret = sev_activate_asid(kvm, start->handle, error); + if (ret) + goto e_free; + + params.handle = start->handle; + if (copy_to_user((void *) argp->data, ¶ms, + sizeof(struct kvm_sev_launch_start))) + ret = -EFAULT; +e_free: + kfree(dh_cert_addr); + kfree(session_addr); + kfree(start); + return ret; +} + static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp) { struct kvm_sev_cmd sev_cmd; @@ -5650,6 +5811,10 @@ static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp) r = sev_guest_init(kvm, &sev_cmd); break; } + case KVM_SEV_LAUNCH_START: { + r = sev_launch_start(kvm, &sev_cmd); + break; + } default: break; }
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 policy, public DH certificates, and session information. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kvm/svm.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+)