[PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Michael Schmitz]

Inserting a gap of 20 bytes avoids user stack corruption
due to a discrepancy between user stack pointer and stack
state when an instruction is interrupted by a bus fault
in mid-execution, in particular a movem instruction with
the user stack pointer as target, such as commonly used in
the function prologue on C function calls.

The user stack pointer in this case reflects the stack
state before the instruction, but as many write operations
as would fit while on a mapped page are already written out
at that point. On 030 processors, the resulting exception
(format b) causes the instruction to be resumed, not
restarted after return from exception, and only at that
point is the user stack pointer updated. We must not corrupt
the user stack by writing beyond what's already stored to
the stack, and using the pre-execution stack pointer to
place the signal stack, that is exactly what happens.

We have two ways to address the issue: avoid running signal
delivery on format b bus fault exception (as suggested by
Andreas), or insert a suitable gap into the signal frame to
avoid it clobbering the stack contets.

Inserting a gap into every signal frame indiscriminately
does appear to cause memory leaks or excessive memory use
on low memory systems (been there ...). Some heuristics
is called for to only use the frame gap trick where
appropriate.

We can use the format b fault address in frame.un.fmtb.daddr
to find the address where the fault occurred, i.e. the
address where the instruction will resume. Addresses
above are no-go, anything up to and including the fault
address is OK to use. But we better make sure that the fault
address is actually on the page below the USP, and that the
start of the signal frame will end up on the same page that
the fault occurred on (again, been there). The latter might
be sufficient to ensure we're not hitting an unmapped page
when copying the signal frame, as the bus fault will have
been resolved when we get to signal delivery (??).

The bug that started this debug effort was reported by Stan
Johnson. Finn Thain developed a test case and debugged the
stack corruption. Fix developed and tested on my Falcon 030
with just 14 MB of RAM.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
CC: Finn Thain <fthain@linux-m68k.org>
CC: Andreas Schwab <schwab@linux-m68k.org>
CC: Stan Johnson <userm57@yahoo.com>
Link: https://lore.kernel.org/r/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com
Link: https://lore.kernel.org/r/e10b8e06-6a36-5c83-89da-bec8fd7d3ed9@linux-m68k.org
---
 arch/m68k/kernel/signal.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
index dfc7590abce8..75f4c4943231 100644
--- a/arch/m68k/kernel/signal.c
+++ b/arch/m68k/kernel/signal.c
@@ -858,11 +858,23 @@ 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 *regs, size_t frame_size)
 {
-	unsigned long usp = sigsp(rdusp(), ksig);
+	struct pt_regs *tregs = rte_regs(regs);
+	unsigned long usp = rdusp();
+	unsigned long ssp = sigsp(usp, ksig);
+
+	if (CPU_IS_020_OR_030 && ssp == usp && tregs->format == 0xb) {
+		struct frame *fmtbframe = (struct frame *) regs;
+		unsigned long fltp = (unsigned long) fmtbframe->un.fmtb.daddr;
+
+		if (fltp < ssp &&
+		   (fltp >> PAGE_SHIFT) == (ssp >> PAGE_SHIFT)-1 &&
+		 (((fltp - frame_size) & -8UL) >> PAGE_SHIFT) == (fltp >> PAGE_SHIFT))
+			ssp = fltp;
+	}
 
-	return (void __user *)((usp - frame_size) & -8UL);
+	return (void __user *)((ssp - frame_size) & -8UL);
 }
 
 static int setup_frame(struct ksignal *ksig, sigset_t *set,
@@ -880,7 +892,7 @@ static int setup_frame(struct ksignal *ksig, sigset_t *set,
 		return -EFAULT;
 	}
 
-	frame = get_sigframe(ksig, sizeof(*frame) + fsize);
+	frame = get_sigframe(ksig, regs, sizeof(*frame) + fsize);
 
 	if (fsize)
 		err |= copy_to_user (frame + 1, regs + 1, fsize);
@@ -952,7 +964,7 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
 		return -EFAULT;
 	}
 
-	frame = get_sigframe(ksig, sizeof(*frame));
+	frame = get_sigframe(ksig, regs, sizeof(*frame));
 
 	if (fsize)
 		err |= copy_to_user (&frame->uc.uc_extra, regs + 1, fsize);
-- 
2.17.1

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Andreas Schwab]

On Apr 29 2023, Michael Schmitz wrote:

> We can use the format b fault address in frame.un.fmtb.daddr
> to find the address where the fault occurred, i.e. the
> address where the instruction will resume.

I don't think you can assume any order of transfer.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Michael Schmitz]

Hi Andreas,

Am 29.04.2023 um 20:17 schrieb Andreas Schwab:
> On Apr 29 2023, Michael Schmitz wrote:
>
>> We can use the format b fault address in frame.un.fmtb.daddr
>> to find the address where the fault occurred, i.e. the
>> address where the instruction will resume.
>
> I don't think you can assume any order of transfer.

