On 08/05/2023 23:11, Wei Liu wrote:
On Fri, May 05, 2023 at 05:20:42PM +0200, Mickaël Salaün wrote:
This enables guests to lock their CR0 and CR4 registers with a subset of
X86_CR0_WP, X86_CR4_SMEP, X86_CR4_SMAP, X86_CR4_UMIP, X86_CR4_FSGSBASE
and X86_CR4_CET flags.
The new KVM_HC_LOCK_CR_UPDATE hypercall takes two arguments. The first
is to identify the control register, and the second is a bit mask to
pin (i.e. mark as read-only).
These register flags should already be pinned by Linux guests, but once
compromised, this self-protection mechanism could be disabled, which is
not the case with this dedicated hypercall.
Cc: Borislav Petkov <bp@xxxxxxxxx>
Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Cc: H. Peter Anvin <hpa@xxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>
Cc: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>
Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
Link: https://lore.kernel.org/r/20230505152046.6575-6-mic@xxxxxxxxxxx
[...]
hw_cr4 = (cr4_read_shadow() & X86_CR4_MCE) | (cr4 & ~X86_CR4_MCE);
if (is_unrestricted_guest(vcpu))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ffab64d08de3..a529455359ac 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7927,11 +7927,77 @@ static unsigned long emulator_get_cr(struct x86_emulate_ctxt *ctxt, int cr)
return value;
}
+#ifdef CONFIG_HEKI
+
+extern unsigned long cr4_pinned_mask;
+
Can this be moved to a header file?
Yep, but I'm not sure which one. Any preference Kees?
+static int heki_lock_cr(struct kvm *const kvm, const unsigned long cr,
+ unsigned long pin)
+{
+ if (!pin)
+ return -KVM_EINVAL;
+
+ switch (cr) {
+ case 0:
+ /* Cf. arch/x86/kernel/cpu/common.c */
+ if (!(pin & X86_CR0_WP))
+ return -KVM_EINVAL;
+
+ if ((read_cr0() & pin) != pin)
+ return -KVM_EINVAL;
+
+ atomic_long_or(pin, &kvm->heki_pinned_cr0);
+ return 0;
+ case 4:
+ /* Checks for irrelevant bits. */
+ if ((pin & cr4_pinned_mask) != pin)
+ return -KVM_EINVAL;
+
It is enforcing the host mask on the guest, right? If the guest's set is a
super set of the host's then it will get rejected.
+ /* Ignores bits not present in host. */
+ pin &= __read_cr4();
+ atomic_long_or(pin, &kvm->heki_pinned_cr4);
We assume that the host's mask is a superset of the guest's mask. I
guess we should check the absolute supported bits instead, even if it
would be weird for the host to not support these bits.
+ return 0;
+ }
+ return -KVM_EINVAL;
+}
+
+int heki_check_cr(const struct kvm *const kvm, const unsigned long cr,
+ const unsigned long val)
+{
+ unsigned long pinned;
+
+ switch (cr) {
+ case 0:
+ pinned = atomic_long_read(&kvm->heki_pinned_cr0);
+ if ((val & pinned) != pinned) {
+ pr_warn_ratelimited(
+ "heki-kvm: Blocked CR0 update: 0x%lx\n", val);
I think if the message contains the VM and VCPU identifier it will
become more useful.
Indeed, and this should be the case for all log messages, but I'd left
that for future work. ;) I'll update the logs for the next series with a
new kvm_warn_ratelimited() helper using VCPU's PID.
