LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
@ 2018-05-13  7:53 Liran Alon
  2018-05-13  8:25 ` Wanpeng Li
  0 siblings, 1 reply; 7+ messages in thread
From: Liran Alon @ 2018-05-13  7:53 UTC (permalink / raw)
  To: kernellwp; +Cc: rkrcmar, pbonzini, linux-kernel, kvm, junaids


----- kernellwp@gmail.com wrote:

> From: Wanpeng Li <wanpengli@tencent.com>
> 
> MSB of CR3 is a reserved bit if the PCIDE bit is not set in CR4. 
> It should be checked when PCIDE bit is not set, however commit 
> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based on 
> its physical address width")' removes the bit 63 checking 
> unconditionally. This patch fixes it by checking bit 63 of CR3 
> when PCIDE bit is not set in CR4.
> 
> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits based on
> its physical address width)
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Junaid Shahid <junaids@google.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/emulate.c | 4 +++-
>  arch/x86/kvm/x86.c     | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index b3705ae..b21f427 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
> x86_emulate_ctxt *ctxt)
>  				maxphyaddr = eax & 0xff;
>  			else
>  				maxphyaddr = 36;
> -			rsvd = rsvd_bits(maxphyaddr, 62);
> +			if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> +				new_val &= ~CR3_PCID_INVD;
> +			rsvd = rsvd_bits(maxphyaddr, 63);

I would prefer instead to do this:
if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
    rsvd &= ~CR3_PCID_INVD;
It makes more sense as opposed to temporary removing the CR3_PCID_INVD bit from new_val.

>  		}
>  
>  		if (new_val & rsvd)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 87e4805..9a90668 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -863,7 +863,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned
> long cr3)
>  	}
>  
>  	if (is_long_mode(vcpu) &&
> -	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 62)))
> +	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
>  		return 1;
>  	else if (is_pae(vcpu) && is_paging(vcpu) &&
>  		   !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
> -- 
> 2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
  2018-05-13  7:53 [PATCH 1/2] KVM: X86: Fix CR3 reserve bits Liran Alon
@ 2018-05-13  8:25 ` Wanpeng Li
  0 siblings, 0 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-05-13  8:25 UTC (permalink / raw)
  To: Liran Alon; +Cc: Radim Krcmar, Paolo Bonzini, LKML, kvm, Junaid Shahid

2018-05-13 15:53 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>
> ----- kernellwp@gmail.com wrote:
>
>> From: Wanpeng Li <wanpengli@tencent.com>
>>
>> MSB of CR3 is a reserved bit if the PCIDE bit is not set in CR4.
>> It should be checked when PCIDE bit is not set, however commit
>> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based on
>> its physical address width")' removes the bit 63 checking
>> unconditionally. This patch fixes it by checking bit 63 of CR3
>> when PCIDE bit is not set in CR4.
>>
>> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits based on
>> its physical address width)
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Junaid Shahid <junaids@google.com>
>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> ---
>>  arch/x86/kvm/emulate.c | 4 +++-
>>  arch/x86/kvm/x86.c     | 2 +-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index b3705ae..b21f427 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
>> x86_emulate_ctxt *ctxt)
>>                               maxphyaddr = eax & 0xff;
>>                       else
>>                               maxphyaddr = 36;
>> -                     rsvd = rsvd_bits(maxphyaddr, 62);
>> +                     if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
>> +                             new_val &= ~CR3_PCID_INVD;
>> +                     rsvd = rsvd_bits(maxphyaddr, 63);
>
> I would prefer instead to do this:
> if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
>     rsvd &= ~CR3_PCID_INVD;
> It makes more sense as opposed to temporary removing the CR3_PCID_INVD bit from new_val.

It tries the same way
https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?id=c19986fea873f3c745122bf79013a872a190f212
pointed out.

Regards,
Wanpeng Li

