linux-kernel - Re: [PATCH 07/22] x86/fpu: Remove fpu->initialized

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: <20190207101301.vlaiikyqweftemxu@linutronix.de>
Date:   Thu, 7 Feb 2019 11:13:01 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Borislav Petkov <bp@...en8.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 07/22] x86/fpu: Remove fpu->initialized

On 2019-02-06 15:01:14 [+0100], Borislav Petkov wrote:
> On Tue, Feb 05, 2019 at 07:03:37PM +0100, Sebastian Andrzej Siewior wrote:
> > Well, nothing changes in regard to the logic. Earlier we had a variable
> > which helped us to distinguish between user & kernel thread. Now we have
> > a different one. 
> > I'm going to add a comment to switch_fpu_prepare() about ->mm since you
> > insist but I would like to avoid it.
> 
> I don't understand what that aversion is towards commenting stuff,
> especially important stuff like the meaning of the presence of ->mm for
> the FPU code. What is the downside to documenting that?

I don't like commenting the obvious things in code but I might be wrong
on what I consider here obvious. The important part is probably that we
don't save/restore FPU registers for kernel threads but this isn't new,
it was always like that (more or less implicit). The ->mm part is an
implementation detail (and is used in other places).
That said I already added this:
|@@ -525,11 +525,14 @@ static inline void fpregs_activate(struc
|  *
|  *  - switch_fpu_finish() restores the new state as
|  *    necessary.
|+ *
|+ * The FPU context is only stored/restore for user task and ->mm is used to
|+ * distinguish between kernel and user threads.
|  */
| static inline void
| switch_fpu_prepare(struct fpu *old_fpu, int cpu)
| {

and I *think* that this is enough. This *what* we do and not *why*. I
don't have an answer towards *why*.

> Considering that in this very thread we ourselves encountered the fact
> that stuff is not documented and we complained that it wasn't!

Yes. We had no idea why we save the FPU registers on user's stack during
signal handling. Was this an implementation detail on kernel side as
part of signal handling or is this required/ expected by the user as
part of a use case? We have now the explanation that signals may
cascade. Do we know by now if userland is supposed to use it or it
accessed the register because they were available?
The MPX code did access the MPX part of the xsave area (others do it for
"testing/debug" as per my I google research). This kind of things should
be part of the ABI document and not only a comment in the kernel.
Are the MAGIC constants only in-kernel use (to check if the user
accidentally overwrote its stack) or should be checked by the user
during signal handling to ensure that the xsave area is available.

> > We have a comment, it is just not helping.
> 
> Why is it not helping?

The part you referred to was:
|-       /* Update the thread's fxstate to save the fsave header. */
|-       if (ia32_fxstate) 
|-               copy_fxregs_to_kernel(fpu);

and it is not helping because it does not explain why it is done. I can
see based on the code that the FXstate is saved in case of a 32bit
frame. It is saved into thread's state. It does not explain why it
needs to be done. That is the "not helping" part.

> > Steven said on IRC that it can be removed.
> 
> Did he give an explanation why is it ok?

I can forward you the IRC pieces offlist if you like. He said I can
remove it if there are no users and I am not aware of any. He pointed
out that sched_wakeup had a "success" member which was relied on by
tools so it remained in order not to break them. So we have
	__entry->success        = 1; /* rudiment, kill when possible */

in the tree now. I can loop him in if this is not enough.

> Thx.

Sebastian