Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731740 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV  LAUNCH_START command Borislav Petkov <bp@alien8.de> - 2017-09-13 19:30 +0200
    Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV  LAUNCH_START command Borislav Petkov <bp@suse.de> - 2017-09-13 20:40 +0200
      Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV  LAUNCH_START command Borislav Petkov <bp@suse.de> - 2017-09-13 23:10 +0200

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

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

[toc] | [next] | [standalone]

#1731776

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 :)

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1731875

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.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web