Re: [PATCH v3 3/5] KVM: nSVM: Sanitize nested DR6 using kvm_dr6_fixed()

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

On 09-07-26 14:21, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The patch identifies that DR6_ACTIVE_LOW unconditionally sets DR6_BUS_LOCK (bit 11) to 1, and replaces it with kvm_dr6_fixed() in nested VMCB02 preparation. However, the patch fails to replace DR6_ACTIVE_LOW in L1 vCPU initialization and #DB payload processing paths, completely breaking Bus Lock Detect for L1 guests.
> --
> 
> --- Patch [3]: [PATCH v3 3/5] KVM: nSVM: Sanitize nested DR6 using kvm_dr6_fixed() ---
> commit 0ee3ae36939ed695ccbd636580058ad35113add9
> Author: Shivansh Dhiman <shivansh.dhiman@amd.com>
> 
> KVM: nSVM: Sanitize nested DR6 using kvm_dr6_fixed()
> 
> When preparing vmcb02 for nested VMRUN, KVM ORs DR6_ACTIVE_LOW into the
> guest DR6 to force the fixed bits to 1. DR6_ACTIVE_LOW forces bit 11
> (DR6_BUS_LOCK) to 1 unconditionally.
> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 84248e6665cfe..0a9e28a02692e 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -811,7 +811,7 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm)
>>
>>       if (unlikely(new_vmcb12 || vmcb12_is_dirty(control, VMCB_DR))) {
>>               vmcb02->save.dr7 = svm->nested.save.dr7 | DR7_FIXED_1;
>> -             svm->vcpu.arch.dr6  = svm->nested.save.dr6 | DR6_ACTIVE_LOW;
>> +             svm->vcpu.arch.dr6  = svm->nested.save.dr6 | kvm_dr6_fixed(vcpu);
> 
> [Severity: High]
> This is a pre-existing issue, but since this patch correctly identifies that
> DR6_ACTIVE_LOW unconditionally forces bit 11 (DR6_BUS_LOCK) to 1, should
> DR6_ACTIVE_LOW also be replaced in the L1 vCPU initialization and #DB payload
> processing paths?
> 
> Currently, DR6_ACTIVE_LOW is still used to initialize L1's dr6:
> 
> arch/x86/kvm/x86.c:kvm_vcpu_reset() {
>     ...
>     vcpu->arch.dr6 = DR6_ACTIVE_LOW;
>     ...
> }
> 
> It is also used as a flip mask when KVM translates the exception payload to
> guest DR6. Because Bus Lock Detect is actually active-high, XORing it with

This looks like a false positive issue, since Bus Lock Detect is active-low.
Meaning if DR6.BLD=0, then it indicates that a Bus Lock has been triggered.
See APM 24593—Rev. 3.43 13.1.1.3:

	Bus Lock Detected (BLD)—Bit 11. The processor clears this bit if
	#DB was generated due to a bus lock.

> DR6_ACTIVE_LOW incorrectly inverts the bus lock bit the guest sees:
> 
> arch/x86/kvm/x86.c:kvm_deliver_exception_payload() {
>     ...
>     vcpu->arch.dr6 ^= ex->payload & DR6_ACTIVE_LOW;
>     ...
> }
> 
> Similarly, SVM constructs the #DB payload by XORing the hardware exit DR6
> with DR6_ACTIVE_LOW:
> 
> arch/x86/kvm/svm/svm.c:db_interception() {
>     ...
>     u32 payload = svm->vmcb->save.dr6 ^ DR6_ACTIVE_LOW;
>     ...
> }
> 
> Could this leave Bus Lock Detect functionally broken for L1 guests by causing
> them to observe corrupted hardware state (false positives or false negatives
> for bus locks)?
> 
>>               vmcb_mark_dirty(vmcb02, VMCB_DR);
>>       }
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709082953.69434-1-shivansh.dhiman@amd.com?part=3