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.
Signed-off-by: Ulrich Weigand <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
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(-)
diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 127ec3f..f5b829a 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -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;
}
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 204cfd1..6979226 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -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;
}
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 8fcfa39..a7403cc 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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);
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 5b5424c..d28052d 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -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:
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index ccb781a..ed7aed3 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -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;
}
diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
index 74e587a..40d88df 100644
--- a/fs/binfmt_som.c
+++ b/fs/binfmt_som.c
@@ -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 */
diff --git a/fs/exec.c b/fs/exec.c
index 67120ec..700edae 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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
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;
}
Reported-by: Arnd Bergmann <[email protected]>
CC: Ulrich Weigand <[email protected]>
Signed-off-by: Roland McGrath <[email protected]>
---
fs/exec.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 4e834f1..ec5df9a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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)
On Wednesday 10 December 2008, Roland McGrath wrote:
> Reported-by: Arnd Bergmann <[email protected]>
> CC: Ulrich Weigand <[email protected]>
> Signed-off-by: Roland McGrath <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
On Tue, Dec 09, 2008 at 08:04:23PM -0800, Roland McGrath wrote:
> 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.
<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
On Thu, Dec 11, 2008 at 08:44:54AM -0800, Greg KH wrote:
> Any objections to that?
No.
--
Regards, Kirill A. Shutemov
+ Belarus, Minsk
+ ALT Linux Team, http://www.altlinux.org/
Roland McGrath <[email protected]> wrote on 12/10/2008 05:04:17 AM:
> 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.)
Hmm, I've definitely seen the problem on a F9 kernel. I haven't tried
any more recent Fedora kernels ...
> My fix (next posting) leaves the call in search_binary_handler(),
> but makes it only on the outermost call (using bprm->recursion_depth).
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