Re: [PATCH v2 10/12] KVM: nSVM: Pass through LBRv2 MSRs to L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: nSVM: missing msrpm02 recalc in svm_update_lbrv() for LBRv2 MSRs
--

commit 97c65be26df9b6afb7c41700b0bfb2f3401bc714
Author: Shivansh Dhiman <shivansh.dhiman@amd.com>

KVM: nSVM: Pass through LBRv2 MSRs to L2

This commit adds the LBRv2 MSRs to merge_msrs[] in
nested_svm_init_msrpm_merge_offsets() so their MSRPM bits are merged into
msrpm02, allowing L2 to access them directly without taking a VM-Exit.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index b356a091e320c..84154f353935c 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -209,7 +209,7 @@ void nested_vmcb02_recalc_intercepts(struct vcpu_svm *svm)
>   * Hardcode the capacity of the array based on the maximum number of _offsets_.
>   * MSRs are batched together, so there are fewer offsets than MSRs.
>   */
> -static int nested_svm_msrpm_merge_offsets[10] __ro_after_init;
> +static int nested_svm_msrpm_merge_offsets[12] __ro_after_init;
>  static int nested_svm_nr_msrpm_merge_offsets __ro_after_init;
>  typedef unsigned long nsvm_msrpm_merge_t;
>  
> @@ -253,6 +253,27 @@ int __init nested_svm_init_msrpm_merge_offsets(void)
>  		MSR_AMD64_PERF_CNTR_GLOBAL_STATUS,
>  		MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR,
>  		MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_SET,
> +
> +		MSR_AMD_DBG_EXTN_CFG,
> +		MSR_AMD64_LBR_SELECT,
> +
> +		/* This part of MSRs should match SVM_LBR_V2_STACK_SIZE * 2. */
> +		MSR_AMD_SAMP_BR_FROM + 0, MSR_AMD_SAMP_BR_FROM + 1,
> +		MSR_AMD_SAMP_BR_FROM + 2, MSR_AMD_SAMP_BR_FROM + 3,
> +		MSR_AMD_SAMP_BR_FROM + 4, MSR_AMD_SAMP_BR_FROM + 5,
> +		MSR_AMD_SAMP_BR_FROM + 6, MSR_AMD_SAMP_BR_FROM + 7,
> +		MSR_AMD_SAMP_BR_FROM + 8, MSR_AMD_SAMP_BR_FROM + 9,
> +		MSR_AMD_SAMP_BR_FROM + 10, MSR_AMD_SAMP_BR_FROM + 11,
> +		MSR_AMD_SAMP_BR_FROM + 12, MSR_AMD_SAMP_BR_FROM + 13,
> +		MSR_AMD_SAMP_BR_FROM + 14, MSR_AMD_SAMP_BR_FROM + 15,
> +		MSR_AMD_SAMP_BR_FROM + 16, MSR_AMD_SAMP_BR_FROM + 17,
> +		MSR_AMD_SAMP_BR_FROM + 18, MSR_AMD_SAMP_BR_FROM + 19,
> +		MSR_AMD_SAMP_BR_FROM + 20, MSR_AMD_SAMP_BR_FROM + 21,
> +		MSR_AMD_SAMP_BR_FROM + 22, MSR_AMD_SAMP_BR_FROM + 23,
> +		MSR_AMD_SAMP_BR_FROM + 24, MSR_AMD_SAMP_BR_FROM + 25,
> +		MSR_AMD_SAMP_BR_FROM + 26, MSR_AMD_SAMP_BR_FROM + 27,
> +		MSR_AMD_SAMP_BR_FROM + 28, MSR_AMD_SAMP_BR_FROM + 29,
> +		MSR_AMD_SAMP_BR_FROM + 30, MSR_AMD_SAMP_BR_FROM + 31,

[Severity: High]
Can this leave stale LBRv2 intercept bits in msrpm02 when L2 toggles
DBG_EXTN_CFG_LBRV2EN without exiting to L1, exposing the host's hardware
LBRv2 registers to L2?

Suppose L1 runs L2 with INTERCEPT_MSR_PROT enabled in vmcb12,
SVM_MISC2_ENABLE_V_LBR disabled in vmcb12->control.misc_ctl2, and the LBRv2
MSRs un-intercepted in vmcb12's MSRPM, while DBG_EXTN_CFG_LBRV2EN is set in
dbg_extn_cfg at nested VMRUN.

During nested_svm_vmrun(), nested_vmcb02_prepare_save() calls
svm_update_lbrv(), which sees svm_lbrv2_active() as true, sets
SVM_MISC2_ENABLE_V_LBR in vmcb02->control.misc_ctl2, and clears the
intercept bits for MSR_AMD64_LBR_SELECT and MSR_AMD_SAMP_BR_FROM + 0..31 in
svm->msrpm (msrpm01) via svm_recalc_lbrv2_msr_intercepts(). Then
nested_svm_merge_msrpm() merges msrpm01 and vmcb12's MSRPM into
svm->nested.msrpm (msrpm02), clearing those intercept bits in msrpm02.

When L2 subsequently writes 0 to MSR_AMD_DBG_EXTN_CFG (which remains
intercepted in msrpm01 for non-SEV-ES guests), hardware exits to L0 and
restores L0's host LBRv2 registers. Because L1 does not intercept
MSR_AMD_DBG_EXTN_CFG in vmcb12, nested_svm_exit_handled_msr() returns
NESTED_EXIT_HOST and L0 handles the write in svm_set_msr():

arch/x86/kvm/svm/svm.c:svm_set_msr() {
	...
	case MSR_AMD_DBG_EXTN_CFG:
		...
		svm->vmcb->save.dbg_extn_cfg = data;
		svm_update_lbrv(vcpu);
		break;
}

svm_update_lbrv() disables LBR virtualization in vmcb02 (clearing
SVM_MISC2_ENABLE_V_LBR) and calls svm_recalc_lbrv2_msr_intercepts():

arch/x86/kvm/svm/svm.c:svm_recalc_lbrv2_msr_intercepts() {
	...
	for (i = 0; i < 32; i++)
		svm_set_intercept_for_msr(vcpu, MSR_AMD_SAMP_BR_FROM + i, MSR_TYPE_RW, intercept);

	svm_set_intercept_for_msr(vcpu, MSR_AMD64_LBR_SELECT, MSR_TYPE_RW, intercept);
}

This re-enables the intercepts in svm->msrpm (msrpm01) and sets
svm->nested.force_msr_bitmap_recalc = true, but nested_svm_merge_msrpm() is
only invoked during nested_svm_vmrun() and svm_get_nested_state_pages()
(via KVM_REQ_GET_NESTED_STATE_PAGES).

Because msrpm01 is not re-merged into msrpm02 before re-entering L2, L0
resumes L2 with SVM_MISC2_ENABLE_V_LBR cleared in vmcb02 while msrpm02 still
has the intercept bits cleared for MSR_AMD64_LBR_SELECT and
MSR_AMD_SAMP_BR_FROM + 0..31, allowing L2 to directly read and write the L0
host's hardware LBRv2 MSRs.

Conversely, if L1 enters L2 with dbg_extn_cfg == 0 and L2 later sets
DBG_EXTN_CFG_LBRV2EN without a nested exit to L1, clearing the intercepts in
msrpm01 is not propagated to msrpm02 until the next nested #VMEXIT and VMRUN.

Should svm_recalc_lbrv2_msr_intercepts() (or svm_set_intercept_for_msr() when
is_guest_mode(vcpu) is true) ensure msrpm02 is updated before re-entering L2?

>  	};
>  	int i, j;
>

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