[PATCH 0/3] ptrace RSE handling

[PATCH 0/3] ptrace RSE handling

[Petr Tesarik]

This patch series fixes handling of RSE by ptrace. It addresses all bugs
and possible shortcomings discussed here in the past month.

Regards,
Petr Tesarik

Re: [PATCH 0/3] ptrace RSE handling

[Roland McGrath]

I am obviously very happy with these patches overall, since you've done
things in the manner I asked for.  There are a few small things that I'll
follow up to the individual patch postings about.

I think it should be sliced up slightly smaller, with the generic ptrace
changes from me in a separate patch.  I'll post my version of that patch
with improved documentation momentarily.


Thanks,
Roland

[PATCH] arch_ptrace_stop

[Roland McGrath]

This adds support to allow asm/ptrace.h to define two new macros,
arch_ptrace_stop_needed and arch_ptrace_stop.  These control special
machine-specific actions to be done before a ptrace stop.  The new
code compiles away to nothing when the new macros are not defined.
This is the case on all machines to begin with.

On ia64, these macros will be defined to solve the long-standing
issue of ptrace vs register backing store.

Signed-off-by: Roland McGrath <roland@redhat.com>
CC: Petr Tesarik <ptesarik@suse.cz>
CC: Tony Luck <tony.luck@intel.com>
---
 include/linux/ptrace.h |   35 +++++++++++++++++++++++++++++++++++
 kernel/signal.c        |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index ae8146a..7168757 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -128,6 +128,41 @@ int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data);
 #define force_successful_syscall_return() do { } while (0)
 #endif
 
+#ifndef arch_ptrace_stop_needed
+/**
+ * arch_ptrace_stop_needed - Decide whether arch_ptrace_stop() should be called
+ * @code:	current->exit_code value ptrace will stop with
+ * @info:	siginfo_t pointer (or %NULL) for signal ptrace will stop with
+ *
+ * This is called with the siglock held, to decide whether or not it's
+ * necessary to release the siglock and call arch_ptrace_stop() with the
+ * same @code and @info arguments.  It can be defined to a constant if
+ * arch_ptrace_stop() is never required, or always is.  On machines where
+ * this makes sense, it should be defined to a quick test to optimize out
+ * calling arch_ptrace_stop() when it would be superfluous.  For example,
+ * if the thread has not been back to user mode since the last stop, the
+ * thread state might indicate that nothing needs to be done.
+ */
+#define arch_ptrace_stop_needed(code, info)	(0)
+#endif
+
+#ifndef arch_ptrace_stop
+/**
+ * arch_ptrace_stop - Do machine-specific work before stopping for ptrace
+ * @code:	current->exit_code value ptrace will stop with
+ * @info:	siginfo_t pointer (or %NULL) for signal ptrace will stop with
+ *
+ * This is called with no locks held when arch_ptrace_stop_needed() has
+ * just returned nonzero.  It is allowed to block, e.g. for user memory
+ * access.  The arch can have machine-specific work to be done before
+ * ptrace stops.  On ia64, register backing store gets written back to user
+ * memory here.  Since this can be costly (requires dropping the siglock),
+ * we only do it when the arch requires it for this particular stop, as
+ * indicated by arch_ptrace_stop_needed().
+ */
+#define arch_ptrace_stop(code, info)		do { } while (0)
+#endif
+
 #endif
 
 #endif
