diff mbox series

[RFC,1/2] kvm/x86: intercept guest changes to X86_CR4_LA57

Message ID 20200928083047.3349-1-jiangshanlai@gmail.com (mailing list archive)
State New, archived
Headers show
Series [RFC,1/2] kvm/x86: intercept guest changes to X86_CR4_LA57 | expand

Commit Message

Lai Jiangshan Sept. 28, 2020, 8:30 a.m. UTC 
From: Lai Jiangshan <laijs@linux.alibaba.com>

When shadowpaping is enabled, guest should not be allowed
to toggle X86_CR4_LA57. And X86_CR4_LA57 is a rarely changed
bit, so we can just intercept all the attempts to toggle it
no matter shadowpaping is in used or not.

Fixes: fd8cb433734ee ("KVM: MMU: Expose the LA57 feature to VM.")
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
  No test to toggle X86_CR4_LA57 in guest since I can't access to
  any CPU supports it. Maybe it is not a real problem.

 arch/x86/kvm/kvm_cache_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sean Christopherson Sept. 28, 2020, 4:24 p.m. UTC | #1 
On Mon, Sep 28, 2020 at 04:30:46PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> When shadowpaping is enabled, guest should not be allowed
> to toggle X86_CR4_LA57. And X86_CR4_LA57 is a rarely changed
> bit, so we can just intercept all the attempts to toggle it
> no matter shadowpaping is in used or not.
> 
> Fixes: fd8cb433734ee ("KVM: MMU: Expose the LA57 feature to VM.")
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>   No test to toggle X86_CR4_LA57 in guest since I can't access to
>   any CPU supports it. Maybe it is not a real problem.

LA57 doesn't need to be intercepted.  It can't be toggled in 64-bit mode
(causes a #GP), and it's ignored in 32-bit mode.  That means LA57 can only
take effect when 64-bit mode is enabled, at which time KVM will update its
MMU context accordingly.

>  arch/x86/kvm/kvm_cache_regs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index cfe83d4ae625..ca0781b41df9 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -7,7 +7,7 @@
>  #define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
>  #define KVM_POSSIBLE_CR4_GUEST_BITS				  \
>  	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR  \
> -	 | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_PGE | X86_CR4_TSD)
> +	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD)
>  
>  #define BUILD_KVM_GPR_ACCESSORS(lname, uname)				      \
>  static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu *vcpu)\
> -- 
> 2.19.1.6.gb485710b
>
Lai Jiangshan Sept. 29, 2020, 5:32 a.m. UTC | #2 
On Tue, Sep 29, 2020 at 12:24 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Sep 28, 2020 at 04:30:46PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <laijs@linux.alibaba.com>
> >
> > When shadowpaping is enabled, guest should not be allowed
> > to toggle X86_CR4_LA57. And X86_CR4_LA57 is a rarely changed
> > bit, so we can just intercept all the attempts to toggle it
> > no matter shadowpaping is in used or not.
> >
> > Fixes: fd8cb433734ee ("KVM: MMU: Expose the LA57 feature to VM.")
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > ---
> >   No test to toggle X86_CR4_LA57 in guest since I can't access to
> >   any CPU supports it. Maybe it is not a real problem.
>


Hello

Thanks for reviewing.

