LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range
Date: Tue, 24 Aug 2021 15:24:26 +0000	[thread overview]
Message-ID: <YSUPKmtP6Dcl1yio@google.com> (raw)
In-Reply-To: <20210824110743.531127-3-xiaoyao.li@intel.com>

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.

> 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.

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.

P.S. If you want to introduce a bit of churn, s/addr_range/nr_addr_ranges would
     be a welcome change as well.

>  			return 1;
>  		if (is_noncanonical_address(data, vcpu))
>  			return 1;
> -- 
> 2.27.0
> 

  reply	other threads:[~2021-08-24 15:24 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 11:07 [PATCH 0/5] KVM: VMX: PT (processor trace) optimizations and fixes Xiaoyao Li
2021-08-24 11:07 ` [PATCH 1/5] KVM: VMX: Restore host's MSR_IA32_RTIT_CTL when it's not zero Xiaoyao Li
2021-08-24 17:54   ` Sean Christopherson
2021-08-24 11:07 ` [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range Xiaoyao Li
2021-08-24 15:24   ` Sean Christopherson [this message]
2021-08-24 15:42     ` Xiaoyao Li
2021-08-24 11:07 ` [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit Xiaoyao Li
2021-08-25  3:30   ` Like Xu
2021-08-25  4:19     ` Xiaoyao Li
2021-08-25  6:08       ` Like Xu
2021-08-25  6:33         ` Xiaoyao Li
2021-08-25  8:14           ` Like Xu
2021-08-25  8:58             ` Xiaoyao Li
2021-08-25 11:53           ` Alexander Shishkin
2021-08-24 11:07 ` [PATCH 4/5] KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest Xiaoyao Li
2021-08-24 14:20   ` Sean Christopherson
2021-08-24 15:35     ` Xiaoyao Li
2021-08-24 16:48       ` Sean Christopherson
2021-08-24 11:07 ` [PATCH 5/5] KVM: VMX: Check Intel PT related CPUID leaves Xiaoyao Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YSUPKmtP6Dcl1yio@google.com \
    --to=seanjc@google.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=xiaoyao.li@intel.com \
    --subject='Re: [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).