Re: [PATCH] KVM: SEV: Return INVALID_INPUT on SNP req/resp buffer access failure

[Tom Lendacky <thomas.lendacky@amd.com>]

On 9/10/26 16:06, Jacky Li wrote:
> Currently, snp_handle_(ext_)guest_req() returns -EIO when
> kvm_{read/write/clear}_guest() fails while accessing guest-provided
> buffers. Returning -EIO causes KVM_RUN to exit to userspace, likely
> killing the VM.
> 
> Fix this by returning GHCB_HV_RESP_MALFORMED_INPUT with sub-error code
> GHCB_ERR_INVALID_INPUT to the guest and resuming the vCPU.
> 
> Per the GHCB specification, guest-provided GPA buffers that cannot
> be accessed by the hypervisor (e.g. private pages) should be treated
> as guest input errors. Because kvm_{read/write/clear}_guest() only
> returns -EFAULT on failure, treating this failure as an invalid input
> aligns with the definition of -EFAULT ("Bad address").
> 
> Returning GHCB_ERR_INVALID_INPUT also matches existing SNP handling
> in KVM, which already returns this error code for unaligned or
> overlapping buffers. It also aligns with other hypercall implementations
> in KVM (e.g. Hyper-V returning INVALID_HYPERCALL_INPUT on
> kvm_read_guest() failures in kvm_hv_flush_tlb()).
> 
> Performing upfront validation (e.g. via kvm_mem_is_private()) is
> avoided because it is prone to TOCTOU races with concurrent Page State
> Changes.

I'm ok with this approach overall, but will we run into a sequence
number problem now?

If the kvm_write_guest() in snp_handle_guest_req() fails, invalid input
is going to be returned, but we will have successfully called
SEV_CMD_SNP_GUEST_REQUEST. The sequence number will have advanced in the
firmware, but I think the guest will not think that it has and not
increment the sequence number causing subsequent requests to fail. At
that point the guest will need to zero out the associated VMPCK used and
move to the next one (there are a max of 4). If this continues happening
the guest will eventually not be able to make guest requests anymore.
But, I guess, if the kvm_write_guest() is failing, we're probably
already in a bad situation, so maybe it is fine.

Thanks,
Tom

> 
> Fixes: 88caf544c930 ("KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event")
> Fixes: 74458e4859d8 ("KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event")
> Signed-off-by: Jacky Li <jackyli@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5705723f1f41..d07562310519 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -4228,8 +4228,10 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
>  
>  	guard(mutex)(&sev->guest_req_mutex);
>  
> -	if (kvm_read_guest(kvm, req_gpa, sev->guest_req_buf, PAGE_SIZE))
> -		return -EIO;
> +	if (kvm_read_guest(kvm, req_gpa, sev->guest_req_buf, PAGE_SIZE)) {
> +		svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
> +		return 1;
> +	}
>  
>  	data.gctx_paddr = __psp_pa(sev->snp_context);
>  	data.req_paddr = __psp_pa(sev->guest_req_buf);
> @@ -4244,8 +4246,10 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
>  	if (ret && !fw_err)
>  		return ret;
>  
> -	if (kvm_write_guest(kvm, resp_gpa, sev->guest_resp_buf, PAGE_SIZE))
> -		return -EIO;
> +	if (kvm_write_guest(kvm, resp_gpa, sev->guest_resp_buf, PAGE_SIZE)) {
> +		svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
> +		return 1;
> +	}
>  
>  	/* No action is requested *from KVM* if there was a firmware error. */
>  	svm_vmgexit_no_action(svm, SNP_GUEST_ERR(0, fw_err));
> @@ -4296,7 +4300,7 @@ static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t r
>  
>  	if (kvm_read_guest(kvm, req_gpa + offsetof(struct snp_guest_msg_hdr, msg_type),
>  			   &msg_type, 1))
> -		return -EIO;
> +		goto request_invalid;
>  
>  	/*
>  	 * As per GHCB spec, requests of type MSG_REPORT_REQ also allow for
> @@ -4336,7 +4340,7 @@ static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t r
>  		 * certificate table is terminated by 24-bytes of zeroes.
>  		 */
>  		if (data_npages && kvm_clear_guest(kvm, data_gpa, 24))
> -			return -EIO;
> +			goto request_invalid;
>  	}
>  
>  	return snp_handle_guest_req(svm, req_gpa, resp_gpa);
> 
> ---
> base-commit: 50d05c7c76c96b90462f24debacca971d2e86713
> change-id: 20260910-snp-invalid-input-5dbb6a408e7f
> 
> Best regards,