Re: [PATCH v3 5/5] KVM: SVM: Add Bus Lock Detect support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unprivileged host userspace can trigger a KVM_BUG_ON (resulting in a WARN_ON_ONCE and potential host panic) by dynamically toggling MSR_IA32_DEBUGCTLMSR for a SEV-ES guest.
- [High] Enabling Bus Lock Detect inadvertently un-intercepts LBR MSRs, granting the guest unauthorized hardware access to LBRs.

Pre-existing issues:
- [High] L2 guest can bypass MSR intercepts and overwrite L1's MSR_IA32_DEBUGCTLMSR and LBR state when L1 enables Bus Lock Detect or LBR.
- [High] Bus Lock Trap #DB causes are lost during reinjection, rendering the feature non-functional in the guest.
--

--- Patch [5]: [PATCH v3 5/5] KVM: SVM: Add Bus Lock Detect support ---
commit d2f71feda9dbdbb51e59bc110b5518d4f10bcf93
Author: Shivansh Dhiman <shivansh.dhiman@amd.com>

KVM: SVM: Add Bus Lock Detect support

Add Bus Lock Detect support in AMD SVM. Bus Lock Detect is enabled through
MSR_IA32_DEBUGCTLMSR and MSR_IA32_DEBUGCTLMSR is virtualized only if LBR
Virtualization is enabled. Add this dependency in the SVM.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a59389c322da1..bd204aa33e47f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -882,7 +882,8 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
>  	bool current_enable_lbrv = svm->vmcb->control.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR;
>  	bool enable_lbrv = false;
>  
> -	if (svm->vmcb->save.dbgctl & DEBUGCTLMSR_LBR)
> +	/* Bus Lock Detect in guest depends on LBR Virtualization */
> +	if (svm->vmcb->save.dbgctl & (DEBUGCTLMSR_LBR | DEBUGCTLMSR_BUS_LOCK_DETECT))
>  		enable_lbrv = true;

[Severity: High]
Could this change allow unprivileged host userspace to trigger a kernel panic
on systems with panic_on_warn=1?

If userspace calls KVM_SET_MSRS for a SEV-ES vCPU before the guest is
finalized and sets MSR_IA32_DEBUGCTLMSR with DEBUGCTLMSR_LBR or
DEBUGCTLMSR_BUS_LOCK_DETECT, svm_update_lbrv() will set enable_lbrv = true.

If userspace subsequently writes 0 to clear the bit, enable_lbrv becomes
false, causing KVM to call __svm_disable_lbrv().

This would then hit the following assertion:

arch/x86/kvm/svm/svm.c:__svm_disable_lbrv() {
    ...
    KVM_BUG_ON(is_sev_es_guest(vcpu), vcpu->kvm);
    ...
}


[Severity: High]
Does enabling Bus Lock Detect inadvertently grant the guest unauthorized
hardware access to LBRs?

When a guest with Bus Lock Detect sets DEBUGCTLMSR_BUS_LOCK_DETECT,
enable_lbrv becomes true. This leads to a call to
svm_recalc_lbr_msr_intercepts():

arch/x86/kvm/svm/svm.c:svm_recalc_lbr_msr_intercepts() {
    ...
    bool intercept = !(svm->vmcb->control.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR);
    ...
}

Because intercepts are disabled entirely based on SVM_MISC2_ENABLE_V_LBR,
could this un-intercept all LBR MSRs (e.g., MSR_IA32_LASTBRANCHFROMIP) even
if the guest does not possess the LBRV CPUID feature?


[Severity: High]
This is a pre-existing issue, but can an L2 guest bypass MSR intercepts and
overwrite L1's state if L1 enables Bus Lock Detect or LBRs?

If L1 enables these features but does not expose LBRV to L2, KVM populates
vmcb02->save.dbgctl with L1's state. When svm_update_lbrv() sees these bits,
it appears to activate hardware LBRV (SVM_MISC2_ENABLE_V_LBR) for L2.

This would un-intercept MSRs for L2, allowing L2 to modify DEBUGCTLMSR
natively. Upon nested VMEXIT, KVM copies this modified state back to L1:

arch/x86/kvm/svm/nested.c:nested_svm_vmexit() {
    ...
    if (!nested_vmcb12_has_lbrv(vcpu)) {
        svm_copy_lbrs(&vmcb01->save, &vmcb02->save);
        vmcb_mark_dirty(vmcb01, VMCB_LBR);
    }
    ...
}

Would this allow a malicious L2 guest to corrupt L1's state?

[ ... ]

> @@ -5595,9 +5596,17 @@ static __init void svm_set_cpu_caps(void)
>  	 * Clear capabilities that are automatically configured by common code,
>  	 * but that require explicit SVM support (that isn't yet implemented).
>  	 */
> -	kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT);
>  	kvm_cpu_cap_clear(X86_FEATURE_MSR_IMM);
>  
> +	/*
> +	 * LBR Virtualization must be enabled to support BusLockTrap inside the
> +	 * guest, since BusLockTrap is enabled through MSR_IA32_DEBUGCTLMSR and
> +	 * MSR_IA32_DEBUGCTLMSR is virtualized only if LBR Virtualization is
> +	 * enabled.
> +	 */
> +	if (!lbrv)
> +		kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT);

[Severity: High]
This isn't a bug introduced by this patch, but are Bus Lock Trap exceptions
actually reaching the guest?

According to the AMD APM, when a #DB is intercepted, the CPU provides the
new DR6 value (containing the DR6_BUS_LOCK bit) in EXITINFO1 and does not
update the guest's DR6.

However, when KVM reinjects the exception, it reads the stale save.dr6:

arch/x86/kvm/svm/svm.c:db_interception() {
    ...
    u32 payload = svm->vmcb->save.dr6 ^ DR6_ACTIVE_LOW;
    kvm_queue_exception_p(vcpu, DB_VECTOR, payload);
    ...
}

Does this cause the DR6_BUS_LOCK bit to be lost during reinjection, rendering
the feature non-functional in the guest?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709082953.69434-1-shivansh.dhiman@amd.com?part=5