linux-kernel - Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YppVupW+IWsm7Osr@xz-m1.local>
Date:   Fri, 3 Jun 2022 14:40:58 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sasha Levin <sashal@...nel.org>, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Leonardo Bras <leobras@...hat.com>,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org,
        chang.seok.bae@...el.com, luto@...nel.org, kvm@...r.kernel.org,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest
 user_xfeatures to supported bits of XCR0

On Tue, Mar 01, 2022 at 09:22:10PM +0100, Paolo Bonzini wrote:
> On 3/1/22 21:13, Sasha Levin wrote:
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index d28829403ed08..6ac01f9828530 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -1563,7 +1563,10 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
> >   		fpregs_restore_userregs();
> >   	newfps->xfeatures = curfps->xfeatures | xfeatures;
> > -	newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
> > +
> > +	if (!guest_fpu)
> > +		newfps->user_xfeatures = curfps->user_xfeatures | xfeatures;
> > +
> >   	newfps->xfd = curfps->xfd & ~xfeatures;
> >   	curfps = fpu_install_fpstate(fpu, newfps);
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index bf18679757c70..875dce4aa2d28 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -276,6 +276,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >   	vcpu->arch.guest_supported_xcr0 =
> >   		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
> > +	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
> > +
> >   	kvm_update_pv_runtime(vcpu);
> >   	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> 
> Leonardo, was this also buggy in 5.16?  (I should have asked for a Fixes
> tag...).

I just stumbled over this patch on some migration tests in the past few
days..

In short, I was migrating a VM from 5.15 host to 5.18 host and the guest
trigger double fault immediately after the switch-over (I think that's when
it's trying to do vmenter, a VECTOR_DF was injected), with either precopy
or postcopy.

After I upgrade 5.15 src host to 5.18 host, problem goes away. I did a
bisect on dest and surprisingly it points to this commit.

Side note: I'm using two hosts that have the same processor model, so no
case of missing features on either side - they just match.

I'm not really sure whether this is a bug or by design - do we require this
patch to be applied to all stable branches to make the guest not crash
after migration, or it is unexpected?

FWICT, this patch modifies user_xfeatures while we don't do that trick
before. It sounds reasonable to me from the 1st glance, say if the guest
didn't enable some of the fpu features so we don't need to migrate those
fpu state chunks as we're migrating things based on user_xfeatures, and it
sounds good to solve the migration issue on "has-pksu" host to "no-pksu"
host as described in the patch commit message.

However there seems to be something missing at least to me, on why it'll
fail a migration from 5.15 (without this patch) to 5.18 (with this patch).
In my test case, user_xfeatures will be 0x7 (FP|SSE|YMM) if without this
patch, but 0x0 if with it.

I think what it should be happening is user_xfeatures will be set on src
with 0x7 (old kernel), so we should have migrated some more chunks to dest,
but I just don't quickly understand why that's a problem there because
fundamentally when we restore the fpu status (fpu_swap_kvm_fpstate) we use
the max feature bitmask anyway, and the dest hardware should support all of
them.  I don't quickly see how that could trigger a double fault, though.

I'll continue the dig probably next week, before that, any thoughts?

-- 
Peter Xu