Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Fri, Dec 03, 2021, Tom Lendacky wrote:
> On 12/3/21 10:39 AM, Sean Christopherson wrote:
> > On Thu, Dec 02, 2021, Tom Lendacky wrote:
> > > +			goto e_scratch;
> > >   		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
> > >   			/* Unable to copy scratch area from guest */
> > >   			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
> > >   			kvfree(scratch_va);
> > > -			return -EFAULT;
> > > +			goto e_scratch;
> > 
> > Same here, failure to read guest memory is a userspace issue and needs to be
> > reported to userspace.
> 
> But it could be a guest issue as well...  whichever is preferred is ok by me.

Arguably, any guest issue is a violation of the guest's contract with userspace,
and thus userspace needs to decide how to proceed.  E.g. userspace defines what
is RAM vs. MMIO and communicates that directly to the guest, KVM is not involved
in deciding what is/isn't RAM nor in communicating that information to the guest.
If the scratch GPA doesn't resolve to a memslot, then the guest is not honoring
the memory configuration as defined by userspace.

And if userspace unmaps an hva for whatever reason, then exiting to userspace
with -EFAULT is absolutely the right thing to do.  KVM's ABI currently sucks and
doesn't provide enough information to act on the -EFAULT, but I really want to
change that as there are multiple use cases, e.g. uffd and virtiofs truncation,
that shouldn't require any work in KVM beyond returning -EFAULT with a small
amount of metadata.

KVM could define its ABI such that failure to access the scratch area is reflected
into the guest, i.e. establish a contract with userspace, but IMO that's undesirable
as it limits KVM's options in the future, e.g. IIRC, in the potential uffd case any
failure on a uaccess needs to kick out to userspace.  KVM does have several cases
where it reflects these errors into the guest, e.g. kvm_pv_clock_pairing() and
Hyper-V emulation, but I would prefer we change those instead of adding more code
that assumes any memory failure is the guest's fault.