True - I've been seeing this too much through the lens of this 
particular bug - moveml to user stack faulting while crossing a page 
boundary.

The more typical case appears to look like this:

> [ 4703.880000] get_sigframe: signal stack 0xeffff758 fault daddr 0x8003f328
> [ 5009.830000] get_sigframe: signal stack 0xefffef80 fault daddr 0x80029c88
> [ 6152.750000] get_sigframe: signal stack 0xeffff3a4 fault daddr 0x8006a290
> [ 6985.290000] get_sigframe: signal stack 0xeffff3a4 fault daddr 0x8006a290
> [ 7260.060000] get_sigframe: signal stack 0xeffff020 fault daddr 0x80050cec
> [ 7794.080000] get_sigframe: signal stack 0xc0001ea0 fault daddr 0x8000c69c
> [ 7797.240000] get_sigframe: signal stack 0xeffff08c fault daddr 0xc00f4f7e
> [ 7942.460000] get_sigframe: signal stack 0xeffff3a4 fault daddr 0x80050cec
> [ 9833.180000] get_sigframe: signal stack 0xeffff3a4 fault daddr 0x80050cec
> [10153.500000] get_sigframe: signal stack 0xeffff020 fault daddr 0x8006a290
> [10781.110000] get_sigframe: signal stack 0xeffff3c8 fault daddr 0x800435dc
> [11648.290000] get_sigframe: signal stack 0xeffff8f0 fault daddr 0x800024f0
> [11773.260000] get_sigframe: signal stack 0xeffff3d0 fault daddr 0x8002f858
> [12117.380000] get_sigframe: signal stack 0xeffffa08 fault daddr 0x8006a290
> [12526.350000] get_sigframe: signal stack 0xefffef78 fault daddr 0xc006fba0
> [12875.800000] get_sigframe: signal stack 0xc043de08 fault daddr 0x800b5f68
> [15682.160000] get_sigframe: signal stack 0xeffff8f0 fault daddr 0xc00c37c8
> [16503.110000] get_sigframe: signal stack 0xeffff730 fault daddr 0x80050cec
> [16545.560000] get_sigframe: signal stack 0xeffff6ac fault daddr 0x80050cec

Fault address and USP are entirely unrelated (and no risk of overwriting 
the area where the fault occurred).

My (rather convoluted) checks that are meant to ensure fault address and 
stack pointer are on immediately adjacent pages, and not miles apart 
should take care of these above cases.

But I had indeed overlooked the case of crossing a page boundary 
upwards. Thanks for pointing that out.

As it turns out, in this case using the area on the user stack 
immediately below USP won't hurt - the transfer direction is away from 
the end of the signal frame, and as long as the signal stack remains 
below USP, it does not overwrite anything already on the stack that a 
resume would miss. No need to shift the signal frame out of the way.

What else am I missing? Examination of the stack contents from the 
signal handler before resuming the movem through rte has shown the order 
and direction of transfer is as expect for @sp-, i.e. downwards...

Cheers,

	Michael

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Finn Thain]

On Sat, 29 Apr 2023, Michael Schmitz wrote:

