RE: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal stack overflow

[David Laight]

From: Bae, Chang Seok
> Sent: 22 April 2021 17:31

> 
> On Apr 22, 2021, at 01:46, David Laight <David.Laight@xxxxxxxxxx> wrote:
> > From: Chang S. Bae
> >> Sent: 22 April 2021 05:49
> >>
> >> The kernel pushes context on to the userspace stack to prepare for the
> >> user's signal handler. When the user has supplied an alternate signal
> >> stack, via sigaltstack(2), it is easy for the kernel to verify that the
> >> stack size is sufficient for the current hardware context.
> >>
> >> Check if writing the hardware context to the alternate stack will exceed
> >> it's size. If yes, then instead of corrupting user-data and proceeding with
> >> the original signal handler, an immediate SIGSEGV signal is delivered.
> >
> > What happens if SIGSEGV is caught?
> 
> Boris pointed out the relevant notes before [1]. I think "unpredictable
> results" is a somewhat vague statement but process termination is unavoidable
> in this situation.
> 
> In the thread [1], a new signal number was discussed for the signal delivery
> failure, but my takeaway is this SIGSEGV is still recognizable.
> 
> FWIW, Len summarized other possible approaches as well [2].

Let's see...
I use an on-stack buffer for the alternate stack and then setup
siglongjmp() to return back from whatever ran out of stack space
back to the processing loop.

So my attempts to trap over-deep recursion cause the main stack
to get corrupted - this is a normal stack overwrite that might
be exploitable!

Alternatively I used malloc() and we have a potentially exploitable
heap overrun.

The only thing the kernel can do is to immediately kill the process
(possibly with a core dump).
Since signals can get nested the kernel needs to ensure there
is a reasonably amount of space left after the signal info is
written to the alternate stack.

> 
> >> Refactor the stack pointer check code from on_sig_stack() and use the new
> >> helper.
> >>
> >> While the kernel allows new source code to discover and use a sufficient
> >> alternate signal stack size, this check is still necessary to protect
> >> binaries with insufficient alternate signal stack size from data
> >> corruption.
> > ...
> >> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> >> index 3f6a0fcaa10c..ae60f838ebb9 100644
> >> --- a/include/linux/sched/signal.h
> >> +++ b/include/linux/sched/signal.h
> >> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
> >> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
> >> #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
> >>
> >> +static inline int __on_sig_stack(unsigned long sp)
> >> +{
> >> +#ifdef CONFIG_STACK_GROWSUP
> >> +	return sp >= current->sas_ss_sp &&
> >> +		sp - current->sas_ss_sp < current->sas_ss_size;
> >> +#else
> >> +	return sp > current->sas_ss_sp &&
> >> +		sp - current->sas_ss_sp <= current->sas_ss_size;
> >> +#endif
> >> +}
> >> +
> >
> > Those don't look different enough.
> 
> The difference is on the SS_AUTODISARM flag check.  This refactoring was
> suggested as on_sig_stack() brought confusion [3].

I was just confused by the #ifdef.
Whether %sp points to the last item or the next space is actually
independent of the stack direction.
A stack might usually use pre-decrement and post-increment but it
doesn't have to.
The stack pointer can't be right at one end of the alt-stack
area (because that is the address you'd use when you switch to it),
and if you are any where near the other end you are hosed.
So a common test:
	return (unsigned long)(sp - current->sas_ss_sp) < current->sas_ss_size;
will always work.

It isn't as though the stack pointer should be anywhere else
other than the 'real' thread stack.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)