diff --git a/kernel/signal.c b/kernel/signal.c
index afa4f78..2e85fcb 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1594,6 +1594,17 @@ static inline int may_ptrace_stop(void)
 }
 
 /*
+ * Return nonzero if there is a SIGKILL that should be waking us up.
+ * Called with the siglock held.
+ */
+static int sigkill_pending(struct task_struct *tsk)
+{
+	return ((sigismember(&tsk->pending.signal, SIGKILL) ||
+		 sigismember(&tsk->signal->shared_pending.signal, SIGKILL)) &&
+		!unlikely(sigismember(&tsk->blocked, SIGKILL)));
+}
+
+/*
  * This must be called with current->sighand->siglock held.
  *
  * This should be the path for all ptrace stops.
@@ -1606,6 +1617,26 @@ static inline int may_ptrace_stop(void)
  */
 static void ptrace_stop(int exit_code, int nostop_code, siginfo_t *info)
 {
+	int killed = 0;
+
+	if (arch_ptrace_stop_needed(exit_code, info)) {
+		/*
+		 * The arch code has something special to do before a
+		 * ptrace stop.  This is allowed to block, e.g. for faults
+		 * on user stack pages.  We can't keep the siglock while
+		 * calling arch_ptrace_stop, so we must release it now.
+		 * To preserve proper semantics, we must do this before
+		 * any signal bookkeeping like checking group_stop_count.
+		 * Meanwhile, a SIGKILL could come in before we retake the
+		 * siglock.  That must prevent us from sleeping in TASK_TRACED.
+		 * So after regaining the lock, we must check for SIGKILL.
+		 */
+		spin_unlock_irq(&current->sighand->siglock);
+		arch_ptrace_stop(exit_code, info);
+		spin_lock_irq(&current->sighand->siglock);
+		killed = sigkill_pending(current);
+	}
+
 	/*
 	 * If there is a group stop in progress,
 	 * we must participate in the bookkeeping.
@@ -1621,7 +1652,7 @@ static void ptrace_stop(int exit_code, int nostop_code, siginfo_t *info)
 	spin_unlock_irq(&current->sighand->siglock);
 	try_to_freeze();
 	read_lock(&tasklist_lock);
-	if (may_ptrace_stop()) {
+	if (!unlikely(killed) && may_ptrace_stop()) {
 		do_notify_parent_cldstop(current, CLD_TRAPPED);
 		read_unlock(&tasklist_lock);
 		schedule();

Re: [PATCH] arch_ptrace_stop

[Andrew Morton]

On Fri,  7 Dec 2007 17:11:52 -0800 (PST) Roland McGrath <roland@redhat.com> wrote:

> 
> This adds support to allow asm/ptrace.h to define two new macros,
> arch_ptrace_stop_needed and arch_ptrace_stop.  These control special
> machine-specific actions to be done before a ptrace stop.  The new
> code compiles away to nothing when the new macros are not defined.
> This is the case on all machines to begin with.
> 
> On ia64, these macros will be defined to solve the long-standing
> issue of ptrace vs register backing store.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>
> CC: Petr Tesarik <ptesarik@suse.cz>
> CC: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/ptrace.h |   35 +++++++++++++++++++++++++++++++++++
>  kernel/signal.c        |   33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index ae8146a..7168757 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -128,6 +128,41 @@ int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data);
>  #define force_successful_syscall_return() do { } while (0)
>  #endif
>  
> +#ifndef arch_ptrace_stop_needed
> +/**
> + * arch_ptrace_stop_needed - Decide whether arch_ptrace_stop() should be called
> + * @code:	current->exit_code value ptrace will stop with
> + * @info:	siginfo_t pointer (or %NULL) for signal ptrace will stop with
> + *
> + * This is called with the siglock held, to decide whether or not it's
> + * necessary to release the siglock and call arch_ptrace_stop() with the
> + * same @code and @info arguments.  It can be defined to a constant if
> + * arch_ptrace_stop() is never required, or always is.  On machines where
> + * this makes sense, it should be defined to a quick test to optimize out
> + * calling arch_ptrace_stop() when it would be superfluous.  For example,
> + * if the thread has not been back to user mode since the last stop, the
> + * thread state might indicate that nothing needs to be done.
> + */
> +#define arch_ptrace_stop_needed(code, info)	(0)
> +#endif
> +
> +#ifndef arch_ptrace_stop
> +/**
> + * arch_ptrace_stop - Do machine-specific work before stopping for ptrace
> + * @code:	current->exit_code value ptrace will stop with
> + * @info:	siginfo_t pointer (or %NULL) for signal ptrace will stop with
> + *
> + * This is called with no locks held when arch_ptrace_stop_needed() has
> + * just returned nonzero.  It is allowed to block, e.g. for user memory
> + * access.  The arch can have machine-specific work to be done before
> + * ptrace stops.  On ia64, register backing store gets written back to user
> + * memory here.  Since this can be costly (requires dropping the siglock),
> + * we only do it when the arch requires it for this particular stop, as
> + * indicated by arch_ptrace_stop_needed().
> + */
> +#define arch_ptrace_stop(code, info)		do { } while (0)

Mutter.  These would be better as static inlines.  A macro just invites
variable-unused warnings on non-ia64 and outright compilation errors on
ia64.  Speaking from experience...

static inline void arch_ptrace_stop(int exit_code, siginfo_t *info)
{
}
#define arch_ptrace_stop arch_ptrace_stop

should work?

>  /*
> + * Return nonzero if there is a SIGKILL that should be waking us up.
> + * Called with the siglock held.
> + */
> +static int sigkill_pending(struct task_struct *tsk)
> +{
> +	return ((sigismember(&tsk->pending.signal, SIGKILL) ||
> +		 sigismember(&tsk->signal->shared_pending.signal, SIGKILL)) &&
> +		!unlikely(sigismember(&tsk->blocked, SIGKILL)));
> +}

Could you please take a peek at the infrastructure added by
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.24-rc4/2.6.24-rc4-mm1/broken-out/add-lock_page_killable.patch
and see if there is exploitable commonality?

Re: [PATCH] arch_ptrace_stop

[Roland McGrath]

> Mutter.  These would be better as static inlines.  A macro just invites
> variable-unused warnings on non-ia64 and outright compilation errors on
> ia64.  Speaking from experience...
> 
> static inline void arch_ptrace_stop(int exit_code, siginfo_t *info)
> {
> }
> #define arch_ptrace_stop arch_ptrace_stop
> 
> should work?

That's fine with me.  I just followed the example of e.g. arch_ptrace_attach.

> >  /*
> > + * Return nonzero if there is a SIGKILL that should be waking us up.
> > + * Called with the siglock held.
> > + */
> > +static int sigkill_pending(struct task_struct *tsk)
> > +{
> > +	return ((sigismember(&tsk->pending.signal, SIGKILL) ||
> > +		 sigismember(&tsk->signal->shared_pending.signal, SIGKILL)) &&
> > +		!unlikely(sigismember(&tsk->blocked, SIGKILL)));
> > +}
> 
> Could you please take a peek at the infrastructure added by
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.24-rc4/2.6.24-rc4-mm1/broken-out/add-lock_page_killable.patch
> and see if there is exploitable commonality?

I haven't reviewed that whole set in detail yet.  So I'd rather leave this
for a later cleanup.  (This ptrace change alone, along with its ia64
pieces, can probably go in much earlier without disrupting anything else.)

The fatal_signal_pending function in that patch is not exactly the same as
this check, though it might turn out that it's actually equivalent due to
higher-level constraints.  


Thanks,
Roland