> LA57 doesn't need to be intercepted.  It can't be toggled in 64-bit mode
> (causes a #GP), and it's ignored in 32-bit mode.  That means LA57 can only
> take effect when 64-bit mode is enabled, at which time KVM will update its
> MMU context accordingly.
>

Oh, I missed that part which is so obvious that the patch
seems impertinent.

But X86_CR4_LA57 is so fundamental that it makes me afraid to
give it over to guests. And it is rarely changed too. At least,
there is no better reason to give it to the guest than
intercepting it.

There might be another reason that this patch is still needed with
an updated changelog.

When a user (via VMM such as qemu) launches a VM with LA57 disabled
in its cpuid on a LA57 enabled host. The hypervisor, IMO, needs to
intercept guest's changes to X86_CR4_LA57 even when the guest is still
in the non-paging mode. Otherwise the hypervisor failed to detective
such combination when the guest changes paging mode later.

Anyway, maybe it is still not a real problem.

Thanks
Lai
Sean Christopherson Sept. 29, 2020, 8:32 a.m. UTC | #3 
On Tue, Sep 29, 2020 at 01:32:45PM +0800, Lai Jiangshan wrote:
> On Tue, Sep 29, 2020 at 12:24 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 04:30:46PM +0800, Lai Jiangshan wrote:
> > > From: Lai Jiangshan <laijs@linux.alibaba.com>
> > >
> > > When shadowpaping is enabled, guest should not be allowed
> > > to toggle X86_CR4_LA57. And X86_CR4_LA57 is a rarely changed
> > > bit, so we can just intercept all the attempts to toggle it
> > > no matter shadowpaping is in used or not.
> > >
> > > Fixes: fd8cb433734ee ("KVM: MMU: Expose the LA57 feature to VM.")
> > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > > ---
> > >   No test to toggle X86_CR4_LA57 in guest since I can't access to
> > >   any CPU supports it. Maybe it is not a real problem.
> >
> 
> 
> Hello
> 
> Thanks for reviewing.
> 
> > LA57 doesn't need to be intercepted.  It can't be toggled in 64-bit mode
> > (causes a #GP), and it's ignored in 32-bit mode.  That means LA57 can only
> > take effect when 64-bit mode is enabled, at which time KVM will update its
> > MMU context accordingly.
> >
> 
> Oh, I missed that part which is so obvious that the patch
> seems impertinent.
> 
> But X86_CR4_LA57 is so fundamental that it makes me afraid to
> give it over to guests. And it is rarely changed too. At least,
> there is no better reason to give it to the guest than
> intercepting it.
> 
> There might be another reason that this patch is still needed with
> an updated changelog.
> 
> When a user (via VMM such as qemu) launches a VM with LA57 disabled
> in its cpuid on a LA57 enabled host. The hypervisor, IMO, needs to
> intercept guest's changes to X86_CR4_LA57 even when the guest is still
> in the non-paging mode. Otherwise the hypervisor failed to detective
> such combination when the guest changes paging mode later.
> 
> Anyway, maybe it is still not a real problem.

Oof, the above is a KVM bug, though in a more generic manner.  All reserved
bits should be intercepted, not just LA57.  LA57 is the only affected bit at
the moment, but proper support is needed as the follow-on patch to let the
guest toggle FSGSBASE would introduce the same bug.

Sadly, fixing this is a bit of a mess.  Well, fixing LA57 is easy, e.g. this
patch will do the trick.  But actually refreshing the CR4 guest/host mask when
the guest's CPUID is updated is a pain, and that's what's needed for proper
FSGSBASE support.

I'll send a series, bookended by these two RFC patches, with patches to
intercept CR4 reserved bits smushed in between.  I agree there's no point in
letting the guest write LA57 directly, it's almost literally a once-per-boot
thing.  I wouldn't be surprised if intercepting it is a net win (but still
inconsequential), e.g. due to the MMU having to grab CR4 out of the VMCS.
Lai Jiangshan Sept. 29, 2020, 8:45 a.m. UTC | #4 
On Tue, Sep 29, 2020 at 4:32 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Sep 29, 2020 at 01:32:45PM +0800, Lai Jiangshan wrote:
> > On Tue, Sep 29, 2020 at 12:24 AM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 04:30:46PM +0800, Lai Jiangshan wrote:
> > > > From: Lai Jiangshan <laijs@linux.alibaba.com>
> > > >
> > > > When shadowpaping is enabled, guest should not be allowed
> > > > to toggle X86_CR4_LA57. And X86_CR4_LA57 is a rarely changed
> > > > bit, so we can just intercept all the attempts to toggle it
> > > > no matter shadowpaping is in used or not.
> > > >
> > > > Fixes: fd8cb433734ee ("KVM: MMU: Expose the LA57 feature to VM.")
> > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > Cc: Yu Zhang <yu.c.zhang@linux.intel.com>
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > > > ---
> > > >   No test to toggle X86_CR4_LA57 in guest since I can't access to
> > > >   any CPU supports it. Maybe it is not a real problem.
> > >
> >
> >
> > Hello
> >
> > Thanks for reviewing.
> >
> > > LA57 doesn't need to be intercepted.  It can't be toggled in 64-bit mode
> > > (causes a #GP), and it's ignored in 32-bit mode.  That means LA57 can only
> > > take effect when 64-bit mode is enabled, at which time KVM will update its
> > > MMU context accordingly.
> > >
> >
> > Oh, I missed that part which is so obvious that the patch
> > seems impertinent.
> >
> > But X86_CR4_LA57 is so fundamental that it makes me afraid to
> > give it over to guests. And it is rarely changed too. At least,
> > there is no better reason to give it to the guest than
> > intercepting it.
> >
> > There might be another reason that this patch is still needed with
> > an updated changelog.
> >
> > When a user (via VMM such as qemu) launches a VM with LA57 disabled
> > in its cpuid on a LA57 enabled host. The hypervisor, IMO, needs to
> > intercept guest's changes to X86_CR4_LA57 even when the guest is still
> > in the non-paging mode. Otherwise the hypervisor failed to detective
> > such combination when the guest changes paging mode later.
> >
> > Anyway, maybe it is still not a real problem.
>
> Oof, the above is a KVM bug, though in a more generic manner.  All reserved
> bits should be intercepted, not just LA57.  LA57 is the only affected bit at
> the moment, but proper support is needed as the follow-on patch to let the
> guest toggle FSGSBASE would introduce the same bug.
>
> Sadly, fixing this is a bit of a mess.  Well, fixing LA57 is easy, e.g. this
> patch will do the trick.  But actually refreshing the CR4 guest/host mask when
> the guest's CPUID is updated is a pain, and that's what's needed for proper
> FSGSBASE support.
>
> I'll send a series, bookended by these two RFC patches, with patches to

Thanks for illustrating deep inside.
I'm looking forward to the series.

> intercept CR4 reserved bits smushed in between.  I agree there's no point in
> letting the guest write LA57 directly, it's almost literally a once-per-boot
> thing.  I wouldn't be surprised if intercepting it is a net win (but still
> inconsequential), e.g. due to the MMU having to grab CR4 out of the VMCS.
diff mbox series

Patch

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index cfe83d4ae625..ca0781b41df9 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -7,7 +7,7 @@ 
 #define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
 #define KVM_POSSIBLE_CR4_GUEST_BITS				  \
 	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR  \
-	 | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_PGE | X86_CR4_TSD)
+	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD)
 
 #define BUILD_KVM_GPR_ACCESSORS(lname, uname)				      \
 static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu *vcpu)\