diff mbox series

[v2,8/8] KVM: SVM: Allocate SEV command structures on local stack

Message ID 20210406224952.4177376-9-seanjc@google.com
State New
Headers show
Series ccp: KVM: SVM: Use stack for SEV command buffers | expand

Commit Message

Sean Christopherson April 6, 2021, 10:49 p.m. UTC
Use the local stack to "allocate" the structures used to communicate with
the PSP.  The largest struct used by KVM, sev_data_launch_secret, clocks
in at 52 bytes, well within the realm of reasonable stack usage.  The
smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
is larger than the allocation itself.

Now that the PSP driver plays nice with vmalloc pointers, putting the
data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
explosions.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 166 deletions(-)

Comments

Christophe Leroy April 7, 2021, 5:24 a.m. UTC | #1
Le 07/04/2021 à 00:49, Sean Christopherson a écrit :
> Use the local stack to "allocate" the structures used to communicate with

> the PSP.  The largest struct used by KVM, sev_data_launch_secret, clocks

> in at 52 bytes, well within the realm of reasonable stack usage.  The

> smallest structs are a mere 4 bytes, i.e. the pointer for the allocation

> is larger than the allocation itself.

> 

> Now that the PSP driver plays nice with vmalloc pointers, putting the

> data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause

> explosions.

> 

> Cc: Brijesh Singh <brijesh.singh@amd.com>

> Cc: Tom Lendacky <thomas.lendacky@amd.com>

> Signed-off-by: Sean Christopherson <seanjc@google.com>

> ---

>   arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------

>   1 file changed, 96 insertions(+), 166 deletions(-)

> 

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c

> index 5457138c7347..316fd39c7aef 100644

> --- a/arch/x86/kvm/svm/sev.c

> +++ b/arch/x86/kvm/svm/sev.c

> @@ -150,35 +150,22 @@ static void sev_asid_free(int asid)

>   

>   static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)

>   {

> -	struct sev_data_decommission *decommission;

> -	struct sev_data_deactivate *data;

> +	struct sev_data_decommission decommission;

> +	struct sev_data_deactivate deactivate;

>   

>   	if (!handle)

>   		return;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL);

> -	if (!data)

> -		return;

> -

> -	/* deactivate handle */

> -	data->handle = handle;

> +	deactivate.handle = handle;

>   

>   	/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */

>   	down_read(&sev_deactivate_lock);

> -	sev_guest_deactivate(data, NULL);

> +	sev_guest_deactivate(&deactivate, NULL);

>   	up_read(&sev_deactivate_lock);

>   

> -	kfree(data);

> -

> -	decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);

> -	if (!decommission)

> -		return;

> -

>   	/* decommission handle */

> -	decommission->handle = handle;

> -	sev_guest_decommission(decommission, NULL);

> -

> -	kfree(decommission);

> +	decommission.handle = handle;

> +	sev_guest_decommission(&decommission, NULL);

>   }

>   

>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)

> @@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   

>   static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)

>   {

> -	struct sev_data_activate *data;

> +	struct sev_data_activate activate;

>   	int asid = sev_get_asid(kvm);

>   	int ret;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> -

>   	/* activate ASID on the given handle */

> -	data->handle = handle;

> -	data->asid   = asid;

> -	ret = sev_guest_activate(data, error);

> -	kfree(data);

> +	activate.handle = handle;

> +	activate.asid   = asid;

> +	ret = sev_guest_activate(&activate, error);

>   

>   	return ret;

>   }

> @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)

>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_launch_start *start;

> +	struct sev_data_launch_start start;


struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};

>   	struct kvm_sev_launch_start params;

>   	void *dh_blob, *session_blob;

>   	int *error = &argp->error;

> @@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))

>   		return -EFAULT;

>   

> -	start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);

> -	if (!start)

> -		return -ENOMEM;

> +	memset(&start, 0, sizeof(start));


Not needed.

>   

>   	dh_blob = NULL;

>   	if (params.dh_uaddr) {

>   		dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);

> -		if (IS_ERR(dh_blob)) {

> -			ret = PTR_ERR(dh_blob);

> -			goto e_free;

> -		}

> +		if (IS_ERR(dh_blob))

> +			return PTR_ERR(dh_blob);

>   

> -		start->dh_cert_address = __sme_set(__pa(dh_blob));

> -		start->dh_cert_len = params.dh_len;

> +		start.dh_cert_address = __sme_set(__pa(dh_blob));

> +		start.dh_cert_len = params.dh_len;

>   	}

