linux-kernel - Re: [PATCH 02/19] x86/fpu: Prepare KVM for dynamically enabled states

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87v8zsthc6.ffs@tglx>
Date:   Mon, 13 Dec 2021 20:50:17 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Yang Zhong <yang.zhong@...el.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com
Cc:     seanjc@...gle.com, jun.nakajima@...el.com, kevin.tian@...el.com,
        jing2.liu@...ux.intel.com, jing2.liu@...el.com
Subject: Re: [PATCH 02/19] x86/fpu: Prepare KVM for dynamically enabled states

Paolo,

On Mon, Dec 13 2021 at 13:45, Paolo Bonzini wrote:
> On 12/13/21 13:00, Thomas Gleixner wrote:
>> On Mon, Dec 13 2021 at 10:12, Paolo Bonzini wrote:
>>> Please rename to alloc_xfeatures
>> 
>> That name makes no sense at all. This has nothing to do with alloc.
>
> Isn't that the features for which space is currently allocated?

It is, but from the kernel POV this is user. :)

> Reading "user_xfeatures" in there is cryptic, it seems like it's 
> something related to the userspace thread or group that has invoked the 
> KVM ioctl.  If it's renamed to alloc_xfeatures, then this:
>
> +		missing = request & ~guest_fpu->alloc_xfeatures;
> +		if (missing) {
> +			vcpu->arch.guest_fpu.realloc_request |= missing;
> +			return true;
> +		}
>
> makes it obvious that the allocation is for features that are requested 
> but haven't been allocated in the xstate yet.

Let's rename it to xfeatures and perm and be done with it.

>> Why? Yet another export of FPU internals just because?
>
> It's one function more and one field less.  I prefer another export of 
> FPU internals, to a write to a random field with undocumented
> invariants.

We want less not more exports. :)

> For example, why WARN_ON_ONCE if enter_guest == true?  If you enter the 
> guest after the host has restored MSR_IA32_XFD with KVM_SET_MSR, the

Indeed restoring a guest might require buffer reallocation, I missed
that, duh!

On restore the following components are involved:

   XCR0, XFD, XSTATE

XCR0 and XFD have to be restored _before_ XSTATE and that needs to
be enforced.

But independent of the ordering of XCR0 and XFD restore the following
check applies to both the restore and the runtime logic:

int kvm_fpu_realloc(struct kvm_vcpu *vcpu, u64 xcr0, u64 xfd)
{
   	u64 expand, enabled = xcr0 & ~xfd;

        expand = enabled & ~vcpu->arch.guest_fpu.xfeatures;
        if (!expand)
        	return 0;
        
        return fpu_enable_guest_features(&vcpu->arch.guest_fpu, expand);
}

int fpu_enable_guest_features(struct guest_fpu *gfpu, u64 which)
{
        permission_checks();
        ...
        return fpstate_realloc(.....)
}

fpstate_realloc() needs to be careful about flipping the pointers
depending on the question whether guest_fpu->fpstate is actually active,
i.e.:

        current->thread.fpu.fpstate == gfpu->fpstate

I'm halfways done with that. Will send something soonish.

Thanks,

        tglx