68020/030 signal handling following exceptions

PATCH In review Finn Thain 4 participants 9 messages May 6, 2023 – May 25, 2023

Subsystems (2)

Stable branch m68k architecture

lore

Share & tools

lore

Finn Thain

author

2023-05-06 09:36 UTC #0 lore raw

cover 68020/030 signal handling following exceptions

This patch prevents potential stack corruption on 68020/030 when delivering signals following a bus error or (theoretically) an address error.

Changed since RFC:
- Dropped patch 1 because, as Andreas pointed out, it will not work properly.

Finn Thain (1):
m68k: Move signal frame following exception on 68020/030

 arch/m68k/kernel/signal.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Signature · 1 line

2.37.5

Finn Thain

author

2023-05-06 09:36 UTC #1 lore raw

PATCH 1/1 m68k: Move signal frame following exception on 68020/030

On 68030/020, an instruction such as, moveml %a2-%a3/%a5,%sp@- may cause a stack page fault during instruction execution (i.e. not at an instruction boundary) and produce a format 0xB exception frame.

In this situation, the value of USP will be unreliable. If a signal is to be delivered following the exception, this USP value is used to calculate the location for a signal frame. This can result in a corrupted user stack.

The corruption was detected in dash (actually in glibc) where it showed up as an intermittent "stack smashing detected" message and crash following signal delivery for SIGCHLD.

It was hard to reproduce that failure because delivery of the signal raced with the page fault and because the kernel places an unpredictable gap of up to 7 bytes between the USP and the signal frame.

A format 0xB exception frame can be produced by a bus error or an address error. The 68030 Users Manual says that address errors occur immediately upon detection during instruction prefetch. The instruction pipeline allows prefetch to overlap with other instructions, which means an address error can arise during the execution of a different instruction. So it seems likely that this patch may help in the address error case also.

Reported-and-tested-by Stan Johnson <[email protected]>

Link https://lore.kernel.org/all/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com/ open here

Cc Michael Schmitz <[email protected]>

Cc Andreas Schwab <[email protected]>

Cc [email protected]

Co-developed-by Michael Schmitz <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

Signed-off-by Finn Thain <[email protected]>

---
This is the same patch that was posted previously. The commit log has been revised and tags added.

---
 arch/m68k/kernel/signal.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

arch/m68k/kernel/signal.c +9 -4

index b9f6908a31bc..8aeafbb083f7 100644
@@ -858,11 +858,16 @@ static inline int rt_setup_ucontext(struct ucontext __user *uc, struct pt_regs *
 }
 
 static inline void __user *
-get_sigframe(struct ksignal *ksig, size_t frame_size)
+get_sigframe(struct ksignal *ksig, struct pt_regs *tregs, size_t frame_size)
 {
 	unsigned long usp = sigsp(rdusp(), ksig);
+	unsigned long gap = 0;
 
-	return (void __user *)((usp - frame_size) & -8UL);
+	if (CPU_IS_020_OR_030 && tregs->format == 0xb)
+		/* USP is unreliable so use worst-case value */
+		gap = 256;
+
+	return (void __user *)((usp - gap - frame_size) & -8UL);
 }
 
 static int setup_frame(struct ksignal *ksig, sigset_t *set,

@@ -880,7 +885,7 @@ static int setup_frame(struct ksignal *ksig, sigset_t *set,
 		return -EFAULT;
 	}
 
-	frame = get_sigframe(ksig, sizeof(*frame) + fsize);
+	frame = get_sigframe(ksig, tregs, sizeof(*frame) + fsize);
 
 	if (fsize)
 		err |= copy_to_user (frame + 1, regs + 1, fsize);

@@ -952,7 +957,7 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
 		return -EFAULT;
 	}
 
-	frame = get_sigframe(ksig, sizeof(*frame));
+	frame = get_sigframe(ksig, tregs, sizeof(*frame));
 
 	if (fsize)
 		err |= copy_to_user (&frame->uc.uc_extra, regs + 1, fsize);

Signature · 1 line

2.37.5

Geert Uytterhoeven

maintainer Reviewed-by on v1 1/1

2023-05-22 11:41 UTC #2 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

Quoted 24 lines · Finn Thain