>   

>   	session_blob = NULL;

> @@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   			goto e_free_dh;

>   		}

>   

> -		start->session_address = __sme_set(__pa(session_blob));

> -		start->session_len = params.session_len;

> +		start.session_address = __sme_set(__pa(session_blob));

> +		start.session_len = params.session_len;

>   	}

>   

> -	start->handle = params.handle;

> -	start->policy = params.policy;

> +	start.handle = params.handle;

> +	start.policy = params.policy;

>   

>   	/* create memory encryption context */

> -	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);

> +	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, &start, error);

>   	if (ret)

>   		goto e_free_session;

>   

>   	/* Bind ASID to this guest */

> -	ret = sev_bind_asid(kvm, start->handle, error);

> +	ret = sev_bind_asid(kvm, start.handle, error);

>   	if (ret)

>   		goto e_free_session;

>   

>   	/* return handle to userspace */

> -	params.handle = start->handle;

> +	params.handle = start.handle;

>   	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params))) {

> -		sev_unbind_asid(kvm, start->handle);

> +		sev_unbind_asid(kvm, start.handle);

>   		ret = -EFAULT;

>   		goto e_free_session;

>   	}

>   

> -	sev->handle = start->handle;

> +	sev->handle = start.handle;

>   	sev->fd = argp->sev_fd;

>   

>   e_free_session:

>   	kfree(session_blob);

>   e_free_dh:

>   	kfree(dh_blob);

> -e_free:

> -	kfree(start);

>   	return ret;

>   }

>   

> @@ -446,7 +422,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

>   	struct kvm_sev_launch_update_data params;

> -	struct sev_data_launch_update_data *data;

> +	struct sev_data_launch_update_data data;

>   	struct page **inpages;

>   	int ret;

>   

> @@ -456,20 +432,14 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))

>   		return -EFAULT;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> -

>   	vaddr = params.uaddr;

>   	size = params.len;

>   	vaddr_end = vaddr + size;

>   

>   	/* Lock the user memory. */

>   	inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);

> -	if (IS_ERR(inpages)) {

> -		ret = PTR_ERR(inpages);

> -		goto e_free;

> -	}

> +	if (IS_ERR(inpages))

> +		return PTR_ERR(inpages);

>   

>   	/*

>   	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in

> @@ -477,6 +447,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	 */

>   	sev_clflush_pages(inpages, npages);

>   

> +	data.reserved = 0;

> +	data.handle = sev->handle;

> +

>   	for (i = 0; vaddr < vaddr_end; vaddr = next_vaddr, i += pages) {

>   		int offset, len;

>   

> @@ -491,10 +464,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   

>   		len = min_t(size_t, ((pages * PAGE_SIZE) - offset), size);

>   

> -		data->handle = sev->handle;

> -		data->len = len;

> -		data->address = __sme_page_pa(inpages[i]) + offset;

> -		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, data, &argp->error);

> +		data.len = len;

> +		data.address = __sme_page_pa(inpages[i]) + offset;

> +		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, &data, &argp->error);

>   		if (ret)

>   			goto e_unpin;

>   

> @@ -510,8 +482,6 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	}

>   	/* unlock the user pages */

>   	sev_unpin_memory(kvm, inpages, npages);

> -e_free:

> -	kfree(data);

>   	return ret;

>   }

>   

> @@ -563,16 +533,14 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)

>   static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_launch_update_vmsa *vmsa;

> +	struct sev_data_launch_update_vmsa vmsa;

>   	struct kvm_vcpu *vcpu;

>   	int i, ret;

>   

>   	if (!sev_es_guest(kvm))

>   		return -ENOTTY;

>   

> -	vmsa = kzalloc(sizeof(*vmsa), GFP_KERNEL);

> -	if (!vmsa)

> -		return -ENOMEM;

> +	vmsa.reserved = 0;

>   

>   	kvm_for_each_vcpu(i, vcpu, kvm) {

>   		struct vcpu_svm *svm = to_svm(vcpu);

> @@ -580,7 +548,7 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   		/* Perform some pre-encryption checks against the VMSA */

>   		ret = sev_es_sync_vmsa(svm);

>   		if (ret)

> -			goto e_free;

> +			return ret;

>   

>   		/*

>   		 * The LAUNCH_UPDATE_VMSA command will perform in-place

> @@ -590,27 +558,25 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   		 */

>   		clflush_cache_range(svm->vmsa, PAGE_SIZE);

>   

> -		vmsa->handle = sev->handle;

> -		vmsa->address = __sme_pa(svm->vmsa);

> -		vmsa->len = PAGE_SIZE;

> -		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, vmsa,

> +		vmsa.handle = sev->handle;

> +		vmsa.address = __sme_pa(svm->vmsa);

> +		vmsa.len = PAGE_SIZE;

> +		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa,

>   				    &argp->error);

