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

[Michael Schmitz <schmitzmic@gmail.com>]

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