[MODERATED] Re: [PATCH 1/2] L1TF KVM 1

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

On 30/05/2018 00:49, speck for Thomas Gleixner wrote:
> On Tue, 29 May 2018, speck for Paolo Bonzini wrote:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Subject: [PATCH 1/2] kvm: x86: mitigation for L1 cache terminal fault vulnerabilities
>>
>> This patch adds two mitigation modes for CVE-2018-3620, aka L1 terminal
>> fault.  The two modes are "vmexit_l1d_flush=1" and "vmexit_l1d_flush=2".
> 
> This is confusing at best. Why is this vmexit_l1d_flush? You flush on
> VMENTER not on VMEXIT.

Yes, that makes sense.

>> Notably, L1 cache flushes are performed on EPT violations (which are
>> basically KVM-level page faults), vmexits that involve the emulator,
>> and on every KVM_RUN invocation (so each userspace exit).  However,
>> most vmexits are considered safe.  I singled out the emulator because
>> it may be a good target for other speculative execution-based threats,
>> and the MMU because it can bring host page tables in the L1 cache.
> 
> What about interrupts?

Will fix the commit message and rework the patch to set vcpu_unconfined
at vmexit time.

>> +		/*
>> +		 * The next three set vcpu->arch.vcpu_unconfined themselves, so
>> +		 * we consider them confined here.
> 
> What's the logic behind that?
> 
>> +		 */
>> +	case EXIT_REASON_EPT_VIOLATION:
>> +	case EXIT_REASON_EPT_MISCONFIG:
>> +	case EXIT_REASON_IO_INSTRUCTION:
>> +		return true;

EPT misconfig and I/O instruction are very frequent, and most of the
time they run just a small fast path.  EPT violation can be put together
with the "always slow" ones.

>> +	case EXIT_REASON_EXTERNAL_INTERRUPT: {
>> +		int cpu = raw_smp_processor_id();
>> +		int vector = per_cpu(last_vector, cpu);
>> +		return vector == LOCAL_TIMER_VECTOR || vector == RESCHEDULE_VECTOR;
> 
> That wants a comment why these two are considered safe.
> 
> The timer vector is a doubtful one. It does not necessarily cause a
> reschedule and it can run arbitrary softirq code on interrupt return. I
> wouldn't be that sure that it's safe.

It's also the most frequent one. :(  But I see your and Peter's point,
I'll drop it and consider all external interrupts to be unconfined.

May there could be some kind of "ran softirq" generation count.

>> @@ -9457,11 +9515,13 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
>> 		unsigned long entry;
>> 		gate_desc *desc;
>> 		struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +		int cpu = raw_smp_processor_id();
>>  #ifdef CONFIG_X86_64
>>  		unsigned long tmp;
>>  #endif
>>  
>> 		vector =  exit_intr_info & INTR_INFO_VECTOR_MASK;
>> +		per_cpu(last_vector, cpu) = vector;
> 
> Why aren't you doing the evaluation of the vector right here and set the
> unconfined bit instead of having yet another indirection and probably
> another cache line for that per_cpu() storage? That does not make any
> sense at all.

Because that's how the patches I got from Intel did it, and I kind of
liked keeping all the logic in one function.  But it's going to go away,
it's not safe.

>> 		desc = (gate_desc *)vmx->host_idt_base + vector;
>> 		entry = gate_offset(desc);
> 
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6509,10 +6512,40 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
>>  };
>>  #endif
>>  
>> +
>> +#define L1D_CACHE_ORDER 3
> 
> This should use the cache size information and not a hard coded value I think. 

Plus it seems wrong.  Order 3 is 32K, not 64K, isn't it? :/

> And this whole magic should be documented.
> 
> Aside of that do we really need that manual flush thingy? Is that ucode
> update going to take forever?

As Andrew said, this was also copied from Intel's patch, assuming they
knew what they were doing.  I'll just drop it and just use the microcode
MSR.

Paolo