linux-kernel - Re: [PATCH v3 06/11] rlimits: do security check under task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun,  6 Jun 2010 22:23:03 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	akpm@...ux-foundation.org
Cc:	adobriyan@...il.com, nhorman@...driver.com, oleg@...hat.com,
	linux-kernel@...r.kernel.org, jirislaby@...il.com
Subject: Re: [PATCH v3 06/11] rlimits: do security check under task_lock

Andrew Morton wrote:
> On Mon, 10 May 2010 20:00:46 +0200
> Jiri Slaby <jslaby@...e.cz> wrote:
> > @@ -1309,11 +1305,13 @@ int do_setrlimit(struct task_struct *tsk, unsigned int resource,
> >  
> >  	old_rlim = tsk->signal->rlim + resource;
> >  	task_lock(tsk->group_leader);
> > -	if ((new_rlim->rlim_max <= old_rlim->rlim_max) ||
> > -				capable(CAP_SYS_RESOURCE))
> > -		*old_rlim = *new_rlim;
> > -	else
> > +	if ((new_rlim->rlim_max > old_rlim->rlim_max) &&
> > +				!capable(CAP_SYS_RESOURCE))
> >  		retval = -EPERM;
> > +	if (!retval)
> > +		retval = security_task_setrlimit(tsk, resource, new_rlim);
> 
> I looked at the amount of code which exists under
> security_task_setrlimit() and nearly died.  Please convince me that it
> is correct to do all that work under tasklist_lock, and that it is also
> maintainable.

When I wrote this code, I looked how it is done in the current code
and redid what others, including getioprio, setnice and task_kill
(this one even with task_lock write-locked) do. I need the lock
to protect task->signal (which is checked inside the security
function) from disappearing.

> > +	if (!retval)
> > +		*old_rlim = *new_rlim;
> >  	task_unlock(tsk->group_leader);
> >  
> >  	if (retval || resource != RLIMIT_CPU)
> 
> Yikes, so the locking around all that selinux code becomes even more
> brutal.  How much rope are you tying around the selinux developers'
> hands here?

I agree with that and didn't find a better solution than is attached
here.

--
The code performed in security_task_setrlimit is very long when
CONFIG_SECURITY is enabled. Don't execute the function under
alloc_lock, unlock it temporarily and check whether the limits changed
in the meantime while the lock was not held. If yes, redo do check.

Signed-off-by: Jiri Slaby <jslaby@...e.cz>
---
 kernel/sys.c |   39 +++++++++++++++++++++++++++++++++++----
 1 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index d7fcd4a..48cb21d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1304,12 +1304,37 @@ static void rlim64_to_rlim(const struct rlimit64 *rlim64, struct rlimit *rlim)
 		rlim->rlim_max = (unsigned long)rlim64->rlim_max;
 }
 
+/**
+ * check_security_task_setrlimit_unlocked - calls security_task_setrlimit
+ *
+ * But security_task_setrlimit is complex with much of code, so that we don't
+ * want to call it while holding the task_lock. We temporarily unlock the lock
+ * and after the check we test whether the limits changed in the meantime.
+ */
+static int check_security_task_setrlimit_unlocked(struct task_struct *tsk,
+		unsigned int resource, struct rlimit *new_rlim,
+		struct rlimit *old_rlim)
+{
+	struct rlimit rlim;
+	int ret;
+
+	memcpy(&rlim, old_rlim, sizeof(rlim));
+
+	task_unlock(tsk->group_leader);
+	ret = security_task_setrlimit(tsk, resource, new_rlim);
+	task_lock(tsk->group_leader);
+
+	if (!ret && memcmp(&rlim, old_rlim, sizeof(rlim)))
+		return -EAGAIN;
+	return ret;
+}
+
 /* make sure you are allowed to change @tsk limits before calling this */
 int do_prlimit(struct task_struct *tsk, unsigned int resource,
 		struct rlimit *new_rlim, struct rlimit *old_rlim)
 {
 	struct rlimit *rlim;
-	int retval = 0;
+	int retval;
 
 	if (resource >= RLIM_NLIMITS)
 		return -EINVAL;
@@ -1339,13 +1364,19 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
 
 	rlim = tsk->signal->rlim + resource;
 	task_lock(tsk->group_leader);
+again:
+	retval = 0;
 	if (new_rlim) {
 		if ((new_rlim->rlim_max > rlim->rlim_max) &&
 					!capable(CAP_SYS_RESOURCE))
 			retval = -EPERM;
-		if (!retval)
-			retval = security_task_setrlimit(tsk, resource,
-					new_rlim);
+		if (!retval) {
+			retval = check_security_task_setrlimit_unlocked(tsk,
+					resource, new_rlim, rlim);
+			if (retval == -EAGAIN) {
+				goto again;
+			}
+		}
 	}
 	if (!retval) {
 		if (old_rlim)
-- 
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/