>
>>               }
>>
>>               if (new_val & rsvd)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 87e4805..9a90668 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -863,7 +863,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned
>> long cr3)
>>       }
>>
>>       if (is_long_mode(vcpu) &&
>> -         (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 62)))
>> +         (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
>>               return 1;
>>       else if (is_pae(vcpu) && is_paging(vcpu) &&
>>                  !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
>> --
>> 2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
  2018-05-13  9:09 Liran Alon
@ 2018-05-13  9:14 ` Wanpeng Li
  0 siblings, 0 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-05-13  9:14 UTC (permalink / raw)
  To: Liran Alon; +Cc: Radim Krcmar, Paolo Bonzini, LKML, kvm, Junaid Shahid

2018-05-13 17:09 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>
> ----- kernellwp@gmail.com wrote:
>
>> 2018-05-13 16:28 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>> >
>> > ----- kernellwp@gmail.com wrote:
>> >
>> >> 2018-05-13 15:53 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>> >> >
>> >> > ----- kernellwp@gmail.com wrote:
>> >> >
>> >> >> From: Wanpeng Li <wanpengli@tencent.com>
>> >> >>
>> >> >> MSB of CR3 is a reserved bit if the PCIDE bit is not set in
>> CR4.
>> >> >> It should be checked when PCIDE bit is not set, however commit
>> >> >> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based
>> on
>> >> >> its physical address width")' removes the bit 63 checking
>> >> >> unconditionally. This patch fixes it by checking bit 63 of CR3
>> >> >> when PCIDE bit is not set in CR4.
>> >> >>
>> >> >> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits
>> based
>> >> on
>> >> >> its physical address width)
>> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> >> >> Cc: Junaid Shahid <junaids@google.com>
>> >> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> >> >> ---
>> >> >>  arch/x86/kvm/emulate.c | 4 +++-
>> >> >>  arch/x86/kvm/x86.c     | 2 +-
>> >> >>  2 files changed, 4 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> >> >> index b3705ae..b21f427 100644
>> >> >> --- a/arch/x86/kvm/emulate.c
>> >> >> +++ b/arch/x86/kvm/emulate.c
>> >> >> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
>> >> >> x86_emulate_ctxt *ctxt)
>> >> >>                               maxphyaddr = eax & 0xff;
>> >> >>                       else
>> >> >>                               maxphyaddr = 36;
>> >> >> -                     rsvd = rsvd_bits(maxphyaddr, 62);
>> >> >> +                     if (ctxt->ops->get_cr(ctxt, 4) &
>> >> X86_CR4_PCIDE)
>> >> >> +                             new_val &= ~CR3_PCID_INVD;
>> >> >> +                     rsvd = rsvd_bits(maxphyaddr, 63);
>> >> >
>> >> > I would prefer instead to do this:
>> >> > if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
>> >> >     rsvd &= ~CR3_PCID_INVD;
>> >> > It makes more sense as opposed to temporary removing the
>> >> CR3_PCID_INVD bit from new_val.
>> >>
>> >> It tries the same way
>> >>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_virt_kvm_kvm.git_commit_-3Fid-3Dc19986fea873f3c745122bf79013a872a190f212&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=r52WDgKBorUHwe_B_5Nw2Le_F_E0ne8lqqWW6n-3bSg&s=ufTcXvhhAMkY3XP6gAx-HiKCT8ynPWo2fs2z9DqCzM4&e=
>> >> pointed out.
>> >>
>> >> Regards,
>> >> Wanpeng Li
>> >
>> > Yes but there it makes sense as new CR3 value should not have bit 63
>> set in vcpu->arch.cr3.
>>
>> When X86_CR4_PCIDE == 0 and CR3 63 bit is set, a #GP is missing in
>> your suggestion.
>>
>> Regards,
>> Wanpeng Li
>
> Why?
>
> I suggest the following change:
> - rsvd = rsvd_bits(maxphyaddr, 62);
> + rsvd = rsvd_bits(maxphyaddr, 63);
> + if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> +     rsvd &= ~CR3_PCID_INVD;
>
> In this case, if PCIDE=0 then bit 63 is set in rsvd and therefore check_cr_write() will emulate_gp() as needed.

Ok, I misread your first reply, will send out v2.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
@ 2018-05-13  9:09 Liran Alon
  2018-05-13  9:14 ` Wanpeng Li
  0 siblings, 1 reply; 7+ messages in thread
From: Liran Alon @ 2018-05-13  9:09 UTC (permalink / raw)
  To: kernellwp; +Cc: rkrcmar, pbonzini, linux-kernel, kvm, junaids


----- kernellwp@gmail.com wrote:

> 2018-05-13 16:28 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> >
> > ----- kernellwp@gmail.com wrote:
> >
> >> 2018-05-13 15:53 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> >> >
> >> > ----- kernellwp@gmail.com wrote:
> >> >
> >> >> From: Wanpeng Li <wanpengli@tencent.com>
> >> >>
> >> >> MSB of CR3 is a reserved bit if the PCIDE bit is not set in
> CR4.
> >> >> It should be checked when PCIDE bit is not set, however commit
> >> >> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based
> on
> >> >> its physical address width")' removes the bit 63 checking
> >> >> unconditionally. This patch fixes it by checking bit 63 of CR3
> >> >> when PCIDE bit is not set in CR4.
> >> >>
> >> >> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits
> based
> >> on
> >> >> its physical address width)
> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
> >> >> Cc: Junaid Shahid <junaids@google.com>
> >> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> >> >> ---
> >> >>  arch/x86/kvm/emulate.c | 4 +++-
> >> >>  arch/x86/kvm/x86.c     | 2 +-
> >> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >> >> index b3705ae..b21f427 100644
> >> >> --- a/arch/x86/kvm/emulate.c
> >> >> +++ b/arch/x86/kvm/emulate.c
> >> >> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
> >> >> x86_emulate_ctxt *ctxt)
> >> >>                               maxphyaddr = eax & 0xff;
> >> >>                       else
> >> >>                               maxphyaddr = 36;
> >> >> -                     rsvd = rsvd_bits(maxphyaddr, 62);
> >> >> +                     if (ctxt->ops->get_cr(ctxt, 4) &
> >> X86_CR4_PCIDE)
> >> >> +                             new_val &= ~CR3_PCID_INVD;
> >> >> +                     rsvd = rsvd_bits(maxphyaddr, 63);
> >> >
> >> > I would prefer instead to do this:
> >> > if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> >> >     rsvd &= ~CR3_PCID_INVD;
> >> > It makes more sense as opposed to temporary removing the
> >> CR3_PCID_INVD bit from new_val.
> >>
> >> It tries the same way
> >>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_virt_kvm_kvm.git_commit_-3Fid-3Dc19986fea873f3c745122bf79013a872a190f212&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=r52WDgKBorUHwe_B_5Nw2Le_F_E0ne8lqqWW6n-3bSg&s=ufTcXvhhAMkY3XP6gAx-HiKCT8ynPWo2fs2z9DqCzM4&e=
> >> pointed out.
> >>
> >> Regards,
> >> Wanpeng Li
> >
> > Yes but there it makes sense as new CR3 value should not have bit 63
> set in vcpu->arch.cr3.
> 
> When X86_CR4_PCIDE == 0 and CR3 63 bit is set, a #GP is missing in
> your suggestion.
> 
> Regards,
> Wanpeng Li