>   		if (ret)

> -			goto e_free;

> +			return ret;

>   

>   		svm->vcpu.arch.guest_state_protected = true;

>   	}

>   

> -e_free:

> -	kfree(vmsa);

> -	return ret;

> +	return 0;

>   }

>   

>   static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	void __user *measure = (void __user *)(uintptr_t)argp->data;

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_launch_measure *data;

> +	struct sev_data_launch_measure data;


struct sev_data_launch_measure data = {0, 0, 0, 0};

>   	struct kvm_sev_launch_measure params;

>   	void __user *p = NULL;

>   	void *blob = NULL;

> @@ -622,9 +588,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	if (copy_from_user(&params, measure, sizeof(params)))

>   		return -EFAULT;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> +	memset(&data, 0, sizeof(data));


Not needed

>   

>   	/* User wants to query the blob length */

>   	if (!params.len)

> @@ -632,23 +596,20 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   

>   	p = (void __user *)(uintptr_t)params.uaddr;

>   	if (p) {

> -		if (params.len > SEV_FW_BLOB_MAX_SIZE) {

> -			ret = -EINVAL;

> -			goto e_free;

> -		}

> +		if (params.len > SEV_FW_BLOB_MAX_SIZE)

> +			return -EINVAL;

>   

> -		ret = -ENOMEM;

>   		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);

>   		if (!blob)

> -			goto e_free;

> +			return -ENOMEM;

>   

> -		data->address = __psp_pa(blob);

> -		data->len = params.len;

> +		data.address = __psp_pa(blob);

> +		data.len = params.len;

>   	}

>   

>   cmd:

> -	data->handle = sev->handle;

> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, data, &argp->error);

> +	data.handle = sev->handle;

> +	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, &data, &argp->error);

>   

>   	/*

>   	 * If we query the session length, FW responded with expected data.

> @@ -665,63 +626,50 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	}

>   

>   done:

> -	params.len = data->len;

> +	params.len = data.len;

>   	if (copy_to_user(measure, &params, sizeof(params)))

>   		ret = -EFAULT;

>   e_free_blob:

>   	kfree(blob);

> -e_free:

> -	kfree(data);

>   	return ret;

>   }

>   

>   static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_launch_finish *data;

> -	int ret;

> +	struct sev_data_launch_finish data;

>   

>   	if (!sev_guest(kvm))

>   		return -ENOTTY;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> -

> -	data->handle = sev->handle;

> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, data, &argp->error);

> -

> -	kfree(data);

> -	return ret;

> +	data.handle = sev->handle;

> +	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, &data, &argp->error);

>   }

>   

>   static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

>   	struct kvm_sev_guest_status params;

> -	struct sev_data_guest_status *data;

> +	struct sev_data_guest_status data = {0, 0, 0, 0};

>   	int ret;

>   

>   	if (!sev_guest(kvm))

>   		return -ENOTTY;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> +	memset(&data, 0, sizeof(data));


not needed

>   

> -	data->handle = sev->handle;

> -	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error);

> +	data.handle = sev->handle;

> +	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, &data, &argp->error);

>   	if (ret)

> -		goto e_free;

> +		return ret;

>   

> -	params.policy = data->policy;

> -	params.state = data->state;

> -	params.handle = data->handle;

> +	params.policy = data.policy;

> +	params.state = data.state;

> +	params.handle = data.handle;

>   

>   	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))

>   		ret = -EFAULT;

> -e_free:

> -	kfree(data);

> +

>   	return ret;

>   }

>   

> @@ -730,23 +678,17 @@ static int __sev_issue_dbg_cmd(struct kvm *kvm, unsigned long src,

>   			       int *error, bool enc)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_dbg *data;

> -	int ret;

> +	struct sev_data_dbg data;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> +	data.reserved = 0;

> +	data.handle = sev->handle;

> +	data.dst_addr = dst;

> +	data.src_addr = src;

> +	data.len = size;

>   

> -	data->handle = sev->handle;

> -	data->dst_addr = dst;

> -	data->src_addr = src;

> -	data->len = size;

> -

> -	ret = sev_issue_cmd(kvm,

> -			    enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,

> -			    data, error);

> -	kfree(data);

> -	return ret;

> +	return sev_issue_cmd(kvm,

> +			     enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,

> +			     &data, error);

>   }

>   

>   static int __sev_dbg_decrypt(struct kvm *kvm, unsigned long src_paddr,

> @@ -966,7 +908,7 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)

>   static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_launch_secret *data;

> +	struct sev_data_launch_secret data;

>   	struct kvm_sev_launch_secret params;

>   	struct page **pages;

>   	void *blob, *hdr;

> @@ -998,41 +940,36 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   		goto e_unpin_memory;

>   	}

>   

> -	ret = -ENOMEM;

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		goto e_unpin_memory;

> +	memset(&data, 0, sizeof(data));

>   

>   	offset = params.guest_uaddr & (PAGE_SIZE - 1);

> -	data->guest_address = __sme_page_pa(pages[0]) + offset;

> -	data->guest_len = params.guest_len;

> +	data.guest_address = __sme_page_pa(pages[0]) + offset;

> +	data.guest_len = params.guest_len;

>   

>   	blob = psp_copy_user_blob(params.trans_uaddr, params.trans_len);

>   	if (IS_ERR(blob)) {

>   		ret = PTR_ERR(blob);

> -		goto e_free;

> +		goto e_unpin_memory;

>   	}

>   

> -	data->trans_address = __psp_pa(blob);

> -	data->trans_len = params.trans_len;

> +	data.trans_address = __psp_pa(blob);

> +	data.trans_len = params.trans_len;

>   

>   	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);

>   	if (IS_ERR(hdr)) {

>   		ret = PTR_ERR(hdr);

>   		goto e_free_blob;

>   	}

> -	data->hdr_address = __psp_pa(hdr);

> -	data->hdr_len = params.hdr_len;

> +	data.hdr_address = __psp_pa(hdr);

> +	data.hdr_len = params.hdr_len;

>   

> -	data->handle = sev->handle;

> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, data, &argp->error);

> +	data.handle = sev->handle;

> +	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, &data, &argp->error);

>   

>   	kfree(hdr);

>   

>   e_free_blob:

>   	kfree(blob);

> -e_free:

> -	kfree(data);

>   e_unpin_memory:

>   	/* content of memory is updated, mark pages dirty */

