|
| 1 | +From cebcb3bb8361857c498bfb80df7f2d7228ba95a1 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Kees Cook < [email protected]> |
| 3 | +Date: Wed, 10 Aug 2016 16:28:09 -0700 |
| 4 | +Subject: [PATCH] UPSTREAM: seccomp: Fix tracer exit notifications during fatal |
| 5 | + signals |
| 6 | + |
| 7 | +This fixes a ptrace vs fatal pending signals bug as manifested in |
| 8 | +seccomp now that seccomp was reordered to happen after ptrace. The |
| 9 | +short version is that seccomp should not attempt to call do_exit() |
| 10 | +while fatal signals are pending under a tracer. The existing code was |
| 11 | +trying to be as defensively paranoid as possible, but it now ends up |
| 12 | +confusing ptrace. Instead, the syscall can just be skipped (which solves |
| 13 | +the original concern that the do_exit() was addressing) and normal signal |
| 14 | +handling, tracer notification, and process death can happen. |
| 15 | + |
| 16 | +Paraphrasing from the original bug report: |
| 17 | + |
| 18 | +If a tracee task is in a PTRACE_EVENT_SECCOMP trap, or has been resumed |
| 19 | +after such a trap but not yet been scheduled, and another task in the |
| 20 | +thread-group calls exit_group(), then the tracee task exits without the |
| 21 | +ptracer receiving a PTRACE_EVENT_EXIT notification. Test case here: |
| 22 | +https://gist.github.com/khuey/3c43ac247c72cef8c956ca73281c9be7 |
| 23 | + |
| 24 | +The bug happens because when __seccomp_filter() detects |
| 25 | +fatal_signal_pending(), it calls do_exit() without dequeuing the fatal |
| 26 | +signal. When do_exit() sends the PTRACE_EVENT_EXIT notification and |
| 27 | +that task is descheduled, __schedule() notices that there is a fatal |
| 28 | +signal pending and changes its state from TASK_TRACED to TASK_RUNNING. |
| 29 | +That prevents the ptracer's waitpid() from returning the ptrace event. |
| 30 | +A more detailed analysis is here: |
| 31 | +https://github.com/mozilla/rr/issues/1762#issuecomment-237396255. |
| 32 | + |
| 33 | +Reported-by: Robert O'Callahan < [email protected]> |
| 34 | +Reported-by: Kyle Huey < [email protected]> |
| 35 | +Tested-by: Kyle Huey < [email protected]> |
| 36 | +Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace") |
| 37 | +Signed-off-by: Kees Cook < [email protected]> |
| 38 | +Acked-by: Oleg Nesterov < [email protected]> |
| 39 | +Acked-by: James Morris < [email protected]> |
| 40 | +(cherry picked from commit 485a252a5559b45d7df04c819ec91177c62c270b) |
| 41 | + |
| 42 | +Bug: 119769499 |
| 43 | +Change-Id: I444e69093e88d58587b4d5c4f2d777985591c32d |
| 44 | +Signed-off-by: Greg Hackmann < [email protected]> |
| 45 | +--- |
| 46 | + kernel/seccomp.c | 12 ++++++++---- |
| 47 | + 1 file changed, 8 insertions(+), 4 deletions(-) |
| 48 | + |
| 49 | +diff --git a/kernel/seccomp.c b/kernel/seccomp.c |
| 50 | +index 98b48c2793a26..99bb8734fc88d 100644 |
| 51 | +--- a/kernel/seccomp.c |
| 52 | ++++ b/kernel/seccomp.c |
| 53 | +@@ -650,12 +650,16 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, |
| 54 | + ptrace_event(PTRACE_EVENT_SECCOMP, data); |
| 55 | + /* |
| 56 | + * The delivery of a fatal signal during event |
| 57 | +- * notification may silently skip tracer notification. |
| 58 | +- * Terminating the task now avoids executing a system |
| 59 | +- * call that may not be intended. |
| 60 | ++ * notification may silently skip tracer notification, |
| 61 | ++ * which could leave us with a potentially unmodified |
| 62 | ++ * syscall that the tracer would have liked to have |
| 63 | ++ * changed. Since the process is about to die, we just |
| 64 | ++ * force the syscall to be skipped and let the signal |
| 65 | ++ * kill the process and correctly handle any tracer exit |
| 66 | ++ * notifications. |
| 67 | + */ |
| 68 | + if (fatal_signal_pending(current)) |
| 69 | +- do_exit(SIGSYS); |
| 70 | ++ goto skip; |
| 71 | + /* Check if the tracer forced the syscall to be skipped. */ |
| 72 | + this_syscall = syscall_get_nr(current, task_pt_regs(current)); |
| 73 | + if (this_syscall < 0) |
0 commit comments