Re: [PATCH 3/7] KVM: SVM: Emulate guest accesses to LBR v2 MSRs

On 25-07-26 01:43, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] LBRv2 emulation ignores the `lbrv` module parameter, allowing guest LBRv2 usage even when LBR Virtualization is globally disabled.
> - [High] The newly added LBR v2 MSRs are missing from KVM's list of saveable MSRs, causing them to be silently dropped during live migration.
> - [High] Missing nested SVM state synchronization for LBRv2 fields causes state corruption and cross-tenant information leaks between L2 guests.
> --
> 
> --- Patch [3]: [PATCH 3/7] KVM: SVM: Emulate guest accesses to LBR v2 MSRs ---
> commit ea17366da9bcd5ccfa5c4c9307a6f68cda7d1255
> Author: Shivansh Dhiman <[email protected]>
> 
> KVM: SVM: Emulate guest accesses to LBR v2 MSRs
>     
> Back the LBR v2 MSRs with their VMCB save-area fields in
> svm_{get,set}_msr(). This is required when guest accesses to these
> MSRs are intercepted. This allows the guest to retain the values
> of these MSRs on VMRUN/#VMEXIT cycle when vLBR is disabled.
> 
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index ef69a51ab27f9..13ac62604ca05 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
> 
> [ ... ]
> 
>> @@ -744,6 +744,12 @@ static void svm_recalc_lbr_msr_intercepts(struct kvm_vcpu *vcpu)
>>  	svm->lbr_msrs_intercepted = intercept;
>>  }
>>  
>> +static bool svm_lbrv2_supported(struct kvm_vcpu *vcpu)
>> +{
>> +	return guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_LBR_V2) &&
>> +	       kvm_vcpu_has_mediated_pmu(vcpu);
>> +}
> 
> [Severity: High]
> Does this capability check need to respect the global lbrv module parameter?
> 
> Unlike the legacy LBR logic which explicitly checks lbrv, this function only
> checks the X86_FEATURE_AMD_LBR_V2 capability. 
> 
> Could this allow a local guest to bypass the host administrator's explicit
> disablement of LBR Virtualization (when loaded with lbrv=0) by directly
> accessing LBRv2 MSRs?

The lbrv check is not needed here because that capability is already gated
on lbrv in svm_set_cpu_caps(), and the guest cannot have LBRv2 if kvm
capability is clear.

	if (!lbrv || !enable_mediated_pmu ||
	    !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2))
		kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);


> 
> [ ... ]
> 
>> @@ -2882,6 +2901,21 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  	case MSR_IA32_LASTINTTOIP:
>>  		msr_info->data = lbrv ? *svm_vmcb_lbr(svm, msr_info->index) : 0;
>>  		break;
>> +	case MSR_AMD_DBG_EXTN_CFG:
>> +		if (!svm_lbrv2_supported(vcpu))
>> +			return KVM_MSR_RET_UNSUPPORTED;
>> +		msr_info->data = svm->vmcb->save.dbg_extn_cfg;
>> +		break;
> 
> [Severity: High]
> Are these new LBR v2 MSRs missing from KVM's list of saveable MSRs?
> 
> Because the new LBRv2 MSRs (MSR_AMD_DBG_EXTN_CFG, MSR_AMD64_LBR_SELECT, and
> the MSR_AMD_SAMP_BR_FROM array) are omitted from msrs_to_save_base in
> arch/x86/kvm/msrs.c, they won't be returned to the VMM by
> KVM_GET_MSR_INDEX_LIST.
> 
> Will this cause the LBRv2 state to be silently dropped during live migration?

Good catch. I missed adding the MSRs here. Will do that in v2.

> 
> [ ... ]
> 
>> @@ -3181,6 +3215,40 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  		*svm_vmcb_lbr(svm, ecx) = data;
>>  		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
>>  		break;
>> +	case MSR_AMD_DBG_EXTN_CFG:
>> +		if (!svm_lbrv2_supported(vcpu))
>> +			return KVM_MSR_RET_UNSUPPORTED;
>> +
>> +		if (data & DBG_EXTN_CFG_RESERVED_BITS)
>> +			return 1;
>> +
>> +		if (svm->vmcb->save.dbg_extn_cfg == data)
>> +			break;
>> +
>> +		svm->vmcb->save.dbg_extn_cfg = data;
>> +		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
>> +		svm_update_lbrv(vcpu);
>> +		break;
> 
> [Severity: High]
> Does nested SVM state synchronization need to be updated for these new LBRv2
> fields?
> 
> Looking at nested virtualization, L2 guest state is synchronized using
> svm_copy_lbrs() in arch/x86/kvm/svm/svm.h, which is called during
> nested_vmcb02_prepare_save().
> 
> Because svm_copy_lbrs() was not updated to copy the new dbg_extn_cfg,
> lbr_select, and lbr array fields, vmcb02 can retain a previous L2 guest's
> branch records. When a new L2 guest reads its MSRs, it could observe the prior
> L2 guest's execution trace.
> 
> Additionally, upon nested_svm_vmexit(), wouldn't L2 modifications be dropped
> because they aren't copied back to L1's vmcb12?

Nested support isn't part of v1. Will implement it in upcoming versions.