linux-kernel - Re: [PATCH 06/22] x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to

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: <20190124111714.GA11554@zn.tnic>
Date:   Thu, 24 Jan 2019 12:17:14 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 06/22] x86/fpu: Don't save fxregs for ia32 frames in
 copy_fpstate_to_sigframe()

On Wed, Jan 09, 2019 at 12:47:28PM +0100, Sebastian Andrzej Siewior wrote:
> Why does copy_fpstate_to_sigframe() do copy_fxregs_to_kernel() in the
> ia32_fxstate case? I don't know. It just does.
> Maybe it was required at some point, maybe it was added by accident and
> nobody noticed it because it makes no difference.

So

  72a671ced66d ("x86, fpu: Unify signal handling code paths for x86 and x86_64 kernels")

talks about some exclusion of legacy fsave state.

> In copy_fpstate_to_sigframe() we stash the FPU state into the task's
> stackframe. Then the CPU's FPU registers (and its fpu->state) are
> cleared (handle_signal() does fpu__clear()).

So that fpu__clear() name is not optimal. It should be
fpu__reinitialize() or so. The comment above it says so too:

/*
 * Clear the FPU state back to init state.

> So it makes *no* difference
> what happens to fpu->state after copy_fpregs_to_sigframe().
> 
> Remove copy_fxregs_to_kernel() since it does not matter what it does and
> save a few cycles.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  arch/x86/kernel/fpu/signal.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index c136a4327659d..047390a45e016 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -174,9 +174,6 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  	/* Save the live register state to the user directly. */
>  	if (copy_fpregs_to_sigframe(buf_fx))
>  		return -1;
> -	/* Update the thread's fxstate to save the fsave header. */
> -	if (ia32_fxstate)
> -		copy_fxregs_to_kernel(fpu);

Need to get rid of that local "fpu" var too:

arch/x86/kernel/fpu/signal.c: In function ‘copy_fpstate_to_sigframe’:
arch/x86/kernel/fpu/signal.c:159:14: warning: unused variable ‘fpu’ [-Wunused-variable]
  struct fpu *fpu = &current->thread.fpu;
              ^~~

>  	/* Save the fsave header for the 32-bit frames. */
>  	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
> -- 
> 2.20.1
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.