Re: [PATCH 7/8] KVM: nSVM: Add support for hardware-switched PMU

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] L1 hypervisor can bypass KVM's PMU event filter for L2 guests.
- [High] KVM's software PMU state is not context-switched between L1 and L2, allowing L2 MSR writes to permanently corrupt L1's PMU state and leak state.
- [Medium] KVM silently breaks PMU functionality for L1 and L2 if L1 enables `V_PMC` but L0 does not use hardware-switched mediated PMU.
--

--- Patch [7]: [PATCH 7/8] KVM: nSVM: Add support for hardware-switched PMU ---
commit 93d914b77cd285073e93f03a803ccbca4c41f03f
Author: Sandipan Das <[email protected]>

KVM: nSVM: Add support for hardware-switched PMU

This commit adds nested support for PMC virtualization. It advertises
X86_FEATURE_PERFCTR_VIRT to allow an L1 hypervisor to use it for L2 guests,
switching PMU state using vmcb02's save area.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 3e6c671a8dc26..666f8ca38a928 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
[ ... ]
> @@ -741,6 +742,17 @@ static bool nested_vmcb12_has_lbrv(struct kvm_vcpu *vcpu)
>  		(to_svm(vcpu)->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR);
>  }
>  
> +static bool nested_vmcb12_has_vpmc(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * Since nested AVIC is not supported, L2 PMIs can only be delivered
> +	 * via VNMI, so make it a hard requirement.
> +	 */
> +	return guest_cpu_cap_has(vcpu, X86_FEATURE_PERFCTR_VIRT) &&
> +		nested_vnmi_enabled(to_svm(vcpu)) &&
> +		(to_svm(vcpu)->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_PMC);
> +}

[Severity: Medium]
Will this break PMU functionality if L0 does not use hardware-switched
mediated PMU?

If nested_vmcb12_has_vpmc() returns true because the guest CPUID supports
PERFCTR_VIRT and V_PMC is set in vmcb12, KVM skips the software PMU context
switch in enter_svm_guest_mode() and nested_svm_vmexit().

However, if KVM uses the legacy software PMU
(KVM_MEDIATED_PMU_CAP_HW_SWITCHED is not set), SVM_MISC2_ENABLE_V_PMC is
never set in vmcb02->control.misc_ctl2. Does this result in neither hardware
nor software PMU context switching occurring for both L1 and L2?

[ ... ]
> @@ -1065,6 +1085,9 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa, bool from_vmrun)
>  	nested_vmcb02_prepare_control(svm);
>  	nested_vmcb02_prepare_save(svm);
>  
> +	if (!nested_vmcb12_has_vpmc(vcpu))
> +		svm_pmu_handle_nested_transition(svm);
> +

[Severity: Critical]
By skipping svm_pmu_handle_nested_transition() here, are we omitting the PMU
event filter validation for L2?

The PMU configuration is copied directly from L1's cache into vmcb02 earlier
in nested_vmcb02_prepare_save(). Without calling the transition handler,
kvm_mediated_pmu_refresh_event_filter() is bypassed. Could this allow L1 to
count restricted host events by placing forbidden selectors into vmcb12,
since hardware will load these unvalidated perf_ctl values on VMRUN?

[ ... ]
> @@ -1329,7 +1358,9 @@ void nested_svm_vmexit(struct vcpu_svm *svm)
>  
>  	/* Exit Guest-Mode */
>  	leave_guest_mode(vcpu);
> -	svm_pmu_handle_nested_transition(svm);
> +
> +	if (!nested_vmcb12_has_vpmc(vcpu))
> +		svm_pmu_handle_nested_transition(svm);
>  
>  	svm->nested.vmcb12_gpa = 0;

[Severity: High]
Since svm_pmu_handle_nested_transition() is skipped here, does KVM fail to
restore L1's software PMU state on nested exit?

Because EVNTSEL MSRs are deliberately excluded from MSR merging, L2 writes
to EVNTSEL MSRs will cause an L0 intercept. This intercept updates KVM's
shared software PMU state. If we do not context switch this software state
back to L1's values upon exiting L2, could L2 MSR writes permanently corrupt
L1's PMU tracking state and leak information?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7