>   	for (i = 0; i < n; i++) {

> @@ -1047,7 +984,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   {

>   	void __user *report = (void __user *)(uintptr_t)argp->data;

>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> -	struct sev_data_attestation_report *data;

> +	struct sev_data_attestation_report data;

>   	struct kvm_sev_attestation_report params;

>   	void __user *p;

>   	void *blob = NULL;

> @@ -1059,9 +996,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))

>   		return -EFAULT;

>   

> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);

> -	if (!data)

> -		return -ENOMEM;

> +	memset(&data, 0, sizeof(data));

>   

>   	/* User wants to query the blob length */

>   	if (!params.len)

> @@ -1069,23 +1004,20 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   

>   	p = (void __user *)(uintptr_t)params.uaddr;

>   	if (p) {

> -		if (params.len > SEV_FW_BLOB_MAX_SIZE) {

> -			ret = -EINVAL;

> -			goto e_free;

> -		}

> +		if (params.len > SEV_FW_BLOB_MAX_SIZE)

> +			return -EINVAL;

>   

> -		ret = -ENOMEM;

>   		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);

>   		if (!blob)

> -			goto e_free;

> +			return -ENOMEM;

>   

> -		data->address = __psp_pa(blob);

> -		data->len = params.len;

> -		memcpy(data->mnonce, params.mnonce, sizeof(params.mnonce));

> +		data.address = __psp_pa(blob);

> +		data.len = params.len;

> +		memcpy(data.mnonce, params.mnonce, sizeof(params.mnonce));

>   	}

>   cmd:

> -	data->handle = sev->handle;

> -	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, data, &argp->error);

> +	data.handle = sev->handle;

> +	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, &data, &argp->error);

>   	/*

>   	 * If we query the session length, FW responded with expected data.

>   	 */

> @@ -1101,13 +1033,11 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)

>   	}

>   

>   done:

> -	params.len = data->len;

> +	params.len = data.len;

>   	if (copy_to_user(report, &params, sizeof(params)))

>   		ret = -EFAULT;

>   e_free_blob:

>   	kfree(blob);

> -e_free:

> -	kfree(data);

>   	return ret;

>   }

>   

>
Borislav Petkov April 7, 2021, 10:24 a.m. UTC | #2
First of all, I'd strongly suggest you trim your emails when you reply -
that would be much appreciated.

