[PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[Jim Mattson]

Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
local APIC state transition constraints, but the value written must be
valid.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/x86.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d734aa8c5b4f..fb786d9feef1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) |
 		0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE);
 
+	if ((msr_info->data & reserved_bits) != 0 ||
+	    new_state == X2APIC_ENABLE)
+		return 1;
 	if (!msr_info->host_initiated &&
-	    ((msr_info->data & reserved_bits) != 0 ||
-	     new_state == X2APIC_ENABLE ||
-	     (new_state == MSR_IA32_APICBASE_ENABLE &&
+	    ((new_state == MSR_IA32_APICBASE_ENABLE &&
 	      old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
 	     (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
 	      old_state == 0)))
@@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	kvm_x86_ops->set_efer(vcpu, sregs->efer);
 	apic_base_msr.data = sregs->apic_base;
 	apic_base_msr.host_initiated = true;
-	kvm_set_apic_base(vcpu, &apic_base_msr);
+	if (kvm_set_apic_base(vcpu, &apic_base_msr))
+		return -EINVAL;
 
 	mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
 	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
-- 
2.14.0.434.g98096fd7a8-goog

Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[David Hildenbrand]

On 09.08.2017 01:27, Jim Mattson wrote:
> Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
> local APIC state transition constraints, but the value written must be
> valid.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/x86.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d734aa8c5b4f..fb786d9feef1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) |
>  		0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE);
>  
> +	if ((msr_info->data & reserved_bits) != 0 ||
> +	    new_state == X2APIC_ENABLE)

I'd drop the != 0 here and fit it into one line.

> +		return 1;
>  	if (!msr_info->host_initiated &&
> -	    ((msr_info->data & reserved_bits) != 0 ||
> -	     new_state == X2APIC_ENABLE ||
> -	     (new_state == MSR_IA32_APICBASE_ENABLE &&
> +	    ((new_state == MSR_IA32_APICBASE_ENABLE &&
>  	      old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
>  	     (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
>  	      old_state == 0)))
> @@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>  	kvm_x86_ops->set_efer(vcpu, sregs->efer);
>  	apic_base_msr.data = sregs->apic_base;
>  	apic_base_msr.host_initiated = true;
> -	kvm_set_apic_base(vcpu, &apic_base_msr);
> +	if (kvm_set_apic_base(vcpu, &apic_base_msr))
> +		return -EINVAL;

I assume we don't have to document this new behavior.

>  
>  	mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
>  	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
> 

>From what I can tell, this looks good to me.

-- 

Thanks,

David

Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[Jim Mattson]

The only thing that makes me unhappy about this is that the
KVM_SET_SREGS ioctl may modify some VCPU state before returning
-EINVAL. I could hoist the call to kvm_set_apic_base, but that only
works for one mutator. If this doesn't bother anyone else, I'll just
leave it where it is.

On Wed, Aug 9, 2017 at 2:13 AM, David Hildenbrand <david@redhat.com> wrote:
> On 09.08.2017 01:27, Jim Mattson wrote:
>> Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
>> local APIC state transition constraints, but the value written must be
>> valid.
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> ---
>>  arch/x86/kvm/x86.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d734aa8c5b4f..fb786d9feef1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>       u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) |
>>               0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE);
>>
>> +     if ((msr_info->data & reserved_bits) != 0 ||
>> +         new_state == X2APIC_ENABLE)
>
> I'd drop the != 0 here and fit it into one line.
>
>> +             return 1;
>>       if (!msr_info->host_initiated &&
>> -         ((msr_info->data & reserved_bits) != 0 ||
>> -          new_state == X2APIC_ENABLE ||
>> -          (new_state == MSR_IA32_APICBASE_ENABLE &&
>> +         ((new_state == MSR_IA32_APICBASE_ENABLE &&
>>             old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
>>            (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
>>             old_state == 0)))
>> @@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>>       kvm_x86_ops->set_efer(vcpu, sregs->efer);
>>       apic_base_msr.data = sregs->apic_base;
>>       apic_base_msr.host_initiated = true;
>> -     kvm_set_apic_base(vcpu, &apic_base_msr);
>> +     if (kvm_set_apic_base(vcpu, &apic_base_msr))
>> +             return -EINVAL;
>
> I assume we don't have to document this new behavior.
>
>>
>>       mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
>>       kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
>>
>
> From what I can tell, this looks good to me.
>
> --
>
> Thanks,
>
> David

[PATCH v2] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[Jim Mattson]

Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
local APIC state transition constraints, but the value written must be
valid.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/x86.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d734aa8c5b4f..77f17a6ea48a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -313,10 +313,10 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) |
 		0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE);
 
+	if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
+		return 1;
 	if (!msr_info->host_initiated &&
-	    ((msr_info->data & reserved_bits) != 0 ||
-	     new_state == X2APIC_ENABLE ||
-	     (new_state == MSR_IA32_APICBASE_ENABLE &&
+	    ((new_state == MSR_IA32_APICBASE_ENABLE &&
 	      old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
 	     (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
 	      old_state == 0)))
@@ -7444,7 +7444,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	kvm_x86_ops->set_efer(vcpu, sregs->efer);
 	apic_base_msr.data = sregs->apic_base;
 	apic_base_msr.host_initiated = true;
-	kvm_set_apic_base(vcpu, &apic_base_msr);
+	if (kvm_set_apic_base(vcpu, &apic_base_msr))
+		return -EINVAL;
 
 	mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
 	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
-- 
2.14.0.434.g98096fd7a8-goog

Re: [PATCH v2] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[David Hildenbrand]

On 09.08.2017 18:54, Jim Mattson wrote:
> Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
> local APIC state transition constraints, but the value written must be
> valid.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/x86.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d734aa8c5b4f..77f17a6ea48a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -313,10 +313,10 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) |
>  		0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE);
>  
> +	if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
> +		return 1;
>  	if (!msr_info->host_initiated &&
> -	    ((msr_info->data & reserved_bits) != 0 ||
> -	     new_state == X2APIC_ENABLE ||
> -	     (new_state == MSR_IA32_APICBASE_ENABLE &&
> +	    ((new_state == MSR_IA32_APICBASE_ENABLE &&
>  	      old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
>  	     (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
>  	      old_state == 0)))
> @@ -7444,7 +7444,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>  	kvm_x86_ops->set_efer(vcpu, sregs->efer);
>  	apic_base_msr.data = sregs->apic_base;
>  	apic_base_msr.host_initiated = true;
> -	kvm_set_apic_base(vcpu, &apic_base_msr);
> +	if (kvm_set_apic_base(vcpu, &apic_base_msr))
> +		return -EINVAL;
>  
>  	mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
>  	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
> 

With that check moved to the very top

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

[PATCH v3] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[Jim Mattson]

Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow
local APIC state transition constraints, but the value written must be
valid.

Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/x86.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bb05b705c295..008a0b14d5af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -313,10 +313,10 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff |
 		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
 
+	if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
+		return 1;
 	if (!msr_info->host_initiated &&
-	    ((msr_info->data & reserved_bits) != 0 ||
-	     new_state == X2APIC_ENABLE ||
-	     (new_state == MSR_IA32_APICBASE_ENABLE &&
+	    ((new_state == MSR_IA32_APICBASE_ENABLE &&
 	      old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) ||
 	     (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) &&
 	      old_state == 0)))
@@ -7417,6 +7417,11 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 			(sregs->cr4 & X86_CR4_OSXSAVE))
 		return -EINVAL;
 
+	apic_base_msr.data = sregs->apic_base;
+	apic_base_msr.host_initiated = true;
+	if (kvm_set_apic_base(vcpu, &apic_base_msr))
+		return -EINVAL;
+
 	dt.size = sregs->idt.limit;
 	dt.address = sregs->idt.base;
 	kvm_x86_ops->set_idt(vcpu, &dt);
@@ -7433,9 +7438,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 
 	mmu_reset_needed |= vcpu->arch.efer != sregs->efer;
 	kvm_x86_ops->set_efer(vcpu, sregs->efer);
-	apic_base_msr.data = sregs->apic_base;
-	apic_base_msr.host_initiated = true;
-	kvm_set_apic_base(vcpu, &apic_base_msr);
 
 	mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0;
 	kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
-- 
2.14.0.434.g98096fd7a8-goog

Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[David Hildenbrand]

On 09.08.2017 17:14, Jim Mattson wrote:
> The only thing that makes me unhappy about this is that the
> KVM_SET_SREGS ioctl may modify some VCPU state before returning
> -EINVAL. I could hoist the call to kvm_set_apic_base, but that only
> works for one mutator. If this doesn't bother anyone else, I'll just
> leave it where it is.

Good point, but the question is if the caller is even able to recover
from this failure?

If we care, you might have to move kvm_set_apic_base() to the very top
in kvm_arch_vcpu_ioctl_set_sregs. Or just do the check at that point.

Guess Paolo knows the answer to our question, just as always :)

-- 

Thanks,

David

Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values

[Paolo Bonzini]

On 09/08/2017 22:31, David Hildenbrand wrote:
> On 09.08.2017 17:14, Jim Mattson wrote:
>> The only thing that makes me unhappy about this is that the
>> KVM_SET_SREGS ioctl may modify some VCPU state before returning
>> -EINVAL. I could hoist the call to kvm_set_apic_base, but that only
>> works for one mutator. If this doesn't bother anyone else, I'll just
>> leave it where it is.
> 
> Good point, but the question is if the caller is even able to recover
> from this failure?

Likely not, but being cleaner is usually better...

> If we care, you might have to move kvm_set_apic_base() to the very top
> in kvm_arch_vcpu_ioctl_set_sregs. Or just do the check at that point.
> 
> Guess Paolo knows the answer to our question, just as always :)

Not sure I do, but I am (though only slightly) worried about not doing
the kvm_mmu_reset_context if EFER as changed and also about missing
update_cr8_intercept.

Moving kvm_set_apic_base early is harmless, so why not move that to the
beginning.

Paolo