| Message ID | 20210705084453.2151729-2-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] perf: Fix required permissions if sigtrap is requested | expand |
On Mon, Jul 5, 2021 at 10:45 AM Marco Elver <elver@google.com> wrote: > > Refactor the permission check in perf_event_open() into a helper > perf_check_permission(). This makes the permission check logic more > readable (because we no longer have a negated disjunction). Add a > comment mentioning the ptrace check also checks the uid. > > No functional change intended. > > Signed-off-by: Marco Elver <elver@google.com> Reviewed-by: Dmitry Vyukov <dvyukov@google.com> > --- > v3: > * Introduce this patch to refactor the permissions checking logic to > make it more readable (reported by Eric W. Biederman). > --- > kernel/events/core.c | 58 ++++++++++++++++++++++++-------------------- > 1 file changed, 32 insertions(+), 26 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index f79ee82e644a..3008b986994b 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -11912,6 +11912,37 @@ __perf_event_ctx_lock_double(struct perf_event *group_leader, > return gctx; > } > > +static bool > +perf_check_permission(struct perf_event_attr *attr, struct task_struct *task) > +{ > + unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS; > + bool is_capable = perfmon_capable(); > + > + if (attr->sigtrap) { > + /* > + * perf_event_attr::sigtrap sends signals to the other task. > + * Require the current task to also have CAP_KILL. > + */ > + rcu_read_lock(); > + is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL); > + rcu_read_unlock(); > + > + /* > + * If the required capabilities aren't available, checks for > + * ptrace permissions: upgrade to ATTACH, since sending signals > + * can effectively change the target task. > + */ > + ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS; > + } > + > + /* > + * Preserve ptrace permission check for backwards compatibility. The > + * ptrace check also includes checks that the current task and other > + * task have matching uids, and is therefore not done here explicitly. > + */ > + return is_capable || ptrace_may_access(task, ptrace_mode); > +} > + > /** > * sys_perf_event_open - open a performance event, associate it to a task/cpu > * > @@ -12152,43 +12183,18 @@ SYSCALL_DEFINE5(perf_event_open, > } > > if (task) { > - unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS; > - bool is_capable; > - > err = down_read_interruptible(&task->signal->exec_update_lock); > if (err) > goto err_file; > > - is_capable = perfmon_capable(); > - if (attr.sigtrap) { > - /* > - * perf_event_attr::sigtrap sends signals to the other > - * task. Require the current task to also have > - * CAP_KILL. > - */ > - rcu_read_lock(); > - is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL); > - rcu_read_unlock(); > - > - /* > - * If the required capabilities aren't available, checks > - * for ptrace permissions: upgrade to ATTACH, since > - * sending signals can effectively change the target > - * task. > - */ > - ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS; > - } > - > /* > - * Preserve ptrace permission check for backwards compatibility. > - * > * We must hold exec_update_lock across this and any potential > * perf_install_in_context() call for this new event to > * serialize against exec() altering our credentials (and the > * perf_event_exit_task() that could imply). > */ > err = -EACCES; > - if (!is_capable && !ptrace_may_access(task, ptrace_mode)) > + if (!perf_check_permission(&attr, task)) > goto err_cred; > } > > -- > 2.32.0.93.g670b81a890-goog >
diff --git a/kernel/events/core.c b/kernel/events/core.c index f79ee82e644a..3008b986994b 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -11912,6 +11912,37 @@ __perf_event_ctx_lock_double(struct perf_event *group_leader, return gctx; } +static bool +perf_check_permission(struct perf_event_attr *attr, struct task_struct *task) +{ + unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS; + bool is_capable = perfmon_capable(); + + if (attr->sigtrap) { + /* + * perf_event_attr::sigtrap sends signals to the other task. + * Require the current task to also have CAP_KILL. + */ + rcu_read_lock(); + is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL); + rcu_read_unlock(); + + /* + * If the required capabilities aren't available, checks for + * ptrace permissions: upgrade to ATTACH, since sending signals + * can effectively change the target task. + */ + ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS; + } + + /* + * Preserve ptrace permission check for backwards compatibility. The + * ptrace check also includes checks that the current task and other + * task have matching uids, and is therefore not done here explicitly. + */ + return is_capable || ptrace_may_access(task, ptrace_mode); +} + /** * sys_perf_event_open - open a performance event, associate it to a task/cpu * @@ -12152,43 +12183,18 @@ SYSCALL_DEFINE5(perf_event_open, } if (task) { - unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS; - bool is_capable; - err = down_read_interruptible(&task->signal->exec_update_lock); if (err) goto err_file; - is_capable = perfmon_capable(); - if (attr.sigtrap) { - /* - * perf_event_attr::sigtrap sends signals to the other - * task. Require the current task to also have - * CAP_KILL. - */ - rcu_read_lock(); - is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL); - rcu_read_unlock(); - - /* - * If the required capabilities aren't available, checks - * for ptrace permissions: upgrade to ATTACH, since - * sending signals can effectively change the target - * task. - */ - ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS; - } - /* - * Preserve ptrace permission check for backwards compatibility. - * * We must hold exec_update_lock across this and any potential * perf_install_in_context() call for this new event to * serialize against exec() altering our credentials (and the * perf_event_exit_task() that could imply). */ err = -EACCES; - if (!is_capable && !ptrace_may_access(task, ptrace_mode)) + if (!perf_check_permission(&attr, task)) goto err_cred; }
Refactor the permission check in perf_event_open() into a helper perf_check_permission(). This makes the permission check logic more readable (because we no longer have a negated disjunction). Add a comment mentioning the ptrace check also checks the uid. No functional change intended. Signed-off-by: Marco Elver <elver@google.com> --- v3: * Introduce this patch to refactor the permissions checking logic to make it more readable (reported by Eric W. Biederman). --- kernel/events/core.c | 58 ++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 26 deletions(-)