From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Yang, Sheng" <sheng.yang@intel.com>
Subject: Re: KVM: x86: verify MTRR/PAT validity
Date: Thu, 18 Jun 2009 10:39:32 +0800
Message-ID: <200906181039.33205.sheng.yang@intel.com>
References: <20090616120529.GA529@amt.cnet>
Mime-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, kvm <kvm@vger.kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga03.intel.com ([143.182.124.21]:54277 "EHLO mga03.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751291AbZFRCjt (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 17 Jun 2009 22:39:49 -0400
In-Reply-To: <20090616120529.GA529@amt.cnet>
Content-Disposition: inline
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tuesday 16 June 2009 20:05:29 Marcelo Tosatti wrote:
> Do not allow invalid MTRR/PAT values in set_msr_mtrr.
>
> Please review carefully.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
Looks fine to me.

Is it necessary to check reserved bit of MSR_MTRRdefType and variable MTRRs as 
well? Maybe like this:

if (msr == MSR_MTRRdefType) {
	return valid_mtrr_type(data & ~0xc00ull);
}

And variable ones can be:

#define MTRR_VALID_MASK(v, msr) (~(rsvd_bits(cpuid_max_physaddr(v)) | ((msr % 
2) << 11)))

return valid_mtrr_type(data & MTRR_VALID_MASK(vcpu, msr)))


(rsvd_bits() is in mmu.c, both untested)

Maybe we can put cpuid_max_physaddr as a field in vcpu struct?

-- 
regards
Yang, Sheng

>
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -722,11 +722,53 @@ static bool msr_mtrr_valid(unsigned msr)
>  	return false;
>  }
>
> +static unsigned mtrr_types[] = {0, 1, 4, 5, 6};
> +static unsigned pat_types[] = {0, 1, 4, 5, 6, 7};
> +
> +static bool valid_mt(unsigned type, int len, unsigned array[len])
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		if (type == array[i])
> +			return true;
> +
> +	return false;
> +}
> +
> +#define valid_pat_type(a) valid_mt(a, ARRAY_SIZE(pat_types), pat_types)
> +#define valid_mtrr_type(a) valid_mt(a, ARRAY_SIZE(mtrr_types), mtrr_types)
> +
> +static bool mtrr_valid(u32 msr, u64 data)
> +{
> +	int i;
> +
> +	if (!msr_mtrr_valid(msr))
> +		return false;
> +
> +	if (msr == MSR_IA32_CR_PAT) {
> +		for (i = 0; i < 8; i++)
> +			if (!valid_pat_type((data >> (i * 8)) & 0xff))
> +				return false;
> +		return true;
> +	} else if (msr == MSR_MTRRdefType) {
> +		return valid_mtrr_type(data & 0xff);
> +	} else if (msr >= MSR_MTRRfix64K_00000 && msr <= MSR_MTRRfix4K_F8000) {
> +		for (i = 0; i < 8 ; i++)
> +			if (!valid_mtrr_type((data >> (i * 8)) & 0xff))
> +				return false;
> +		return true;
> +	}
> +
> +	/* variable MTRRs, physmaskn have bits 0-10 reserved */
> +	return valid_mtrr_type(data & 0xff);
> +}
> +
>  static int set_msr_mtrr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	u64 *p = (u64 *)&vcpu->arch.mtrr_state.fixed_ranges;
>
> -	if (!msr_mtrr_valid(msr))
> +	if (!mtrr_valid(msr, data))
>  		return 1;
>
>  	if (msr == MSR_MTRRdefType) {