> Inserting a gap of 20 bytes avoids user stack corruption
> due to a discrepancy between user stack pointer and stack
> state when an instruction is interrupted by a bus fault
> in mid-execution, in particular a movem instruction with
> the user stack pointer as target, such as commonly used in
> the function prologue on C function calls.
> 
> The user stack pointer in this case reflects the stack
> state before the instruction, but as many write operations
> as would fit while on a mapped page are already written out
> at that point. On 030 processors, the resulting exception
> (format b) causes the instruction to be resumed, not
> restarted after return from exception, and only at that
> point is the user stack pointer updated. We must not corrupt
> the user stack by writing beyond what's already stored to
> the stack, and using the pre-execution stack pointer to
> place the signal stack, that is exactly what happens.
> 
> We have two ways to address the issue: avoid running signal
> delivery on format b bus fault exception (as suggested by
> Andreas), or insert a suitable gap into the signal frame to
> avoid it clobbering the stack contets.
> 
> Inserting a gap into every signal frame indiscriminately
> does appear to cause memory leaks or excessive memory use
> on low memory systems (been there ...). Some heuristics
> is called for to only use the frame gap trick where
> appropriate.
> 
> We can use the format b fault address in frame.un.fmtb.daddr
> to find the address where the fault occurred, i.e. the
> address where the instruction will resume. Addresses
> above are no-go, anything up to and including the fault
> address is OK to use. But we better make sure that the fault
> address is actually on the page below the USP, and that the
> start of the signal frame will end up on the same page that
> the fault occurred on (again, been there). The latter might
> be sufficient to ensure we're not hitting an unmapped page
> when copying the signal frame, as the bus fault will have
> been resolved when we get to signal delivery (??).
> 
> The bug that started this debug effort was reported by Stan
> Johnson. Finn Thain developed a test case and debugged the
> stack corruption. Fix developed and tested on my Falcon 030
> with just 14 MB of RAM.
> 
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> CC: Finn Thain <fthain@linux-m68k.org>
> CC: Andreas Schwab <schwab@linux-m68k.org>
> CC: Stan Johnson <userm57@yahoo.com>
> Link: https://lore.kernel.org/r/CAMuHMdW3yD22_ApemzW_6me3adq6A458u1_F0v-1EYwK_62jPA@mail.gmail.com
> Link: https://lore.kernel.org/r/e10b8e06-6a36-5c83-89da-bec8fd7d3ed9@linux-m68k.org
> ---
>  arch/m68k/kernel/signal.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
> index dfc7590abce8..75f4c4943231 100644
> --- a/arch/m68k/kernel/signal.c
> +++ b/arch/m68k/kernel/signal.c
> @@ -858,11 +858,23 @@ 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 *regs, size_t frame_size)
>  {
> -	unsigned long usp = sigsp(rdusp(), ksig);
> +	struct pt_regs *tregs = rte_regs(regs);
> +	unsigned long usp = rdusp();
> +	unsigned long ssp = sigsp(usp, ksig);
> +
> +	if (CPU_IS_020_OR_030 && ssp == usp && tregs->format == 0xb) {
> +		struct frame *fmtbframe = (struct frame *) regs;
> +		unsigned long fltp = (unsigned long) fmtbframe->un.fmtb.daddr;
> +
> +		if (fltp < ssp &&
> +		   (fltp >> PAGE_SHIFT) == (ssp >> PAGE_SHIFT)-1 &&
> +		 (((fltp - frame_size) & -8UL) >> PAGE_SHIFT) == (fltp >> PAGE_SHIFT))
> +			ssp = fltp;
> +	}
>  
> -	return (void __user *)((usp - frame_size) & -8UL);
> +	return (void __user *)((ssp - frame_size) & -8UL);
>  }
>  
>  static int setup_frame(struct ksignal *ksig, sigset_t *set,

That seems over-complicated to me... I must be missing something.

What I had in mind was something like this (untested):

	unsigned long usp;

        if (CPU_IS_020_OR_030 && tregs->format == 0xb)
		/* USP is unreliable so use the worst-case value. */
		usp = sigsp(rdusp() - 256, ksig);
	else
		usp = sigsp(rdusp(), ksig);

        return (void __user *)((usp - frame_size) & -8UL);

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Michael Schmitz]

Hi Finn,

thanks for your feedback!

Am 29.04.2023 um 21:49 schrieb Finn Thain:
>> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
>> index dfc7590abce8..75f4c4943231 100644
>> --- a/arch/m68k/kernel/signal.c
>> +++ b/arch/m68k/kernel/signal.c
>> @@ -858,11 +858,23 @@ 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 *regs, size_t frame_size)
>>  {
>> -	unsigned long usp = sigsp(rdusp(), ksig);
>> +	struct pt_regs *tregs = rte_regs(regs);
>> +	unsigned long usp = rdusp();
>> +	unsigned long ssp = sigsp(usp, ksig);
>> +
>> +	if (CPU_IS_020_OR_030 && ssp == usp && tregs->format == 0xb) {
>> +		struct frame *fmtbframe = (struct frame *) regs;
>> +		unsigned long fltp = (unsigned long) fmtbframe->un.fmtb.daddr;
>> +
>> +		if (fltp < ssp &&
>> +		   (fltp >> PAGE_SHIFT) == (ssp >> PAGE_SHIFT)-1 &&
>> +		 (((fltp - frame_size) & -8UL) >> PAGE_SHIFT) == (fltp >> PAGE_SHIFT))
>> +			ssp = fltp;
>> +	}
>>
>> -	return (void __user *)((usp - frame_size) & -8UL);
>> +	return (void __user *)((ssp - frame_size) & -8UL);
>>  }
>>
>>  static int setup_frame(struct ksignal *ksig, sigset_t *set,
>
> That seems over-complicated to me... I must be missing something.
>
> What I had in mind was something like this (untested):
>
> 	unsigned long usp;
>
>         if (CPU_IS_020_OR_030 && tregs->format == 0xb)
> 		/* USP is unreliable so use the worst-case value. */
> 		usp = sigsp(rdusp() - 256, ksig);
> 	else
> 		usp = sigsp(rdusp(), ksig);
>
>         return (void __user *)((usp - frame_size) & -8UL);

sigsp() may return an address from the alternate signal stack. In that 
case, no adjustment to the signal stack address is advised.

The fault address may be on a different page than the normal signal 
stack address (should have tested that perhaps, instead of the second 
condition above). In that case we should not adjust the signal stack, 
either.

As to the rest, it's a matter of how detrimental indiscriminate use of 
the worst case gap size might be. Finding the 'best' adjustment may be 
more hassle than it's worth.

I'll run some stress testing on both versions

Cheers,

	Michael

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Finn Thain]

