Messages in this thread | | | Subject | Re: [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range | From | Xiaoyao Li <> | Date | Tue, 24 Aug 2021 23:42:44 +0800 |
| |
On 8/24/2021 11:24 PM, Sean Christopherson wrote: > On Tue, Aug 24, 2021, Xiaoyao Li wrote: >> The number of guest's valid PT ADDR MSRs is cached in > > Can you do s/cached/precomputed in the shortlog and changelog? Explanation below.
OK.
>> vmx->pt_desc.addr_range. Use it instead of calculating it again. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index e0a9460e4dab..7ed96c460661 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -2202,8 +2202,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> if (!pt_can_write_msr(vmx)) >> return 1; >> index = msr_info->index - MSR_IA32_RTIT_ADDR0_A; >> - if (index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps, >> - PT_CAP_num_address_ranges)) >> + if (index >= 2 * vmx->pt_desc.addr_range) > > Ugh, "validate" is a lie, a better name would be intel_pt_get_cap() or so. There > is no validation, the helper is simply extracting the requested cap from the > passed in array of capabilities. > > That matters in this case because the number of address ranges exposed to the > guest is not bounded by the number of address ranges present in hardware, i.e. > it's not "validated". And that matters because KVM uses vmx->pt_desc.addr_range > to pass through the ADDRn_{A,B} MSRs when tracing enabled. In other words, > userspace can expose MSRs to the guest that do not exist.
That's why I provided patch 5.
> The bug shouldn't be a security issue, so long as Intel CPUs are bug free and > aren't doing silly things with MSR indexes. The number of possible address ranges > is encoded in three bits, thus the theoretical max is 8 ranges. So userspace can't > get access to arbitrary MSRs, just ADDR0_A -> ADDR7_B. > > And since KVM would be modifying the "validated" value, it's more than just a > cache, hence the request to use "precomputed". > > Finally, vmx_get_msr() should use the precomputed value as well.
Argh, I missed it.
> P.S. If you want to introduce a bit of churn, s/addr_range/nr_addr_ranges would > be a welcome change as well.
In a separate patch?
>> return 1; >> if (is_noncanonical_address(data, vcpu)) >> return 1; >> -- >> 2.27.0 >>
| |