aboutsummaryrefslogtreecommitdiffstats
path: root/security
diff options
context:
space:
mode:
authorJann Horn <jann@thejh.net>2016-05-22 00:01:34 -0400
committerJames Morris <james.l.morris@oracle.com>2016-05-25 19:56:18 -0400
commitdca6b4149181baaa363b9a7ce7c550840bb3bc83 (patch)
tree173c4d478300004695fdd584f89553209ef591a3 /security
parentecc5fbd5ef472a4c659dc56a5739b3f041c0530c (diff)
Yama: fix double-spinlock and user access in atomic context
Commit 8a56038c2aef ("Yama: consolidate error reporting") causes lockups when someone hits a Yama denial. Call chain: process_vm_readv -> process_vm_rw -> process_vm_rw_core -> mm_access -> ptrace_may_access task_lock(...) is taken __ptrace_may_access -> security_ptrace_access_check -> yama_ptrace_access_check -> report_access -> kstrdup_quotable_cmdline -> get_cmdline -> access_process_vm -> get_task_mm task_lock(...) is taken again task_lock(p) just calls spin_lock(&p->alloc_lock), so at this point, spin_lock() is called on a lock that is already held by the current process. Also: Since the alloc_lock is a spinlock, sleeping inside security_ptrace_access_check hooks is probably not allowed at all? So it's not even possible to print the cmdline from in there because that might involve paging in userspace memory. It would be tempting to rewrite ptrace_may_access() to drop the alloc_lock before calling the LSM, but even then, ptrace_may_access() itself might be called from various contexts in which you're not allowed to sleep; for example, as far as I understand, to be able to hold a reference to another task, usually an RCU read lock will be taken (see e.g. kcmp() and get_robust_list()), so that also prohibits sleeping. (And using e.g. FUSE, a user can cause pagefault handling to take arbitrary amounts of time - see https://bugs.chromium.org/p/project-zero/issues/detail?id=808.) Therefore, AFAIK, in order to print the name of a process below security_ptrace_access_check(), you'd have to either grab a reference to the mm_struct and defer the access violation reporting or just use the "comm" value that's stored in kernelspace and accessible without big complications. (Or you could try to use some kind of atomic remote VM access that fails if the memory isn't paged in, similar to copy_from_user_inatomic(), and if necessary fall back to comm, but that'd be kind of ugly because the comm/cmdline choice would look pretty random to the user.) Fix it by deferring reporting of the access violation until current exits kernelspace the next time. v2: Don't oops on PTRACE_TRACEME, call report_access under task_lock(current). Also fix nonsensical comment. And don't use GPF_ATOMIC for memory allocation with no locks held. This patch is tested both for ptrace attach and ptrace traceme. Fixes: 8a56038c2aef ("Yama: consolidate error reporting") Signed-off-by: Jann Horn <jann@thejh.net> Acked-by: Kees Cook <keescook@chromium.org> Signed-off-by: James Morris <james.l.morris@oracle.com>
Diffstat (limited to 'security')
-rw-r--r--security/yama/yama_lsm.c69
1 files changed, 63 insertions, 6 deletions
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 9b756b1f3dc5..0309f2111c70 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -19,6 +19,9 @@
19#include <linux/ratelimit.h> 19#include <linux/ratelimit.h>
20#include <linux/workqueue.h> 20#include <linux/workqueue.h>
21#include <linux/string_helpers.h> 21#include <linux/string_helpers.h>
22#include <linux/task_work.h>
23#include <linux/sched.h>
24#include <linux/spinlock.h>
22 25
23#define YAMA_SCOPE_DISABLED 0 26#define YAMA_SCOPE_DISABLED 0
24#define YAMA_SCOPE_RELATIONAL 1 27#define YAMA_SCOPE_RELATIONAL 1
@@ -42,20 +45,71 @@ static DEFINE_SPINLOCK(ptracer_relations_lock);
42static void yama_relation_cleanup(struct work_struct *work); 45static void yama_relation_cleanup(struct work_struct *work);
43static DECLARE_WORK(yama_relation_work, yama_relation_cleanup); 46static DECLARE_WORK(yama_relation_work, yama_relation_cleanup);
44 47
45static void report_access(const char *access, struct task_struct *target, 48struct access_report_info {
46 struct task_struct *agent) 49 struct callback_head work;
50 const char *access;
51 struct task_struct *target;
52 struct task_struct *agent;
53};
54
55static void __report_access(struct callback_head *work)
47{ 56{
57 struct access_report_info *info =
58 container_of(work, struct access_report_info, work);
48 char *target_cmd, *agent_cmd; 59 char *target_cmd, *agent_cmd;
49 60
50 target_cmd = kstrdup_quotable_cmdline(target, GFP_ATOMIC); 61 target_cmd = kstrdup_quotable_cmdline(info->target, GFP_KERNEL);
51 agent_cmd = kstrdup_quotable_cmdline(agent, GFP_ATOMIC); 62 agent_cmd = kstrdup_quotable_cmdline(info->agent, GFP_KERNEL);
52 63
53 pr_notice_ratelimited( 64 pr_notice_ratelimited(
54 "ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n", 65 "ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n",
55 access, target_cmd, target->pid, agent_cmd, agent->pid); 66 info->access, target_cmd, info->target->pid, agent_cmd,
67 info->agent->pid);
56 68
57 kfree(agent_cmd); 69 kfree(agent_cmd);
58 kfree(target_cmd); 70 kfree(target_cmd);
71
72 put_task_struct(info->agent);
73 put_task_struct(info->target);
74 kfree(info);
75}
76
77/* defers execution because cmdline access can sleep */
78static void report_access(const char *access, struct task_struct *target,
79 struct task_struct *agent)
80{
81 struct access_report_info *info;
82 char agent_comm[sizeof(agent->comm)];
83
84 assert_spin_locked(&target->alloc_lock); /* for target->comm */
85
86 if (current->flags & PF_KTHREAD) {
87 /* I don't think kthreads call task_work_run() before exiting.
88 * Imagine angry ranting about procfs here.
89 */
90 pr_notice_ratelimited(
91 "ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n",
92 access, target->comm, target->pid,
93 get_task_comm(agent_comm, agent), agent->pid);
94 return;
95 }
96
97 info = kmalloc(sizeof(*info), GFP_ATOMIC);
98 if (!info)
99 return;
100 init_task_work(&info->work, __report_access);
101 get_task_struct(target);
102 get_task_struct(agent);
103 info->access = access;
104 info->target = target;
105 info->agent = agent;
106 if (task_work_add(current, &info->work, true) == 0)
107 return; /* success */
108
109 WARN(1, "report_access called from exiting task");
110 put_task_struct(target);
111 put_task_struct(agent);
112 kfree(info);
59} 113}
60 114
61/** 115/**
@@ -351,8 +405,11 @@ int yama_ptrace_traceme(struct task_struct *parent)
351 break; 405 break;
352 } 406 }
353 407
354 if (rc) 408 if (rc) {
409 task_lock(current);
355 report_access("traceme", current, parent); 410 report_access("traceme", current, parent);
411 task_unlock(current);
412 }
356 413
357 return rc; 414 return rc;
358} 415}