Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Jim Mattson]

You're right, of course. My only remaining concern is that no real
hardware constrains these MSRs to three values as kvm does. On Intel
P6, only two values are allowed. On AMD CPUs, any value is allowed.

On Thu, Nov 2, 2017 at 10:35 AM, Paolo Bonzini  wrote:
> On 19/10/2017 20:09, Jim Mattson wrote:
>> "(offset & 0x3) == 1" seems like an obfuscated way of writing the
>> predicate, is_mci_status_msr(msr). But other than that, this change
>> looks fine to me.
>>
>> I'm a little more concerned about the code above. At the very least,
>> it needs to let the host set an arbitrary value for save/restore to
>> work.
>
> Why?  The guest cannot have written anything but the three allowed
> values, userspace cannot write anything else either outside save/restore
> without KVM_SET_MSR failing, and KVM itself (specifically
> kvm_vcpu_ioctl_x86_setup_mce) only ever initializes IA32_MCi_CTL to all
> ones.  So save will only ever find those three values, and restore's
> KVM_SET_MSR restore should never fail either.
>
> Thanks,
>
> Paolo
>
>> Reviewed-by: Jim Mattson 
>

Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Jim Mattson]

You're right, of course. My only remaining concern is that no real
hardware constrains these MSRs to three values as kvm does. On Intel
P6, only two values are allowed. On AMD CPUs, any value is allowed.

On Thu, Nov 2, 2017 at 10:35 AM, Paolo Bonzini  wrote:
> On 19/10/2017 20:09, Jim Mattson wrote:
>> "(offset & 0x3) == 1" seems like an obfuscated way of writing the
>> predicate, is_mci_status_msr(msr). But other than that, this change
>> looks fine to me.
>>
>> I'm a little more concerned about the code above. At the very least,
>> it needs to let the host set an arbitrary value for save/restore to
>> work.
>
> Why?  The guest cannot have written anything but the three allowed
> values, userspace cannot write anything else either outside save/restore
> without KVM_SET_MSR failing, and KVM itself (specifically
> kvm_vcpu_ioctl_x86_setup_mce) only ever initializes IA32_MCi_CTL to all
> ones.  So save will only ever find those three values, and restore's
> KVM_SET_MSR restore should never fail either.
>
> Thanks,
>
> Paolo
>
>> Reviewed-by: Jim Mattson 
>

Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Paolo Bonzini]

On 19/10/2017 20:09, Jim Mattson wrote:
> "(offset & 0x3) == 1" seems like an obfuscated way of writing the
> predicate, is_mci_status_msr(msr). But other than that, this change
> looks fine to me.
> 
> I'm a little more concerned about the code above. At the very least,
> it needs to let the host set an arbitrary value for save/restore to
> work.

Why?  The guest cannot have written anything but the three allowed
values, userspace cannot write anything else either outside save/restore
without KVM_SET_MSR failing, and KVM itself (specifically
kvm_vcpu_ioctl_x86_setup_mce) only ever initializes IA32_MCi_CTL to all
ones.  So save will only ever find those three values, and restore's
KVM_SET_MSR restore should never fail either.

Thanks,

Paolo

> Reviewed-by: Jim Mattson 

Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Paolo Bonzini]

On 19/10/2017 20:09, Jim Mattson wrote:
> "(offset & 0x3) == 1" seems like an obfuscated way of writing the
> predicate, is_mci_status_msr(msr). But other than that, this change
> looks fine to me.
> 
> I'm a little more concerned about the code above. At the very least,
> it needs to let the host set an arbitrary value for save/restore to
> work.

Why?  The guest cannot have written anything but the three allowed
values, userspace cannot write anything else either outside save/restore
without KVM_SET_MSR failing, and KVM itself (specifically
kvm_vcpu_ioctl_x86_setup_mce) only ever initializes IA32_MCi_CTL to all
ones.  So save will only ever find those three values, and restore's
KVM_SET_MSR restore should never fail either.

Thanks,

Paolo

> Reviewed-by: Jim Mattson 

Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Jim Mattson]

"(offset & 0x3) == 1" seems like an obfuscated way of writing the
predicate, is_mci_status_msr(msr). But other than that, this change
looks fine to me.

I'm a little more concerned about the code above. At the very least,
it needs to let the host set an arbitrary value for save/restore to
work.

Reviewed-by: Jim Mattson 

On Thu, Oct 19, 2017 at 6:47 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> Both Intel SDM and AMD APM mentioned that MCi_STATUS, when the register is
> implemented, this register can be cleared by explicitly writing 0s to this
> register. Writing 1s to this register will cause a general-protection
> exception.
>
> The mce is emulated in qemu, so just the guest attempts to write 1 to this
> register should cause a #GP, this patch does it.
>
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: Jim Mattson 
> Signed-off-by: Wanpeng Li 
> ---
> v1 -> v2:
>  * just #GP MCi_STATUS
>
>  arch/x86/kvm/x86.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5669af0..a8680ea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
> KVMCLOCK_SYNC_PERIOD);
>  }
>
> -static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
> u64 mcg_cap = vcpu->arch.mcg_cap;
> unsigned bank_num = mcg_cap & 0xff;
> +   u32 msr = msr_info->index;
> +   u64 data = msr_info->data;
>
> switch (msr) {
> case MSR_IA32_MCG_STATUS:
> @@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
> u64 data)
> if ((offset & 0x3) == 0 &&
> data != 0 && (data | (1 << 10)) != ~(u64)0)
> return -1;
> +   if (!msr_info->host_initiated &&
> +   (offset & 0x3) == 1 && data != 0)
> +   return -1;
> vcpu->arch.mce_banks[offset] = data;
> break;
> }
> @@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_MCG_CTL:
> case MSR_IA32_MCG_STATUS:
> case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> -   return set_msr_mce(vcpu, msr, data);
> +   return set_msr_mce(vcpu, msr_info);
>
> case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> --
> 2.7.4
>

