revert breakage from "tracehook: exec"

The patch 6341c39 "tracehook: exec" introduced a small regression in 2.6.27 regarding binfmt_misc exec event reporting. Since the reporting is now done in the common search_binary_handler() function, an exec of a misc binary will result in two (or possibly multiple) exec events being reported, instead of just a single one, because the misc handler contains a recursive call to search_binary_handler.

To add to the confusion, if ptrace event reporting (PTRACE_O_TRACEEVENT) is not active, the multiple instances of send_sig (SIGTRAP) will in fact cause only a single ptrace intercept, as the signals are not queued. However, if PTRACE_O_TRACEEVENT is on, the debugger will actually see multiple ptrace intercepts.

This (untested) patch moves the reporting back to the original location and outside of the binfmt_misc path.

---

 arch/x86/ia32/ia32_aout.c |    1 +
 fs/binfmt_aout.c          |    1 +
 fs/binfmt_elf.c           |    1 +
 fs/binfmt_elf_fdpic.c     |    2 ++
 fs/binfmt_flat.c          |    2 ++
 fs/binfmt_som.c           |    1 +
 fs/exec.c                 |    1 -
 7 files changed, 8 insertions(+), 1 deletion(-)

index 127ec3f..f5b829a 100644
@@ -444,6 +444,7 @@ beyond_if:
 	regs->r8 = regs->r9 = regs->r10 = regs->r11 =
 	regs->r12 = regs->r13 = regs->r14 = regs->r15 = 0;
 	set_fs(USER_DS);
+	tracehook_report_exec(&aout_format, bprm, regs);
 	return 0;
 }
 

index 204cfd1..6979226 100644
@@ -444,6 +444,7 @@ beyond_if:
 	regs->gp = ex.a_gpvalue;
 #endif
 	start_thread(regs, ex.a_entry, current->mm->start_stack);
+	tracehook_report_exec(&aout_format, bprm, regs);
 	return 0;
 }
 

index 8fcfa39..a7403cc 100644
@@ -1003,6 +1003,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 #endif
 
 	start_thread(regs, elf_entry, bprm->p);
+	tracehook_report_exec(&elf_format, bprm, regs);
 	retval = 0;
 out:
 	kfree(loc);

index 5b5424c..d28052d 100644
@@ -434,6 +434,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm,
 	entryaddr = interp_params.entry_addr ?: exec_params.entry_addr;
 	start_thread(regs, entryaddr, current->mm->start_stack);
 
+	tracehook_report_exec(&elf_fdpic_format, bprm, regs);
+
 	retval = 0;
 
 error:

index ccb781a..ed7aed3 100644
@@ -922,6 +922,8 @@ static int load_flat_binary(struct linux_binprm * bprm, struct pt_regs * regs)
 	
 	start_thread(regs, start_addr, current->mm->start_stack);
 
+	tracehook_report_exec(&flat_format, bprm, regs);
+
 	return 0;
 }
 

index 74e587a..40d88df 100644
@@ -274,6 +274,7 @@ load_som_binary(struct linux_binprm * bprm, struct pt_regs * regs)
 	map_hpux_gateway_page(current,current->mm);
 
 	start_thread_som(regs, som_entry, bprm->p);
+	tracehook_report_exec(&som_format, bprm, regs);
 	return 0;
 
 	/* error cleanup */

index 67120ec..700edae 100644
@@ -1225,7 +1225,6 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			read_unlock(&binfmt_lock);
 			retval = fn(bprm, regs);
 			if (retval >= 0) {
-				tracehook_report_exec(fmt, bprm, regs);
 				put_binfmt(fmt);
 				allow_write_access(bprm->file);
 				if (bprm->file)

Thanks for the report, good catch. I've written a test case for the bug (I'll include it with the patch I post momentarily). (Note to testers:
this bug does not manifest on Fedora kernels.)

I still think it's preferable to have the only tracehook calls be in core code, rather than in binfmt modules. It seems much less error-prone in the long run. Also, since we no longer export ptrace_notify(), we'd have to revert that as well to move the calls into binfmt modules when built as modules (I think your patch will barf on CONFIG_BINFMT_AOUT=m, e.g.).

One option would be to move the call further out, to do_execve (and compat_do_execve). But I would like to keep the binfmt and bprm pointers available in tracehook_report_exec() with full information about the binfmt and bprm->file still available. Even though that's not currently used, it will clearly be desireable for future tracing facilities to be able to scavenge details from there.

My fix (next posting) leaves the call in search_binary_handler(), but makes it only on the outermost call (using bprm->recursion_depth).

Thanks,
Roland

Roland McGrath <[email protected]> wrote on 12/10/2008 05:04:17 AM:

Hmm, I've definitely seen the problem on a F9 kernel. I haven't tried any more recent Fedora kernels ...

This fix looks good to me as well. Thanks!

Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Geschäftsführung: Erich 
Baier
  Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht 
Stuttgart, HRB 243294

The following changes since commit 437f2f91d6597c67662f847d9ed4c99cb3c440cd:
Linus Torvalds (1):
Merge master.kernel.org:/home/rmk/linux-2.6-arm

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git to-linus

Roland McGrath (1):
tracehook: exec double-reporting fix

 fs/exec.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

Thanks,
Roland
---
[PATCH] tracehook: exec double-reporting fix

