| Message ID | 20230508154709.30043-1-minipli@grsecurity.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM CR0.WP series backport | expand |
On Mon, May 08, 2023, Mathias Krause wrote: > This is a backport of the CR0.WP KVM series[1] to Linux v5.15. It > differs from the v6.1 backport as in needing additional prerequisite > patches from Lai Jiangshan (and fixes for those) to ensure the > assumption it's safe to let CR0.WP be a guest owned bit still stand. NAK. The CR0.WP changes also very subtly rely on commit 2ba676774dfc ("KVM: x86/mmu: cleanup computation of MMU roles for two-dimensional paging"), which hardcodes WP=1 in the mmu role. Without that, KVM will end up in a weird state when reinitializing the MMU context without reloading the root, as KVM will effectively change the role of an active root. E.g. child pages in the legacy MMU will have a mix of WP=0 and WP=1 in their role. The inconsistency may or may not cause functional problems (I honestly don't know), but this missed dependency is exactly the type of problem that I am/was worried about with respect to backporting these changes all the way to 5.15. I'm simply not comfortable backporting these changes due to the number of modifications and enhancements that we've made to the TDP MMU, and to KVM's MMU handling in general, between 5.15 and 6.1.
[Paolo, can you please take a look at this as well?] On 11.05.23 23:15, Sean Christopherson wrote: > On Mon, May 08, 2023, Mathias Krause wrote: >> This is a backport of the CR0.WP KVM series[1] to Linux v5.15. It >> differs from the v6.1 backport as in needing additional prerequisite >> patches from Lai Jiangshan (and fixes for those) to ensure the >> assumption it's safe to let CR0.WP be a guest owned bit still stand. > > NAK. > > The CR0.WP changes also very subtly rely on commit 2ba676774dfc ("KVM: x86/mmu: > cleanup computation of MMU roles for two-dimensional paging"), which hardcodes > WP=1 in the mmu role. Well, that commit has the MMU role split into two (mmu_role and cpu_role) which was not the case for 5.15 and below. In fact, that split was more confusing than helpful, so commit 7a458f0e1ba1 ("KVM: x86/mmu: remove extended bits from mmu_role, rename field") /degraded/ mmu_role to root_role and made clear that bit test helpers like is_cr0_wp() look at cpu_role instead. In that regard, the backport should be equivalent to what we have in 6.4, as it's using mmu_role for the older kernels instead of cpu_role (which does not exist there yet). > Without that, KVM will end up in a weird state when > reinitializing the MMU context without reloading the root, as KVM will effectively > change the role of an active root. E.g. child pages in the legacy MMU will have > a mix of WP=0 and WP=1 in their role. But does that really matter? Or, asked differently, don't we have that very same situation for 6.4 with cpu_role.base.cr0_wp being the bit looked at and not root_role.cr0_wp? Either way, with EPT this should only matter if emulation is required and patch 8 makes sure we'll use the proper value of CR0.WP prior to starting the guest page table walk by refreshing the relevant cr0_wp bit. Or am I missing something? > > The inconsistency may or may not cause functional problems (I honestly don't know), > but this missed dependency is exactly the type of problem that I am/was worried > about with respect to backporting these changes all the way to 5.15. While doing the backports I carefully looked at the changes and differences between the trees and, honestly, I don't think I missed this dependency as I did account for the mmu_role->{cpu,root}_role split (or rather unification regarding the backport) as explained above. > I'm simply > not comfortable backporting these changes due to the number of modifications and > enhancements that we've made to the TDP MMU, and to KVM's MMU handling in general, > between 5.15 and 6.1. I understand your concerns, fiddling with the MMU implementation is not easy at all, as it has so many combinations to keep in mind and quite some implicit assumptions throughout the code. Moreover, I'm a newcomer to this part of the kernel. However, I tried very hard to look at the changes for the individual backports and double- or even triple-checked the code to make sure the changes are still consistent with the rest of the code base. But I'm just a human, so I might have missed something, but a vague bad feeling doesn't convince me that I did something wrong. Less so, as KUT supports me in not having messed up things too badly. I know your backlog is huge and your review time is a scarce resource. But could you or Paolo please take another look? Thanks, Mathias
This is a backport of the CR0.WP KVM series[1] to Linux v5.15. It differs from the v6.1 backport as in needing additional prerequisite patches from Lai Jiangshan (and fixes for those) to ensure the assumption it's safe to let CR0.WP be a guest owned bit still stand. I used 'ssdd 10 50000' from rt-tests[2] as a micro-benchmark, running on a grsecurity L1 VM. Below table shows the results (runtime in seconds, lower is better): legacy TDP shadow Linux v5.15.106 9.94s 66.1s 64.9s + patches 4.81s 4.79s 64.6s It's interesting to see that using the TDP MMU is even slower than shadow paging on a vanilla kernel, making the impact of this backport even more significant. The KVM unit test suite showed no regressions. Please consider applying. Thanks, Mathias [1] https://lore.kernel.org/kvm/20230322013731.102955-1-minipli@grsecurity.net/ [2] https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git Lai Jiangshan (3): KVM: X86: Don't reset mmu context when X86_CR4_PCIDE 1->0 KVM: X86: Don't reset mmu context when toggling X86_CR4_PGE KVM: x86/mmu: Reconstruct shadow page root if the guest PDPTEs is changed Mathias Krause (3): KVM: x86: Do not unload MMU roots when only toggling CR0.WP with TDP enabled KVM: x86: Make use of kvm_read_cr*_bits() when testing bits KVM: VMX: Make CR0.WP a guest owned bit Paolo Bonzini (1): KVM: x86/mmu: Avoid indirect call for get_cr3 Sean Christopherson (1): KVM: x86/mmu: Refresh CR0.WP prior to checking for emulated permission faults arch/x86/kvm/kvm_cache_regs.h | 2 +- arch/x86/kvm/mmu.h | 42 ++++++++++++++++++++++++++++++---- arch/x86/kvm/mmu/mmu.c | 27 +++++++++++++++++----- arch/x86/kvm/mmu/paging_tmpl.h | 2 +- arch/x86/kvm/pmu.c | 4 ++-- arch/x86/kvm/vmx/nested.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 6 ++--- arch/x86/kvm/vmx/vmx.h | 18 +++++++++++++++ arch/x86/kvm/x86.c | 27 +++++++++++++++++++--- 9 files changed, 110 insertions(+), 22 deletions(-)