On Wed, Sep 25, 2024 at 05:06:45PM -0700, Sean Christopherson wrote:
>
> Yeah, but only because the alternative sucks worse. If KVM unconditionally
> exited
> with an emulation error, then unsuspecting (read: old) VMMs would likely
> terminate
> the guest, which gives guest userspace a way to DoS the entire VM, especially
> on
> older CPUs where KVM needs to emulate much more often.
>
> if (kvm->arch.exit_on_emulation_error ||
> (emulation_type & EMULTYPE_SKIP)) {
> prepare_emulation_ctxt_failure_exit(vcpu);
> return 0;
> }
>
> kvm_queue_exception(vcpu, UD_VECTOR);
>
> if (!is_guest_mode(vcpu) && kvm_x86_call(get_cpl)(vcpu) == 0) {
> prepare_emulation_ctxt_failure_exit(vcpu);
> return 0;
> }
>
> return 1;
>
> And that's exactly why KVM_CAP_EXIT_ON_EMULATION_FAILURE exists. VMMs that
> know
> they won't unintentionally give guest userspace what amounts to a privilege
> escalation can trap the emulation failure, do some logging or whatever, and
> then
> take whatever action it wants to take.
>
Hi Sean,
Makes sense, thank you for the explanation.
> > and I believe how we do this
> > is debatable. I maintain we should either set a flag in
> > emulation_failure.flags
> > to indicate that the error happened due to fetch from mmio (to give more
> > information to VMM),
>
> Generally speaking, I'm not opposed to adding more information along those
> lines.
> Though realistically, I don't know that an extra flag is warranted in this
> case,
> as it shouldn't be _that_ hard for userspace to deduce what went wrong,
> especially
> if KVM_TRANSLATE2[*] lands (though I'm somewhat curious as to why QEMU
> doesn't do
> the page walks itself).
>
> [*] https://lore.kernel.org/all/20240910152207.38974-1-nik...@amazon.de
>
Fair enough, but I still believe that it would be good to provide more
information about the failure to the VMM (considering the fact that KVM
tries to emulate an instruction anyway, adding a flag won't introduce any
performance overhead). I'll think about how we could do that without
being redundant :)
> > or we shouldn't return an error at all... Maybe it should be KVM_EXIT_MMIO
> > with
> > some flag set? What do you think?
>
> It'd be a breaking change and added complexity, for no benefit as far as I can
> tell. KVM_EXIT_INTERNAL_ERROR is _not_ a death sentence, or at least it
> doesn't
> have to be. Most VMMs do terminate the guest, but nothing is stopping
> userspace
> from grabbing RIP and emulating the instruction. I.e. userspace doesn't need
> "permission" in the form of KVM_EXIT_MMIO to try and keep the guest alive.
Yeah, I just thought that "internal error" is not the best exit code for
the situations when guest fetches from MMIO (since it is a perfectly legal
operation from the architectural point of view). But I agree that it
would be a breaking change without functional benefit ( especially if we
provide a flag about what happened :) ).
P.S. I tested the latest kvm/next, and if we set the IDT descriptor base to
an MMIO address it still falls into the infinite loop on SVM. I'm going
to send the fix in the next couple of days.
Kind regards,
Ivan Orlov
