diff mbox series

[RFC] sched: do not issue an audit on unprivileged operation

Message ID 20200902111456.20610-1-cgzones@googlemail.com (mailing list archive)
State Superseded
Headers show
Series [RFC] sched: do not issue an audit on unprivileged operation | expand

Commit Message

Christian Göttsche Sept. 2, 2020, 11:14 a.m. UTC 
sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
issue a CAP_SYS_NICE audit unconditionally, even when the requested
operation does not require that capability.

Use an unaudited check first and perform an additional audited check
only on an actual permission denial.
---
 kernel/sched/core.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Stephen Smalley Sept. 2, 2020, 3:05 p.m. UTC | #1 
On Wed, Sep 2, 2020 at 7:18 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
> issue a CAP_SYS_NICE audit unconditionally, even when the requested
> operation does not require that capability.
>
> Use an unaudited check first and perform an additional audited check
> only on an actual permission denial.

Could we just delay calling capable() until we know we need it?  Also,
this patch will need to go to the scheduler maintainers not just
selinux list.  Might want to also copy linux-security-module list
since it is relevant to all security modules that implement the
capable hook.
Ondrej Mosnacek Sept. 3, 2020, 7:15 a.m. UTC | #2 
On Wed, Sep 2, 2020 at 5:17 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Sep 2, 2020 at 7:18 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
> > issue a CAP_SYS_NICE audit unconditionally, even when the requested
> > operation does not require that capability.
> >
> > Use an unaudited check first and perform an additional audited check
> > only on an actual permission denial.
>
> Could we just delay calling capable() until we know we need it?

Yes, please - because with this patch it could happen that an LSM
policy changes between the ns_capable_noaudit() call and capable()
call, such that the first one is denied and the second one allowed, in
which case the operation would fail without being audited.
diff mbox series

Patch

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8471a0f7eb32..b567697a67ea 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5249,13 +5249,15 @@  static int __sched_setscheduler(struct task_struct *p,
 		return -EINVAL;
 
 	/*
-	 * Allow unprivileged RT tasks to decrease priority:
+	 * Allow unprivileged RT tasks to decrease priority.
+	 * Do not issue an audit event yet, only later on an actual
+	 * permission denial.
 	 */
-	if (user && !capable(CAP_SYS_NICE)) {
+	if (user && !ns_capable_noaudit(&init_user_ns, CAP_SYS_NICE)) {
 		if (fair_policy(policy)) {
 			if (attr->sched_nice < task_nice(p) &&
 			    !can_nice(p, attr->sched_nice))
-				return -EPERM;
+				goto incapable;
 		}
 
 		if (rt_policy(policy)) {
@@ -5264,12 +5266,12 @@  static int __sched_setscheduler(struct task_struct *p,
 
 			/* Can't set/change the rt policy: */
 			if (policy != p->policy && !rlim_rtprio)
-				return -EPERM;
+				goto incapable;
 
 			/* Can't increase priority: */
 			if (attr->sched_priority > p->rt_priority &&
 			    attr->sched_priority > rlim_rtprio)
-				return -EPERM;
+				goto incapable;
 		}
 
 		 /*
@@ -5279,7 +5281,7 @@  static int __sched_setscheduler(struct task_struct *p,
 		  * or reduce their runtime (both ways reducing utilization)
 		  */
 		if (dl_policy(policy))
-			return -EPERM;
+			goto incapable;
 
 		/*
 		 * Treat SCHED_IDLE as nice 20. Only allow a switch to
@@ -5287,16 +5289,16 @@  static int __sched_setscheduler(struct task_struct *p,
 		 */
 		if (task_has_idle_policy(p) && !idle_policy(policy)) {
 			if (!can_nice(p, task_nice(p)))
-				return -EPERM;
+				goto incapable;
 		}
 
 		/* Can't change other user's priorities: */
 		if (!check_same_owner(p))
-			return -EPERM;
+			goto incapable;
 
 		/* Normal users shall not reset the sched_reset_on_fork flag: */
 		if (p->sched_reset_on_fork && !reset_on_fork)
-			return -EPERM;
+			goto incapable;
 	}
 
 	if (user) {
@@ -5470,6 +5472,11 @@  static int __sched_setscheduler(struct task_struct *p,
 	if (pi)
 		cpuset_read_unlock();
 	return retval;
+
+incapable:
+	/* Generate an audit event */
+	(void) capable(CAP_SYS_NICE);
+	return -EPERM;
 }
 
 static int _sched_setscheduler(struct task_struct *p, int policy,