Re: [PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Jim Mattson]

"(offset & 0x3) == 1" seems like an obfuscated way of writing the
predicate, is_mci_status_msr(msr). But other than that, this change
looks fine to me.

I'm a little more concerned about the code above. At the very least,
it needs to let the host set an arbitrary value for save/restore to
work.

Reviewed-by: Jim Mattson 

On Thu, Oct 19, 2017 at 6:47 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> Both Intel SDM and AMD APM mentioned that MCi_STATUS, when the register is
> implemented, this register can be cleared by explicitly writing 0s to this
> register. Writing 1s to this register will cause a general-protection
> exception.
>
> The mce is emulated in qemu, so just the guest attempts to write 1 to this
> register should cause a #GP, this patch does it.
>
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: Jim Mattson 
> Signed-off-by: Wanpeng Li 
> ---
> v1 -> v2:
>  * just #GP MCi_STATUS
>
>  arch/x86/kvm/x86.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5669af0..a8680ea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
> KVMCLOCK_SYNC_PERIOD);
>  }
>
> -static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
> u64 mcg_cap = vcpu->arch.mcg_cap;
> unsigned bank_num = mcg_cap & 0xff;
> +   u32 msr = msr_info->index;
> +   u64 data = msr_info->data;
>
> switch (msr) {
> case MSR_IA32_MCG_STATUS:
> @@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
> u64 data)
> if ((offset & 0x3) == 0 &&
> data != 0 && (data | (1 << 10)) != ~(u64)0)
> return -1;
> +   if (!msr_info->host_initiated &&
> +   (offset & 0x3) == 1 && data != 0)
> +   return -1;
> vcpu->arch.mce_banks[offset] = data;
> break;
> }
> @@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_MCG_CTL:
> case MSR_IA32_MCG_STATUS:
> case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> -   return set_msr_mce(vcpu, msr, data);
> +   return set_msr_mce(vcpu, msr_info);
>
> case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> --
> 2.7.4
>

[PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Wanpeng Li]

From: Wanpeng Li 

Both Intel SDM and AMD APM mentioned that MCi_STATUS, when the register is 
implemented, this register can be cleared by explicitly writing 0s to this
register. Writing 1s to this register will cause a general-protection 
exception.

The mce is emulated in qemu, so just the guest attempts to write 1 to this
register should cause a #GP, this patch does it.

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Jim Mattson 
Signed-off-by: Wanpeng Li 
---
v1 -> v2:
 * just #GP MCi_STATUS

 arch/x86/kvm/x86.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5669af0..a8680ea 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
KVMCLOCK_SYNC_PERIOD);
 }
 
-static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
u64 mcg_cap = vcpu->arch.mcg_cap;
unsigned bank_num = mcg_cap & 0xff;
+   u32 msr = msr_info->index;
+   u64 data = msr_info->data;
 
switch (msr) {
case MSR_IA32_MCG_STATUS:
@@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
u64 data)
if ((offset & 0x3) == 0 &&
data != 0 && (data | (1 << 10)) != ~(u64)0)
return -1;
+   if (!msr_info->host_initiated &&
+   (offset & 0x3) == 1 && data != 0)
+   return -1;
vcpu->arch.mce_banks[offset] = data;
break;
}
@@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_MCG_CTL:
case MSR_IA32_MCG_STATUS:
case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
-   return set_msr_mce(vcpu, msr, data);
+   return set_msr_mce(vcpu, msr_info);
 
case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
-- 
2.7.4

[PATCH v2] KVM: X86: #GP when guest attempts to write MCi_STATUS register w/o 0

[Wanpeng Li]

From: Wanpeng Li 

Both Intel SDM and AMD APM mentioned that MCi_STATUS, when the register is 
implemented, this register can be cleared by explicitly writing 0s to this
register. Writing 1s to this register will cause a general-protection 
exception.

The mce is emulated in qemu, so just the guest attempts to write 1 to this
register should cause a #GP, this patch does it.

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Jim Mattson 
Signed-off-by: Wanpeng Li 
---
v1 -> v2:
 * just #GP MCi_STATUS

 arch/x86/kvm/x86.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5669af0..a8680ea 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
KVMCLOCK_SYNC_PERIOD);
 }
 
-static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
u64 mcg_cap = vcpu->arch.mcg_cap;
unsigned bank_num = mcg_cap & 0xff;
+   u32 msr = msr_info->index;
+   u64 data = msr_info->data;
 
switch (msr) {
case MSR_IA32_MCG_STATUS:
@@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
u64 data)
if ((offset & 0x3) == 0 &&
data != 0 && (data | (1 << 10)) != ~(u64)0)
return -1;
+   if (!msr_info->host_initiated &&
+   (offset & 0x3) == 1 && data != 0)
+   return -1;
vcpu->arch.mce_banks[offset] = data;
break;
}
@@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_MCG_CTL:
case MSR_IA32_MCG_STATUS:
case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
-   return set_msr_mce(vcpu, msr, data);
+   return set_msr_mce(vcpu, msr_info);
 
case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
-- 
2.7.4