linux-kernel - Re: [PATCH V2 3/3] KVM: X86: Reset DR6 only when KVM_DEBUGREG_WONT

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 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:   Tue, 10 Aug 2021 18:34:54 +0800
From:   Lai Jiangshan <laijs@...ux.alibaba.com>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        kvm@...r.kernel.org
Subject: Re: [PATCH V2 3/3] KVM: X86: Reset DR6 only when
 KVM_DEBUGREG_WONT_EXIT



On 2021/8/10 18:14, Paolo Bonzini wrote:
> On 09/08/21 19:43, Lai Jiangshan wrote:
>> From: Lai Jiangshan <laijs@...ux.alibaba.com>
>>
>> The commit efdab992813fb ("KVM: x86: fix escape of guest dr6 to the host")
>> fixed a bug by reseting DR6 unconditionally when the vcpu being scheduled out.
>>
>> But writing to debug registers is slow, and it can be shown in perf results
>> sometimes even neither the host nor the guest activate breakpoints.
>>
>> It'd be better to reset it conditionally and this patch moves the code of
>> reseting DR6 to the path of VM-exit and only reset it when
>> KVM_DEBUGREG_WONT_EXIT which is the only case that DR6 is guest value.
>>
>> Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
>> ---
>>   arch/x86/kvm/x86.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d2aa49722064..f40cdd7687d8 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -4309,12 +4309,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>       static_call(kvm_x86_vcpu_put)(vcpu);
>>       vcpu->arch.last_host_tsc = rdtsc();
>> -    /*
>> -     * If userspace has set any breakpoints or watchpoints, dr6 is restored
>> -     * on every vmexit, but if not, we might have a stale dr6 from the
>> -     * guest. do_debug expects dr6 to be cleared after it runs, do the same.
>> -     */
>> -    set_debugreg(0, 6);
>>   }
>>   static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
>> @@ -9630,6 +9624,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>           static_call(kvm_x86_sync_dirty_debug_regs)(vcpu);
>>           kvm_update_dr0123(vcpu);
>>           kvm_update_dr7(vcpu);
>> +        /* Reset Dr6 which is guest value. */
>> +        set_debugreg(DR6_RESERVED, 6);
>>       }
>>       /*
>>
> 
> ... and this should also be done exclusively for VMX, in vmx_sync_dirty_debug_regs:
> 
>      KVM: VMX: Reset DR6 only when KVM_DEBUGREG_WONT_EXIT
>      The commit efdab992813fb ("KVM: x86: fix escape of guest dr6 to the host")
>      fixed a bug by resetting DR6 unconditionally when the vcpu being scheduled out.
>      But writing to debug registers is slow, and it can be visible in perf results
>      sometimes, even if neither the host nor the guest activate breakpoints.
>      Since KVM_DEBUGREG_WONT_EXIT on Intel processors is the only case
>      where DR6 gets the guest value, and it never happens at all on SVM,
>      the register can be cleared in vmx.c right after reading it.
>      Reported-by: Lai Jiangshan <laijs@...ux.alibaba.com>
>      Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 21a3ef3012cf..3a91302d05c0 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5110,6 +5110,12 @@ static void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
> 
>       vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
>       exec_controls_setbit(to_vmx(vcpu), CPU_BASED_MOV_DR_EXITING);
> +
> +    /*
> +     * do_debug expects dr6 to be cleared after it runs, avoid that it sees
> +     * a stale dr6 from the guest.
> +     */


do_debug() is renamed. Maybe you can use "The host kernel #DB handler".


> +    set_debugreg(DR6_RESERVED, 6);
>   }
> 
>   static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fbc536b21585..04c393551fb0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4313,12 +4313,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> 
>       static_call(kvm_x86_vcpu_put)(vcpu);
>       vcpu->arch.last_host_tsc = rdtsc();
> -    /*
> -     * If userspace has set any breakpoints or watchpoints, dr6 is restored
> -     * on every vmexit, but if not, we might have a stale dr6 from the
> -     * guest. do_debug expects dr6 to be cleared after it runs, do the same.
> -     */
> -    set_debugreg(0, 6);
>   }
> 
>   static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
> 
> 
> Paolo