On Tue, 2023-01-31 at 01:44 +0000, Sean Christopherson wrote:
> On Sat, Jan 28, 2023, Sean Christopherson wrote:
> > On Tue, Nov 29, 2022, Maxim Levitsky wrote:
> > > the V_IRQ and v_TPR bits don't exist when virtual interrupt
> > > masking is not enabled, therefore the KVM should not copy these
> > > bits regardless of V_IRQ intercept.
> >
> > Hmm, the APM disagrees:
Yes, my apologies, after re-reading the APM I agree with you.
> >
> > The APIC's TPR always controls the task priority for physical interrupts, and the
> > V_TPR always controls virtual interrupts.
> >
> > While running a guest with V_INTR_MASKING cleared to 0:
> > • Writes to CR8 affect both the APIC's TPR and the V_TPR register.
> >
> >
> > ...
> >
> > The three VMCB fields V_IRQ, V_INTR_PRIO, and V_INTR_VECTOR indicate whether there
> > is a virtual interrupt pending, and, if so, what its vector number and priority are.
> >
> > IIUC, V_INTR_MASKING_MASK is mostly about EFLAGS.IF, with a small side effect on
> > TPR. E.g. a VMM could pend a V_IRQ but clear V_INTR_MASKING and expect the guest
> > to take the V_IRQ. At least, that's my reading of things.
Yes, this is how I understand it as well.
> >
> > > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > > ---
> > > arch/x86/kvm/svm/nested.c | 23 ++++++++---------------
> > > 1 file changed, 8 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > index 37af0338da7c32..aad3145b2f62fe 100644
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -412,24 +412,17 @@ void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> > > */
> > > void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)
> > > {
> > > - u32 mask;
> > > + u32 mask = 0;
> > > svm->nested.ctl.event_inj = svm->vmcb->control.event_inj;
> > > svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err;
> > >
> > > - /* Only a few fields of int_ctl are written by the processor. */
> > > - mask = V_IRQ_MASK | V_TPR_MASK;
> > > - if (!(svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) &&
> > > - svm_is_intercept(svm, INTERCEPT_VINTR)) {
> > > - /*
> > > - * In order to request an interrupt window, L0 is usurping
> > > - * svm->vmcb->control.int_ctl and possibly setting V_IRQ
> > > - * even if it was clear in L1's VMCB. Restoring it would be
> > > - * wrong. However, in this case V_IRQ will remain true until
> > > - * interrupt_window_interception calls svm_clear_vintr and
> > > - * restores int_ctl. We can just leave it aside.
> > > - */
> > > - mask &= ~V_IRQ_MASK;
>
> Argh! *shakes fist at KVM and SVM*
>
> This is ridiculously convoluted, and I'm pretty sure there are existing bugs. If
> L1 runs L2 with V_IRQ=1 and V_INTR_MASKING=1
Note that there are two cases when we need an interrupt window in nested case:
- If the L1 doesn't intercept interrupts, which is what we are taking about here.
- If the L1 does intercept interrupts, but let L2 control the L1's EFLAGS.IF and/or
L1's GIF
(that is V_INTR_MASKING_MASK is not set, and/or L1 doesn't intercept STGI/CLGI).
In this case a 'real' interrupt will be converted to a VM exit but only
when both L1's EFLAGS.IF is true and L1's GIF is true.
> , and KVM requests an interrupt window,
> then KVM will overwrite vmcb02's int_vector and int_ctl, i.e. clobber L1's V_IRQ,
> but then silently clear INTERCEPT_VINTR in recalc_intercepts() and thus prevent
> svm_clear_vintr() from being reached, i.e. prevent restoring L1's V_IRQ.
>
> Bug #1 is that KVM shouldn't clobber the V_IRQ fields if KVM ultimately decides
> not to open an interrupt window. Bug #2 is that KVM needs to open an interrupt
> window if save.RFLAGS.IF=1, as interrupts may become unblocked in that case,
> e.g. if L2 is in an interrupt shadow.
>
> So I think this over two patches?
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 05d38944a6c0..ad1e70ac8669 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -139,13 +139,18 @@ void recalc_intercepts(struct vcpu_svm *svm)
>
> if (g->int_ctl & V_INTR_MASKING_MASK) {
> /*
> - * Once running L2 with HF_VINTR_MASK, EFLAGS.IF and CR8
> - * does not affect any interrupt we may want to inject;
> - * therefore, writes to CR8 are irrelevant to L0, as are
> - * interrupt window vmexits.
> + * If L2 is active and V_INTR_MASKING is enabled in vmcb12,
> + * disable intercept of CR8 writes as L2's CR8 does not affect
> + * any interrupt KVM may want to inject.
> + *
> + * Similarly, disable intercept of virtual interrupts (used to
> + * detect interrupt windows) if the saved RFLAGS.IF is '0', as
> + * the effective RFLAGS.IF for L1 interrupts will never be set
> + * while L2 is running (L2's RFLAGS.IF doesn't affect L1 IRQs).
> */
> vmcb_clr_intercept(c, INTERCEPT_CR8_WRITE);
> - vmcb_clr_intercept(c, INTERCEPT_VINTR);
> + if (!(svm->vmcb01.ptr->save.rflags & X86_EFLAGS_IF))
> + vmcb_clr_intercept(c, INTERCEPT_VINTR);
How about instead moving this code to svm_set_vintr?
That is, in the guest mode, if the guest has V_INTR_MASKING_MASK, then
then a nested VM exit is the next point the interrupt window could open,
thus we don't set VINTR)
Or even better put the logic in svm_enable_irq_window (that is avoid
calling svm_set_vintr in the first place).
I also think that it worth it to add a warning that 'svm_set_intercept'
didn't work, that is didn't really set an intercept.
In theory that can result in nasty CVEs in addition to logic bugs as you found.
> }
>
> /*
> @@ -416,18 +421,18 @@ void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)
>
> /* Only a few fields of int_ctl are written by the processor. */
> mask = V_IRQ_MASK | V_TPR_MASK;
> - if (!(svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) &&
> - svm_is_intercept(svm, INTERCEPT_VINTR)) {
> - /*
> - * In order to request an interrupt window, L0 is usurping
> - * svm->vmcb->control.int_ctl and possibly setting V_IRQ
> - * even if it was clear in L1's VMCB. Restoring it would be
> - * wrong. However, in this case V_IRQ will remain true until
> - * interrupt_window_interception calls svm_clear_vintr and
> - * restores int_ctl. We can just leave it aside.
> - */
> +
> + /*
> + * Don't sync vmcb02 V_IRQ back to vmcb12 if KVM (L0) is intercepting
> + * virtual interrupts in order to request an interrupt window, as KVM
> + * has usurped vmcb02's int_ctl. If an interrupt window opens before
> + * the next VM-Exit, svm_clear_vintr() will restore vmcb12's int_ctl.
> + * If no window opens, V_IRQ will be correctly preserved in vmcb12's
> + * int_ctl (because it was never recognized while L2 was running).
> + */
> + if (svm_is_intercept(svm, INTERCEPT_VINTR) &&
> + !test_bit(INTERCEPT_VINTR, (unsigned long *)svm->nested.ctl.intercepts))
> mask &= ~V_IRQ_MASK;
This makes sense.
> - }
>
> if (nested_vgif_enabled(svm))
> mask |= V_GIF_MASK;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b103fe7cbc82..59d2891662ef 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1580,6 +1580,16 @@ static void svm_set_vintr(struct vcpu_svm *svm)
>
> svm_set_intercept(svm, INTERCEPT_VINTR);
>
> + /*
> + * Recalculating intercepts may have clear the VINTR intercept. If
> + * V_INTR_MASKING is enabled in vmcb12, then the effective RFLAGS.IF
> + * for L1 physical interrupts is L1's RFLAGS.IF at the time of VMRUN.
> + * Requesting an interrupt window if save.RFLAGS.IF=0 is pointless as
> + * interrupts will never be unblocked while L2 is running.
> + */
> + if (!svm_is_intercept(svm, INTERCEPT_VINTR))
> + return;
This won't be needed if we don't call the svm_set_vintr in the first place.
> +
> /*
> * This is just a dummy VINTR to actually cause a vmexit to happen.
> * Actual injection of virtual interrupts happens through EVENTINJ.
>
With all this said, I also want to note that this patch has *nothing* to do with VNMI,
I only added it due to some refactoring, so feel free to drop it from vNMI queue,
and deal with those bugs separately.
Best regards,
Maxim Levitsky