On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
> > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)

> >   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

> >   {

> >   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> > -	struct sev_data_launch_start *start;

> > +	struct sev_data_launch_start start;

> 

> struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};


I don't know how this is any better than using memset...

Also, you can do

	... start = { };

which is certainly the only other alternative to memset, AFAIK.

But whatever you do, you need to look at the resulting asm the compiler
generates. So let's do that:

Your version:

# arch/x86/kvm/svm/sev.c:261:   struct sev_data_launch_start _tmp = {0, 0, 0, 0, 0, 0, 0};
        movq    $0, 104(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 112(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 120(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 128(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movl    $0, 136(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]


my version:

# arch/x86/kvm/svm/sev.c:261:   struct sev_data_launch_start _tmp = {};
        movq    $0, 104(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 112(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 120(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 128(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movl    $0, 136(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]


the memset version:

# arch/x86/kvm/svm/sev.c:269: 	memset(&_tmp, 0, sizeof(_tmp));
#NO_APP
	movq	$0, 104(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 112(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 120(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 128(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movl	$0, 136(%rsp)	#, MEM <char[1:36]> [(void *)_561]

Ok?

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Sean Christopherson April 7, 2021, 5:05 p.m. UTC | #3
On Wed, Apr 07, 2021, Borislav Petkov wrote:
> First of all, I'd strongly suggest you trim your emails when you reply -

> that would be much appreciated.

> 

> On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:

> > > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)

> > >   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

> > >   {

> > >   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

> > > -	struct sev_data_launch_start *start;

> > > +	struct sev_data_launch_start start;

> > 

> > struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};

> 

> I don't know how this is any better than using memset...

> 

> Also, you can do

> 

> 	... start = { };

> 

> which is certainly the only other alternative to memset, AFAIK.

> 

> But whatever you do, you need to look at the resulting asm the compiler

> generates. So let's do that:


I'm ok with Boris' version, I'm not a fan of having to count zeros.  I used
memset() to defer initialization until after the various sanity checks, and
out of habit.
Christophe Leroy April 7, 2021, 5:06 p.m. UTC | #4
Le 07/04/2021 à 19:05, Sean Christopherson a écrit :
> On Wed, Apr 07, 2021, Borislav Petkov wrote:

>> First of all, I'd strongly suggest you trim your emails when you reply -

>> that would be much appreciated.

>>

>> On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:

>>>> @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)

>>>>    static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

>>>>    {

>>>>    	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

>>>> -	struct sev_data_launch_start *start;

>>>> +	struct sev_data_launch_start start;

>>>

>>> struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};

>>

>> I don't know how this is any better than using memset...

>>

>> Also, you can do

>>

>> 	... start = { };

>>

>> which is certainly the only other alternative to memset, AFAIK.

>>

>> But whatever you do, you need to look at the resulting asm the compiler

>> generates. So let's do that:

> 

> I'm ok with Boris' version, I'm not a fan of having to count zeros.  I used

> memset() to defer initialization until after the various sanity checks, and

> out of habit.

> 


Yes I also like Boris' version 	... start = { };  better than mine.
Borislav Petkov April 7, 2021, 5:34 p.m. UTC | #5
On Wed, Apr 07, 2021 at 05:05:07PM +0000, Sean Christopherson wrote:
> I used memset() to defer initialization until after the various sanity

> checks,


I'd actually vote for that too - I don't like doing stuff which is not
going to be used. I.e., don't change what you have.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Paolo Bonzini April 17, 2021, 12:45 p.m. UTC | #6
On 07/04/21 19:34, Borislav Petkov wrote:
> On Wed, Apr 07, 2021 at 05:05:07PM +0000, Sean Christopherson wrote:

>> I used memset() to defer initialization until after the various sanity

>> checks,

> 

> I'd actually vote for that too - I don't like doing stuff which is not

> going to be used. I.e., don't change what you have.


It's three votes for that then. :)

Sean, I squashed in this change

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ec4c01807272..a4d0ca8c4710 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1039,21 +1039,14 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
  static int sev_send_cancel(struct kvm *kvm, struct kvm_sev_cmd *argp)
  {
  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_send_cancel *data;
+	struct sev_data_send_cancel data;
  	int ret;
  
  	if (!sev_guest(kvm))
  		return -ENOTTY;
  
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_CANCEL, data, &argp->error);
-
-	kfree(data);
-	return ret;
+	data.handle = sev->handle;
+	return sev_issue_cmd(kvm, SEV_CMD_SEND_CANCEL, &data, &argp->error);
  }
  
  int svm_mem_enc_op(struct kvm *kvm, void __user *argp)

to handle SV_CMD_SEND_CANCEL which I merged before this series.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5457138c7347..316fd39c7aef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -150,35 +150,22 @@  static void sev_asid_free(int asid)
 
 static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 {
-	struct sev_data_decommission *decommission;
-	struct sev_data_deactivate *data;
+	struct sev_data_decommission decommission;
+	struct sev_data_deactivate deactivate;
 
 	if (!handle)
 		return;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return;
-
-	/* deactivate handle */
-	data->handle = handle;
+	deactivate.handle = handle;
 
 	/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
 	down_read(&sev_deactivate_lock);
-	sev_guest_deactivate(data, NULL);
+	sev_guest_deactivate(&deactivate, NULL);
 	up_read(&sev_deactivate_lock);
 
-	kfree(data);
-
-	decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
-	if (!decommission)
-		return;
-
 	/* decommission handle */
-	decommission->handle = handle;
-	sev_guest_decommission(decommission, NULL);
-
-	kfree(decommission);
+	decommission.handle = handle;
+	sev_guest_decommission(&decommission, NULL);
 }
 
 static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -216,19 +203,14 @@  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
 {
-	struct sev_data_activate *data;
+	struct sev_data_activate activate;
 	int asid = sev_get_asid(kvm);
 	int ret;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
 	/* activate ASID on the given handle */
-	data->handle = handle;
-	data->asid   = asid;
-	ret = sev_guest_activate(data, error);
-	kfree(data);
+	activate.handle = handle;
+	activate.asid   = asid;
+	ret = sev_guest_activate(&activate, error);
 
 	return ret;
 }
@@ -258,7 +240,7 @@  static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
 static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_start *start;
+	struct sev_data_launch_start start;
 	struct kvm_sev_launch_start params;
 	void *dh_blob, *session_blob;
 	int *error = &argp->error;
@@ -270,20 +252,16 @@  static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
-	if (!start)
-		return -ENOMEM;
+	memset(&start, 0, sizeof(start));
 
 	dh_blob = NULL;
 	if (params.dh_uaddr) {
 		dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
-		if (IS_ERR(dh_blob)) {
-			ret = PTR_ERR(dh_blob);
-			goto e_free;
-		}
+		if (IS_ERR(dh_blob))
+			return PTR_ERR(dh_blob);
 
-		start->dh_cert_address = __sme_set(__pa(dh_blob));
-		start->dh_cert_len = params.dh_len;
+		start.dh_cert_address = __sme_set(__pa(dh_blob));
+		start.dh_cert_len = params.dh_len;
 	}
 
 	session_blob = NULL;
@@ -294,40 +272,38 @@  static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 			goto e_free_dh;
 		}
 
-		start->session_address = __sme_set(__pa(session_blob));
-		start->session_len = params.session_len;
+		start.session_address = __sme_set(__pa(session_blob));
+		start.session_len = params.session_len;
 	}
 
-	start->handle = params.handle;
-	start->policy = params.policy;
+	start.handle = params.handle;
+	start.policy = params.policy;
 
 	/* create memory encryption context */
-	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);
+	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, &start, error);
 	if (ret)
 		goto e_free_session;
 
 	/* Bind ASID to this guest */
-	ret = sev_bind_asid(kvm, start->handle, error);
+	ret = sev_bind_asid(kvm, start.handle, error);
 	if (ret)
 		goto e_free_session;
 
 	/* return handle to userspace */
-	params.handle = start->handle;
+	params.handle = start.handle;
 	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params))) {
-		sev_unbind_asid(kvm, start->handle);
+		sev_unbind_asid(kvm, start.handle);
 		ret = -EFAULT;
 		goto e_free_session;
 	}
 
-	sev->handle = start->handle;
+	sev->handle = start.handle;
 	sev->fd = argp->sev_fd;
 
 e_free_session:
 	kfree(session_blob);
 e_free_dh:
 	kfree(dh_blob);
-e_free:
-	kfree(start);
 	return ret;
 }
 
@@ -446,7 +422,7 @@  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct kvm_sev_launch_update_data params;
-	struct sev_data_launch_update_data *data;
+	struct sev_data_launch_update_data data;
 	struct page **inpages;
 	int ret;
 
@@ -456,20 +432,14 @@  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
 	vaddr = params.uaddr;
 	size = params.len;
 	vaddr_end = vaddr + size;
 
 	/* Lock the user memory. */
 	inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
-	if (IS_ERR(inpages)) {
-		ret = PTR_ERR(inpages);
-		goto e_free;
-	}
+	if (IS_ERR(inpages))
+		return PTR_ERR(inpages);
 
 	/*
 	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
@@ -477,6 +447,9 @@  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	 */
 	sev_clflush_pages(inpages, npages);
 
+	data.reserved = 0;
+	data.handle = sev->handle;
+
 	for (i = 0; vaddr < vaddr_end; vaddr = next_vaddr, i += pages) {
 		int offset, len;
 
@@ -491,10 +464,9 @@  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 		len = min_t(size_t, ((pages * PAGE_SIZE) - offset), size);
 
-		data->handle = sev->handle;
-		data->len = len;
-		data->address = __sme_page_pa(inpages[i]) + offset;
-		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, data, &argp->error);
+		data.len = len;
+		data.address = __sme_page_pa(inpages[i]) + offset;
+		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, &data, &argp->error);
 		if (ret)
 			goto e_unpin;
 
