linux-kernel - Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager

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: <54CAA7F6.2000206@redhat.com>
Date:	Thu, 29 Jan 2015 16:36:54 -0500
From:	Rik van Riel <riel@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>
CC:	Andy Lutomirski <luto@...capital.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Fenghua Yu <fenghua.yu@...el.com>,
	the arch/x86 maintainers <x86@...nel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end()
 if use_eager_fpu()

On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu().
> Unconditional __thread_fpu_end() is only correct if we know that this
> thread can't return to user-mode and use FPU.
> 
> Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(),
> and init_fpu(current) can be only called by the coredumping thread via
> regset->get(). But it is exported to modules, and imo this should be
> fixed anyway.

What about xfpregs_set?

It looks like that code copies an entire FPU state in
from userspace, and expects the kernel to start using
that new FPU state.

This is called from the ptrace code.

When we switch to the traced task, the __thread_fpu_end()
that was called from init_fpu() ensures that
switch_fpu_begin() will actually load the new FPU state
from memory into the registers, and we will not take
the fpu_lazy_restore() branch.

I believe that xstateregs_set and xfpregs_set still
need __thread_fpu_end().

What am I missing?

> And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy()
> and save_init_fpu() do.
> 
> - It seems that even !use_eager_fpu() case doesn't need the unconditional
>   __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.
> 
> - It is still not clear to me if __save_init_fpu() can safely nest with
>   another save + restore from __kernel_fpu_begin(). If not, we can use
>   kernel_fpu_disable() to fix the race.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  arch/x86/kernel/i387.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index c3b92c0..8e070a6 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -120,8 +120,12 @@ void unlazy_fpu(struct task_struct *tsk)
>  {
>  	preempt_disable();
>  	if (__thread_has_fpu(tsk)) {
> -		__save_init_fpu(tsk);
> -		__thread_fpu_end(tsk);
> +		if (use_eager_fpu()) {
> +			__save_fpu(tsk);
> +		} else {
> +			__save_init_fpu(tsk);
> +			__thread_fpu_end(tsk);
> +		}
>  	}
>  	preempt_enable();
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/