[PATCH] riscv: signal: fix sigaltstack frame size checking

[Prashanth Swaminathan]

Per Andy’s request, replying to this thread with my reproduction
instructions and some additional data. I’ll ask for forgiveness in
advance for the roundabout reproduction method, I’m not familiar
enough with this code to know how to intentionally induce this state -
in theory if you can (1) cause a `get_sigframe` to happen on a thread
not ON_SASTACK, and (2) you can ensure that the current `sp` is less
than whatever is being held as the alternate stack’s `sp` (I’m not
sure what it means to reference `current->sas_ss_sp` when you’re not
on the alt stack?), this issue should trigger.

My reproduction method (100% rate):

1. Build Android AOSP phone target (`lunch aosp_cf_riscv64_phone-userdebug`).
2. Launch the emulator:
https://source.android.com/docs/setup/create/cuttlefish-use
3. After boot, turn WiFi on and connect to ‘VirtWifi’. To watch this
error, I attach strace to the networkstack PID at this point.
4. Turn WiFi off. Within a few seconds, the networkstack process will
crash with a SIGSEGV.

Dumping out the variables at the crash location:

```
regs->sp: 72057560689527184
sigsp: 72057560689527184
framesize: 1088
current->sas_ss_size: 32768
sp: 72057560689526096
current->sas_ss_sp: 72057570417491968
```

As we can see, `sp < current->sas_ss_sp` and thus triggers the check.
What's noticeable here though is that `regs->sp` and `sigsp` are
identical, which means that when `sigsp(sp, ksig)` was called, it did
not find that it was on the alternate signal stack and simply returned
`sp` as is. If we're not on the alternate signal stack, then this
check does not make sense - the relative locations of these two
pointers are not guaranteed. We perform a similar check at the top of
this function that does validate whether we're on the altstack before
attempting the subsequent check.


> On Tue, Aug 22, 2023 at 9:49 AM Andy Chiu <andy.chiu at sifive.com> wrote:
>>
>> The alternative stack checking in get_sigframe introduced by the Vector
>> support is not needed and has a problem. It is not needed as we have
>> already validate it at the beginning of the function if we are already
>> on an altstack. If not, the size of an altstack is always validated at
>> its allocation stage with sigaltstack_size_valid().
>>
>> Besides, we must only regard the size of an altstack if the handler of a
>> signal is registered with SA_ONSTACK. So, blindly checking overflow of
>> an altstack if sas_ss_size not equals to zero will check against wrong
>> signal handlers if only a subset of signals are registered with
>> SA_ONSTACK.
>>
>> Fixes: 8ee0b41898fa ("riscv: signal: Add sigcontext save/restore for vector")
>> Reported-by: Prashanth Swaminathan <prashanthsw at google.com>
>> Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
>> ---
>>  arch/riscv/kernel/signal.c | 7 -------
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
>> index 180d951d3624..21a4d0e111bc 100644
>> --- a/arch/riscv/kernel/signal.c
>> +++ b/arch/riscv/kernel/signal.c
>> @@ -311,13 +311,6 @@ static inline void __user *get_sigframe(struct ksignal *ksig,
>>         /* Align the stack frame. */
>>         sp &= ~0xfUL;
>>
>> -       /*
>> -        * Fail if the size of the altstack is not large enough for the
>> -        * sigframe construction.
>> -        */
>> -       if (current->sas_ss_size && sp < current->sas_ss_sp)
>> -               return (void __user __force *)-1UL;
>> -
>>         return (void __user *)sp;
>>  }
>>
>> --
>> 2.17.1
>>