@@ -510,8 +482,6 @@  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 	/* unlock the user pages */
 	sev_unpin_memory(kvm, inpages, npages);
-e_free:
-	kfree(data);
 	return ret;
 }
 
@@ -563,16 +533,14 @@  static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_update_vmsa *vmsa;
+	struct sev_data_launch_update_vmsa vmsa;
 	struct kvm_vcpu *vcpu;
 	int i, ret;
 
 	if (!sev_es_guest(kvm))
 		return -ENOTTY;
 
-	vmsa = kzalloc(sizeof(*vmsa), GFP_KERNEL);
-	if (!vmsa)
-		return -ENOMEM;
+	vmsa.reserved = 0;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct vcpu_svm *svm = to_svm(vcpu);
@@ -580,7 +548,7 @@  static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		/* Perform some pre-encryption checks against the VMSA */
 		ret = sev_es_sync_vmsa(svm);
 		if (ret)
-			goto e_free;
+			return ret;
 
 		/*
 		 * The LAUNCH_UPDATE_VMSA command will perform in-place
@@ -590,27 +558,25 @@  static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		 */
 		clflush_cache_range(svm->vmsa, PAGE_SIZE);
 
-		vmsa->handle = sev->handle;
-		vmsa->address = __sme_pa(svm->vmsa);
-		vmsa->len = PAGE_SIZE;
-		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, vmsa,
+		vmsa.handle = sev->handle;
+		vmsa.address = __sme_pa(svm->vmsa);
+		vmsa.len = PAGE_SIZE;
+		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa,
 				    &argp->error);
 		if (ret)
