From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-19.9 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 04BA9C11F6A for ; Wed, 30 Jun 2021 12:43:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DECED61461 for ; Wed, 30 Jun 2021 12:42:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234717AbhF3Mp1 (ORCPT ); Wed, 30 Jun 2021 08:45:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59122 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234455AbhF3Mp0 (ORCPT ); Wed, 30 Jun 2021 08:45:26 -0400 Received: from mail-wr1-x434.google.com (mail-wr1-x434.google.com [IPv6:2a00:1450:4864:20::434]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A4B4DC061766 for ; Wed, 30 Jun 2021 05:42:57 -0700 (PDT) Received: by mail-wr1-x434.google.com with SMTP id u6so3466696wrs.5 for ; Wed, 30 Jun 2021 05:42:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=GH3eDDlIhhzC5YYUSXzbXcyeMUM12l/VQtqBde75cxY=; b=g8Vc9HiiqGpCCdnXbMmpfPHSBeEKH/sIApjC946R2p27sZT6rDij07rzdDrKpcWNV0 21nM+W0421NmYGGDApDZuulIjfWqVlQ0CNLg9ls1UwwMRfoLWfSw4dNv3/63Bf36WwiQ bbjGxmknfOooBvP8JZFdTFLAGRUMNaQ5lEIFPJrs7dxGWvwzK2lBizhqM/rdX4cbEpVA GbNANCewxlMQ65vBXCZelIabE2Fw5oXXMJhfEhR0O041xGy9OjH3uxi/twtP4/0H69Qp qjogb+MzQ/kAzflzIuYE6SHNio4Ys9ktoW5tC5SxarpSOtlqyjTVyx+u6eWHoEjdVymP hJ/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=GH3eDDlIhhzC5YYUSXzbXcyeMUM12l/VQtqBde75cxY=; b=S0e39NSA6Lna23txQqjBjIVixRU1TQr1U3RvweJglfmSnKh3VPzX6WCx2G3mOWjFPj ec53HdVlgoqfTNQJAuXFwP+wsK2RAWdB/og8BxI1+hHcgD776sZ9yX/qwhyIkBxPIWQA V5alamZaxkdRrng3XjtkBxPsQg7bpxdrAbpl1uSa4Pg8SnPSuN4jZw55dhFHYk4t3jzK u3vf+pvzoDBFMDacBih/EknHHTvEur9gIS29xZrMetAGFND+5qVEO3iRwf/efzj2abFJ FBb416GO69JtxkuJiqle/vuYivED/BvmZmm90+xE6li2HlPBmr1sRos+hvGxGL5YGihE BbBw== X-Gm-Message-State: AOAM5332K9somfUurvzDRMsCYt3pDcDlqN8Kditf1spTf40tVo0bMORd lT/nG0IHNgnay+iu6N5qVDP8YQ== X-Google-Smtp-Source: ABdhPJyxeUMm3N5mON2rwouCk9rz+UnOSWe5m58WSIxdEkdrWBONo1YUOGFyEqq3dwYWQ9IlWHXzkg== X-Received: by 2002:a5d:65c1:: with SMTP id e1mr40845057wrw.196.1625056975754; Wed, 30 Jun 2021 05:42:55 -0700 (PDT) Received: from elver.google.com ([2a00:79e0:15:13:8b0e:c57f:ff29:7e4]) by smtp.gmail.com with ESMTPSA id r16sm13220921wrx.63.2021.06.30.05.42.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Jun 2021 05:42:54 -0700 (PDT) Date: Wed, 30 Jun 2021 14:42:49 +0200 From: Marco Elver To: Ondrej Mosnacek Cc: Peter Zijlstra , Thomas Gleixner , Ingo Molnar , kasan-dev@googlegroups.com, Linux kernel mailing list , "Serge E. Hallyn" , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Linux Security Module list , linux-perf-users@vger.kernel.org, Eric Biederman , Dmitry Vyukov Subject: Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested Message-ID: References: <20210630093709.3612997-1-elver@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.0.5 (2021-01-21) Precedence: bulk List-ID: X-Mailing-List: linux-perf-users@vger.kernel.org On Wed, Jun 30, 2021 at 01:13PM +0200, Ondrej Mosnacek wrote: > On Wed, Jun 30, 2021 at 11:38 AM Marco Elver wrote: [...] > > +static inline bool kill_capable(void) > > +{ > > + return capable(CAP_KILL) || capable(CAP_SYS_ADMIN); > > Is it really necessary to fall back to CAP_SYS_ADMIN here? CAP_PERFMON > and CAP_BPF have been split off from CAP_SYS_ADMIN recently, so they > have it for backwards compatibility. You are adding a new restriction > for a very specific action, so I don't think the fallback is needed. That means someone having CAP_SYS_ADMIN, but not CAP_KILL, can't perform the desired action. Is this what you'd like? If so, I'll just remove the wrapper, and call capable(CAP_KILL) directly. > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index fe88d6eea3c2..1ab4bc867531 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -12152,10 +12152,21 @@ SYSCALL_DEFINE5(perf_event_open, > > } > > > > if (task) { > > + 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 have CAP_KILL. > > + */ > > + is_capable &= kill_capable(); > > Is it necessary to do all this dance just to call perfmon_capable() > first? Couldn't this be simply: > > err = -EPERM; > if (attr.sigtrap && !capable(CAP_KILL)) > goto err_cred; Not so much about perfmon_capable() but about the ptrace_may_access() check. The condition here is supposed to be: want CAP_PERFMON and (CAP_KILL if sigtrap) OR want ptrace access (which includes a check for same thread-group and uid) If we did what you propose, then the ptrace check is effectively ignored if attr.sigtrap, and that's not what we want. There are lots of other ways of writing the same thing, but it should also remain readable and sticking it all into the same condition is not readable. > Also, looking at kill_ok_by_cred() in kernel/signal.c, would it > perhaps be more appropriate to do > ns_capable(__task_cred(task)->user_ns, CAP_KILL) instead? (There might > also need to be some careful locking around getting the target task's > creds - I'm not sure...) That might make sense. AFAIK, the locking is already in place via exec_update_lock. Let me investigate. > > + } > > + > > /* > > * Preserve ptrace permission check for backwards compatibility. > > * > > @@ -12165,7 +12176,7 @@ SYSCALL_DEFINE5(perf_event_open, > > * perf_event_exit_task() that could imply). > > */ > > err = -EACCES; > > BTW, shouldn't this (and several other such cases in this file...) > actually be EPERM, as is the norm for capability checks? I'm not a perf maintainer, so I can't give you a definitive answer. But, this would change the ABI, so I don't think it's realistic to request this change at this point unfortunately. Thanks.