Why?

I suggest the following change:
- rsvd = rsvd_bits(maxphyaddr, 62);
+ rsvd = rsvd_bits(maxphyaddr, 63);
+ if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
+     rsvd &= ~CR3_PCID_INVD;

In this case, if PCIDE=0 then bit 63 is set in rsvd and therefore check_cr_write() will emulate_gp() as needed.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
  2018-05-13  8:28 Liran Alon
@ 2018-05-13  8:54 ` Wanpeng Li
  0 siblings, 0 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-05-13  8:54 UTC (permalink / raw)
  To: Liran Alon; +Cc: Radim Krcmar, Paolo Bonzini, LKML, Junaid Shahid, kvm

2018-05-13 16:28 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>
> ----- kernellwp@gmail.com wrote:
>
>> 2018-05-13 15:53 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>> >
>> > ----- kernellwp@gmail.com wrote:
>> >
>> >> From: Wanpeng Li <wanpengli@tencent.com>
>> >>
>> >> MSB of CR3 is a reserved bit if the PCIDE bit is not set in CR4.
>> >> It should be checked when PCIDE bit is not set, however commit
>> >> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based on
>> >> its physical address width")' removes the bit 63 checking
>> >> unconditionally. This patch fixes it by checking bit 63 of CR3
>> >> when PCIDE bit is not set in CR4.
>> >>
>> >> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits based
>> on
>> >> its physical address width)
>> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> >> Cc: Junaid Shahid <junaids@google.com>
>> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> >> ---
>> >>  arch/x86/kvm/emulate.c | 4 +++-
>> >>  arch/x86/kvm/x86.c     | 2 +-
>> >>  2 files changed, 4 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> >> index b3705ae..b21f427 100644
>> >> --- a/arch/x86/kvm/emulate.c
>> >> +++ b/arch/x86/kvm/emulate.c
>> >> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
>> >> x86_emulate_ctxt *ctxt)
>> >>                               maxphyaddr = eax & 0xff;
>> >>                       else
>> >>                               maxphyaddr = 36;
>> >> -                     rsvd = rsvd_bits(maxphyaddr, 62);
>> >> +                     if (ctxt->ops->get_cr(ctxt, 4) &
>> X86_CR4_PCIDE)
>> >> +                             new_val &= ~CR3_PCID_INVD;
>> >> +                     rsvd = rsvd_bits(maxphyaddr, 63);
>> >
>> > I would prefer instead to do this:
>> > if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
>> >     rsvd &= ~CR3_PCID_INVD;
>> > It makes more sense as opposed to temporary removing the
>> CR3_PCID_INVD bit from new_val.
>>
>> It tries the same way
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_virt_kvm_kvm.git_commit_-3Fid-3Dc19986fea873f3c745122bf79013a872a190f212&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=r52WDgKBorUHwe_B_5Nw2Le_F_E0ne8lqqWW6n-3bSg&s=ufTcXvhhAMkY3XP6gAx-HiKCT8ynPWo2fs2z9DqCzM4&e=
>> pointed out.
>>
>> Regards,
>> Wanpeng Li
>
> Yes but there it makes sense as new CR3 value should not have bit 63 set in vcpu->arch.cr3.

When X86_CR4_PCIDE == 0 and CR3 63 bit is set, a #GP is missing in
your suggestion.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
@ 2018-05-13  8:28 Liran Alon
  2018-05-13  8:54 ` Wanpeng Li
  0 siblings, 1 reply; 7+ messages in thread
