[PATCH 0/2] KVM: x86: Sanitize writes to MSR_KVM_PV_EOI_EN

[PATCH 0/2] KVM: x86: Sanitize writes to MSR_KVM_PV_EOI_EN

[Vitaly Kuznetsov]

This is a continuation of work started by Li RongQing with
"[PATCH] KVM: x86: disable pv eoi if guest gives a wrong address":
https://lore.kernel.org/kvm/1636078404-48617-1-git-send-email-lirongqing@baidu.com/

Instead of resetting 'KVM_MSR_ENABLED' when a bogus address was written to
MSR_KVM_PV_EOI_EN I suggest we refuse to update MSR at all, this aligns
with #GP which is being injected on such writes.

Vitaly Kuznetsov (2):
  KVM: x86: Rename kvm_lapic_enable_pv_eoi()
  KVM: x86: Don't update vcpu->arch.pv_eoi.msr_val when a bogus value
    was written to MSR_KVM_PV_EOI_EN

 arch/x86/kvm/hyperv.c |  4 ++--
 arch/x86/kvm/lapic.c  | 23 ++++++++++++++---------
 arch/x86/kvm/lapic.h  |  2 +-
 arch/x86/kvm/x86.c    |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH 1/2] KVM: x86: Rename kvm_lapic_enable_pv_eoi()

[Vitaly Kuznetsov]

kvm_lapic_enable_pv_eoi() is a misnomer as the function is also
used to disable PV EOI. Rename it to kvm_lapic_set_pv_eoi().

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 4 ++--
 arch/x86/kvm/lapic.c  | 2 +-
 arch/x86/kvm/lapic.h  | 2 +-
 arch/x86/kvm/x86.c    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 4f15c0165c05..4a555f32885a 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1472,7 +1472,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 
 		if (!(data & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE)) {
 			hv_vcpu->hv_vapic = data;
-			if (kvm_lapic_enable_pv_eoi(vcpu, 0, 0))
+			if (kvm_lapic_set_pv_eoi(vcpu, 0, 0))
 				return 1;
 			break;
 		}
@@ -1490,7 +1490,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 			return 1;
 		hv_vcpu->hv_vapic = data;
 		kvm_vcpu_mark_page_dirty(vcpu, gfn);
-		if (kvm_lapic_enable_pv_eoi(vcpu,
+		if (kvm_lapic_set_pv_eoi(vcpu,
 					    gfn_to_gpa(gfn) | KVM_MSR_ENABLED,
 					    sizeof(struct hv_vp_assist_page)))
 			return 1;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 76fb00921203..3573b50d9036 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2852,7 +2852,7 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 	return 0;
 }
 
-int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
+int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
 {
 	u64 addr = data & ~KVM_MSR_ENABLED;
 	struct gfn_to_hva_cache *ghc = &vcpu->arch.pv_eoi.data;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index d7c25d0c1354..2b44e533fc8d 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -127,7 +127,7 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 
-int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
+int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
 void kvm_lapic_exit(void);
 
 #define VEC_POS(v) ((v) & (32 - 1))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ac83d873d65b..41d4fe7374f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3517,7 +3517,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_EOI))
 			return 1;
 
-		if (kvm_lapic_enable_pv_eoi(vcpu, data, sizeof(u8)))
+		if (kvm_lapic_set_pv_eoi(vcpu, data, sizeof(u8)))
 			return 1;
 		break;
 
-- 
2.31.1

[PATCH 2/2] KVM: x86: Don't update vcpu->arch.pv_eoi.msr_val when a bogus value was written to MSR_KVM_PV_EOI_EN

[Vitaly Kuznetsov]

When kvm_gfn_to_hva_cache_init() call from kvm_lapic_set_pv_eoi() fails,
MSR write to MSR_KVM_PV_EOI_EN results in #GP so it is reasonable to
expect that the value we keep internally in KVM wasn't updated.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/lapic.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3573b50d9036..4388d22df500 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2857,20 +2857,25 @@ int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
 	u64 addr = data & ~KVM_MSR_ENABLED;
 	struct gfn_to_hva_cache *ghc = &vcpu->arch.pv_eoi.data;
 	unsigned long new_len;
+	int ret;
 
 	if (!IS_ALIGNED(addr, 4))
 		return 1;
 
-	vcpu->arch.pv_eoi.msr_val = data;
-	if (!pv_eoi_enabled(vcpu))
-		return 0;
+	if (data & KVM_MSR_ENABLED) {
+		if (addr == ghc->gpa && len <= ghc->len)
+			new_len = ghc->len;
+		else
+			new_len = len;
 
-	if (addr == ghc->gpa && len <= ghc->len)
-		new_len = ghc->len;
-	else
-		new_len = len;
+		ret = kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
+		if (ret)
+			return ret;
+	}
+
+	vcpu->arch.pv_eoi.msr_val = data;
 
-	return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
+	return 0;
 }
 
 int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
-- 
2.31.1

Re: [PATCH 0/2] KVM: x86: Sanitize writes to MSR_KVM_PV_EOI_EN

[Paolo Bonzini]

On 11/8/21 16:28, Vitaly Kuznetsov wrote:
> This is a continuation of work started by Li RongQing with
> "[PATCH] KVM: x86: disable pv eoi if guest gives a wrong address":
> https://lore.kernel.org/kvm/1636078404-48617-1-git-send-email-lirongqing@baidu.com/
> 
> Instead of resetting 'KVM_MSR_ENABLED' when a bogus address was written to
> MSR_KVM_PV_EOI_EN I suggest we refuse to update MSR at all, this aligns
> with #GP which is being injected on such writes.
> 
> Vitaly Kuznetsov (2):
>    KVM: x86: Rename kvm_lapic_enable_pv_eoi()
>    KVM: x86: Don't update vcpu->arch.pv_eoi.msr_val when a bogus value
>      was written to MSR_KVM_PV_EOI_EN
> 
>   arch/x86/kvm/hyperv.c |  4 ++--
>   arch/x86/kvm/lapic.c  | 23 ++++++++++++++---------
>   arch/x86/kvm/lapic.h  |  2 +-
>   arch/x86/kvm/x86.c    |  2 +-
>   4 files changed, 18 insertions(+), 13 deletions(-)
> 

Queued, thanks.

Paolo