-			goto e_free;
+			return ret;
 
 		svm->vcpu.arch.guest_state_protected = true;
 	}
 
-e_free:
-	kfree(vmsa);
-	return ret;
+	return 0;
 }
 
 static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	void __user *measure = (void __user *)(uintptr_t)argp->data;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_measure *data;
+	struct sev_data_launch_measure data;
 	struct kvm_sev_launch_measure params;
 	void __user *p = NULL;
 	void *blob = NULL;
@@ -622,9 +588,7 @@  static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, measure, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
 	/* User wants to query the blob length */
 	if (!params.len)
@@ -632,23 +596,20 @@  static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 	p = (void __user *)(uintptr_t)params.uaddr;
 	if (p) {
-		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
-			ret = -EINVAL;
-			goto e_free;
-		}
+		if (params.len > SEV_FW_BLOB_MAX_SIZE)
+			return -EINVAL;
 
-		ret = -ENOMEM;
 		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
-			goto e_free;
+			return -ENOMEM;
 
-		data->address = __psp_pa(blob);
-		data->len = params.len;
+		data.address = __psp_pa(blob);
+		data.len = params.len;
 	}
 
 cmd:
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, &data, &argp->error);
 
 	/*
 	 * If we query the session length, FW responded with expected data.
@@ -665,63 +626,50 @@  static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 done:
-	params.len = data->len;
+	params.len = data.len;
 	if (copy_to_user(measure, &params, sizeof(params)))
 		ret = -EFAULT;
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
 static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_finish *data;
-	int ret;
+	struct sev_data_launch_finish data;
 
 	if (!sev_guest(kvm))
 		return -ENOTTY;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, data, &argp->error);
-
-	kfree(data);
-	return ret;
+	data.handle = sev->handle;
+	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, &data, &argp->error);
 }
 
 static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct kvm_sev_guest_status params;
-	struct sev_data_guest_status *data;
+	struct sev_data_guest_status data;
 	int ret;
 
 	if (!sev_guest(kvm))
 		return -ENOTTY;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, &data, &argp->error);
 	if (ret)
-		goto e_free;
+		return ret;
 
-	params.policy = data->policy;
-	params.state = data->state;
-	params.handle = data->handle;
+	params.policy = data.policy;
+	params.state = data.state;
+	params.handle = data.handle;
 
 	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
 		ret = -EFAULT;
-e_free:
-	kfree(data);
+
 	return ret;
 }
 
@@ -730,23 +678,17 @@  static int __sev_issue_dbg_cmd(struct kvm *kvm, unsigned long src,
 			       int *error, bool enc)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_dbg *data;
-	int ret;
+	struct sev_data_dbg data;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	data.reserved = 0;
+	data.handle = sev->handle;
+	data.dst_addr = dst;
+	data.src_addr = src;
+	data.len = size;
 
-	data->handle = sev->handle;
-	data->dst_addr = dst;
-	data->src_addr = src;
-	data->len = size;
-
-	ret = sev_issue_cmd(kvm,
-			    enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
-			    data, error);
-	kfree(data);
-	return ret;
+	return sev_issue_cmd(kvm,
+			     enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
+			     &data, error);
 }
 
 static int __sev_dbg_decrypt(struct kvm *kvm, unsigned long src_paddr,
@@ -966,7 +908,7 @@  static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
 static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_secret *data;
+	struct sev_data_launch_secret data;
 	struct kvm_sev_launch_secret params;
 	struct page **pages;
 	void *blob, *hdr;
@@ -998,41 +940,36 @@  static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		goto e_unpin_memory;
 	}
 
-	ret = -ENOMEM;
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		goto e_unpin_memory;
+	memset(&data, 0, sizeof(data));
 
 	offset = params.guest_uaddr & (PAGE_SIZE - 1);
-	data->guest_address = __sme_page_pa(pages[0]) + offset;
-	data->guest_len = params.guest_len;
+	data.guest_address = __sme_page_pa(pages[0]) + offset;
+	data.guest_len = params.guest_len;
 
 	blob = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
 	if (IS_ERR(blob)) {
 		ret = PTR_ERR(blob);
-		goto e_free;
+		goto e_unpin_memory;
 	}
 
-	data->trans_address = __psp_pa(blob);
-	data->trans_len = params.trans_len;
+	data.trans_address = __psp_pa(blob);
+	data.trans_len = params.trans_len;
 
 	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
 	if (IS_ERR(hdr)) {
 		ret = PTR_ERR(hdr);
 		goto e_free_blob;
 	}
-	data->hdr_address = __psp_pa(hdr);
-	data->hdr_len = params.hdr_len;
+	data.hdr_address = __psp_pa(hdr);
+	data.hdr_len = params.hdr_len;
 
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, &data, &argp->error);
 
 	kfree(hdr);
 
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 e_unpin_memory:
 	/* content of memory is updated, mark pages dirty */
 	for (i = 0; i < n; i++) {
@@ -1047,7 +984,7 @@  static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	void __user *report = (void __user *)(uintptr_t)argp->data;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_attestation_report *data;
+	struct sev_data_attestation_report data;
 	struct kvm_sev_attestation_report params;
 	void __user *p;
 	void *blob = NULL;
@@ -1059,9 +996,7 @@  static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
 	/* User wants to query the blob length */
 	if (!params.len)
@@ -1069,23 +1004,20 @@  static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 	p = (void __user *)(uintptr_t)params.uaddr;
 	if (p) {
-		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
-			ret = -EINVAL;
-			goto e_free;
-		}
+		if (params.len > SEV_FW_BLOB_MAX_SIZE)
+			return -EINVAL;
 
-		ret = -ENOMEM;
 		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
-			goto e_free;
+			return -ENOMEM;
 
-		data->address = __psp_pa(blob);
-		data->len = params.len;
-		memcpy(data->mnonce, params.mnonce, sizeof(params.mnonce));
+		data.address = __psp_pa(blob);
+		data.len = params.len;
+		memcpy(data.mnonce, params.mnonce, sizeof(params.mnonce));
 	}
 cmd:
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, &data, &argp->error);
 	/*
 	 * If we query the session length, FW responded with expected data.
 	 */
@@ -1101,13 +1033,11 @@  static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 done:
-	params.len = data->len;
+	params.len = data.len;
 	if (copy_to_user(report, &params, sizeof(params)))
 		ret = -EFAULT;
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 	return ret;
 }