The patch 6341c39 "tracehook: exec" introduced a small regression in 2.6.27 regarding binfmt_misc exec event reporting. Since the reporting is now done in the common search_binary_handler() function, an exec of a misc binary will result in two (or possibly multiple) exec events being reported, instead of just a single one, because the misc handler contains a recursive call to search_binary_handler.

To add to the confusion, if PTRACE_O_TRACEEXEC is not active, the multiple SIGTRAP signals will in fact cause only a single ptrace intercept, as the signals are not queued. However, if PTRACE_O_TRACEEXEC is on, the debugger will actually see multiple ptrace intercepts (PTRACE_EVENT_EXEC).

The test program included below demonstrates the problem.

This change fixes the bug by calling tracehook_report_exec() only in the outermost search_binary_handler() call (bprm->recursion_depth == 0).

The additional change to restore bprm->recursion_depth after each binfmt load_binary call is actually superfluous for this bug, since we test the value saved on entry to search_binary_handler(). But it keeps the use of of the depth count to its most obvious expected meaning. Depending on what binfmt handlers do in certain cases, there could have been false-positive tests for recursion limits before this change.

    /* Test program using PTRACE_O_TRACEEXEC.
       This forks and exec's the first argument with the rest of the arguments,
       while ptrace'ing.  It expects to see one PTRACE_EVENT_EXEC stop and
       then a successful exit, with no other signals or events in between.

       Test for kernel doing two PTRACE_EVENT_EXEC stops for a binfmt_misc exec:

       $ gcc -g traceexec.c -o traceexec
       $ sudo sh -c 'echo :test:M::foobar::/bin/cat: > /proc/sys/fs/binfmt_misc/register'
       $ echo 'foobar test' > ./foobar
       $ chmod +x ./foobar
       $ ./traceexec ./foobar; echo $?
       ==> good <==
       foobar test
       0
       $
       ==> bad <==
       foobar test
       unexpected status 0x4057f != 0
       3
       $

    */

    #include <stdio.h>
    #include <sys/types.h>
    #include <sys/wait.h>
    #include <sys/ptrace.h>
    #include <unistd.h>
    #include <signal.h>
    #include <stdlib.h>

    static void
    wait_for (pid_t child, int expect)
    {
      int status;
      pid_t p = wait (&status);
      if (p != child)
	{
	  perror ("wait");
	  exit (2);
	}
      if (status != expect)
	{
	  fprintf (stderr, "unexpected status %#x != %#x\n", status, expect);
	  exit (3);
	}
    }

    int
    main (int argc, char **argv)
    {
      pid_t child = fork ();

      if (child < 0)
	{
	  perror ("fork");
	  return 127;
	}
      else if (child == 0)
	{
	  ptrace (PTRACE_TRACEME);
	  raise (SIGUSR1);
	  execv (argv[1], &argv[1]);
	  perror ("execve");
	  _exit (127);
	}

      wait_for (child, W_STOPCODE (SIGUSR1));

      if (ptrace (PTRACE_SETOPTIONS, child,
		  0L, (void *) (long) PTRACE_O_TRACEEXEC) != 0)
	{
	  perror ("PTRACE_SETOPTIONS");
	  return 4;
	}

      if (ptrace (PTRACE_CONT, child, 0L, 0L) != 0)
	{
	  perror ("PTRACE_CONT");
	  return 5;
	}

      wait_for (child, W_STOPCODE (SIGTRAP | (PTRACE_EVENT_EXEC << 8)));

      if (ptrace (PTRACE_CONT, child, 0L, 0L) != 0)
	{
	  perror ("PTRACE_CONT");
	  return 6;
	}

      wait_for (child, W_EXITCODE (0, 0));

      return 0;
    }

---
 fs/exec.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

index 4e834f1..ec5df9a 100644
@@ -1159,6 +1159,7 @@ EXPORT_SYMBOL(remove_arg_zero);
  */
 int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 {
+	unsigned int depth = bprm->recursion_depth;
 	int try,retval;
 	struct linux_binfmt *fmt;
 #ifdef __alpha__

@@ -1219,8 +1220,15 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 				continue;
 			read_unlock(&binfmt_lock);
 			retval = fn(bprm, regs);
+			/*
+			 * Restore the depth counter to its starting value
+			 * in this call, so we don't have to rely on every
+			 * load_binary function to restore it on return.
+			 */
+			bprm->recursion_depth = depth;
 			if (retval >= 0) {
-				tracehook_report_exec(fmt, bprm, regs);
+				if (depth == 0)
+					tracehook_report_exec(fmt, bprm, regs);
 				put_binfmt(fmt);
 				allow_write_access(bprm->file);
 				if (bprm->file)

<snip>

Unfortunatly, bprm->recursion_depth was added in 2.6.28-rc1, so this patch can't easily apply to the 2.6.27.y tree :(

Should I also apply commit id bf2a9a39639b8b51377905397a5005f444e9a892:
From: Kirill A. Shutemov <[email protected]>
Date: Wed, 15 Oct 2008 22:02:39 -0700
Subject: [PATCH] Allow recursion in binfmt_script and binfmt_misc

To get this one to apply properly?

Hm, in looking at bf2a9a39639b8b51377905397a5005f444e9a892, that seems like a valid fix for the 2.6.27.y series as well, so perhaps I should do this anyway.

Any objections to that?

thanks,

greg k-h

No.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.org/