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

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

[Andy Chiu]

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@google.com>
Signed-off-by: Andy Chiu <andy.chiu@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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [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@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@google.com>
>> Signed-off-by: Andy Chiu <andy.chiu@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
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 22 Aug 2023 16:49:03 +0000 you 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.
> 
> [...]

Here is the summary with links:
  - riscv: signal: fix sigaltstack frame size checking
    https://git.kernel.org/riscv/c/0caa0b447398

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Tue, 22 Aug 2023 16:49:03 +0000, Andy Chiu 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.
> 
> [...]

Applied, thanks!

[1/1] riscv: signal: fix sigaltstack frame size checking
      https://git.kernel.org/palmer/c/d77303a57c95

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Linux regression tracking (Thorsten Leemhuis)]

On 31.08.23 23:58, Palmer Dabbelt wrote:
> 
> On Tue, 22 Aug 2023 16:49:03 +0000, Andy Chiu 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.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] riscv: signal: fix sigaltstack frame size checking
>       https://git.kernel.org/palmer/c/d77303a57c95

Just wondering: what happened to this patch, which afaics is currently
in neither mainline nor next? Because according to
https://bugzilla.kernel.org/show_bug.cgi?id=217923 it fixes rustc
userspace crashes with 6.5. Was a different approach found?

Ciao, Thorsten

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Linux regression tracking (Thorsten Leemhuis)]

On 31.08.23 23:58, Palmer Dabbelt wrote:
> 
> On Tue, 22 Aug 2023 16:49:03 +0000, Andy Chiu 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.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] riscv: signal: fix sigaltstack frame size checking
>       https://git.kernel.org/palmer/c/d77303a57c95

Just wondering: what happened to this patch, which afaics is currently
in neither mainline nor next? Because according to
https://bugzilla.kernel.org/show_bug.cgi?id=217923 it fixes rustc
userspace crashes with 6.5. Was a different approach found?

Ciao, Thorsten

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

[Palmer Dabbelt]

On Mon, 25 Sep 2023 03:07:47 PDT (-0700), regressions@leemhuis.info wrote:
> On 31.08.23 23:58, Palmer Dabbelt wrote:
>>
>> On Tue, 22 Aug 2023 16:49:03 +0000, Andy Chiu 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.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/1] riscv: signal: fix sigaltstack frame size checking
>>       https://git.kernel.org/palmer/c/d77303a57c95
>
> Just wondering: what happened to this patch, which afaics is currently
> in neither mainline nor next? Because according to
> https://bugzilla.kernel.org/show_bug.cgi?id=217923 it fixes rustc
> userspace crashes with 6.5. Was a different approach found?

We talked about this in the patchwork meeting.  I think I just dropped 
the ball somewhere -- we moved offices and then I was at the cauldron, 
so things are a bit more hectic than usual.

I got back last night and I'm still a bit out of it.  I'm going to try 
and dig whatever computer I was using out of a moving box and find the 
actual commit, as I'm kind of worried I might have lost something else 
as well.  Might not be super fast, though, as I've got stuff all over 
the place and I'm pretty much falling asleep already...

>
> Ciao, Thorsten

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Mon, 25 Sep 2023 03:07:47 PDT (-0700), regressions@leemhuis.info wrote:
> On 31.08.23 23:58, Palmer Dabbelt wrote:
>>
>> On Tue, 22 Aug 2023 16:49:03 +0000, Andy Chiu 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.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/1] riscv: signal: fix sigaltstack frame size checking
>>       https://git.kernel.org/palmer/c/d77303a57c95
>
> Just wondering: what happened to this patch, which afaics is currently
> in neither mainline nor next? Because according to
> https://bugzilla.kernel.org/show_bug.cgi?id=217923 it fixes rustc
> userspace crashes with 6.5. Was a different approach found?

We talked about this in the patchwork meeting.  I think I just dropped 
the ball somewhere -- we moved offices and then I was at the cauldron, 
so things are a bit more hectic than usual.

I got back last night and I'm still a bit out of it.  I'm going to try 
and dig whatever computer I was using out of a moving box and find the 
actual commit, as I'm kind of worried I might have lost something else 
as well.  Might not be super fast, though, as I've got stuff all over 
the place and I'm pretty much falling asleep already...

>
> Ciao, Thorsten

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

[Aurelien Jarno]

Hi,

The patch below is an important fix, as it is necessary to run rustc on riscv.
It has been merged as commit 14a270bfab7ab1c4b605c01eeca5557447ad5a2b. I have
seen that other commits from the same pull request have already been queued for
6.5, but not this one. Would it be possible to queue it for stable 6.5?

Thanks
Aurelien

On 2023-08-22 16:49, Andy Chiu 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@google.com>
> Signed-off-by: Andy Chiu <andy.chiu@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
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

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

[Greg KH]

On Sun, Oct 15, 2023 at 12:52:16PM +0200, Aurelien Jarno wrote:
> Hi,
> 
> The patch below is an important fix, as it is necessary to run rustc on riscv.
> It has been merged as commit 14a270bfab7ab1c4b605c01eeca5557447ad5a2b. I have
> seen that other commits from the same pull request have already been queued for
> 6.5, but not this one. Would it be possible to queue it for stable 6.5?

Now queued up, thanks.

greg k-h