[RFC] sched: only issue an audit on privileged operation

Commit Message

Christian Göttsche Sept. 4, 2020, 4 p.m. UTC

sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
issue a CAP_SYS_NICE audit event unconditionally, even when the requested
operation does not require that capability / is un-privileged.

Perform privilged/unprivileged catigorization first and perform a
capable test only if needed.

Signed-off-by: Christian Göttsche <[e-mail]>
---
 kernel/sched/core.c | 65 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 18 deletions(-)

Comments

Peter Zijlstra Sept. 8, 2020, 10:25 a.m. UTC | #1

On Fri, Sep 04, 2020 at 06:00:31PM +0200, Christian Göttsche wrote:
> sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
> issue a CAP_SYS_NICE audit event unconditionally, even when the requested
> operation does not require that capability / is un-privileged.
> 
> Perform privilged/unprivileged catigorization first and perform a
> capable test only if needed.
> 
> Signed-off-by: Christian Göttsche <[e-mail]>
> ---
>  kernel/sched/core.c | 65 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 18 deletions(-)

So who sodding cares about audit, and why is that a reason to make a
trainwreck of code?

Ondrej Mosnacek Sept. 8, 2020, 11:28 a.m. UTC | #2

On Tue, Sep 8, 2020 at 12:26 PM <[e-mail]> wrote:
> On Fri, Sep 04, 2020 at 06:00:31PM +0200, Christian Göttsche wrote:
> > sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
> > issue a CAP_SYS_NICE audit event unconditionally, even when the requested
> > operation does not require that capability / is un-privileged.
> >
> > Perform privilged/unprivileged catigorization first and perform a
> > capable test only if needed.
> >
> > Signed-off-by: Christian Göttsche <[e-mail]>
> > ---
> >  kernel/sched/core.c | 65 ++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 47 insertions(+), 18 deletions(-)
>
> So who sodding cares about audit, and why is that a reason to make a
> trainwreck of code?

The commit message should be more specific. I believe Christian is
talking about the case where SELinux or other LSM denies the
capability, in which case the denial is usually logged to the audit
log. Obviously, we don't want to get a denial logged when the
capability wasn't actually required for the operation to be allowed.

Unfortunately, the capability interface doesn't provide a way to first
get the decision value and only trigger the auditing when it was
actually used in the decision, so in complex scenarios like this the
caller needs to jump through some hoops to avoid such false-positive
denial records.

--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

Patch

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8471a0f7eb32..954f968d2466 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5249,13 +5249,19 @@  static int __sched_setscheduler(struct task_struct *p,
 		return -EINVAL;
 
 	/*
-	 * Allow unprivileged RT tasks to decrease priority:
+	 * Allow unprivileged RT tasks to decrease priority.
+	 * Only issue a capable test if needed to avoid audit
+	 * event on non-privileged operations:
 	 */
-	if (user && !capable(CAP_SYS_NICE)) {
+	if (user) {
 		if (fair_policy(policy)) {
 			if (attr->sched_nice < task_nice(p) &&
-			    !can_nice(p, attr->sched_nice))
-				return -EPERM;
+			    !can_nice(p, attr->sched_nice)) {
+				if (capable(CAP_SYS_NICE))
+					goto sys_nice_capable;
+				else
+					return -EPERM;
+			}
 		}
 
 		if (rt_policy(policy)) {
@@ -5263,13 +5269,21 @@  static int __sched_setscheduler(struct task_struct *p,
 					task_rlimit(p, RLIMIT_RTPRIO);
 
 			/* Can't set/change the rt policy: */
-			if (policy != p->policy && !rlim_rtprio)
-				return -EPERM;
+			if (policy != p->policy && !rlim_rtprio) {
+				if (capable(CAP_SYS_NICE))
+					goto sys_nice_capable;
+				else
+					return -EPERM;
+			}
 
 			/* Can't increase priority: */
 			if (attr->sched_priority > p->rt_priority &&
-			    attr->sched_priority > rlim_rtprio)
-				return -EPERM;
+			    attr->sched_priority > rlim_rtprio) {
+				if (capable(CAP_SYS_NICE))
+					goto sys_nice_capable;
+				else
+					return -EPERM;
+			}
 		}
 
 		 /*
@@ -5278,28 +5292,43 @@  static int __sched_setscheduler(struct task_struct *p,
 		  * unprivileged DL tasks to increase their relative deadline
 		  * or reduce their runtime (both ways reducing utilization)
 		  */
-		if (dl_policy(policy))
-			return -EPERM;
+		if (dl_policy(policy)) {
+			if (capable(CAP_SYS_NICE))
+				goto sys_nice_capable;
+			else
+				return -EPERM;
+		}
 
 		/*
 		 * Treat SCHED_IDLE as nice 20. Only allow a switch to
 		 * SCHED_NORMAL if the RLIMIT_NICE would normally permit it.
 		 */
 		if (task_has_idle_policy(p) && !idle_policy(policy)) {
-			if (!can_nice(p, task_nice(p)))
-				return -EPERM;
+			if (!can_nice(p, task_nice(p))) {
+				if (capable(CAP_SYS_NICE))
+					goto sys_nice_capable;
+				else
+					return -EPERM;
+			}
 		}
 
 		/* Can't change other user's priorities: */
-		if (!check_same_owner(p))
-			return -EPERM;
+		if (!check_same_owner(p)) {
+			if (capable(CAP_SYS_NICE))
+				goto sys_nice_capable;
+			else
+				return -EPERM;
+		}
 
 		/* Normal users shall not reset the sched_reset_on_fork flag: */
-		if (p->sched_reset_on_fork && !reset_on_fork)
-			return -EPERM;
-	}
+		if (p->sched_reset_on_fork && !reset_on_fork) {
+			if (capable(CAP_SYS_NICE))
+				goto sys_nice_capable;
+			else
+				return -EPERM;
+		}
 
-	if (user) {
+sys_nice_capable:
 		if (attr->sched_flags & SCHED_FLAG_SUGOV)
 			return -EINVAL;