From: Liran Alon @ 2018-05-13  8:28 UTC (permalink / raw)
  To: kernellwp; +Cc: rkrcmar, pbonzini, linux-kernel, junaids, kvm


----- kernellwp@gmail.com wrote:

> 2018-05-13 15:53 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> >
> > ----- kernellwp@gmail.com wrote:
> >
> >> From: Wanpeng Li <wanpengli@tencent.com>
> >>
> >> MSB of CR3 is a reserved bit if the PCIDE bit is not set in CR4.
> >> It should be checked when PCIDE bit is not set, however commit
> >> 'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based on
> >> its physical address width")' removes the bit 63 checking
> >> unconditionally. This patch fixes it by checking bit 63 of CR3
> >> when PCIDE bit is not set in CR4.
> >>
> >> Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits based
> on
> >> its physical address width)
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
> >> Cc: Junaid Shahid <junaids@google.com>
> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> >> ---
> >>  arch/x86/kvm/emulate.c | 4 +++-
> >>  arch/x86/kvm/x86.c     | 2 +-
> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >> index b3705ae..b21f427 100644
> >> --- a/arch/x86/kvm/emulate.c
> >> +++ b/arch/x86/kvm/emulate.c
> >> @@ -4189,7 +4189,9 @@ static int check_cr_write(struct
> >> x86_emulate_ctxt *ctxt)
> >>                               maxphyaddr = eax & 0xff;
> >>                       else
> >>                               maxphyaddr = 36;
> >> -                     rsvd = rsvd_bits(maxphyaddr, 62);
> >> +                     if (ctxt->ops->get_cr(ctxt, 4) &
> X86_CR4_PCIDE)
> >> +                             new_val &= ~CR3_PCID_INVD;
> >> +                     rsvd = rsvd_bits(maxphyaddr, 63);
> >
> > I would prefer instead to do this:
> > if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> >     rsvd &= ~CR3_PCID_INVD;
> > It makes more sense as opposed to temporary removing the
> CR3_PCID_INVD bit from new_val.
> 
> It tries the same way
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_virt_kvm_kvm.git_commit_-3Fid-3Dc19986fea873f3c745122bf79013a872a190f212&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=r52WDgKBorUHwe_B_5Nw2Le_F_E0ne8lqqWW6n-3bSg&s=ufTcXvhhAMkY3XP6gAx-HiKCT8ynPWo2fs2z9DqCzM4&e=
> pointed out.
> 
> Regards,
> Wanpeng Li

