Messages in this thread | | | From | Dmitry Vyukov <> | Date | Tue, 6 Jul 2021 08:16:39 +0200 | Subject | Re: [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission() |
| |
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 >
| |