Skip to contentSkip to conversation

KVM: x86: verify MTRR/PAT validity

PATCH Applied Marcelo Tosatti 3 participants 5 messages Jun 16, 2009 – Jun 24, 2009

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

Do not allow invalid MTRR/PAT values in set_msr_mtrr.

Please review carefully.

Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +43 -1
Index: 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) {
#1 lore raw
Quoted 6 lines · 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 <[email protected]>

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?

Signature · 2 lines
regards
Yang, Sheng
Quoted 60 lines

Index: kvm/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c +43 -1
@@ -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) {
author
#2 lore raw
Quoted 21 lines · Sheng
Quoted 6 lines · 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 <[email protected]>

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

Quoted 6 lines

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?

Sheng,

This in the BIOS is writing 1's into the reserved address bits into variable MTRR:

    wrmsr_smp(MTRRphysMask_MSR(0), ~(0x20000000ull - 1) | 0x800);

So i'll leave just memory type validity checking and MSR_MTRRdefType valid bit checks in for now:

KVM: x86: verify MTRR/PAT validity

Do not allow invalid memory types in MTRR/PAT (generating a #GP otherwise).

Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +38 -1
Index: kvm/arch/x86/kvm/x86.c
===================================================================
@@ -721,11 +721,48 @@ static bool msr_mtrr_valid(unsigned msr)
 	return false;
 }
 
+static bool valid_pat_type(unsigned t)
+{
+	return t < 8 && (1 << t) & 0xf3; /* 0, 1, 4, 5, 6, 7 */
+}
+
+static bool valid_mtrr_type(unsigned t)
+{
+	return t < 8 && (1 << t) & 0x73; /* 0, 1, 4, 5, 6 */
+}
+
+static bool mtrr_valid(struct kvm_vcpu *vcpu, 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) {
+		if (data & ~0xcff)
+			return false;
+		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 */
+	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(vcpu, msr, data))
 		return 1;
 
 	if (msr == MSR_MTRRdefType) {
Applied
#3 lore raw
Quoted 9 lines · Marcelo Tosatti

This in the BIOS is writing 1's into the reserved address bits into variable MTRR:

     wrmsr_smp(MTRRphysMask_MSR(0), ~(0x20000000ull - 1) | 0x800);

So i'll leave just memory type validity checking and MSR_MTRRdefType valid bit checks in for now:

Applied, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function
#4 lore raw
In reply to Marcelo Tosatti
Quoted 15 lines · Marcelo Tosatti

Do not allow invalid MTRR/PAT values in set_msr_mtrr.

Please review carefully.

Signed-off-by Marcelo Tosatti<[email protected]>
arch/x86/kvm/x86.c +7 -0
+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;
+

arch/x86/kvm/x86.c static bool msr_mtrr_valid(unsigned msr) · line 741

Quoted 10 lines
arch/x86/kvm/x86.c +10 -0
+	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)
+

A little pointless since this is so performance-insensitive, but still:

valid_pat_type(t)

{
    return t < 8 && (1 << t) & 0xf3; /* 0, 1, 4, 5, 6, 7 */
}

valid_mtrr_type(t)

{
    return t < 8 && (1 << t) & 0x73; /* 0, 1, 4, 5, 6 */
}
Signature · 1 line
error compiling committee.c: too many arguments to function