Yes but there it makes sense as new CR3 value should not have bit 63 set in vcpu->arch.cr3.

> 
> >
> >>               }
> >>
> >>               if (new_val & rsvd)
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 87e4805..9a90668 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -863,7 +863,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
> unsigned
> >> long cr3)
> >>       }
> >>
> >>       if (is_long_mode(vcpu) &&
> >> -         (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 62)))
> >> +         (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
> >>               return 1;
> >>       else if (is_pae(vcpu) && is_paging(vcpu) &&
> >>                  !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
> >> --
> >> 2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] KVM: X86: Fix CR3 reserve bits
@ 2018-05-13  3:22 Wanpeng Li
  0 siblings, 0 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-05-13  3:22 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář, Junaid Shahid

From: Wanpeng Li <wanpengli@tencent.com>

MSB of CR3 is a reserved bit if the PCIDE bit is not set in CR4. 
It should be checked when PCIDE bit is not set, however commit 
'd1cd3ce900441 ("KVM: MMU: check guest CR3 reserved bits based on 
its physical address width")' removes the bit 63 checking 
unconditionally. This patch fixes it by checking bit 63 of CR3 
when PCIDE bit is not set in CR4.

Fixes: d1cd3ce900441 (KVM: MMU: check guest CR3 reserved bits based on its physical address width)
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Junaid Shahid <junaids@google.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/emulate.c | 4 +++-
 arch/x86/kvm/x86.c     | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b3705ae..b21f427 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4189,7 +4189,9 @@ static int check_cr_write(struct x86_emulate_ctxt *ctxt)
 				maxphyaddr = eax & 0xff;
 			else
 				maxphyaddr = 36;
-			rsvd = rsvd_bits(maxphyaddr, 62);
+			if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
+				new_val &= ~CR3_PCID_INVD;
+			rsvd = rsvd_bits(maxphyaddr, 63);
 		}
 
 		if (new_val & rsvd)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 87e4805..9a90668 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -863,7 +863,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	}
 
 	if (is_long_mode(vcpu) &&
-	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 62)))
+	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
 		return 1;
 	else if (is_pae(vcpu) && is_paging(vcpu) &&
 		   !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-05-13  9:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-13  7:53 [PATCH 1/2] KVM: X86: Fix CR3 reserve bits Liran Alon
2018-05-13  8:25 ` Wanpeng Li
  -- strict thread matches above, loose matches on Subject: below --
2018-05-13  9:09 Liran Alon
2018-05-13  9:14 ` Wanpeng Li
2018-05-13  8:28 Liran Alon
2018-05-13  8:54 ` Wanpeng Li
2018-05-13  3:22 Wanpeng Li

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).