On Tue, May 03, 2022, Zdenek Kaspar wrote:
> On Tue, 3 May 2022 16:20:31 +0000 Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> Bisect is later on my TODO if needed... I build this kernel now on
> debian/sid (saw some compiler/binutils updates) and added KASAN as
> Maciej pointed out.
> [ 229.423151] ==================================================================
> [ 229.423284] BUG: KASAN: slab-out-of-bounds in fpu_copy_uabi_to_guest_fpstate+0x86/0x130
Aha! A clue, Sherlock! I can reproduce in a VM by hiding XSAVE from the VM; that's
why this only repros on Core2.
KASAN blames fpu_copy_uabi_to_guest_fpstate() first, but the '3' data corruption
likely comes from this line in fpu_copy_guest_fpstate_to_uabi(), as the FP+SEE
mask == 3.
/* Make it restorable on a XSAVE enabled host */
ustate->xsave.header.xfeatures = XFEATURE_MASK_FPSSE;
One or both of these commits is/are to blame, depending on whether we want to blame
the bad calculation, the first use of the bad calculation, or yell at both.
be50b2065dfa ("kvm: x86: Add support for getting/setting expanded xstate buffer")
c60427dd50ba ("x86/fpu: Add uabi_size to guest_fpu")
I believe the right way to fix this is to set the starting uABI size to KVM's
actual base uABI size, struct kvm_xsave. I'll test the below more broadly and
send a patch.
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index c049561f373a..99caae7e8b01 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -14,6 +14,8 @@
#include <asm/traps.h>
#include <asm/irq_regs.h>
+#include <uapi/asm/kvm.h>
+
#include <linux/hardirq.h>
#include <linux/pkeys.h>
#include <linux/vmalloc.h>
@@ -247,7 +249,20 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
gfpu->fpstate = fpstate;
gfpu->xfeatures = fpu_user_cfg.default_features;
gfpu->perm = fpu_user_cfg.default_features;
- gfpu->uabi_size = fpu_user_cfg.default_size;
+
+ /*
+ * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
+ * to userspace, even when XSAVE is unsupported, so that restoring FPU
+ * state on a different CPU that does support XSAVE can cleanly load
+ * the incoming state using its natural XSAVE. In other words, KVM's
+ * uABI size may be larger than this host's default size. Conversely,
+ * the default size should never be larger than KVM's base uABI size;
+ * all features that can expand the uABI size must be opt-in.
+ */
+ gfpu->uabi_size = sizeof(struct kvm_xsave);
+ if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
+ gfpu->uabi_size = fpu_user_cfg.default_size;
+
fpu_init_guest_permissions(gfpu);
return true;
