| Message ID | 1529512560-28007-1-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] rseq: Avoid infinite recursion when delivering SIGSEGV | expand |
----- On Jun 20, 2018, at 12:36 PM, Will Deacon will.deacon@arm.com wrote: > When delivering a signal to a task that is using rseq, we call into > __rseq_handle_notify_resume() so that the registers pushed in the > sigframe are updated to reflect the state of the restartable sequence > (for example, ensuring that the signal returns to the abort handler if > necessary). > > However, if the rseq management fails due to an unrecoverable fault when > accessing userspace or certain combinations of RSEQ_CS_* flags, then we > will attempt to deliver a SIGSEGV. This has the potential for infinite > recursion if the rseq code continuously fails on signal delivery. > > Avoid this problem by using force_sigsegv() instead of force_sig(), which > is explicitly designed to reset the SEGV handler to SIG_DFL in the case > of a recursive fault. Your approach looks good. One nit below implementation-wise: > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > > Sending as an RFC since I only spotted this via code inspection and haven't > tried to trigger it yet. > > arch/arm/kernel/signal.c | 2 +- > arch/powerpc/kernel/signal.c | 2 +- > arch/x86/kernel/signal.c | 2 +- > include/linux/sched.h | 20 ++++++++++++++------ > kernel/rseq.c | 7 ++++--- > 5 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c > index f09e9d66d605..6434a62591e8 100644 > --- a/arch/arm/kernel/signal.c > +++ b/arch/arm/kernel/signal.c > @@ -544,7 +544,7 @@ static void handle_signal(struct ksignal *ksig, struct > pt_regs *regs) > * Increment event counter and perform fixup for the pre-signal > * frame. > */ > - rseq_signal_deliver(regs); > + rseq_signal_deliver(ksig, regs); > > /* > * Set up the stack frame > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c > index 17fe4339ba59..af822e764569 100644 > --- a/arch/powerpc/kernel/signal.c > +++ b/arch/powerpc/kernel/signal.c > @@ -134,7 +134,7 @@ static void do_signal(struct task_struct *tsk) > /* Re-enable the breakpoints for the signal stack */ > thread_change_pc(tsk, tsk->thread.regs); > > - rseq_signal_deliver(tsk->thread.regs); > + rseq_signal_deliver(&ksig, tsk->thread.regs); > > if (is32) { > if (ksig.ka.sa.sa_flags & SA_SIGINFO) > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > index 445ca11ff863..92a3b312a53c 100644 > --- a/arch/x86/kernel/signal.c > +++ b/arch/x86/kernel/signal.c > @@ -692,7 +692,7 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs) > * Increment event counter and perform fixup for the pre-signal > * frame. > */ > - rseq_signal_deliver(regs); > + rseq_signal_deliver(ksig, regs); > > /* Set up the stack frame */ > if (is_ia32_frame(ksig)) { > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 87bf02d93a27..277f8ccb0661 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1799,20 +1799,27 @@ static inline void rseq_set_notify_resume(struct > task_struct *t) > set_tsk_thread_flag(t, TIF_NOTIFY_RESUME); > } > > -void __rseq_handle_notify_resume(struct pt_regs *regs); > +void __rseq_handle_notify_resume(struct ksignal *sig, struct pt_regs *regs); > > -static inline void rseq_handle_notify_resume(struct pt_regs *regs) > +static inline void _rseq_handle_notify_resume(struct ksignal *ksig, > + struct pt_regs *regs) > { > if (current->rseq) > - __rseq_handle_notify_resume(regs); > + __rseq_handle_notify_resume(ksig, regs); > +} > + > +static inline void rseq_handle_notify_resume(struct pt_regs *regs) > +{ > + _rseq_handle_notify_resume(NULL, regs); > } If we add _rseq_handle_notify_resume() here, we'd need the CONFIG_RSEQ=n code to also have an empty static inline for it. So I'm not sure we want all 3 of: void __rseq_handle_notify_resume(struct ksignal *sig, struct pt_regs *regs); static inline void _rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) static inline void rseq_handle_notify_resume(struct pt_regs *regs) Instead, can we just have: __rseq_handle_notify_resume(struct ksignal *sig, struct pt_regs *regs); static inline void rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) and change the callers to rseq_handle_notify_resume to pass a NULL pointer when they are not within signal delivery ? Thanks, Mathieu > > -static inline void rseq_signal_deliver(struct pt_regs *regs) > +static inline void rseq_signal_deliver(struct ksignal *ksig, > + struct pt_regs *regs) > { > preempt_disable(); > __set_bit(RSEQ_EVENT_SIGNAL_BIT, ¤t->rseq_event_mask); > preempt_enable(); > - rseq_handle_notify_resume(regs); > + _rseq_handle_notify_resume(ksig, regs); > } > > /* rseq_preempt() requires preemption to be disabled. */ > @@ -1867,7 +1874,8 @@ static inline void rseq_set_notify_resume(struct > task_struct *t) > static inline void rseq_handle_notify_resume(struct pt_regs *regs) > { > } > -static inline void rseq_signal_deliver(struct pt_regs *regs) > +static inline void rseq_signal_deliver(struct ksignal *ksig, > + struct pt_regs *regs) > { > } > static inline void rseq_preempt(struct task_struct *t) > diff --git a/kernel/rseq.c b/kernel/rseq.c > index ae306f90c514..22b6acf1ad63 100644 > --- a/kernel/rseq.c > +++ b/kernel/rseq.c > @@ -251,10 +251,10 @@ static int rseq_ip_fixup(struct pt_regs *regs) > * respect to other threads scheduled on the same CPU, and with respect > * to signal handlers. > */ > -void __rseq_handle_notify_resume(struct pt_regs *regs) > +void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) > { > struct task_struct *t = current; > - int ret; > + int ret, sig; > > if (unlikely(t->flags & PF_EXITING)) > return; > @@ -268,7 +268,8 @@ void __rseq_handle_notify_resume(struct pt_regs *regs) > return; > > error: > - force_sig(SIGSEGV, t); > + sig = ksig ? ksig->sig : 0; > + force_sigsegv(sig, t); > } > > #ifdef CONFIG_DEBUG_RSEQ > -- > 2.1.4 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
On Wed, Jun 20, 2018 at 12:55:35PM -0400, Mathieu Desnoyers wrote: > ----- On Jun 20, 2018, at 12:36 PM, Will Deacon will.deacon@arm.com wrote: > > > When delivering a signal to a task that is using rseq, we call into > > __rseq_handle_notify_resume() so that the registers pushed in the > > sigframe are updated to reflect the state of the restartable sequence > > (for example, ensuring that the signal returns to the abort handler if > > necessary). > > > > However, if the rseq management fails due to an unrecoverable fault when > > accessing userspace or certain combinations of RSEQ_CS_* flags, then we > > will attempt to deliver a SIGSEGV. This has the potential for infinite > > recursion if the rseq code continuously fails on signal delivery. > > > > Avoid this problem by using force_sigsegv() instead of force_sig(), which > > is explicitly designed to reset the SEGV handler to SIG_DFL in the case > > of a recursive fault. > > Your approach looks good. One nit below implementation-wise: Nit accepted :) v2 on its way. Will
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c index f09e9d66d605..6434a62591e8 100644 --- a/arch/arm/kernel/signal.c +++ b/arch/arm/kernel/signal.c @@ -544,7 +544,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) * Increment event counter and perform fixup for the pre-signal * frame. */ - rseq_signal_deliver(regs); + rseq_signal_deliver(ksig, regs); /* * Set up the stack frame diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c index 17fe4339ba59..af822e764569 100644 --- a/arch/powerpc/kernel/signal.c +++ b/arch/powerpc/kernel/signal.c @@ -134,7 +134,7 @@ static void do_signal(struct task_struct *tsk) /* Re-enable the breakpoints for the signal stack */ thread_change_pc(tsk, tsk->thread.regs); - rseq_signal_deliver(tsk->thread.regs); + rseq_signal_deliver(&ksig, tsk->thread.regs); if (is32) { if (ksig.ka.sa.sa_flags & SA_SIGINFO) diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 445ca11ff863..92a3b312a53c 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -692,7 +692,7 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs) * Increment event counter and perform fixup for the pre-signal * frame. */ - rseq_signal_deliver(regs); + rseq_signal_deliver(ksig, regs); /* Set up the stack frame */ if (is_ia32_frame(ksig)) { diff --git a/include/linux/sched.h b/include/linux/sched.h index 87bf02d93a27..277f8ccb0661 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1799,20 +1799,27 @@ static inline void rseq_set_notify_resume(struct task_struct *t) set_tsk_thread_flag(t, TIF_NOTIFY_RESUME); } -void __rseq_handle_notify_resume(struct pt_regs *regs); +void __rseq_handle_notify_resume(struct ksignal *sig, struct pt_regs *regs); -static inline void rseq_handle_notify_resume(struct pt_regs *regs) +static inline void _rseq_handle_notify_resume(struct ksignal *ksig, + struct pt_regs *regs) { if (current->rseq) - __rseq_handle_notify_resume(regs); + __rseq_handle_notify_resume(ksig, regs); +} + +static inline void rseq_handle_notify_resume(struct pt_regs *regs) +{ + _rseq_handle_notify_resume(NULL, regs); } -static inline void rseq_signal_deliver(struct pt_regs *regs) +static inline void rseq_signal_deliver(struct ksignal *ksig, + struct pt_regs *regs) { preempt_disable(); __set_bit(RSEQ_EVENT_SIGNAL_BIT, ¤t->rseq_event_mask); preempt_enable(); - rseq_handle_notify_resume(regs); + _rseq_handle_notify_resume(ksig, regs); } /* rseq_preempt() requires preemption to be disabled. */ @@ -1867,7 +1874,8 @@ static inline void rseq_set_notify_resume(struct task_struct *t) static inline void rseq_handle_notify_resume(struct pt_regs *regs) { } -static inline void rseq_signal_deliver(struct pt_regs *regs) +static inline void rseq_signal_deliver(struct ksignal *ksig, + struct pt_regs *regs) { } static inline void rseq_preempt(struct task_struct *t) diff --git a/kernel/rseq.c b/kernel/rseq.c index ae306f90c514..22b6acf1ad63 100644 --- a/kernel/rseq.c +++ b/kernel/rseq.c @@ -251,10 +251,10 @@ static int rseq_ip_fixup(struct pt_regs *regs) * respect to other threads scheduled on the same CPU, and with respect * to signal handlers. */ -void __rseq_handle_notify_resume(struct pt_regs *regs) +void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) { struct task_struct *t = current; - int ret; + int ret, sig; if (unlikely(t->flags & PF_EXITING)) return; @@ -268,7 +268,8 @@ void __rseq_handle_notify_resume(struct pt_regs *regs) return; error: - force_sig(SIGSEGV, t); + sig = ksig ? ksig->sig : 0; + force_sigsegv(sig, t); } #ifdef CONFIG_DEBUG_RSEQ
When delivering a signal to a task that is using rseq, we call into __rseq_handle_notify_resume() so that the registers pushed in the sigframe are updated to reflect the state of the restartable sequence (for example, ensuring that the signal returns to the abort handler if necessary). However, if the rseq management fails due to an unrecoverable fault when accessing userspace or certain combinations of RSEQ_CS_* flags, then we will attempt to deliver a SIGSEGV. This has the potential for infinite recursion if the rseq code continuously fails on signal delivery. Avoid this problem by using force_sigsegv() instead of force_sig(), which is explicitly designed to reset the SEGV handler to SIG_DFL in the case of a recursive fault. Signed-off-by: Will Deacon <will.deacon@arm.com> --- Sending as an RFC since I only spotted this via code inspection and haven't tried to trigger it yet. arch/arm/kernel/signal.c | 2 +- arch/powerpc/kernel/signal.c | 2 +- arch/x86/kernel/signal.c | 2 +- include/linux/sched.h | 20 ++++++++++++++------ kernel/rseq.c | 7 ++++--- 5 files changed, 21 insertions(+), 12 deletions(-) -- 2.1.4