| Message ID | 1636078404-48617-1-git-send-email-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: disable pv eoi if guest gives a wrong address | expand |
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 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
"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.
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)
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(-)