linux-kernel - Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

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: <87ee944hvj.ffs@tglx>
Date:   Fri, 01 Oct 2021 22:20:16 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>, bp@...e.de,
        luto@...nel.org, mingo@...nel.org, x86@...nel.org
Cc:     len.brown@...el.com, lenb@...nel.org, dave.hansen@...el.com,
        thiago.macieira@...el.com, jing2.liu@...el.com,
        ravi.v.shankar@...el.com, linux-kernel@...r.kernel.org,
        chang.seok.bae@...el.com
Subject: Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to
 protect dynamic user state

Chang,

On Fri, Oct 01 2021 at 17:02, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> +/**
>> + * xfd_switch - Switches the MSR IA32_XFD context if needed.
>> + * @prev:	The previous task's struct fpu pointer
>> + * @next:	The next task's struct fpu pointer
>> + */
>> +static inline void xfd_switch(struct fpu *prev, struct fpu *next)
>> +{
>> +	u64 prev_xfd_mask, next_xfd_mask;
>> +
>> +	if (!cpu_feature_enabled(X86_FEATURE_XFD) || !xfeatures_mask_user_dynamic)
>> +		return;
>
> This is context switch, so this wants to be a static key which is turned
> on during init when the CPU supports XFD and user dynamic features are
> available.
>
>> +
>> +	prev_xfd_mask = prev->state_mask & xfeatures_mask_user_dynamic;
>> +	next_xfd_mask = next->state_mask & xfeatures_mask_user_dynamic;
>> +
>> +	if (unlikely(prev_xfd_mask != next_xfd_mask))
>> +		wrmsrl_safe(MSR_IA32_XFD, xfeatures_mask_user_dynamic ^ next_xfd_mask);
>> +}
>> +
>>  /*
>>   * Delay loading of the complete FPU state until the return to userland.
>>   * PKRU is handled separately.
>>   */
>> -static inline void switch_fpu_finish(struct fpu *new_fpu)
>> +static inline void switch_fpu_finish(struct fpu *old_fpu, struct fpu *new_fpu)
>>  {
>> -	if (cpu_feature_enabled(X86_FEATURE_FPU))
>> +	if (cpu_feature_enabled(X86_FEATURE_FPU)) {
>>  		set_thread_flag(TIF_NEED_FPU_LOAD);
>> +		xfd_switch(old_fpu, new_fpu);
>
> Why has this to be done on context switch? Zero explanation provided.
>
> Why can't this be done in exit_to_user() where the FPU state restore is
> handled?

DEFINE_PER_CPU(xfd_state);

update_xfd(fpu)
{
	if (__this_cpu_read(xfd_state) != fpu->xfd_state) {
		wrmsrl(XFD, fpu->xfd_state);
		__this_cpu_write(xfd_state, fpu->xfd_state);
	}
}

fpregs_restore_userregs()
{
	if (!fpregs_state_valid(fpu, cpu)) {
        	if (static_branch_unlikely(xfd_switching_enabled))
                	update_xfd(fpu);
	...
	}
}

Hmm?

Thanks,

        tglx