Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 May 2022 16:23:19 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Christian =?iso-8859-1?Q?G=F6ttsche?= <cgzones@googlemail.com>
Cc:     selinux@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] [RFC PATCH] sched: only perform capability check on
 privileged operation
Message-ID: <20220504142319.GB2501@worktop.programming.kicks-ass.net>
References: <20200904160031.6444-1-cgzones@googlemail.com>
 <20220502152414.110922-1-cgzones@googlemail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220502152414.110922-1-cgzones@googlemail.com>
Precedence: bulk

On Mon, May 02, 2022 at 05:24:14PM +0200, Christian G?ttsche wrote:
> sched_setattr(2) issues via kernel/sched/core.c:__sched_setscheduler()
> a CAP_SYS_NICE audit event unconditionally, even when the requested
> operation does not require that capability / is unprivileged, i.e. for
> reducing niceness.
> This is relevant in connection with SELinux, where a capability check
> results in a policy decision and by default a denial message on
> insufficient permission is issued.
> It can lead to three undesired cases:
>   1. A denial message is generated, even in case the operation was an
>      unprivileged one and thus the syscall succeeded, creating noise.
>   2. To avoid the noise from 1. the policy writer adds a rule to ignore
>      those denial messages, hiding future syscalls, where the task
>      performs an actual privileged operation, leading to hidden limited
>      functionality of that task.
>   3. To avoid the noise from 1. the policy writer adds a rule to allow
>      the task the capability CAP_SYS_NICE, while it does not need it,
>      violating the principle of least privilege.
> 
> Conduct privilged/unprivileged categorization first and perform a
> capable test (and at most once) only if needed.
> 
> Signed-off-by: Christian G?ttsche <cgzones@googlemail.com>

Does something like so on top work?

---
 kernel/sched/core.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ba5a9a1ce1e5..d3b5a2757c5f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6931,17 +6931,27 @@ void set_user_nice(struct task_struct *p, long nice)
 EXPORT_SYMBOL(set_user_nice);
 
 /*
- * can_nice - check if a task can reduce its nice value
+ * is_nice_reduction - check if nice value is an actual reduction
+ *
  * @p: task
  * @nice: nice value
  */
-int can_nice(const struct task_struct *p, const int nice)
+static bool is_nice_reduction(const struct task_struct *p, const int nice)
 {
 	/* Convert nice value [19,-20] to rlimit style value [1,40]: */
 	int nice_rlim = nice_to_rlimit(nice);
 
-	return (nice_rlim <= task_rlimit(p, RLIMIT_NICE) ||
-		capable(CAP_SYS_NICE));
+	return (nice_rlim <= task_rlimit(p, RLIMIT_NICE));
+}
+
+/*
+ * can_nice - check if a task can reduce its nice value
+ * @p: task
+ * @nice: nice value
+ */
+int can_nice(const struct task_struct *p, const int nice)
+{
+	return is_nice_reduction(p, nice) || capable(CAP_SYS_NICE);
 }
 
 #ifdef __ARCH_WANT_SYS_NICE
@@ -7220,22 +7230,6 @@ static bool check_same_owner(struct task_struct *p)
 	return match;
 }
 
-/*
- * is_nice_reduction - check if nice value is an actual reduction
- *
- * Similar to can_nice() but does not perform a capability check.
- *
- * @p: task
- * @nice: nice value
- */
-static bool is_nice_reduction(const struct task_struct *p, const int nice)
-{
-	/* Convert nice value [19,-20] to rlimit style value [1,40]: */
-	int nice_rlim = nice_to_rlimit(nice);
-
-	return (nice_rlim <= task_rlimit(p, RLIMIT_NICE));
-}
-
 /*
  * Allow unprivileged RT tasks to decrease priority.
  * Only issue a capable test if needed and only once to avoid an audit
@@ -7247,13 +7241,12 @@ static int user_check_sched_setscheduler(struct task_struct *p,
 {
 	if (fair_policy(policy)) {
 		if (attr->sched_nice < task_nice(p) &&
-			!is_nice_reduction(p, attr->sched_nice))
+		    !is_nice_reduction(p, attr->sched_nice))
 			goto req_priv;
 	}
 
 	if (rt_policy(policy)) {
-		unsigned long rlim_rtprio =
-		task_rlimit(p, RLIMIT_RTPRIO);
+		unsigned long rlim_rtprio = task_rlimit(p, RLIMIT_RTPRIO);
 
 		/* Can't set/change the rt policy: */
 		if (policy != p->policy && !rlim_rtprio)
@@ -7261,7 +7254,7 @@ static int user_check_sched_setscheduler(struct task_struct *p,
 
 		/* Can't increase priority: */
 		if (attr->sched_priority > p->rt_priority &&
-			attr->sched_priority > rlim_rtprio)
+		    attr->sched_priority > rlim_rtprio)
 			goto req_priv;
 	}