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@xxxxxxx>
> ---
> 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.