On 68030/020, an instruction such as, moveml %a2-%a3/%a5,%sp@- may cause a stack page fault during instruction execution (i.e. not at an instruction boundary) and produce a format 0xB exception frame.
In this situation, the value of USP will be unreliable. If a signal is to be delivered following the exception, this USP value is used to calculate the location for a signal frame. This can result in a corrupted user stack.
The corruption was detected in dash (actually in glibc) where it showed up as an intermittent "stack smashing detected" message and crash following signal delivery for SIGCHLD.
It was hard to reproduce that failure because delivery of the signal raced with the page fault and because the kernel places an unpredictable gap of up to 7 bytes between the USP and the signal frame.
A format 0xB exception frame can be produced by a bus error or an address error. The 68030 Users Manual says that address errors occur immediately upon detection during instruction prefetch. The instruction pipeline allows prefetch to overlap with other instructions, which means an address error can arise during the execution of a different instruction. So it seems likely that this patch may help in the address error case also.

Quoted 8 lines

Reported-and-tested-by Stan Johnson <[email protected]>
Link https://lore.kernel.org/all/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com/ open here
Cc Michael Schmitz <[email protected]>
Cc Andreas Schwab <[email protected]>
Cc [email protected]
Co-developed-by Michael Schmitz <[email protected]>
Signed-off-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>

Reviewed-by Geert Uytterhoeven <[email protected]>

i.e. will queue as a fix in the m68k for-v6.4 branch.

I plan to send this upstream later this week, so any additional testing would be appreciated.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Michael Schmitz

2023-05-23 01:12 UTC #3 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

Hi Geert,

Quoted 33 lines · Geert Uytterhoeven

Quoted 24 lines · Finn Thain
On 68030/020, an instruction such as, moveml %a2-%a3/%a5,%sp@- may cause a stack page fault during instruction execution (i.e. not at an instruction boundary) and produce a format 0xB exception frame.
In this situation, the value of USP will be unreliable. If a signal is to be delivered following the exception, this USP value is used to calculate the location for a signal frame. This can result in a corrupted user stack.
The corruption was detected in dash (actually in glibc) where it showed up as an intermittent "stack smashing detected" message and crash following signal delivery for SIGCHLD.
It was hard to reproduce that failure because delivery of the signal raced with the page fault and because the kernel places an unpredictable gap of up to 7 bytes between the USP and the signal frame.
A format 0xB exception frame can be produced by a bus error or an address error. The 68030 Users Manual says that address errors occur immediately upon detection during instruction prefetch. The instruction pipeline allows prefetch to overlap with other instructions, which means an address error can arise during the execution of a different instruction. So it seems likely that this patch may help in the address error case also.
Quoted 8 lines
Reported-and-tested-by Stan Johnson <[email protected]>
Link https://lore.kernel.org/all/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com/ open here
Cc Michael Schmitz <[email protected]>
Cc Andreas Schwab <[email protected]>
Cc [email protected]
Co-developed-by Michael Schmitz <[email protected]>
Signed-off-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>

Quoted 5 lines

Reviewed-by Geert Uytterhoeven <[email protected]>
i.e. will queue as a fix in the m68k for-v6.4 branch.
I plan to send this upstream later this week, so any additional testing would be appreciated.

I've given this some lengthy stress testing, and haven't seen it fail once.

