On Tue, 3 May 2022 23:52:14 +0000
Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 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;
Patch tested, everything works fine now,
Thanks, Z.
