Re: signal delivery, was Re: reliable reproducer

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

On 25/04/23 14:32, Michael Schmitz wrote:
> Hi Finn,
>
> Am 25.04.2023 um 13:55 schrieb Finn Thain:
>> On Tue, 25 Apr 2023, Finn Thain wrote:
>>
>>> On Tue, 25 Apr 2023, Michael Schmitz wrote:
>>>
>>>> As to a cause for the corruption: all the calculations in setup_frame
>>>> and sys_sigreturn use fsize, but get_sigframe() masks off the result
>>>> of usp - sizeof(sigframe) - fsize to place the entire frame at a
>>>> quadword boundary.
>>> ...
>>>
>>> I wonder if we are seeing some fallout from the issue described in
>>> do_page_fault() i.e. usp is unreliable.
>>>
>>>                 /* Accessing the stack below usp is always a bug.  The
>>>                    "+ 256" is there due to some instructions doing
>>>                    pre-decrement on the stack and that doesn't show up
>>>                    until later.  */
>>>                 if (address + 256 < rdusp())
>>>                         goto map_err;
>>>
Minimal gap that avoids the corruption for me (in your patch below) is 
20 bytes. Still can't explain that (and I won't have time to throw more 
confusion into the discussion until maybe the weekend).

Cheers,

     Michael


>>> Maybe we should try modifying get_sigframe() to increase the gap 
>>> between
>>> the signal and exception frames from 0-1 long words up to 64-65 long
>>> words.
>>>
>>
>> It turns out that doing so (patch below) does make the problem go away.
>> Was the exception frame getting clobbered?
>
> Might happen, if the frame gap isn't actually equal to the exception 
> frame extra size anymore? Aligning the start of the signal frame to 
> the next lower quadword boundary increases the gap size.
>
> When setting up the sigframe, the extra is copied to the correct 
> location (right past struct sigframe, or into uc_filler). When moving 
> that exception frame into place, the assumption is that the gap is the 
> extra size, not more.
>
> I'll try dropping the quadword alignment constraint - the return 
> trampoline still ought to remain longword aligned.
>
> Cheers,
>
>     Michael
>
>
>>
>> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
>> index b9f6908a31bc..94104699f5a8 100644
>> --- a/arch/m68k/kernel/signal.c
>> +++ b/arch/m68k/kernel/signal.c
>> @@ -862,7 +862,7 @@ get_sigframe(struct ksignal *ksig, size_t 
>> frame_size)
>>  {
>>      unsigned long usp = sigsp(rdusp(), ksig);
>>
>> -    return (void __user *)((usp - frame_size) & -8UL);
>> +    return (void __user *)((usp - 256 - frame_size) & -8UL);
>>  }
>>
>>  static int setup_frame(struct ksignal *ksig, sigset_t *set,
>>