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

[Tom Lendacky]

On 9/15/26 09:31, Sean Christopherson wrote:
> On Mon, Sep 14, 2026, Tom Lendacky wrote:
>> 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.
> 
> Oof, "fine" is definitely not ideal though.  
> 
>>> @@ -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;
>>> +	}
> 
> What if we keep this one as -EIO (or better, change it to -EFAULT in a separate
> patch?), but add a comment explaning why KVM needs to exit to userspace in this
> particular case?  That would be a good compromise; if the guest is attempting to
> access non-existent memory, the initial READ will fail, i.e. we still get most
> of the behavior Jacky wants.  The only fatal case would be where either userspace
> really did screw up, or the guest managed to find read-only memory (though I
> would probably argue that's likely also a userspace bug?).

That sounds good to me.

Thanks,
Tom