In contrast, various attempts of mine to improve on the concept (by only moving the signal frame away from the USP in case it's likely to clash) sometimes came up against a kernel bus error in setup_frame() when copying the signo to the signal frame. I must be making some incorrect assumptions still ...

Limiting the signal frame shift to bus fault exceptions that happen mid-instruction is not too much of an overhead even in low memory settings, and using 256 bytes (the largest possible operand size, i.e. the largest adjustment to USP that might occur on completion of the interrupted instruction) did not seem to cause any issues with stack growth either.

I can give this some more testing in ARAnyM (extending the stack shift to format 7 frames) but I'd say it's got as much testing on 030 hardware as we can do.

Cheers,

Michael

Quoted 14 lines

Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Geert Uytterhoeven

maintainer

2023-05-23 06:59 UTC #4 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

Hi Michael,

Quoted 55 lines · Michael Schmitz

Quoted 33 lines · Geert Uytterhoeven
Quoted 24 lines · Finn Thain
On 68030/020, an instruction such as, moveml %a2-%a3/%a5,%sp@- may cause a stack page fault during instruction execution (i.e. not at an instruction boundary) and produce a format 0xB exception frame.
In this situation, the value of USP will be unreliable. If a signal is to be delivered following the exception, this USP value is used to calculate the location for a signal frame. This can result in a corrupted user stack.
The corruption was detected in dash (actually in glibc) where it showed up as an intermittent "stack smashing detected" message and crash following signal delivery for SIGCHLD.
It was hard to reproduce that failure because delivery of the signal raced with the page fault and because the kernel places an unpredictable gap of up to 7 bytes between the USP and the signal frame.
A format 0xB exception frame can be produced by a bus error or an address error. The 68030 Users Manual says that address errors occur immediately upon detection during instruction prefetch. The instruction pipeline allows prefetch to overlap with other instructions, which means an address error can arise during the execution of a different instruction. So it seems likely that this patch may help in the address error case also.
Quoted 8 lines
Reported-and-tested-by Stan Johnson <[email protected]>
Link https://lore.kernel.org/all/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com/ open here
Cc Michael Schmitz <[email protected]>
Cc Andreas Schwab <[email protected]>
Cc [email protected]
Co-developed-by Michael Schmitz <[email protected]>
Signed-off-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
Quoted 5 lines
Reviewed-by Geert Uytterhoeven <[email protected]>
i.e. will queue as a fix in the m68k for-v6.4 branch.
I plan to send this upstream later this week, so any additional testing would be appreciated.
I've given this some lengthy stress testing, and haven't seen it fail once.
In contrast, various attempts of mine to improve on the concept (by only moving the signal frame away from the USP in case it's likely to clash) sometimes came up against a kernel bus error in setup_frame() when copying the signo to the signal frame. I must be making some incorrect assumptions still ...
Limiting the signal frame shift to bus fault exceptions that happen mid-instruction is not too much of an overhead even in low memory settings, and using 256 bytes (the largest possible operand size, i.e. the largest adjustment to USP that might occur on completion of the interrupted instruction) did not seem to cause any issues with stack growth either.

Quoted 3 lines

I can give this some more testing in ARAnyM (extending the stack shift to format 7 frames) but I'd say it's got as much testing on 030 hardware as we can do.

Thank you for your continued testing!
And thanks a lot to anyone involved in nailing this issue!

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Eero Tamminen

2023-05-23 11:12 UTC #5 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

In reply to Michael Schmitz

Hi,

Quoted 1 line · Michael Schmitz

On 22/05/23 23:41, Geert Uytterhoeven wrote:

...

Quoted 10 lines

Quoted 2 lines
I plan to send this upstream later this week, so any additional testing would be appreciated.
I've given this some lengthy stress testing, and haven't seen it fail once.
In contrast, various attempts of mine to improve on the concept (by only moving the signal frame away from the USP in case it's likely to clash) sometimes came up against a kernel bus error in setup_frame() when copying the signo to the signal frame. I must be making some incorrect assumptions still ...
Limiting the signal frame shift to bus fault exceptions that happen mid-instruction is not too much of an overhead even in low memory settings, and using 256 bytes (the largest possible operand size, i.e. the largest adjustment to USP that might occur on completion of the interrupted instruction) did not seem to cause any issues with stack growth either.
I can give this some more testing in ARAnyM (extending the stack shift to format 7 frames) but I'd say it's got as much testing on 030 hardware as we can do.

Aranym emulates only 040 (without CPU cache).

For 030 system emulation, you can use either WinUAE (Amiga) or Hatari (Atari).

Those emulate (using WinUAE CPU core) all 68000 - 68060 models, including MMU *and* CPU cache.

Note: MMU+cache combination has some issues with Linux on Hatari though, so you need to disable cache & prefetch emulation to be able to run Linux user-space with Hatari [1]. Apparently these issues do not happen on WinUAE, so that seems better 030 Linux test platform for now.

- Eero

[1] https://hatari.tuxfamily.org/doc/m68k-linux.txt

Geert Uytterhoeven

maintainer

2023-05-23 12:05 UTC #6 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

Hi Eero,

Quoted 21 lines · Eero Tamminen

Quoted 1 line · Michael Schmitz
On 22/05/23 23:41, Geert Uytterhoeven wrote:
...
Quoted 18 lines
Quoted 2 lines
I plan to send this upstream later this week, so any additional testing would be appreciated.
I've given this some lengthy stress testing, and haven't seen it fail once.
In contrast, various attempts of mine to improve on the concept (by only moving the signal frame away from the USP in case it's likely to clash) sometimes came up against a kernel bus error in setup_frame() when copying the signo to the signal frame. I must be making some incorrect assumptions still ...
Limiting the signal frame shift to bus fault exceptions that happen mid-instruction is not too much of an overhead even in low memory settings, and using 256 bytes (the largest possible operand size, i.e. the largest adjustment to USP that might occur on completion of the interrupted instruction) did not seem to cause any issues with stack growth either.

Quoted 5 lines

Quoted 3 lines
I can give this some more testing in ARAnyM (extending the stack shift to format 7 frames) but I'd say it's got as much testing on 030 hardware as we can do.
Aranym emulates only 040 (without CPU cache).

Hence Michael's comment to extend the stack shift to format-7 frames, which is what is generated on '040 in case of an access error.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Michael Schmitz

2023-05-23 21:30 UTC #7 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

Hi Geert,

Quoted 23 lines · Geert Uytterhoeven

Hi Eero,
Quoted 20 lines · Eero Tamminen
Quoted 1 line · Michael Schmitz
On 22/05/23 23:41, Geert Uytterhoeven wrote:
...
Quoted 17 lines
Quoted 2 lines
I plan to send this upstream later this week, so any additional testing would be appreciated.
I've given this some lengthy stress testing, and haven't seen it fail once.
In contrast, various attempts of mine to improve on the concept (by only moving the signal frame away from the USP in case it's likely to clash) sometimes came up against a kernel bus error in setup_frame() when copying the signo to the signal frame. I must be making some incorrect assumptions still ...
Limiting the signal frame shift to bus fault exceptions that happen mid-instruction is not too much of an overhead even in low memory settings, and using 256 bytes (the largest possible operand size, i.e. the largest adjustment to USP that might occur on completion of the interrupted instruction) did not seem to cause any issues with stack growth either.

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
I can give this some more testing in ARAnyM (extending the stack shift to format 7 frames) but I'd say it's got as much testing on 030 hardware as we can do.
Aranym emulates only 040 (without CPU cache).
Hence Michael's comment to extend the stack shift to format-7 frames, which is what is generated on '040 in case of an access error.

Precisely :-)

And I ought to add that this was Finn's idea, to give us more test coverage.

As far as I can remember, the cache isn't involved in this type of problem. And I might note that due to instruction restart on 040, any minor stack corruption we cause by the signal stack will be repaired on return from exception.

Cheers,

Michael

Quoted 5 lines

Gr{oetje,eeting}s,
Geert

Finn Thain

author

2023-05-25 02:15 UTC #8 lore raw

Re: [PATCH v1 1/1] m68k: Move signal frame following exception on 68020/030

In reply to Geert Uytterhoeven

Quoted 4 lines · Geert Uytterhoeven

Reviewed-by Geert Uytterhoeven <[email protected]>
i.e. will queue as a fix in the m68k for-v6.4 branch.

Thanks Geert!

Quoted 3 lines

I plan to send this upstream later this week, so any additional testing would be appreciated.

If there is a userland program that needs all of its sigaltstack space, and cannot accomodate a 256 byte margin, it will be hard to find that program. Often these signal stacks are used for handling (would-be) fatal signals that may not happen in regression testing.

If we fix the kernel and a failure eventually does show up in a userland program, we may be able to patch that program. Looking through Debian's codebase, which is easily searchable, I found lots of different ways to determine the signal stack size. But it's not clear to me which programs calculate it correctly.

Many programs take the architecture's SIGSTKSZ or MINSIGSTKSZ or getauxval(AT_MINSIGSTKSZ) or sysconf(_SC_SIGSTKSZ), or some combination, and then apply an arbitrary safety factor. Some add a constant or apply a lower bound that may reflect the actual requirements of the signal handler. Some just use page_size().