Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 24 Aug 2021 15:24:26 +0000
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
Message-ID: <YSUPKmtP6Dcl1yio@google.com>
References: <20210824110743.531127-1-xiaoyao.li@intel.com>
 <20210824110743.531127-3-xiaoyao.li@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210824110743.531127-3-xiaoyao.li@intel.com>
Precedence: bulk

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
>