Re: [PATCH 6/7] KVM: SVM: Add LBR/PMC freeze support

On 25-07-26 01:43, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] KVM incorrectly advertises AMD_LBR_PMC_FREEZE to the guest even when prerequisite features (mediated PMU or LBR virtualization) are disabled, leading to broken PMU state.
> - [High] An L1 hypervisor can bypass CPUID capability checks and enable LBR/PMC freeze bits for L2 by injecting them via VMCB12's DEBUGCTL.
> --
> 
> --- Patch [6]: [PATCH 6/7] KVM: SVM: Add LBR/PMC freeze support ---
> commit 8a4ef61c23eb3f96d674c17183bad9ad198a18dd
> Author: Shivansh Dhiman <[email protected]>
> 
> KVM: SVM: Add LBR/PMC freeze support
> 
> This commit adds support for the LBR and PMC freeze-on-PMI feature.
> It updates CPUID initialization and allows the guest to set the
> corresponding bits in DEBUGCTL if the feature is advertised.
> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index d450a5b2499d9..03501b3145eb0 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1268,6 +1268,7 @@ void kvm_initialize_cpu_caps(void)
>>  	kvm_cpu_cap_init(CPUID_8000_0022_EAX,
>>  		F(PERFMON_V2),
>>  		SCATTERED_F(AMD_LBR_V2),
>> +		SCATTERED_F(AMD_LBR_PMC_FREEZE),
>>  	);
> 
> [Severity: High]
> Will this feature remain advertised to the guest even when prerequisite
> features like mediated PMU or LBR virtualization are disabled?
> 
> Looking at svm_hardware_setup() in arch/x86/kvm/svm/svm.c,
> X86_FEATURE_AMD_LBR_V2 is dynamically cleared when prerequisites are missing:
> 
> if (!lbrv || !enable_mediated_pmu ||
>     !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2))
>         kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);
> 
> If X86_FEATURE_AMD_LBR_PMC_FREEZE is not cleared similarly, a guest might
> see the feature in CPUID and attempt to use it. If the host administrator
> configured KVM with lbrv=0, svm_set_msr() silently ignores writes to
> MSR_IA32_DEBUGCTLMSR without injecting a #GP, which could break guest PMU
> functionality.

Right, I'll add this in v2.

> 
> [ ... ]
> 
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index 01dae3828a7aa..105a71d69d68a 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -784,7 +784,9 @@ BUILD_SVM_MSR_BITMAP_HELPERS(bool, test, test)
>>  BUILD_SVM_MSR_BITMAP_HELPERS(void, clear, __clear)
>>  BUILD_SVM_MSR_BITMAP_HELPERS(void, set, __set)
>>  
>> -#define DEBUGCTL_RESERVED_BITS (~DEBUGCTLMSR_LBR)
>> +#define DEBUGCTL_LBR_PMC_FREEZE_BITS	(DEBUGCTLMSR_FREEZE_LBRS_ON_PMI |	\
>> +					 DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI)
>> +#define DEBUGCTL_RESERVED_BITS	(~(DEBUGCTLMSR_LBR | DEBUGCTL_LBR_PMC_FREEZE_BITS))
> 
> [Severity: High]
> Does modifying this macro globally allow an L1 hypervisor to bypass CPUID
> capability checks for its L2 guest?
> 
> In arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save(), VMCB12's dbgctl
> is masked directly against DEBUGCTL_RESERVED_BITS:
> 
> vmcb02->save.dbgctl &= ~DEBUGCTL_RESERVED_BITS;
> 
> By unreserving the freeze bits globally here, an unprivileged L1 hypervisor
> with LBRV enabled could write DEBUGCTL_LBR_PMC_FREEZE_BITS into its
> vmcb12->save.dbgctl. Since KVM copies this to L2's VMCB without checking if
> L1 actually possesses the X86_FEATURE_AMD_LBR_PMC_FREEZE capability, this
> appears to allow a nested capability bypass.

Will add this as part of the nested implementation of LBRv2.

Thanks.