| Message ID | 20230822164904.21660-1-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0caa0b4473987b26a1bdc3a38cdc516544eec565 |
| Headers | show |
| Series | riscv: signal: fix sigaltstack frame size checking | expand |
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 >>
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!
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,
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
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
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; }
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(-)