linux-kernel - Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Apr 2022 15:36:53 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of
 retrying the instruction

On 28.04.2022 11:37, Maxim Levitsky wrote:
> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>> Re-inject INT3/INTO instead of retrying the instruction if the CPU
>> encountered an intercepted exception while vectoring the software
>> exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
>> shadow paging.  Retrying the instruction is architecturally wrong, e.g.
>> will result in a spurious #DB if there's a code breakpoint on the INT3/O,
>> and lack of re-injection also breaks nested virtualization, e.g. if L1
>> injects a software exception and vectoring the injected exception
>> encounters an exception that is intercepted by L0 but not L1.
>>
>> Due to, ahem, deficiencies in the SVM architecture, acquiring the next
>> RIP may require flowing through the emulator even if NRIPS is supported,
>> as the CPU clears next_rip if the VM-Exit is due to an exception other
>> than "exceptions caused by the INT3, INTO, and BOUND instructions".  To
>> deal with this, "skip" the instruction to calculate next_rip (if it's
>> not already known), and then unwind the RIP write and any side effects
>> (RFLAGS updates).
>>
>> Save the computed next_rip and use it to re-stuff next_rip if injection
>> doesn't complete.  This allows KVM to do the right thing if next_rip was
>> known prior to injection, e.g. if L1 injects a soft event into L2, and
>> there is no backing INTn instruction, e.g. if L1 is injecting an
>> arbitrary event.
>>
>> Note, it's impossible to guarantee architectural correctness given SVM's
>> architectural flaws.  E.g. if the guest executes INTn (no KVM injection),
>> an exit occurs while vectoring the INTn, and the guest modifies the code
>> stream while the exit is being handled, KVM will compute the incorrect
>> next_rip due to "skipping" the wrong instruction.  A future enhancement
>> to make this less awful would be for KVM to detect that the decoded
>> instruction is not the correct INTn and drop the to-be-injected soft
>> event (retrying is a lesser evil compared to shoving the wrong RIP on the
>> exception stack).
>>
>> Reported-by: Maciej S. Szmigiero <maciej.szmigiero@...cle.com>
>> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
>> ---
>>   arch/x86/kvm/svm/nested.c |  28 +++++++-
>>   arch/x86/kvm/svm/svm.c    | 140 +++++++++++++++++++++++++++-----------
>>   arch/x86/kvm/svm/svm.h    |   6 +-
>>   3 files changed, 130 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 461c5f247801..0163238aa198 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>>   	}
>>   }
>>   
>> +static inline bool is_evtinj_soft(u32 evtinj)
>> +{
>> +	u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
>> +	u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
>> +
>> +	if (!(evtinj & SVM_EVTINJ_VALID))
>> +		return false;
>> +
>> +	/*
>> +	 * Intentionally return false for SOFT events, SVM doesn't yet support
>> +	 * re-injecting soft interrupts.
>> +	 */
>> +	return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
>> +}
>> +
>>   static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>>   					  unsigned long vmcb12_rip)
>>   {
>> @@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>>   	else if (boot_cpu_has(X86_FEATURE_NRIPS))
>>   		vmcb02->control.next_rip    = vmcb12_rip;
>>   
>> +	if (is_evtinj_soft(vmcb02->control.event_inj)) {
>> +		svm->soft_int_injected = true;
>> +		svm->soft_int_csbase = svm->vmcb->save.cs.base;
>> +		svm->soft_int_old_rip = vmcb12_rip;
>> +		if (svm->nrips_enabled)
>> +			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
>> +		else
>> +			svm->soft_int_next_rip = vmcb12_rip;
>> +	}
>> +
>>   	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
>>   					      LBR_CTL_ENABLE_MASK;
>>   	if (svm->lbrv_enabled)
>> @@ -849,6 +874,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>>   
>>   out_exit_err:
>>   	svm->nested.nested_run_pending = 0;
>> +	svm->soft_int_injected = false;
>>   
>>   	svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
>>   	svm->vmcb->control.exit_code_hi = 0;
>> @@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>>   	nested_copy_vmcb_control_to_cache(svm, ctl);
>>   
>>   	svm_switch_vmcb(svm, &svm->nested.vmcb02);
>> -	nested_vmcb02_prepare_control(svm, save->rip);
>> +	nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);
> 
> Is this change intentional?

It looks to me the final code is correct since "svm->vmcb->save"
contains L2 register save, while "save" has L1 register save.

It was the patch 1 from this series that was incorrect in
using "save->rip" here instead.

Thanks,
Maciej