lkml.org 
[lkml]   [2022]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/5] KVM: nSVM: Don't forget about L1-injected events
On 4.04.2022 11:53, Maxim Levitsky wrote:
> On Thu, 2022-03-10 at 22:38 +0100, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> In SVM synthetic software interrupts or INT3 or INTO exception that L1
>> wants to inject into its L2 guest are forgotten if there is an intervening
>> L0 VMEXIT during their delivery.
>>
>> They are re-injected correctly with VMX, however.
>>
>> This is because there is an assumption in SVM that such exceptions will be
>> re-delivered by simply re-executing the current instruction.
>> Which might not be true if this is a synthetic exception injected by L1,
>> since in this case the re-executed instruction will be one already in L2,
>> not the VMRUN instruction in L1 that attempted the injection.
>>
>> Leave the pending L1 -> L2 event in svm->nested.ctl.event_inj{,err} until
>> it is either re-injected successfully or returned to L1 upon a nested
>> VMEXIT.
>> Make sure to always re-queue such event if returned in EXITINTINFO.
>>
>> The handling of L0 -> {L1, L2} event re-injection is left as-is to avoid
>> unforeseen regressions.
>
> Some time ago I noticed this too, but haven't dug into this too much.
> I rememeber I even had some half-baked patch for this I never posted,
> because I didn't think about the possibility of this syntetic injection.
>
> Just to be clear that I understand this correctly:
>
> 1. What is happening is that L1 is injecting INTn/INTO/INT3 but L2 code
> doesn't actualy contain an INTn/INTO/INT3 instruction.
> This is wierd but legal thing to do.
> Again, if L2 actually contained the instruction, it would have worked?

I think so (haven't tested it though).

>
> 2. When actual INTn/INT0/INT3 are intercepted on SVM, then
> save.RIP points to address of the instruction, and control.next_rip
> points to address of next instruction after (as expected)

Yes.

> 3. When EVENTINJ is used with '(TYPE = 3) with vectors 3 or 4'
> or 'TYPE=4', then next_rip is pushed on the stack, while save.RIP is
> pretty much ignored, and exectution jumps to the handler in the IDT.

Yes.

> also at least for INT3/INTO, PRM states that IDT's DPL field is checked
> before dispatch, meaning that we can get legit #GP during delivery.
> (this looks like another legit reason to fix exception merging in KVM)
>

That's right.

> Best regards,
> Maxim Levitsky
>
>
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> ---
>> arch/x86/kvm/svm/nested.c | 65 +++++++++++++++++++++++++++++++++++++--
>> arch/x86/kvm/svm/svm.c | 17 ++++++++--
>> arch/x86/kvm/svm/svm.h | 47 ++++++++++++++++++++++++++++
>> 3 files changed, 125 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 9656f0d6815c..75017bf77955 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -420,8 +420,17 @@ void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
>> void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)
>> {
>> u32 mask;
>> - svm->nested.ctl.event_inj = svm->vmcb->control.event_inj;
>> - svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err;
>> +
>> + /*
>> + * Leave the pending L1 -> L2 event in svm->nested.ctl.event_inj{,err}
>> + * if its re-injection is needed
>> + */
>> + if (!exit_during_event_injection(svm, svm->nested.ctl.event_inj,
>> + svm->nested.ctl.event_inj_err)) {
>> + WARN_ON_ONCE(svm->vmcb->control.event_inj & SVM_EVTINJ_VALID);
>> + svm->nested.ctl.event_inj = svm->vmcb->control.event_inj;
>> + svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err;
>> + }
>
> Beware that this could backfire in regard to nested migration.
>
> I once chased a nasty bug related to this.
>
> The bug was:
>
> - L1 does VMRUN with injection (EVENTINJ set)
>
> - VMRUN exit handler is 'executed' by KVM,
> This copies EVENINJ fields from VMCB12 to VMCB02
>
> - Once VMRUN exit handler is done executing, we exit to userspace to start migration
> (basically static_call(kvm_x86_handle_exit)(...) handles the SVM_EXIT_VMRUN,
> and that is all, vcpu_enter_guest isn't called again, so injection is not canceled)
>
> - migration happens and it migrates the control area of vmcb02 with EVENTINJ fields set.
>
> - on migration target, we inject another interrupt to the guest via EVENTINJ
> because svm_check_nested_events checks nested_run_pending to avoid doing this
> but nested_run_pending was not migrated correctly,
> and overwrites the EVENTINJ - injection is lost.
>
> Paolo back then proposed to me that instead of doing direct copy from VMCB12 to VMCB02
> we should instead go through 'vcpu->arch.interrupt' and such.
> I had a prototype of this but never gotten to clean it up to be accepted upstream,
> knowing that current way also works.
>

This sounds like a valid, but different, bug - to be honest, it would
look much cleaner to me, too, if EVENTINJ was parsed from VMCB12 into
relevant KVM injection structures on a nested VMRUN rather than following
a hybrid approach:
1) Copy the field from VMCB12 to VMCB02 directly on a nested VMRUN,

2) Parse the EXITINTINFO into KVM injection structures when re-injecting.

