Re: 答复: [PATCH] KVM: x86: disable pv eoi if guest gives a wrong address

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"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