Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV LAUNCH_START command

[Borislav Petkov]

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(&params, (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.