* [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 related [flat|nested] 7+ messages in thread
* 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 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
* 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 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 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
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