On Sun, 30 Apr 2023, Michael Schmitz wrote:

> Am 29.04.2023 um 21:49 schrieb Finn Thain:
> 
> >
> > That seems over-complicated to me... I must be missing something.
> >

If signals don't nest, a page fault on the signal stack should never 
produce a new signal frame there, so this bug should never show up there. 
I think that was the point I'd missed, because it means the signal stack 
may be exempted here. But is the extra complexity worth it?

> > What I had in mind was something like this (untested):
> >
> > unsigned long usp;
> >
> > if (CPU_IS_020_OR_030 && tregs->format == 0xb)
> >     /* USP is unreliable so use the worst-case value. */
> >     usp = sigsp(rdusp() - 256, ksig);
> > else
> >     usp = sigsp(rdusp(), ksig);
> >
> > return (void __user *)((usp - frame_size) & -8UL);
> 
> sigsp() may return an address from the alternate signal stack. In that 
> case, no adjustment to the signal stack address is advised.
> ...

It'd be ill advised as it would reduce data locality when it expanded the 
signal stack across a cache line or page boundary, and there may be a 
performance penalty for that. However, the signal stack seems to be prone 
to poor data locality anyway, compared to the normal user stack. So I 
wonder if this penalty could be measured. It seems like a premature 
optimization to me. Unfortunately, I don't think we get to measure cache 
misses on m68k.

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Michael Schmitz]

Hi Finn,

Am 30.04.2023 um 12:24 schrieb Finn Thain:
>
> On Sun, 30 Apr 2023, Michael Schmitz wrote:
>
>> Am 29.04.2023 um 21:49 schrieb Finn Thain:
>>
>>>
>>> That seems over-complicated to me... I must be missing something.
>>>
>
> If signals don't nest, a page fault on the signal stack should never
> produce a new signal frame there, so this bug should never show up there.

I'm sure it doesn't. I'd just seen problems in an earlier version where 
none of these checks were in place. Using a signal stack location based 
on the fault address without making sure the fault actually happened on 
the user stack (i.e. below USP but not too far below) means we may place 
the signal stack in the text segment or static data or the heap area ...

> I think that was the point I'd missed, because it means the signal stack
> may be exempted here. But is the extra complexity worth it?

Any area that a bus fault happened on, except that right below USP 
should be exempted.

But your point about complexity is well taken. I must be missing another 
case where signal stack placement based on fault address is incorrect 
(kernel fault in __clear_user called from padzero when loading an ELF 
binary - odd ...)

Back to the drawing board.

>>> What I had in mind was something like this (untested):
>>>
>>> unsigned long usp;
>>>
>>> if (CPU_IS_020_OR_030 && tregs->format == 0xb)
>>>     /* USP is unreliable so use the worst-case value. */
>>>     usp = sigsp(rdusp() - 256, ksig);
>>> else
>>>     usp = sigsp(rdusp(), ksig);
>>>
>>> return (void __user *)((usp - frame_size) & -8UL);
>>
>> sigsp() may return an address from the alternate signal stack. In that
>> case, no adjustment to the signal stack address is advised.
>> ...
>
> It'd be ill advised as it would reduce data locality when it expanded the
> signal stack across a cache line or page boundary, and there may be a

You can't expand the alternate signal stack. It's obtained by malloc(), 
and we can't just hope that extending it down another page 'just works' 
in the same way as when we use the user stack (even there, it will only 
work up to the processes' stack limit).

> performance penalty for that. However, the signal stack seems to be prone
> to poor data locality anyway, compared to the normal user stack. So I
> wonder if this penalty could be measured. It seems like a premature
> optimization to me. Unfortunately, I don't think we get to measure cache
> misses on m68k.

Yep, the whole scheme does look overly complex with a little more 
thought (and testing).

A fixed offset, or skipping signal delivery entirely is much the easiest 
solution for now.

Cheers,

	Michael

Re: [PATCH RFC v1] m68k: signal.c: use signal frame gap to avoid stack corruption

[Finn Thain]

On Sun, 30 Apr 2023, Michael Schmitz wrote:

> 
> A fixed offset, or skipping signal delivery entirely is much the easiest 
> solution for now.
> 

I agree -- if address errors don't produce an unreliable USP then skipping 
signal delivery on bus errors seems like the best solution (given that RAM 
is so limited).