Message-ID: <20190118211401.4komqsnvuof7563p@linutronix.de>
Date:   Fri, 18 Jan 2019 22:14:01 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...nel.org>,
        Oleg Nesterov <oleg@...hat.com>
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 05/22] x86/fpu: Remove fpu->initialized usage in
 copy_fpstate_to_sigframe()

tl;dr
The kernel saves task's FPU registers on user's signal stack before
entering the signal handler. Can we avoid that and have in-kernel memory
for that? Does someone rely on the FPU registers from the task in the
signal handler?

On 2019-01-17 13:22:53 [+0100], Borislav Petkov wrote:
> > The task is running and using FPU registers. Then an evil mind sends a
> > signal. The task goes into kernel, prepares itself and is about to
> > handle the signal in userland. It saves its FPU registers on the stack
> > frame. It zeros its current FPU registers (ready for a fresh start),
> > loads the address of the signal handler and returns to user land
> > handling the signal.
> > 
> > Now. The signal handler may use FPU registers and the signal handler
> > maybe be preempted so you need to save the FPU registers of the signal
> > handler and you can't mix them up with the FPU register's of the task
> > (before it started handling the signal).
> > 
> > So in order to avoid a second FPU struct it saves them on user's stack
> > frame. I *think* this (avoiding a second FPU struct) is the primary
> > motivation.
> 
> Yah, makes sense. Sounds like something we'd do :-)
> 
> > A bonus point might be that the signal handler has a third
> > argument the `context'. That means you can use can access the task's FPU
> > registers from the signal handler. Not sure *why* you want to do so but
> > yo can.
> 
> For <raisins>.
> 
> > I can't imagine a use case and I was looking for a user and expecting it
> > to be glibc but I didn't find anything in the glibc that would explain
> > it. Intel even defines a few bytes as "user reserved" which are used by
> > "struct _fpx_sw_bytes" to add a marker in the signal and recognise it on
> > restore.
> > The only user that seems to make use of that is `criu' (or it looked
> > like it does use it). I would prefer to add a second struct-FPU and use
> > that for the signal handler. This would avoid the whole dance here.
> 
> That would be interesting from the perspective of making the code
> straight-forward and not having to document all that dance somewhere.
> 
> > And `criu' could maybe become a proper interface. I don't think as of
> > now that it will break something in userland if the signal handler
> > suddenly does not have a pointer to the FPU struct.
> 
> Well, but allocating a special FPU pointer for the signal handler
> context sounds simple and clean, no? Or are we afraid that that would
> slowdown signal handling, the whole allocation and assignment and
> stuff...?

So I *think* we could allocate a second struct fpu for the signal
handler at task creation time and use it.
It should not slow-down signal handling. So instead saving it to user's
stack we would save it to "our" memory. On the restore path we could
trust our buffer and simply load it again.

Sebastian

Powered by blists - more mailing lists