All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
@ 2017-08-08 23:27 Jim Mattson
  2017-08-09  9:13 ` David Hildenbrand
  0 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2017-08-08 23:27 UTC (permalink / raw)
  To: kvm; +Cc: 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-08 23:27 [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values Jim Mattson
@ 2017-08-09  9:13 ` David Hildenbrand
  2017-08-09 15:14   ` Jim Mattson
  0 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2017-08-09  9:13 UTC (permalink / raw)
  To: Jim Mattson, kvm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-09  9:13 ` David Hildenbrand
@ 2017-08-09 15:14   ` Jim Mattson
  2017-08-09 16:54     ` [PATCH v2] " Jim Mattson
  2017-08-09 20:31     ` [PATCH] " David Hildenbrand
  0 siblings, 2 replies; 8+ messages in thread
From: Jim Mattson @ 2017-08-09 15:14 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: kvm list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-09 15:14   ` Jim Mattson
@ 2017-08-09 16:54     ` Jim Mattson
  2017-08-10 10:00       ` David Hildenbrand
  2017-08-09 20:31     ` [PATCH] " David Hildenbrand
  1 sibling, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2017-08-09 16:54 UTC (permalink / raw)
  To: David Hildenbrand, kvm list; +Cc: 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-09 15:14   ` Jim Mattson
  2017-08-09 16:54     ` [PATCH v2] " Jim Mattson
@ 2017-08-09 20:31     ` David Hildenbrand
  2017-08-10  9:37       ` Paolo Bonzini
  1 sibling, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2017-08-09 20:31 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm list, Paolo Bonzini

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-09 20:31     ` [PATCH] " David Hildenbrand
@ 2017-08-10  9:37       ` Paolo Bonzini
  0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2017-08-10  9:37 UTC (permalink / raw)
  To: David Hildenbrand, Jim Mattson; +Cc: kvm list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-09 16:54     ` [PATCH v2] " Jim Mattson
@ 2017-08-10 10:00       ` David Hildenbrand
  2017-08-10 17:14         ` [PATCH v3] " Jim Mattson
  0 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2017-08-10 10:00 UTC (permalink / raw)
  To: Jim Mattson, kvm list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v3] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values
  2017-08-10 10:00       ` David Hildenbrand
@ 2017-08-10 17:14         ` Jim Mattson
  0 siblings, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2017-08-10 17:14 UTC (permalink / raw)
  To: David Hildenbrand, kvm list; +Cc: 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-10 17:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-08 23:27 [PATCH] kvm: x86: Disallow illegal IA32_APIC_BASE MSR values Jim Mattson
2017-08-09  9:13 ` David Hildenbrand
2017-08-09 15:14   ` Jim Mattson
2017-08-09 16:54     ` [PATCH v2] " Jim Mattson
2017-08-10 10:00       ` David Hildenbrand
2017-08-10 17:14         ` [PATCH v3] " Jim Mattson
2017-08-09 20:31     ` [PATCH] " David Hildenbrand
2017-08-10  9:37       ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.