Am 18.07.2021 um 08:09 schrieb Michael Schmitz:

quoted

Hi Eric,

Am 18.07.2021 um 06:52 schrieb Eric W. Biederman:

quoted

quoted

I should have looked more closely at skeleton.S - most FPU exceptions
handled there call trap_c the same way as is done for generic traps,
i.e. SAVE_ALL_INT before, ret_from_exception after.

Instead of adding code to entry.S, much better to add it in
skeleton.S. I'll try to come up with a way to test this code path
(calling fpsp040_die from the dz exception hander seems much the
easiest way) to make sure this doesn't have side effects.

Does do_exit() ever return?

No.  The function do_exit never returns.

Fine - nothing to worry about as regards restoring the stack pointer
correctly then.

quoted

If it is not too much difficulty I would be in favor of having the code
do force_sigsegv(SIGSEGV), instead of calling do_exit directly.

That _would_ force a return, right? The exception handling in skeleton.S
won't be set up for that.

See attached patch - note that when you change fpsp040_die() to call
force_sig(SIGSEGV), the access exception handler will return to whatever
function in fpsp040 attempted the user space access, and continue that operation
with quite likely bogus data. That may well force another FPU trap before the
SIGSEGV is delivered (will force_sig() immediately force a trap, or wait until
returning to user space?).

Compile tested - haven't found an easy way to execute that code path yet.

Cheers,

	Michael

quoted

quoted

Looking at that code I have not been able to figure out the call paths
that get into skeleton.S.  I am not certain saving all of the registers
on an the exceptions that reach there make sense.  In practice I suspect

The registers are saved only so trap_c has a stack frame to work with.
In that sense, adding a stack frame before calling fpsp040_die is no
different.

quoted

taking an exception is much more expensive than saving the registers
so it
might not make any difference.  But this definitely looks like code that
is performance sensitive.

We're only planning to add a stack frame save before calling out of the
user access exception handler, right? I doubt that will be called very
often.

quoted

My sense when I was reading through skeleton.S was just one or two
registers were saved before the instruction emulation was called.

skeleton.S only contains the entry points for code to handle FPU
exceptions, from what I've seen (plus the user space access code).

Wherever that exception handling requires calling into the C exception
handler (trap_c), a stack frame is added.

Cheers,

    Michael

quoted

From 1e9be9238fb88dc0b87a7ffdd48068f944d8626c Mon Sep 17 00:00:00 2001
From: Michael Schmitz <schmitzmic@gmail.com>
Date: Sun, 18 Jul 2021 10:31:42 +1200
Subject: [PATCH] m68k/fpsp040 - save full stack frame before calling
 fpsp040_die

The FPSP040 floating point support code does not know how to
handle user space access faults gracefully, and just calls
do_exit(SIGSEGV) indirectly on these faults to abort.

do_exit() may stop if traced, and needs a full stack frame
available to avoid exposing kernel data.

Add the current stack frame before calling do_exit() from the
fpsp040 user access exception handler. Unwind the stack frame
and return to caller once done, in case do_exit() is replaced
by force_sig() later on.

CC: Eric W. Biederman <redacted>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/fpsp040/skeleton.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/m68k/fpsp040/skeleton.S b/arch/m68k/fpsp040/skeleton.S
index a8f4161..6c92d38 100644
--- a/arch/m68k/fpsp040/skeleton.S
+++ b/arch/m68k/fpsp040/skeleton.S

@@ -502,7 +502,17 @@ in_ea:
 	.section .fixup,#alloc,#execinstr
 	.even
 1:
+
+	SAVE_ALL_INT
+	SAVE_SWITCH_STACK