On Fri, Dec 13, 2024, Ivan Orlov wrote:
> On Thu, Dec 12, 2024 at 11:42:37AM -0800, Sean Christopherson wrote:
> > Unprotect and re-execute is fine, what I'm worried about is *successfully*
> > emulating the instruction. E.g.
> >
> > 1. CPU executes instruction X and hits a #GP.
> > 2. While vectoring the #GP, a shadow #PF is taken.
> > 3. On VM-Exit, KVM re-injects the #GP (see __vmx_complete_interrupts()).
> > 4. KVM emulates because of the write-protected page.
> > 5. KVM "successfully" emulates and also detects the #GP
> > 6. KVM synthesizes a #GP, and because the vCPU already has injected #GP,
> > incorrectly escalates to a #DF.
> >
> > The above is a bit contrived, but I think it could happen if the guest reused a
> > page that _was_ a page table, for a vCPU's kernel stack.
> >
>
> Does it work like that only for contributory exceptions / page faults?
The #DF case, yes.
> In case if it's not #GP but (for instance) #UD, (as far as I understand)
> KVM will queue only one of them without causing #DF so it's gonna be
> valid?
No, it can still be invalid. E.g. initialize hit a #BP, replace it with a #UD,
but there may be guest-visibile side effects from the original #BP.
> > > However, I'm not sure what happens if vectoring is caused by external
> > > interrupt: if we unprotect the page and re-execute the instruction,
> > > will IRQ be delivered nonetheless, or it will be lost as irq is
> > > already in ISR? Do we need to re-inject it in such a case?
> >
> > In all cases, the event that was being vectored is re-injected. Restarting from
> > scratch would be a bug. E.g. if the cause of initial exception was "fixed", say
> > because the initial exception was #BP, and the guest finished patching out the INT3,
> > then restarting would execute the _new_ instruction, and the INT3 would be lost.
> >
>
> Cool, that is what I was concerned about, glad that it is already
> implemented :)
>
> >
> > As far as unprotect+retry being viable, I think we're on the same page. What I'm
> > getting at is that I think KVM should never allow emulating on #PF when the #PF
> > occurred while vectoring. E.g. this:
> >
> > static inline bool kvm_can_emulate_event_vectoring(int emul_type)
> > {
> > return !(emul_type & EMULTYPE_PF);
> > }
> >
>
> Yeah, I agree. I'll post a V3 with suggested fixes (after running all of the
> selftests to be sure that it doesn't break anything).
>
> > and then I believe this? Where this diff can be a separate prep patch (though I'm
> > pretty sure it's technically pointless without the vectoring angle, because shadow
> > #PF can't coincide with any of the failure paths for kvm_check_emulate_insn()).
> >
>
> Looks good. If you don't mind, I could add this patch to the series with `Suggested-by`
> tag since it's neccessary to allow unprotect+retry in case of shadow #PF during
> vectoring.
Ya, go for it.
