| Message ID | 20230322013731.102955-7-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: MMU: performance tweaks for heavy CR0.WP users | expand |
On 3/22/2023 9:37 AM, Mathias Krause wrote: > Guests like grsecurity that make heavy use of CR0.WP to implement kernel > level W^X will suffer from the implied VMEXITs. > > With EPT there is no need to intercept a guest change of CR0.WP, so > simply make it a guest owned bit if we can do so. I'm interested in the performance gain. Do you have data like Patch 2? > This implies that a read of a guest's CR0.WP bit might need a VMREAD. > However, the only potentially affected user seems to be kvm_init_mmu() > which is a heavy operation to begin with. But also most callers already > cache the full value of CR0 anyway, so no additional VMREAD is needed. > The only exception is nested_vmx_load_cr3(). > > This change is VMX-specific, as SVM has no such fine grained control > register intercept control. > > Suggested-and-co-developed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Mathias Krause <minipli@grsecurity.net> > --- > arch/x86/kvm/kvm_cache_regs.h | 2 +- > arch/x86/kvm/vmx/nested.c | 4 ++-- > arch/x86/kvm/vmx/vmx.c | 2 +- > arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++ > 4 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 4c91f626c058..e50d353b5c1c 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -4,7 +4,7 @@ > > #include <linux/kvm_host.h> > > -#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS > +#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP) > #define KVM_POSSIBLE_CR4_GUEST_BITS \ > (X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \ > | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE) > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index f63b28f46a71..61d940fc91ba 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, > * CR0_GUEST_HOST_MASK is already set in the original vmcs01 > * (KVM doesn't change it); > */ > - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmx_set_cr0(vcpu, vmcs12->host_cr0); > > /* Same as above - no reason to call set_cr4_guest_host_mask(). */ > @@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) > */ > vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx)); > > - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW)); > > vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK); > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 8fc1a0c7856f..e501f6864a72 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx) > /* 22.2.1, 20.8.1 */ > vm_entry_controls_set(vmx, vmx_vmentry_ctrl()); > > - vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits); > > set_cr4_guest_host_mask(vmx); > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 2acdc54bc34b..423e9d3c9c40 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64) > (1 << VCPU_EXREG_EXIT_INFO_1) | \ > (1 << VCPU_EXREG_EXIT_INFO_2)) > > +static inline unsigned long vmx_l1_guest_owned_cr0_bits(void) > +{ > + unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + > + /* > + * CR0.WP needs to be intercepted when KVM is shadowing legacy paging > + * in order to construct shadow PTEs with the correct protections. > + * Note! CR0.WP technically can be passed through to the guest if > + * paging is disabled, but checking CR0.PG would generate a cyclical > + * dependency of sorts due to forcing the caller to ensure CR0 holds > + * the correct value prior to determining which CR0 bits can be owned > + * by L1. Keep it simple and limit the optimization to EPT. > + */ > + if (!enable_ept) > + bits &= ~X86_CR0_WP; > + return bits; > +} > + > static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm) > { > return container_of(kvm, struct kvm_vmx, kvm);
On 27.03.23 10:33, Xiaoyao Li wrote: > On 3/22/2023 9:37 AM, Mathias Krause wrote: >> Guests like grsecurity that make heavy use of CR0.WP to implement kernel >> level W^X will suffer from the implied VMEXITs. >> >> With EPT there is no need to intercept a guest change of CR0.WP, so >> simply make it a guest owned bit if we can do so. > > I'm interested in the performance gain. Do you have data like Patch 2? It's mentioned in the cover letter[1], quoted below: [1] https://lore.kernel.org/kvm/20230322013731.102955-1-minipli@grsecurity.net/ : I used 'ssdd 10 50000' from rt-tests[5] as a micro-benchmark, running on a : grsecurity L1 VM. Below table shows the results (runtime in seconds, lower : is better): : : legacy TDP shadow : kvm-x86/next@d8708b 8.43s 9.45s 70.3s : + patches 1-3 5.39s 5.63s 70.2s : + patches 4-6 3.51s 3.47s 67.8s Thanks, Mathias
On 3/27/2023 4:37 PM, Mathias Krause wrote: > On 27.03.23 10:33, Xiaoyao Li wrote: >> On 3/22/2023 9:37 AM, Mathias Krause wrote: >>> Guests like grsecurity that make heavy use of CR0.WP to implement kernel >>> level W^X will suffer from the implied VMEXITs. >>> >>> With EPT there is no need to intercept a guest change of CR0.WP, so >>> simply make it a guest owned bit if we can do so. >> >> I'm interested in the performance gain. Do you have data like Patch 2? > > It's mentioned in the cover letter[1], quoted below: Sorry I missed it. The data of not intercepting CR0.WP looks great as well. > [1] > https://lore.kernel.org/kvm/20230322013731.102955-1-minipli@grsecurity.net/ > > : I used 'ssdd 10 50000' from rt-tests[5] as a micro-benchmark, running on a > : grsecurity L1 VM. Below table shows the results (runtime in seconds, lower > : is better): > : > : legacy TDP shadow > : kvm-x86/next@d8708b 8.43s 9.45s 70.3s > : + patches 1-3 5.39s 5.63s 70.2s > : + patches 4-6 3.51s 3.47s 67.8s > > > Thanks, > Mathias
On 22.03.23 02:37, Mathias Krause wrote: > Guests like grsecurity that make heavy use of CR0.WP to implement kernel > level W^X will suffer from the implied VMEXITs. > > With EPT there is no need to intercept a guest change of CR0.WP, so > simply make it a guest owned bit if we can do so. > > This implies that a read of a guest's CR0.WP bit might need a VMREAD. > However, the only potentially affected user seems to be kvm_init_mmu() > which is a heavy operation to begin with. But also most callers already > cache the full value of CR0 anyway, so no additional VMREAD is needed. > The only exception is nested_vmx_load_cr3(). > > This change is VMX-specific, as SVM has no such fine grained control > register intercept control. Just a heads up! We did more tests, especially with the backports we did internally already, and ran into a bug when running a nested guest on an ESXi host. Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2) The Linux system, especially the kernel, is the same for L1 and L2. It's a grsecurity kernel, so makes use of toggling CR0.WP at runtime. The bug we see is that when L2 disables CR0.WP and tries to write to an r/o memory region (implicitly to the r/o GDT via LTR in our use case), this triggers a fault (EPT violation?) that gets ignored by L1, as, apparently, everything is fine from its point of view. I suspect the L0 VMM to be at fault here, as the VMCS structures look good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault(): […] VMX: TRIPLE FAULT! […] VMCS ffff8883a9f18000, last attempted VM-entry on CPU 8 […] *** Guest State *** […] CR0: actual=0x0000000080040033, shadow=0x0000000080050033, gh_mask=fffffffffffefff7 CR0 in the L2 VM has CR0.WP disabled. However, it is set in the shadow CR0 but masked out via CR0_MASK, so should be read as the guest's value, according to the SDM. I also tried masking the shadow CR0 value in vmx_set_cr0(), but that makes no difference. […] CR4: actual=0x00000000003220f0, shadow=0x00000000003208b0, gh_mask=fffffffffffff871 […] CR3 = 0x0000000002684000 […] PDPTR0 = 0x0000000007d39001 PDPTR1 = 0x00000000033b5001 […] PDPTR2 = 0x000000000238c001 PDPTR3 = 0x0000000001c54001 […] RSP = 0xfffffe8040087f50 RIP = 0xffffffff8105d435 […] RFLAGS=0x00010006 DR7 = 0x0000000000000400 […] Sysenter RSP=0000000000000000 CS:RIP=0000:0000000000000000 […] CS: sel=0x0010, attr=0x0a09b, limit=0xffffffff, base=0x0000000000000000 […] DS: sel=0x0000, attr=0x1c001, limit=0xffffffff, base=0x0000000000000000 […] SS: sel=0x0000, attr=0x1c001, limit=0xffffffff, base=0x0000000000000000 […] ES: sel=0x0000, attr=0x1c001, limit=0xffffffff, base=0x0000000000000000 […] FS: sel=0x0000, attr=0x1c001, limit=0xffffffff, base=0x0000000000000000 […] GS: sel=0x0000, attr=0x1c001, limit=0xffffffff, base=0xffff888232e00000 […] GDTR: limit=0x00000097, base=0xfffffe0000201000 […] LDTR: sel=0x0000, attr=0x00082, limit=0x0000ffff, base=0x0000000000000000 […] IDTR: limit=0x000001ff, base=0xffffffff84004000 […] TR: sel=0x0000, attr=0x0008b, limit=0x0000ffff, base=0x0000000000000000 […] EFER= 0x0000000000000d01 (effective) […] PAT = 0x0007040600070406 […] DebugCtl = 0x0000000000000000 DebugExceptions = 0x0000000000000000 […] Interruptibility = 00000000 ActivityState = 00000000 […] *** Host State *** […] RIP = 0xffffffff86d28db6 RSP = 0xfffffe8040927d50 […] CS=0010 SS=0018 DS=0000 ES=0000 FS=0000 GS=0000 TR=0040 […] FSBase=0000639563fce700 GSBase=ffff88881a800000 TRBase=fffffe0001003000 […] GDTBase=fffffe0001001000 IDTBase=fffffe0000000000 […] CR0=0000000080050033 CR3=00000000026a0004 CR4=00000000007626f0 The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is what I think confuses ESXi to enforce the read-only property to the L2 guest as well -- for unknown reasons so far. […] Sysenter RSP=fffffe0001003000 CS:RIP=0010:ffffffff810031a0 […] PAT = 0x0407050600070106 […] *** Control State *** […] PinBased=0000003f CPUBased=b5a06dfa SecondaryExec=001034ee […] EntryControls=000053ff ExitControls=000befff […] ExceptionBitmap=00060042 PFECmask=00000000 PFECmatch=00000000 […] VMEntry: intr_info=00000000 errcode=00000003 ilen=00000000 […] VMExit: intr_info=00000000 errcode=00000000 ilen=00000000 […] reason=00000002 qualification=0000000000000000 […] IDTVectoring: info=00000000 errcode=00000000 […] TSC Offset = 0xffffad7a1057f4cc […] TPR Threshold = 0x00 […] virt-APIC addr = 0x000000015716b000 […] EPT pointer = 0x0000000583c1e05e […] PLE Gap=00000080 Window=00001000 […] Virtual processor ID = 0x0002 I tried to reproduce the bug with different KVM based L0 VMMs (with and without this series; vanilla and grsecurity kernels) but no luck. That's why I'm suspecting a ESXi bug. I'm leaning to make CR0.WP guest owned only iff we're running on bare metal or the VMM is KVM to not play whack-a-mole for all the VMMs that might have similar bugs. (Will try to test a few others here as well.) However, that would prevent them from getting the performance gain, so I'd rather have this fixed / worked around in KVM instead. Any ideas how to investigate this further? Thanks, Mathias PS: ...should have left the chicken bit of v3 to be able to disable the feature by a module parameter ;) > > Suggested-and-co-developed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Mathias Krause <minipli@grsecurity.net> > --- > arch/x86/kvm/kvm_cache_regs.h | 2 +- > arch/x86/kvm/vmx/nested.c | 4 ++-- > arch/x86/kvm/vmx/vmx.c | 2 +- > arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++ > 4 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 4c91f626c058..e50d353b5c1c 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -4,7 +4,7 @@ > > #include <linux/kvm_host.h> > > -#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS > +#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP) > #define KVM_POSSIBLE_CR4_GUEST_BITS \ > (X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \ > | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE) > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index f63b28f46a71..61d940fc91ba 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, > * CR0_GUEST_HOST_MASK is already set in the original vmcs01 > * (KVM doesn't change it); > */ > - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmx_set_cr0(vcpu, vmcs12->host_cr0); > > /* Same as above - no reason to call set_cr4_guest_host_mask(). */ > @@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) > */ > vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx)); > > - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW)); > > vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK); > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 8fc1a0c7856f..e501f6864a72 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx) > /* 22.2.1, 20.8.1 */ > vm_entry_controls_set(vmx, vmx_vmentry_ctrl()); > > - vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); > vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits); > > set_cr4_guest_host_mask(vmx); > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 2acdc54bc34b..423e9d3c9c40 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64) > (1 << VCPU_EXREG_EXIT_INFO_1) | \ > (1 << VCPU_EXREG_EXIT_INFO_2)) > > +static inline unsigned long vmx_l1_guest_owned_cr0_bits(void) > +{ > + unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS; > + > + /* > + * CR0.WP needs to be intercepted when KVM is shadowing legacy paging > + * in order to construct shadow PTEs with the correct protections. > + * Note! CR0.WP technically can be passed through to the guest if > + * paging is disabled, but checking CR0.PG would generate a cyclical > + * dependency of sorts due to forcing the caller to ensure CR0 holds > + * the correct value prior to determining which CR0 bits can be owned > + * by L1. Keep it simple and limit the optimization to EPT. > + */ > + if (!enable_ept) > + bits &= ~X86_CR0_WP; > + return bits; > +} > + > static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm) > { > return container_of(kvm, struct kvm_vmx, kvm);
On Thu, Mar 30, 2023, Mathias Krause wrote: > On 22.03.23 02:37, Mathias Krause wrote: > > Guests like grsecurity that make heavy use of CR0.WP to implement kernel > > level W^X will suffer from the implied VMEXITs. > > > > With EPT there is no need to intercept a guest change of CR0.WP, so > > simply make it a guest owned bit if we can do so. > > > > This implies that a read of a guest's CR0.WP bit might need a VMREAD. > > However, the only potentially affected user seems to be kvm_init_mmu() > > which is a heavy operation to begin with. But also most callers already > > cache the full value of CR0 anyway, so no additional VMREAD is needed. > > The only exception is nested_vmx_load_cr3(). > > > > This change is VMX-specific, as SVM has no such fine grained control > > register intercept control. > > Just a heads up! We did more tests, especially with the backports we did > internally already, and ran into a bug when running a nested guest on an > ESXi host. > > Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2) > > The Linux system, especially the kernel, is the same for L1 and L2. It's > a grsecurity kernel, so makes use of toggling CR0.WP at runtime. > > The bug we see is that when L2 disables CR0.WP and tries to write to an > r/o memory region (implicitly to the r/o GDT via LTR in our use case), > this triggers a fault (EPT violation?) that gets ignored by L1, as, > apparently, everything is fine from its point of view. It's not an EPT violation if there's a triple fault. Given that you're dumping the VMCS from handle_triple_fault(), I assume that L2 gets an unexpected #PF that leads to a triple fault in L2. Just to make sure we're on the same page: L1 is still alive after this, correct? > I suspect the L0 VMM to be at fault here, as the VMCS structures look > good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault(): ... > The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is > what I think confuses ESXi to enforce the read-only property to the L2 > guest as well -- for unknown reasons so far. ... > I tried to reproduce the bug with different KVM based L0 VMMs (with and > without this series; vanilla and grsecurity kernels) but no luck. That's > why I'm suspecting a ESXi bug. ... > I'm leaning to make CR0.WP guest owned only iff we're running on bare > metal or the VMM is KVM to not play whack-a-mole for all the VMMs that > might have similar bugs. (Will try to test a few others here as well.) > However, that would prevent them from getting the performance gain, so > I'd rather have this fixed / worked around in KVM instead. Before we start putting bandaids on this, let's (a) confirm this appears to be an issue with ESXi and (b) pull in VMware folks to get their input. > Any ideas how to investigate this further? Does the host in question support UMIP? Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting any kind of exception? E.g. to get the KVM kitchen sink: echo 1 > /sys/kernel/debug/tracing/tracing_on echo 1 > /sys/kernel/debug/tracing/events/kvm/enable cat /sys/kernel/debug/tracing/trace > log Or if that's too noisy, a more targeted trace (exception injection + emulation) woud be: echo 1 > /sys/kernel/debug/tracing/tracing_on echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_emulate_insn/enable echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_inj_exception/enable echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_entry/enable echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_exit/enable > PS: ...should have left the chicken bit of v3 to be able to disable the > feature by a module parameter ;) A chicken bit isn't a good solution for this sort of thing. Toggling a KVM module param requires (a) knowing that it exists and (b) knowing the conditions under which it is/isn't safe to toggle the bit. E.g. if this ends up being an ESXi L0 bug, then an option might be to add something in vmware_platform_setup() to communicate the bug to KVM so that KVM can precisely disable the optimization on affected platforms.
On 30.03.23 19:12, Sean Christopherson wrote: > On Thu, Mar 30, 2023, Mathias Krause wrote: >> Just a heads up! We did more tests, especially with the backports we did >> internally already, and ran into a bug when running a nested guest on an >> ESXi host. >> >> Setup is like: ESXi (L0) -> Linux (L1) -> Linux (L2) >> >> The Linux system, especially the kernel, is the same for L1 and L2. It's >> a grsecurity kernel, so makes use of toggling CR0.WP at runtime. >> >> The bug we see is that when L2 disables CR0.WP and tries to write to an >> r/o memory region (implicitly to the r/o GDT via LTR in our use case), >> this triggers a fault (EPT violation?) that gets ignored by L1, as, >> apparently, everything is fine from its point of view. > > It's not an EPT violation if there's a triple fault. Given that you're dumping > the VMCS from handle_triple_fault(), I assume that L2 gets an unexpected #PF that > leads to a triple fault in L2. Indeed it does, more on this below. > > Just to make sure we're on the same page: L1 is still alive after this, correct? Yes, L1 is still alive and doing fine. >> I suspect the L0 VMM to be at fault here, as the VMCS structures look >> good, IMO. Here is a dump of vmx->loaded_vmcs in handle_triple_fault(): > > ... > >> The "host" (which is our L1 VMM, I guess) has CR0.WP enabled and that is >> what I think confuses ESXi to enforce the read-only property to the L2 >> guest as well -- for unknown reasons so far. > > ... > >> I tried to reproduce the bug with different KVM based L0 VMMs (with and >> without this series; vanilla and grsecurity kernels) but no luck. That's >> why I'm suspecting a ESXi bug. > > ... > >> I'm leaning to make CR0.WP guest owned only iff we're running on bare >> metal or the VMM is KVM to not play whack-a-mole for all the VMMs that >> might have similar bugs. (Will try to test a few others here as well.) >> However, that would prevent them from getting the performance gain, so >> I'd rather have this fixed / worked around in KVM instead. > > Before we start putting bandaids on this, let's (a) confirm this appears to be > an issue with ESXi and (b) pull in VMware folks to get their input. > >> Any ideas how to investigate this further? > > Does the host in question support UMIP? It should, but I've no access to the host. Within L1 I don't see umip in /proc/cpuinfo. It's a "Intel(R) Xeon(R) Gold 6258R", though, so Cascade Lake. > > Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting > any kind of exception? E.g. to get the KVM kitchen sink: > > echo 1 > /sys/kernel/debug/tracing/tracing_on > echo 1 > /sys/kernel/debug/tracing/events/kvm/enable > > cat /sys/kernel/debug/tracing/trace > log > > Or if that's too noisy, a more targeted trace (exception injection + emulation) > woud be: I've even added 'echo 1 > /sys/kernel/tracing/events/kvmmmu/enable' to the mix to get some more info and that leads to below (trimmed to the part covering the error; L2 has 2 vCPUs bound to L1's vCPUs 1 and 2): qemu-system-x86-8397 [002] d.... 1404.389366: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff8100007a info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389373: kvm_cr: cr_write 0 = 0x80050033 That's the initialization of L2's CR0 in secondary_startup_64() and also the last CR0 write we'll see, as toggling CR0.WP won't generate a VMEXIT because the bit is guest owned. So this is the value KVM will cache as vcpu->arch.cr0. qemu-system-x86-8397 [002] ..... 1404.389384: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 4 old_spte 2c583e907 new_spte 0 qemu-system-x86-8397 [002] ..... 1404.389384: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l3 8-byte q0 direct wux nxe ad root 0 sync qemu-system-x86-8397 [002] ..... 1404.389386: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 3 old_spte 2c583d907 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389386: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l2 8-byte q0 direct wux nxe ad root 0 sync qemu-system-x86-8397 [002] ..... 1404.389388: kvm_tdp_mmu_spte_changed: as id 0 gfn 0 level 2 old_spte 2c583c907 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389388: kvm_mmu_prepare_zap_page: sp gen 0 gfn 0 l1 8-byte q0 direct wux nxe ad root 0 sync qemu-system-x86-8397 [002] ..... 1404.389391: kvm_tdp_mmu_spte_changed: as id 0 gfn 99 level 1 old_spte 600000455299b77 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389393: kvm_tdp_mmu_spte_changed: as id 0 gfn 9a level 1 old_spte 60000045529ab77 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389395: kvm_tdp_mmu_spte_changed: as id 0 gfn 9c level 1 old_spte 60000045529cb77 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389408: kvm_tdp_mmu_spte_changed: as id 0 gfn 9d level 1 old_spte 60000045529db77 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389411: kvm_tdp_mmu_spte_changed: as id 0 gfn 9e level 1 old_spte 60000045529eb77 new_spte 5a0 qemu-system-x86-8396 [001] d.... 1404.389421: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8108aca7 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000fb error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389425: kvm_tdp_mmu_spte_changed: as id 0 gfn 1000 level 2 old_spte 2c583b907 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389425: kvm_mmu_prepare_zap_page: sp gen 0 gfn 1000 l1 8-byte q0 direct wux nxe ad root 0 sync qemu-system-x86-8397 [002] ..... 1404.389427: kvm_tdp_mmu_spte_changed: as id 0 gfn 1000 level 1 old_spte 6000002c7c00b77 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389430: kvm_tdp_mmu_spte_changed: as id 0 gfn 2400 level 2 old_spte 60000021be00bf3 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389434: kvm_tdp_mmu_spte_changed: as id 0 gfn 2800 level 2 old_spte 60000021ba00bf3 new_spte 5a0 qemu-system-x86-8396 [001] d.... 1404.389435: kvm_entry: vcpu 0, rip 0xffffffff8108aca7 qemu-system-x86-8397 [002] ..... 1404.389436: kvm_tdp_mmu_spte_changed: as id 0 gfn 2a00 level 2 old_spte 60000021d400bf3 new_spte 5a0 qemu-system-x86-8397 [002] ..... 1404.389439: kvm_tdp_mmu_spte_changed: as id 0 gfn 2c00 level 2 old_spte 60000021d600bf3 new_spte 5a0 qemu-system-x86-8396 [001] d.... 1404.389448: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8113b1f9 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000fb error_code 0x00000000 qemu-system-x86-8396 [001] d.... 1404.389456: kvm_entry: vcpu 0, rip 0xffffffff8113b1f9 qemu-system-x86-8397 [002] d.... 1404.389461: kvm_entry: vcpu 1, rip 0xffffffff8100007d qemu-system-x86-8396 [001] d.... 1404.389462: kvm_exit: vcpu 0 reason EXTERNAL_INTERRUPT rip 0xffffffff8113b1f9 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000f6 error_code 0x00000000 qemu-system-x86-8396 [001] d.... 1404.389462: kvm_entry: vcpu 0, rip 0xffffffff8113b1f9 qemu-system-x86-8397 [002] d.... 1404.389481: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff810000a0 info1 0x0000000000000104 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389488: kvm_cr: cr_write 4 = 0xb0 Above is L2's init of CR4, still in secondary_startup_64(). qemu-system-x86-8397 [002] d.... 1404.389491: kvm_entry: vcpu 1, rip 0xffffffff810000a3 qemu-system-x86-8397 [002] d.... 1404.389501: kvm_exit: vcpu 1 reason CPUID rip 0xffffffff81000127 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389501: kvm_cpuid: func 80000001 idx d3f0eef rax 50657 rbx 0 rcx 121 rdx 2c100800, cpuid entry found qemu-system-x86-8397 [002] d.... 1404.389501: kvm_entry: vcpu 1, rip 0xffffffff81000129 qemu-system-x86-8397 [002] d.... 1404.389509: kvm_exit: vcpu 1 reason MSR_READ rip 0xffffffff81000130 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389509: kvm_msr: msr_read c0000080 = 0xd01 qemu-system-x86-8397 [002] d.... 1404.389510: kvm_entry: vcpu 1, rip 0xffffffff81000132 qemu-system-x86-8397 [002] d.... 1404.389517: kvm_exit: vcpu 1 reason MSR_WRITE rip 0xffffffff81000149 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389519: kvm_msr: msr_write c0000080 = 0xd01 That was the EFER setup... qemu-system-x86-8397 [002] d.... 1404.389520: kvm_entry: vcpu 1, rip 0xffffffff8100014b qemu-system-x86-8397 [002] d.... 1404.389536: kvm_exit: vcpu 1 reason CR_ACCESS rip 0xffffffff81053aee info1 0x0000000000000004 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389537: kvm_cr: cr_write 4 = 0x3208b0 ...remaining CR4 bits in cr4_init()... qemu-system-x86-8397 [002] d.... 1404.389562: kvm_entry: vcpu 1, rip 0xffffffff81053af1 qemu-system-x86-8397 [002] d.... 1404.389574: kvm_exit: vcpu 1 reason EXTERNAL_INTERRUPT rip 0xffffffff81053af1 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x800000f6 error_code 0x00000000 qemu-system-x86-8397 [002] d.... 1404.389583: kvm_entry: vcpu 1, rip 0xffffffff81053af1 qemu-system-x86-8397 [002] d.... 1404.389594: kvm_exit: vcpu 1 reason MSR_WRITE rip 0xffffffff81054faa info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 qemu-system-x86-8397 [002] ..... 1404.389595: kvm_msr: msr_write c0000103 = 0x1 ...TSC_AUX in setup_getcpu()... qemu-system-x86-8397 [002] d.... 1404.389595: kvm_entry: vcpu 1, rip 0xffffffff81054fac qemu-system-x86-8397 [002] d.... 1404.389646: kvm_exit: vcpu 1 reason LDTR_TR rip 0xffffffff810551b8 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 ...and, finally, the trapping LTR in cpu_init_exception_handling()! Now I get why you where asking for UMIP. No, that warning in handle_desc() doesn't trigger for me. ;) But now the interesting part: the emulation of LTR: qemu-system-x86-8397 [002] ..... 1404.389656: kvm_mmu_pagetable_walk: addr ffffffff810551b8 pferr 10 F ~~~~~ Just a nit, but this 'pferr' should probably be 'access' as it's not an ---------------------------^ an error code (yet) but the access mode for this page table walk. qemu-system-x86-8397 [002] ..... 1404.389661: kvm_mmu_paging_element: pte 2929067 level 4 qemu-system-x86-8397 [002] ..... 1404.389662: kvm_mmu_paging_element: pte 2b2c063 level 3 qemu-system-x86-8397 [002] ..... 1404.389662: kvm_mmu_paging_element: pte 10001e3 level 2 qemu-system-x86-8397 [002] ..... 1404.389664: kvm_emulate_insn: 0:ffffffff810551b8:0f 00 d8 (prot64) Instruction fetch went fine. qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 0 qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 800000000292b063 level 4 qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002b2f063 level 3 qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002d31063 level 2 qemu-system-x86-8397 [002] ..... 1404.389666: kvm_mmu_paging_element: pte 8000000002f3a161 level 1 qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_pagetable_walk: addr fffffe0000201048 pferr 0 qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 800000000292b063 level 4 qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002b2f063 level 3 qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002d31063 level 2 qemu-system-x86-8397 [002] ..... 1404.389667: kvm_mmu_paging_element: pte 8000000002f3a161 level 1 Read of the descriptor as well, but... qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 2 W qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 800000000292b063 level 4 qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 8000000002b2f063 level 3 qemu-system-x86-8397 [002] ..... 1404.389668: kvm_mmu_paging_element: pte 8000000002d31063 level 2 qemu-system-x86-8397 [002] ..... 1404.389669: kvm_mmu_paging_element: pte 8000000002f3a161 level 1 qemu-system-x86-8397 [002] ..... 1404.389669: kvm_mmu_walker_error: pferr 3 P|W qemu-system-x86-8397 [002] ..... 1404.389675: kvm_mmu_pagetable_walk: addr fffffe0000201040 pferr 2 W qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 800000000292b063 level 4 qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 8000000002b2f063 level 3 qemu-system-x86-8397 [002] ..... 1404.389677: kvm_mmu_paging_element: pte 8000000002d31063 level 2 qemu-system-x86-8397 [002] ..... 1404.389678: kvm_mmu_paging_element: pte 8000000002f3a161 level 1 qemu-system-x86-8397 [002] ..... 1404.389678: kvm_mmu_walker_error: pferr 3 P|W qemu-system-x86-8397 [002] ..... 1404.389684: kvm_inj_exception: #PF (0x3) ...the write failed and that's the page fault you were looking for... qemu-system-x86-8397 [002] d.... 1404.389711: kvm_entry: vcpu 1, rip 0xffffffff810551b8 qemu-system-x86-8397 [002] d.... 1404.389730: kvm_exit: vcpu 1 reason TRIPLE_FAULT rip 0xffffffff810551b8 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000 ...and here the triple fault I was seeing. The page faults look legitimate on first sight, as the write bit isn't set in the PTE. However, CR0.WP is 0, as show earlier in the VMCS dump, so this shouldn't trigger a page fault. Looks like I was too early to blame ESXi and this is more like a bug in KVM's emulation of guest writes. I just don't see where the kvm_read_cr0() is missing, yet -- obviously before we do the page table walk, but even after adding one to init_emulate_ctxt() I still get the fault, so there must be more to it. :/ Maybe it's not a stale CR0 value but the page table walker not taking the guest's CR0.WP into account? Maybe a missing role update? > [...] > >> PS: ...should have left the chicken bit of v3 to be able to disable the >> feature by a module parameter ;) > > A chicken bit isn't a good solution for this sort of thing. Toggling a KVM module > param requires (a) knowing that it exists and (b) knowing the conditions under which > it is/isn't safe to toggle the bit. Sure. But it would have allowed me to notice that's indeed the last patch of the series that's the culprit. > > E.g. if this ends up being an ESXi L0 bug, then an option might be to add something > in vmware_platform_setup() to communicate the bug to KVM so that KVM can precisely > disable the optimization on affected platforms. Agreed. But, as noted above, the signs are pointing to a bug in KVM. Thanks, Mathias
On 30.03.23 22:15, Mathias Krause wrote: > [...] > Maybe it's not a stale CR0 value but the page table walker not taking > the guest's CR0.WP into account? Maybe a missing role update? Indeed, it is. This does the trick for me: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 31be188aa842..6a9e90725c84 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8372,6 +8372,9 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) init_decode_cache(ctxt); vcpu->arch.emulate_regs_need_sync_from_vcpu = false; + /* Ensure we're doing page table walks with an up2date MMU role */ + if ((vcpu->arch.cr0 ^ kvm_read_cr0(vcpu)) == X86_CR0_WP) + kvm_init_mmu(vcpu); } void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip) Very heavy weight and misplaced, but a start :) It should (1) be limited to VMX as that's the only one that would make CR0.WP a guest owned bit and (2) limited to emulated instructions that actually do write operations, as read are harmless, obviously. Mathias
On Thu, Mar 30, 2023, Sean Christopherson wrote: > On Thu, Mar 30, 2023, Mathias Krause wrote: > > On 22.03.23 02:37, Mathias Krause wrote: > > I'm leaning to make CR0.WP guest owned only iff we're running on bare > > metal or the VMM is KVM to not play whack-a-mole for all the VMMs that > > might have similar bugs. (Will try to test a few others here as well.) > > However, that would prevent them from getting the performance gain, so > > I'd rather have this fixed / worked around in KVM instead. > > Before we start putting bandaids on this, let's (a) confirm this appears to be > an issue with ESXi and (b) pull in VMware folks to get their input. > > > Any ideas how to investigate this further? > > Does the host in question support UMIP? > > Can you capture a tracepoint log from L1 to verify that L1 KVM is _not_ injecting > any kind of exception? E.g. to get the KVM kitchen sink: > > echo 1 > /sys/kernel/debug/tracing/tracing_on > echo 1 > /sys/kernel/debug/tracing/events/kvm/enable > > cat /sys/kernel/debug/tracing/trace > log > > Or if that's too noisy, a more targeted trace (exception injection + emulation) > woud be: > > echo 1 > /sys/kernel/debug/tracing/tracing_on > > echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_emulate_insn/enable > echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_inj_exception/enable > echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_entry/enable > echo 1 > /sys/kernel/debug/tracing/events/kvm/kvm_exit/enable Duh, this is likely a KVM bug. I expect the issue will go away if you revert fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit") KVM doesn't consume CR0.WP for _its_ MMU, but it does consume CR0.WP for the guest walker. By passing through CR0.WP, toggling only CR0.WP will not trap (obviously) and thus won't run through kvm_post_set_cr0(), thus resulting in stle information due to not invoking kvm_init_mmu(). I'm preetty sure I even called that we needed to refresh the permissions, but then obviously forgot to actually make that happen. I believe this will remedy the issue. If it does, I'll post a proper patch (likely won't be until next week). Compile tested only. --- arch/x86/kvm/mmu.h | 8 +++++++- arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index 89f532516a45..4a303aa735dd 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly, bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu); int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, u64 fault_address, char *insn, int insn_len); +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu); int kvm_mmu_load(struct kvm_vcpu *vcpu); void kvm_mmu_unload(struct kvm_vcpu *vcpu); @@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, u64 implicit_access = access & PFERR_IMPLICIT_ACCESS; bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC; int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1; - bool fault = (mmu->permissions[index] >> pte_access) & 1; u32 errcode = PFERR_PRESENT_MASK; + bool fault; + + if (tdp_enabled) + kvm_mmu_refresh_passthrough_bits(vcpu, mmu); + + fault = (mmu->permissions[index] >> pte_access) & 1; WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK)); if (unlikely(mmu->pkru_mask)) { diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4c874d4ec68f..2a63b5725f36 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu, return role; } +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) +{ + const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP); + + BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP); + BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS)); + + if (is_cr0_wp(mmu) == cr0_wp) + return; + + mmu->cpu_role.base.cr0_wp = cr0_wp; + reset_guest_paging_metadata(vcpu, mmu); +} + static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu) { /* tdp_root_level is architecture forced level, use it if nonzero */ base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393 --
On Thu, Mar 30, 2023, Mathias Krause wrote: > On 30.03.23 22:15, Mathias Krause wrote: > > [...] > > Maybe it's not a stale CR0 value but the page table walker not taking > > the guest's CR0.WP into account? Maybe a missing role update? > > Indeed, it is. This does the trick for me: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 31be188aa842..6a9e90725c84 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8372,6 +8372,9 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) > > init_decode_cache(ctxt); > vcpu->arch.emulate_regs_need_sync_from_vcpu = false; > + /* Ensure we're doing page table walks with an up2date MMU role */ > + if ((vcpu->arch.cr0 ^ kvm_read_cr0(vcpu)) == X86_CR0_WP) > + kvm_init_mmu(vcpu); > } > > void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip) > > Very heavy weight and misplaced, but a start :) > > It should (1) be limited to VMX as that's the only one that would make > CR0.WP a guest owned bit and (2) limited to emulated instructions that > actually do write operations, as read are harmless, obviously. For the record, I wrote my email before I saw this ;-)
On 30.03.23 22:33, Sean Christopherson wrote: > Duh, this is likely a KVM bug. I expect the issue will go away if you revert > > fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit") > > KVM doesn't consume CR0.WP for _its_ MMU, but it does consume CR0.WP for the > guest walker. By passing through CR0.WP, toggling only CR0.WP will not trap > (obviously) and thus won't run through kvm_post_set_cr0(), thus resulting in stle > information due to not invoking kvm_init_mmu(). That reasoning sounds familiar ;) > > I'm preetty sure I even called that we needed to refresh the permissions, but then > obviously forgot to actually make that happen. :( > > I believe this will remedy the issue. If it does, I'll post a proper patch > (likely won't be until next week). Compile tested only. > > --- > arch/x86/kvm/mmu.h | 8 +++++++- > arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 89f532516a45..4a303aa735dd 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly, > bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu); > int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, > u64 fault_address, char *insn, int insn_len); > +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu); > > int kvm_mmu_load(struct kvm_vcpu *vcpu); > void kvm_mmu_unload(struct kvm_vcpu *vcpu); > @@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > u64 implicit_access = access & PFERR_IMPLICIT_ACCESS; > bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC; > int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1; > - bool fault = (mmu->permissions[index] >> pte_access) & 1; > u32 errcode = PFERR_PRESENT_MASK; > + bool fault; > + > + if (tdp_enabled) > + kvm_mmu_refresh_passthrough_bits(vcpu, mmu); > + > + fault = (mmu->permissions[index] >> pte_access) & 1; > > WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK)); > if (unlikely(mmu->pkru_mask)) { > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4c874d4ec68f..2a63b5725f36 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu, > return role; > } > > +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) > +{ > + const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP); > + > + BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP); > + BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS)); > + > + if (is_cr0_wp(mmu) == cr0_wp) > + return; > + > + mmu->cpu_role.base.cr0_wp = cr0_wp; > + reset_guest_paging_metadata(vcpu, mmu); > +} > + > static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu) > { > /* tdp_root_level is architecture forced level, use it if nonzero */ > > base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393 I tested a backported version of this patch on v6.1 as that's what I was testing with and it worked fine. :) I'll do more thorough tests tomorrow and actually on kvm-x86/next's HEAD. Thanks a lot, Sean! Mathias
On 30.03.23 22:55, Mathias Krause wrote: > On 30.03.23 22:33, Sean Christopherson wrote: >> I believe this will remedy the issue. If it does, I'll post a proper patch >> (likely won't be until next week). Compile tested only. >> >> --- >> arch/x86/kvm/mmu.h | 8 +++++++- >> arch/x86/kvm/mmu/mmu.c | 14 ++++++++++++++ >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h >> index 89f532516a45..4a303aa735dd 100644 >> --- a/arch/x86/kvm/mmu.h >> +++ b/arch/x86/kvm/mmu.h >> @@ -113,6 +113,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly, >> bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu); >> int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, >> u64 fault_address, char *insn, int insn_len); >> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu); >> >> int kvm_mmu_load(struct kvm_vcpu *vcpu); >> void kvm_mmu_unload(struct kvm_vcpu *vcpu); >> @@ -184,8 +185,13 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, >> u64 implicit_access = access & PFERR_IMPLICIT_ACCESS; >> bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC; >> int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1; >> - bool fault = (mmu->permissions[index] >> pte_access) & 1; >> u32 errcode = PFERR_PRESENT_MASK; >> + bool fault; >> + >> + if (tdp_enabled) >> + kvm_mmu_refresh_passthrough_bits(vcpu, mmu); >> + >> + fault = (mmu->permissions[index] >> pte_access) & 1; >> >> WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK)); >> if (unlikely(mmu->pkru_mask)) { >> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c >> index 4c874d4ec68f..2a63b5725f36 100644 >> --- a/arch/x86/kvm/mmu/mmu.c >> +++ b/arch/x86/kvm/mmu/mmu.c >> @@ -5186,6 +5186,20 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu, >> return role; >> } >> >> +void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) >> +{ >> + const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP); >> + >> + BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP); >> + BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS)); >> + >> + if (is_cr0_wp(mmu) == cr0_wp) >> + return; >> + >> + mmu->cpu_role.base.cr0_wp = cr0_wp; >> + reset_guest_paging_metadata(vcpu, mmu); >> +} >> + >> static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu) >> { >> /* tdp_root_level is architecture forced level, use it if nonzero */ >> >> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393 > > I tested a backported version of this patch on v6.1 as that's what I was > testing with and it worked fine. :) > > I'll do more thorough tests tomorrow and actually on kvm-x86/next's HEAD. I extended the KUT CR0.WP tests and could confirm that (a) commit fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit") without the above change triggers errors both ways in the emulator (denies access while it should be allowed when CR0.WP=0; allows access while it should trigger a page fault when CR0.WP=1) and (b) the above patch fixes these. The tests are available here: https://lore.kernel.org/kvm/20230331135709.132713-1-minipli@grsecurity.net/ Thanks, Mathias
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h index 4c91f626c058..e50d353b5c1c 100644 --- a/arch/x86/kvm/kvm_cache_regs.h +++ b/arch/x86/kvm/kvm_cache_regs.h @@ -4,7 +4,7 @@ #include <linux/kvm_host.h> -#define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS +#define KVM_POSSIBLE_CR0_GUEST_BITS (X86_CR0_TS | X86_CR0_WP) #define KVM_POSSIBLE_CR4_GUEST_BITS \ (X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR \ | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index f63b28f46a71..61d940fc91ba 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4481,7 +4481,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, * CR0_GUEST_HOST_MASK is already set in the original vmcs01 * (KVM doesn't change it); */ - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); vmx_set_cr0(vcpu, vmcs12->host_cr0); /* Same as above - no reason to call set_cr4_guest_host_mask(). */ @@ -4632,7 +4632,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) */ vmx_set_efer(vcpu, nested_vmx_get_vmcs01_guest_efer(vmx)); - vcpu->arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; + vcpu->arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); vmx_set_cr0(vcpu, vmcs_readl(CR0_READ_SHADOW)); vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 8fc1a0c7856f..e501f6864a72 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4790,7 +4790,7 @@ static void init_vmcs(struct vcpu_vmx *vmx) /* 22.2.1, 20.8.1 */ vm_entry_controls_set(vmx, vmx_vmentry_ctrl()); - vmx->vcpu.arch.cr0_guest_owned_bits = KVM_POSSIBLE_CR0_GUEST_BITS; + vmx->vcpu.arch.cr0_guest_owned_bits = vmx_l1_guest_owned_cr0_bits(); vmcs_writel(CR0_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr0_guest_owned_bits); set_cr4_guest_host_mask(vmx); diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 2acdc54bc34b..423e9d3c9c40 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -640,6 +640,24 @@ BUILD_CONTROLS_SHADOW(tertiary_exec, TERTIARY_VM_EXEC_CONTROL, 64) (1 << VCPU_EXREG_EXIT_INFO_1) | \ (1 << VCPU_EXREG_EXIT_INFO_2)) +static inline unsigned long vmx_l1_guest_owned_cr0_bits(void) +{ + unsigned long bits = KVM_POSSIBLE_CR0_GUEST_BITS; + + /* + * CR0.WP needs to be intercepted when KVM is shadowing legacy paging + * in order to construct shadow PTEs with the correct protections. + * Note! CR0.WP technically can be passed through to the guest if + * paging is disabled, but checking CR0.PG would generate a cyclical + * dependency of sorts due to forcing the caller to ensure CR0 holds + * the correct value prior to determining which CR0 bits can be owned + * by L1. Keep it simple and limit the optimization to EPT. + */ + if (!enable_ept) + bits &= ~X86_CR0_WP; + return bits; +} + static __always_inline struct kvm_vmx *to_kvm_vmx(struct kvm *kvm) { return container_of(kvm, struct kvm_vmx, kvm);
Guests like grsecurity that make heavy use of CR0.WP to implement kernel level W^X will suffer from the implied VMEXITs. With EPT there is no need to intercept a guest change of CR0.WP, so simply make it a guest owned bit if we can do so. This implies that a read of a guest's CR0.WP bit might need a VMREAD. However, the only potentially affected user seems to be kvm_init_mmu() which is a heavy operation to begin with. But also most callers already cache the full value of CR0 anyway, so no additional VMREAD is needed. The only exception is nested_vmx_load_cr3(). This change is VMX-specific, as SVM has no such fine grained control register intercept control. Suggested-and-co-developed-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Mathias Krause <minipli@grsecurity.net> --- arch/x86/kvm/kvm_cache_regs.h | 2 +- arch/x86/kvm/vmx/nested.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 2 +- arch/x86/kvm/vmx/vmx.h | 18 ++++++++++++++++++ 4 files changed, 22 insertions(+), 4 deletions(-)