>>
>> /* Only a few fields of int_ctl are written by the processor. */
>> mask = V_IRQ_MASK | V_TPR_MASK;
>> @@ -669,6 +678,54 @@ static void nested_svm_copy_common_state(struct vmcb *from_vmcb, struct vmcb *to
>> to_vmcb->save.spec_ctrl = from_vmcb->save.spec_ctrl;
>> }
>>
>> +void nested_svm_maybe_reinject(struct kvm_vcpu *vcpu)
>
> A personal taste note: I don't like the 'maybe' for some reason.
> But I won't fight over this.

What's you proposed name then?

>> +{
>> + struct vcpu_svm *svm = to_svm(vcpu);
>> + unsigned int vector, type;
>> + u32 exitintinfo = svm->vmcb->control.exit_int_info;
>> +
>> + if (WARN_ON_ONCE(!is_guest_mode(vcpu)))
>> + return;
>> +
>> + /*
>> + * No L1 -> L2 event to re-inject?
>> + *
>> + * In this case event_inj will be cleared by
>> + * nested_sync_control_from_vmcb02().
>> + */
>> + if (!(svm->nested.ctl.event_inj & SVM_EVTINJ_VALID))
>> + return;
>> +
>> + /* If the last event injection was successful there shouldn't be any pending event */
>> + if (WARN_ON_ONCE(!(exitintinfo & SVM_EXITINTINFO_VALID)))
>> + return;
>> +
>> + kvm_make_request(KVM_REQ_EVENT, vcpu);
>> +
>> + vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
>> + type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
>> +
>> + switch (type) {
>> + case SVM_EXITINTINFO_TYPE_NMI:
>> + vcpu->arch.nmi_injected = true;
>> + break;
>> + case SVM_EXITINTINFO_TYPE_EXEPT:
>> + if (exitintinfo & SVM_EXITINTINFO_VALID_ERR)
>> + kvm_requeue_exception_e(vcpu, vector,
>> + svm->vmcb->control.exit_int_info_err);
>> + else
>> + kvm_requeue_exception(vcpu, vector);
>> + break;
>> + case SVM_EXITINTINFO_TYPE_SOFT:
>
> Note that AFAIK, SVM_EXITINTINFO_TYPE_SOFT is only for INTn instructions,
> while INT3 and INTO are considered normal exceptions but EVENTINJ
> handling has special case for them.
>
That's right.

> On VMX it is a bit cleaner:
> It has:
>
> 3 - normal stock exception caused by CPU itself, like #PF and such
>
> 4 - INTn
> * does DPL check and uses VM_EXIT_INSTRUCTION_LEN
>
> 5 - ICEBP/INT1, which SVM doesnt support to re-inject
> * doesn't do DPL check, but uses VM_EXIT_INSTRUCTION_LEN I think
>
> 6 - software exception (INT3/INTO)
> * does DPL check and uses VM_EXIT_INSTRUCTION_LEN as well
>
> I don't know if there is any difference between 4 and 6.
>
>
>
>
(..)
>
>
> I will also review Sean's take on this, let see which one is simplier.

Since Sean's patch set is supposed to supersede this one let's continue
the discussion there.

> Best regards,
> Maxim Levitsky

Thanks,
Maciej

\
 
 \ /
  Last update: 2022-04-05 00:28    [W:0.078 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site