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

[Michael Schmitz <schmitzmic@gmail.com>]

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