linux-kernel - Re: [PATCH V2 2/3] KVM: X86: Set the hardware 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 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]

Message-ID: <7f86316b-5010-5250-4223-5a4d62f942c8@linux.alibaba.com>
Date:   Tue, 10 Aug 2021 18:46:04 +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 2/3] KVM: X86: Set the hardware DR6 only when
 KVM_DEBUGREG_WONT_EXIT



On 2021/8/10 18:35, Paolo Bonzini wrote:
> On 10/08/21 12:30, Lai Jiangshan wrote:
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index ae8e62df16dd..21a3ef3012cf 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -6625,6 +6625,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>>           vmx->loaded_vmcs->host_state.cr4 = cr4;
>>>       }
>>>
>>> +    /* When KVM_DEBUGREG_WONT_EXIT, dr6 is accessible in guest. */
>>> +    if (vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)
>>> +        set_debugreg(vcpu->arch.dr6, 6);
>>
>>
>> I also noticed the related code in svm.c, but I refrained myself
>> to add a new branch in vmx_vcpu_run().  But after I see you put
>> the code of resetting dr6 in vmx_sync_dirty_debug_regs(), the whole
>> solution is much clean and better.
>>
>> And if any chance you are also concern about the additional branch,
>> could you add a new callback to set dr6 and call the callback from
>> x86.c when KVM_DEBUGREG_WONT_EXIT.
> 
> The extra branch should be well predicted, and the idea you sketched below would cause DR6 to be marked uselessly as 
> dirty in SVM, so I think this is cleaner.  Let's add an "unlikely" around it too.

I'm OK with it. But I don't think the sketched idea would cause DR6 to be marked uselessly as dirty in SVM. It doesn't 
mark it dirty if the value is unchanged, and the value is always DR6_ACTIVE_LOW except when it just clears 
KVM_DEBUGREG_WONT_EXIT.

> 
> Paolo
> 
>> The possible implementation of the callback:
>> for vmx: set_debugreg(vcpu->arch.dr6, 6);
>> for svm: svm_set_dr6(svm, vcpu->arch.dr6);
>>           and always do svm_set_dr6(svm, DR6_ACTIVE_LOW); at the end of the
>>           svm_handle_exit().
>>
>> Thanks
>> Lai
>>
>>> +
>>>       /* When single-stepping over STI and MOV SS, we must clear the
>>>        * corresponding interruptibility bits in the guest state. Otherwise
>>>        * vmentry fails as it then expects bit 14 (BS) in pending debug
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index a111899ab2b4..fbc536b21585 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -9597,7 +9597,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>           set_debugreg(vcpu->arch.eff_db[1], 1);
>>>           set_debugreg(vcpu->arch.eff_db[2], 2);
>>>           set_debugreg(vcpu->arch.eff_db[3], 3);
>>> -        set_debugreg(vcpu->arch.dr6, 6);
>>>       } else if (unlikely(hw_breakpoint_active())) {
>>>           set_debugreg(0, 7);
>>>       }
>>>
>>> Paolo
>>