diff mbox series

[RFC,v6,75/92] kvm: x86: disable gpa_available optimization in emulator_read_write_onepage()

Message ID 20190809160047.8319-76-alazar@bitdefender.com (mailing list archive)
State New, archived
Headers show
Series VM introspection | expand

Commit Message

Adalbert Lazăr Aug. 9, 2019, 4 p.m. UTC 
If the EPT violation was caused by an execute restriction imposed by the
introspection tool, gpa_available will point to the instruction pointer,
not the to the read/write location that has to be used to emulate the
current instruction.

This optimization should be disabled only when the VM is introspected,
not just because the introspection subsystem is present.

Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paolo Bonzini Aug. 13, 2019, 8:47 a.m. UTC | #1 
On 09/08/19 18:00, Adalbert Lazăr wrote:
> If the EPT violation was caused by an execute restriction imposed by the
> introspection tool, gpa_available will point to the instruction pointer,
> not the to the read/write location that has to be used to emulate the
> current instruction.
> 
> This optimization should be disabled only when the VM is introspected,
> not just because the introspection subsystem is present.
> 
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>

The right thing to do is to not set gpa_available for fetch failures in 
kvm_mmu_page_fault instead:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 24843cf49579..1bdca40fa831 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -5364,8 +5364,12 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
 	enum emulation_result er;
 	bool direct = vcpu->arch.mmu->direct_map;
 
-	/* With shadow page tables, fault_address contains a GVA or nGPA.  */
-	if (vcpu->arch.mmu->direct_map) {
+	/*
+	 * With shadow page tables, fault_address contains a GVA or nGPA.
+	 * On a fetch fault, fault_address contains the instruction pointer.
+	 */
+	if (vcpu->arch.mmu->direct_map &&
+	    likely(!(error_code & PFERR_FETCH_MASK)) {
 		vcpu->arch.gpa_available = true;
 		vcpu->arch.gpa_val = cr2;
 	}


Paolo

> ---
>  arch/x86/kvm/x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 965c4f0108eb..3975331230b9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5532,7 +5532,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	 * operation using rep will only have the initial GPA from the NPF
>  	 * occurred.
>  	 */
> -	if (vcpu->arch.gpa_available &&
> +	if (vcpu->arch.gpa_available && !kvmi_is_present() &&
>  	    emulator_can_use_gpa(ctxt) &&
>  	    (addr & ~PAGE_MASK) == (vcpu->arch.gpa_val & ~PAGE_MASK)) {
>  		gpa = vcpu->arch.gpa_val;
>
Paolo Bonzini Aug. 13, 2019, 2:35 p.m. UTC | #2 
On 13/08/19 16:33, Adalbert Lazăr wrote:
> On Tue, 13 Aug 2019 10:47:34 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 09/08/19 18:00, Adalbert Lazăr wrote:
>>> If the EPT violation was caused by an execute restriction imposed by the
>>> introspection tool, gpa_available will point to the instruction pointer,
>>> not the to the read/write location that has to be used to emulate the
>>> current instruction.
>>>
>>> This optimization should be disabled only when the VM is introspected,
>>> not just because the introspection subsystem is present.
>>>
>>> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
>>
>> The right thing to do is to not set gpa_available for fetch failures in 
>> kvm_mmu_page_fault instead:
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 24843cf49579..1bdca40fa831 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -5364,8 +5364,12 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
>>  	enum emulation_result er;
>>  	bool direct = vcpu->arch.mmu->direct_map;
>>  
>> -	/* With shadow page tables, fault_address contains a GVA or nGPA.  */
>> -	if (vcpu->arch.mmu->direct_map) {
>> +	/*
>> +	 * With shadow page tables, fault_address contains a GVA or nGPA.
>> +	 * On a fetch fault, fault_address contains the instruction pointer.
>> +	 */
>> +	if (vcpu->arch.mmu->direct_map &&
>> +	    likely(!(error_code & PFERR_FETCH_MASK)) {
>>  		vcpu->arch.gpa_available = true;
>>  		vcpu->arch.gpa_val = cr2;
>>  	}
>
> Sure, but I think we'll have to extend the check.
> 
> Searching the logs I've found:
> 
>     kvm/x86: re-translate broken translation that caused EPT violation
>     
>     Signed-off-by: Mircea Cirjaliu <mcirjaliu@bitdefender.com>
> 
>  arch/x86/kvm/x86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> /home/b/kvmi@9cad844~1/arch/x86/kvm/x86.c:4757,4762 - /home/b/kvmi@9cad844/arch/x86/kvm/x86.c:4757,4763
>   	 */
>   	if (vcpu->arch.gpa_available &&
>   	    emulator_can_use_gpa(ctxt) &&
> + 	    (vcpu->arch.error_code & PFERR_GUEST_FINAL_MASK) &&
>   	    (addr & ~PAGE_MASK) == (vcpu->arch.gpa_val & ~PAGE_MASK)) {
>   		gpa = vcpu->arch.gpa_val;
>   		ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> 

Yes, adding that check makes sense as well (still in kvm_mmu_page_fault).

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 965c4f0108eb..3975331230b9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5532,7 +5532,7 @@  static int emulator_read_write_onepage(unsigned long addr, void *val,
 	 * operation using rep will only have the initial GPA from the NPF
 	 * occurred.
 	 */
-	if (vcpu->arch.gpa_available &&
+	if (vcpu->arch.gpa_available && !kvmi_is_present() &&
 	    emulator_can_use_gpa(ctxt) &&
 	    (addr & ~PAGE_MASK) == (vcpu->arch.gpa_val & ~PAGE_MASK)) {
 		gpa = vcpu->arch.gpa_val;