Re: [PATCH 1/2] KVM: x86: Allow userspace to opt out of hypercall patching

[Oliver Upton <oupton@google.com>]

On Thu, Mar 24, 2022 at 06:57:18PM +0100, Paolo Bonzini wrote:
> On 3/24/22 18:44, Sean Christopherson wrote:
> > On Wed, Mar 16, 2022, Oliver Upton wrote:
> > > KVM handles the VMCALL/VMMCALL instructions very strangely. Even though
> > > both of these instructions really should #UD when executed on the wrong
> > > vendor's hardware (i.e. VMCALL on SVM, VMMCALL on VMX), KVM replaces the
> > > guest's instruction with the appropriate instruction for the vendor.
> > > Nonetheless, older guest kernels without commit c1118b3602c2 ("x86: kvm:
> > > use alternatives for VMCALL vs. VMMCALL if kernel text is read-only")
> > > do not patch in the appropriate instruction using alternatives, likely
> > > motivating KVM's intervention.
> > > 
> > > Add a quirk allowing userspace to opt out of hypercall patching.
> > 
> > A quirk may not be appropriate, per Paolo, the whole cross-vendor thing is
> > intentional.
> > 
> > https://lore.kernel.org/all/20211210222903.3417968-1-seanjc@google.com
> 
> It's intentional, but the days of the patching part are over.
> 
> KVM should just call emulate_hypercall if called with the wrong opcode
> (which in turn can be quirked away).  That however would be more complex to
> implement because the hypercall path wants to e.g. inject a #UD with
> kvm_queue_exception().
> 
> All this makes me want to just apply Oliver's patch.
> 
> > > +	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_FIX_HYPERCALL_INSN)) {
> > > +		ctxt->exception.error_code_valid = false;
> > > +		ctxt->exception.vector = UD_VECTOR;
> > > +		ctxt->have_exception = true;
> > > +		return X86EMUL_PROPAGATE_FAULT;
> > 
> > This should return X86EMUL_UNHANDLEABLE instead of manually injecting a #UD.  That
> > will also end up generating a #UD in most cases, but will play nice with
> > KVM_CAP_EXIT_ON_EMULATION_FAILURE.

Sean and I were looking at this together right now, and it turns out
that returning X86EMUL_UNHANDLEABLE at this point will unconditionally
bounce out to userspace.

x86_decode_emulated_instruction() would need to be the spot we bail to
guard these exits with the CAP.

> Hmm, not sure about that.  This is not an emulation failure in the sense
> that we don't know what to do.  We know that for this x86 vendor the right
> thing to do is to growl at the guest.
> 
> KVM_CAP_EXIT_ON_EMULATION_FAILURE would not have a way to ask KVM to invoke
> the hypercall, anyway, so it's not really possible for userspace to do the
> emulation.

Userspace could theoretically patch the hypercall itself and retry execution.
But IMO, userspace should just leave the quirk enabled and accept the default
KVM behavior if it still wants hypercall patching.

--
Thanks,
Oliver