* [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
@ 2021-11-05 2:13 Li RongQing
2021-11-05 9:08 ` Vitaly Kuznetsov
0 siblings, 1 reply; 4+ messages in thread
From: Li RongQing @ 2021-11-05 2:13 UTC (permalink / raw)
To: kvm, pbonzini, seanjc, vkuznets, lirongqing
disable pv eoi if guest gives a wrong address, this can reduces
the attacked possibility for a malicious guest, and can avoid
unnecessary write/read pv eoi memory
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
arch/x86/kvm/lapic.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index b1de23e..0f37a8d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2853,6 +2853,7 @@ int kvm_lapic_enable_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;
@@ -2866,7 +2867,13 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
else
new_len = len;
- return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
+ ret = kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
+
+ if (ret && (vcpu->arch.pv_eoi.msr_val & KVM_MSR_ENABLED)) {
+ vcpu->arch.pv_eoi.msr_val &= ~KVM_MSR_ENABLED;
+ pr_warn_once("Disabled PV EOI during wrong address\n");
+ }
+ return ret;
}
int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
2021-11-05 2:13 [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address Li RongQing
@ 2021-11-05 9:08 ` Vitaly Kuznetsov
2021-11-05 9:51 ` 答复: " Li,Rongqing
0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2021-11-05 9:08 UTC (permalink / raw)
To: Li RongQing; +Cc: kvm, pbonzini, seanjc, lirongqing
Li RongQing <lirongqing@baidu.com> writes:
> disable pv eoi if guest gives a wrong address, this can reduces
> the attacked possibility for a malicious guest, and can avoid
> unnecessary write/read pv eoi memory
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> arch/x86/kvm/lapic.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b1de23e..0f37a8d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2853,6 +2853,7 @@ int kvm_lapic_enable_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;
> @@ -2866,7 +2867,13 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
> else
> new_len = len;
>
> - return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
> + ret = kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
> +
> + if (ret && (vcpu->arch.pv_eoi.msr_val & KVM_MSR_ENABLED)) {
> + vcpu->arch.pv_eoi.msr_val &= ~KVM_MSR_ENABLED;
> + pr_warn_once("Disabled PV EOI during wrong address\n");
Personally, I see little value in this message: it's not easy to say
which particular guest triggered it so it's unclear what system
administrator is supposed to do upon seeing this message.
Also, while on it, I think kvm_lapic_enable_pv_eoi() is misnamed: it is
also used for *disabling* PV EOI.
Instead of dropping KVM_MSR_ENABLED bit, I'd suggest we only set
vcpu->arch.pv_eoi.msr_val in case of success. In case
kvm_gfn_to_hva_cache_init() fails, we inject #GP so it's reasonable to
expect that MSR's value didn't change.
Completely untested:
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 1cdcf3ad5684..9fe5e2a2df25 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,9 +1490,9 @@ 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,
- gfn_to_gpa(gfn) | KVM_MSR_ENABLED,
- sizeof(struct hv_vp_assist_page)))
+ if (kvm_lapic_set_pv_eoi(vcpu,
+ gfn_to_gpa(gfn) | KVM_MSR_ENABLED,
+ sizeof(struct hv_vp_assist_page)))
return 1;
break;
}
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4da5db83736f..38b9cb26a81d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2851,25 +2851,31 @@ 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;
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);
- return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
+ if (ret)
+ return ret;
+ }
+
+ vcpu->arch.pv_eoi.msr_val = data;
+
+ return 0;
}
int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
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 dccf927baa4d..7d9bc8c185da 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;
> + }
> + return ret;
> }
>
> int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
--
Vitaly
^ permalink raw reply related [flat|nested] 4+ messages in thread
* 答复: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
2021-11-05 9:08 ` Vitaly Kuznetsov
@ 2021-11-05 9:51 ` Li,Rongqing
2021-11-05 10:17 ` Vitaly Kuznetsov
0 siblings, 1 reply; 4+ messages in thread
From: Li,Rongqing @ 2021-11-05 9:51 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, pbonzini, seanjc
> -----邮件原件-----
> 发件人: Vitaly Kuznetsov <vkuznets@redhat.com>
> 发送时间: 2021年11月5日 17:08
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: kvm@vger.kernel.org; pbonzini@redhat.com; seanjc@google.com;
> Li,Rongqing <lirongqing@baidu.com>
> 主题: Re: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
>
> Li RongQing <lirongqing@baidu.com> writes:
>
> > disable pv eoi if guest gives a wrong address, this can reduces the
> > attacked possibility for a malicious guest, and can avoid unnecessary
> > write/read pv eoi memory
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > arch/x86/kvm/lapic.c | 9 ++++++++-
> > 1 files changed, 8 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index
> > b1de23e..0f37a8d 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -2853,6 +2853,7 @@ int kvm_lapic_enable_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;
> > @@ -2866,7 +2867,13 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu
> *vcpu, u64 data, unsigned long len)
> > else
> > new_len = len;
> >
> > - return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
> > + ret = kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
> > +
> > + if (ret && (vcpu->arch.pv_eoi.msr_val & KVM_MSR_ENABLED)) {
> > + vcpu->arch.pv_eoi.msr_val &= ~KVM_MSR_ENABLED;
> > + pr_warn_once("Disabled PV EOI during wrong address\n");
>
> Personally, I see little value in this message: it's not easy to say which particular
> guest triggered it so it's unclear what system administrator is supposed to do
> upon seeing this message.
>
> Also, while on it, I think kvm_lapic_enable_pv_eoi() is misnamed: it is also used
> for *disabling* PV EOI.
>
> Instead of dropping KVM_MSR_ENABLED bit, I'd suggest we only set
> vcpu->arch.pv_eoi.msr_val in case of success. In case
> kvm_gfn_to_hva_cache_init() fails, we inject #GP so it's reasonable to expect
> that MSR's value didn't change.
>
Hi Vitaly:
Could you submit your patch?
Thanks
-Li
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 答复: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
2021-11-05 9:51 ` 答复: " Li,Rongqing
@ 2021-11-05 10:17 ` Vitaly Kuznetsov
0 siblings, 0 replies; 4+ messages in thread
From: Vitaly Kuznetsov @ 2021-11-05 10:17 UTC (permalink / raw)
To: Li,Rongqing; +Cc: kvm, pbonzini, seanjc
"Li,Rongqing" <lirongqing@baidu.com> writes:
>> -----邮件原件-----
>> 发件人: Vitaly Kuznetsov <vkuznets@redhat.com>
>> 发送时间: 2021年11月5日 17:08
>> 收件人: Li,Rongqing <lirongqing@baidu.com>
>> 抄送: kvm@vger.kernel.org; pbonzini@redhat.com; seanjc@google.com;
>> Li,Rongqing <lirongqing@baidu.com>
>> 主题: Re: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address
>>
>> Li RongQing <lirongqing@baidu.com> writes:
>>
>> > disable pv eoi if guest gives a wrong address, this can reduces the
>> > attacked possibility for a malicious guest, and can avoid unnecessary
>> > write/read pv eoi memory
>> >
>> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
>> > ---
>> > arch/x86/kvm/lapic.c | 9 ++++++++-
>> > 1 files changed, 8 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index
>> > b1de23e..0f37a8d 100644
>> > --- a/arch/x86/kvm/lapic.c
>> > +++ b/arch/x86/kvm/lapic.c
>> > @@ -2853,6 +2853,7 @@ int kvm_lapic_enable_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;
>> > @@ -2866,7 +2867,13 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu
>> *vcpu, u64 data, unsigned long len)
>> > else
>> > new_len = len;
>> >
>> > - return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
>> > + ret = kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
>> > +
>> > + if (ret && (vcpu->arch.pv_eoi.msr_val & KVM_MSR_ENABLED)) {
>> > + vcpu->arch.pv_eoi.msr_val &= ~KVM_MSR_ENABLED;
>> > + pr_warn_once("Disabled PV EOI during wrong address\n");
>>
>> Personally, I see little value in this message: it's not easy to say which particular
>> guest triggered it so it's unclear what system administrator is supposed to do
>> upon seeing this message.
>>
>> Also, while on it, I think kvm_lapic_enable_pv_eoi() is misnamed: it is also used
>> for *disabling* PV EOI.
>>
>> Instead of dropping KVM_MSR_ENABLED bit, I'd suggest we only set
>> vcpu->arch.pv_eoi.msr_val in case of success. In case
>> kvm_gfn_to_hva_cache_init() fails, we inject #GP so it's reasonable to expect
>> that MSR's value didn't change.
>>
>
>
> Hi Vitaly:
>
> Could you submit your patch?
>
Sure, I will.
--
Vitaly
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-05 10:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 2:13 [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address Li RongQing
2021-11-05 9:08 ` Vitaly Kuznetsov
2021-11-05 9:51 ` 答复: " Li,Rongqing
2021-11-05 10:17 ` Vitaly Kuznetsov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).