LinuxLists.cc - [PATCH v1 0/1] 68020/030 signal handling following exceptions

2023-05-06 09:41:22

Subject: [PATCH v1 0/1] 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(-)

--
2.37.5

2023-05-06 09:41:44

by Finn Thain

[permalink] [raw]

Subject: [PATCH v1 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/
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(-)

diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
index b9f6908a31bc..8aeafbb083f7 100644
--- a/arch/m68k/kernel/signal.c
+++ b/arch/m68k/kernel/signal.c
@@ -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);
--
2.37.5

2023-05-22 11:51:30

by Geert Uytterhoeven

[permalink] [raw]

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

On Sat, May 6, 2023 at 11:36 AM Finn Thain <[email protected]> wrote:
> 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/
> 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

2023-05-23 01:20:50

by Michael Schmitz

[permalink] [raw]

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

Hi Geert,

On 22/05/23 23:41, Geert Uytterhoeven wrote:
> On Sat, May 6, 2023 at 11:36 AM Finn Thain <[email protected]> wrote:
>> 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/
>> 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.

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

>
> 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

2023-05-23 07:15:20

by Geert Uytterhoeven

[permalink] [raw]

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

Hi Michael,

On Tue, May 23, 2023 at 3:11 AM Michael Schmitz <[email protected]> wrote:
> On 22/05/23 23:41, Geert Uytterhoeven wrote:
> > On Sat, May 6, 2023 at 11:36 AM Finn Thain <[email protected]> wrote:
> >> 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/
> >> 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.
>
> 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.

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

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

2023-05-25 02:20:15

by Finn Thain

[permalink] [raw]

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