linux-kernel - Re: [PATCH] KVM: SEV: Fix guest memory leak when handling guest requests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZktbBRLXeOp9X6aH@google.com>
Date: Mon, 20 May 2024 07:17:13 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Michael Roth <mdroth@...xas.edu>
Cc: pbonzini@...hat.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	ashish.kalra@....com, thomas.lendacky@....com, rick.p.edgecombe@...el.com
Subject: Re: [PATCH] KVM: SEV: Fix guest memory leak when handling guest requests

This needs a

  From: Michael Roth <michael.roth@....com>

otherwise Author will be assigned to your @utexas.edu email.

On Sat, May 18, 2024, Michael Roth wrote:
> Before forwarding guest requests to firmware, KVM takes a reference on
> the 2 pages the guest uses for its request/response buffers. Make sure
> to release these when cleaning up after the request is completed.
> 
> Signed-off-by: Michael Roth <michael.roth@....com>
> ---

..

> @@ -3970,14 +3980,11 @@ static int __snp_handle_guest_req(struct kvm *kvm, gpa_t req_gpa, gpa_t resp_gpa
>  		return ret;
>  
>  	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err);
> -	if (ret)
> -		return ret;
>  
> -	ret = snp_cleanup_guest_buf(&data);
> -	if (ret)
> -		return ret;
> +	if (snp_cleanup_guest_buf(&data))
> +		return -EINVAL;

EINVAL feels wrong.  The input was completely valid.  Also, forwarding the error
to the guest doesn't seem like the right thing to do if KVM can't reclaim the
response PFN.  Shouldn't that be fatal to the VM?

> -	return 0;
> +	return ret;

I find the setup/cleanup split makes this code harder to read, not easier.  It
won't be pretty no matter waht due to the potential RMP failures, but IMO this
is easier to follow:

	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
	struct sev_data_snp_guest_request data = {0};
	kvm_pfn_t req_pfn, resp_pfn;
	int ret;

	if (!sev_snp_guest(kvm))
		return -EINVAL;

	if (!PAGE_ALIGNED(req_gpa) || !PAGE_ALIGNED(resp_gpa))
		return -EINVAL;

	req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
	if (is_error_noslot_pfn(req_pfn))
		return -EINVAL;

	ret = -EINVAL;

	resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
	if (is_error_noslot_pfn(resp_pfn))
		goto release_req;

	if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) {
		kvm_release_pfn_clean(resp_pfn);
		goto release_req;
	}

	data.gctx_paddr = __psp_pa(sev->snp_context);
	data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
	data.res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err);

	if (snp_page_reclaim(resp_pfn) ||
	    rmp_make_shared(resp_pfn, PG_LEVEL_4K))
		ret = ret ?: -EIO;
	else
		kvm_release_pfn_dirty(resp_pfn);
release_req:
	kvm_release_pfn_clean(req_pfn);
	return ret;