[PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[Jiri Slaby]

I posted this one earlier, but for the sake of completeness and
applicability, reposting.
--

Don't pass rlim_cur member of RLIM_NLIMITS-1=RLIMIT_RTTIME limit
to update_rlimit_cpu() in selinux_bprm_committing_creds.

Use proper rlim[RLIMIT_CPU].rlim_cur instead.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: James Morris <jmorris@namei.org>
Cc: Eric Paris <eparis@parisplace.org>
Cc: David Howells <dhowells@redhat.com>
---
 security/selinux/hooks.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index cf41988..496e626 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2360,7 +2360,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
 			initrlim = init_task.signal->rlim + i;
 			rlim->rlim_cur = min(rlim->rlim_max, initrlim->rlim_cur);
 		}
-		update_rlimit_cpu(rlim->rlim_cur);
+		update_rlimit_cpu(current->signal->rlim[RLIMIT_CPU].rlim_cur);
 	}
 }
 
-- 
1.6.3.3

[PATCH 2/6] SECURITY: add task_struct to setrlimit

[Jiri Slaby]

Add task_struct to task_setrlimit of security_operations to be able to set
rlimit of different task than current.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: James Morris <jmorris@namei.org>
Cc: Eric Paris <eparis@parisplace.org>
---
 include/linux/security.h |    9 ++++++---
 kernel/sys.c             |    2 +-
 security/capability.c    |    3 ++-
 security/security.c      |    5 +++--
 security/selinux/hooks.c |    7 ++++---
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 0f81525..880a21c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1520,7 +1520,8 @@ struct security_operations {
 	int (*task_setnice) (struct task_struct *p, int nice);
 	int (*task_setioprio) (struct task_struct *p, int ioprio);
 	int (*task_getioprio) (struct task_struct *p);
-	int (*task_setrlimit) (unsigned int resource, struct rlimit *new_rlim);
+	int (*task_setrlimit) (struct task_struct *p, unsigned int resource,
+			struct rlimit *new_rlim);
 	int (*task_setscheduler) (struct task_struct *p, int policy,
 				  struct sched_param *lp);
 	int (*task_getscheduler) (struct task_struct *p);
@@ -1775,7 +1776,8 @@ int security_task_setgroups(struct group_info *group_info);
 int security_task_setnice(struct task_struct *p, int nice);
 int security_task_setioprio(struct task_struct *p, int ioprio);
 int security_task_getioprio(struct task_struct *p);
-int security_task_setrlimit(unsigned int resource, struct rlimit *new_rlim);
+int security_task_setrlimit(struct task_struct *p, unsigned int resource,
+		struct rlimit *new_rlim);
 int security_task_setscheduler(struct task_struct *p,
 				int policy, struct sched_param *lp);
 int security_task_getscheduler(struct task_struct *p);
@@ -2385,7 +2387,8 @@ static inline int security_task_getioprio(struct task_struct *p)
 	return 0;
 }
 
-static inline int security_task_setrlimit(unsigned int resource,
+static inline int security_task_setrlimit(struct task_struct *p,
+					  unsigned int resource,
 					  struct rlimit *new_rlim)
 {
 	return 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index a7e36b6..ae7be2e 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1254,7 +1254,7 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
 	if (resource == RLIMIT_NOFILE && new_rlim.rlim_max > sysctl_nr_open)
 		return -EPERM;
 
-	retval = security_task_setrlimit(resource, &new_rlim);
+	retval = security_task_setrlimit(current, resource, &new_rlim);
 	if (retval)
 		return retval;
 
diff --git a/security/capability.c b/security/capability.c
index 6c38f4d..90d6daf 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -447,7 +447,8 @@ static int cap_task_getioprio(struct task_struct *p)
 	return 0;
 }
 
-static int cap_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
+static int cap_task_setrlimit(struct task_struct *p, unsigned int resource,
+		struct rlimit *new_rlim)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 0b7f9eb..b2c21dc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -779,9 +779,10 @@ int security_task_getioprio(struct task_struct *p)
 	return security_ops->task_getioprio(p);
 }
 
-int security_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
+int security_task_setrlimit(struct task_struct *p, unsigned int resource,
+		struct rlimit *new_rlim)
 {
-	return security_ops->task_setrlimit(resource, new_rlim);
+	return security_ops->task_setrlimit(p, resource, new_rlim);
 }
 
 int security_task_setscheduler(struct task_struct *p,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 496e626..839622a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3355,16 +3355,17 @@ static int selinux_task_getioprio(struct task_struct *p)
 	return current_has_perm(p, PROCESS__GETSCHED);
 }
 
-static int selinux_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
+static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
+		struct rlimit *new_rlim)
 {
-	struct rlimit *old_rlim = current->signal->rlim + resource;
+	struct rlimit *old_rlim = p->signal->rlim + resource;
 
 	/* Control the ability to change the hard limit (whether
 	   lowering or raising it), so that the hard limit can
 	   later be used as a safe reset point for the soft limit
 	   upon context transitions.  See selinux_bprm_committing_creds. */
 	if (old_rlim->rlim_max != new_rlim->rlim_max)
-		return current_has_perm(current, PROCESS__SETRLIMIT);
+		return current_has_perm(p, PROCESS__SETRLIMIT);
 
 	return 0;
 }
-- 
1.6.3.3

Re: [PATCH 2/6] SECURITY: add task_struct to setrlimit

[Eric Paris]

On Mon, 2009-08-31 at 20:56 +0200, Jiri Slaby wrote:
> Add task_struct to task_setrlimit of security_operations to be able to set
> rlimit of different task than current.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Paris <eparis@parisplace.org>

It looks ok to me.  We still seem to have complete SELinux control and I
don't see that you missed anything elsewhere in the security subsystem.

Acked-by: Eric Paris <eparis@redhat.com>


> ---
>  include/linux/security.h |    9 ++++++---
>  kernel/sys.c             |    2 +-
>  security/capability.c    |    3 ++-
>  security/security.c      |    5 +++--
>  security/selinux/hooks.c |    7 ++++---
>  5 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0f81525..880a21c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1520,7 +1520,8 @@ struct security_operations {
>  	int (*task_setnice) (struct task_struct *p, int nice);
>  	int (*task_setioprio) (struct task_struct *p, int ioprio);
>  	int (*task_getioprio) (struct task_struct *p);
> -	int (*task_setrlimit) (unsigned int resource, struct rlimit *new_rlim);
> +	int (*task_setrlimit) (struct task_struct *p, unsigned int resource,
> +			struct rlimit *new_rlim);
>  	int (*task_setscheduler) (struct task_struct *p, int policy,
>  				  struct sched_param *lp);
>  	int (*task_getscheduler) (struct task_struct *p);
> @@ -1775,7 +1776,8 @@ int security_task_setgroups(struct group_info *group_info);
>  int security_task_setnice(struct task_struct *p, int nice);
>  int security_task_setioprio(struct task_struct *p, int ioprio);
>  int security_task_getioprio(struct task_struct *p);
> -int security_task_setrlimit(unsigned int resource, struct rlimit *new_rlim);
> +int security_task_setrlimit(struct task_struct *p, unsigned int resource,
> +		struct rlimit *new_rlim);
>  int security_task_setscheduler(struct task_struct *p,
>  				int policy, struct sched_param *lp);
>  int security_task_getscheduler(struct task_struct *p);
> @@ -2385,7 +2387,8 @@ static inline int security_task_getioprio(struct task_struct *p)
>  	return 0;
>  }
>  
> -static inline int security_task_setrlimit(unsigned int resource,
> +static inline int security_task_setrlimit(struct task_struct *p,
> +					  unsigned int resource,
>  					  struct rlimit *new_rlim)
>  {
>  	return 0;
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a7e36b6..ae7be2e 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1254,7 +1254,7 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
>  	if (resource == RLIMIT_NOFILE && new_rlim.rlim_max > sysctl_nr_open)
>  		return -EPERM;
>  
> -	retval = security_task_setrlimit(resource, &new_rlim);
> +	retval = security_task_setrlimit(current, resource, &new_rlim);
>  	if (retval)
>  		return retval;
>  
> diff --git a/security/capability.c b/security/capability.c
> index 6c38f4d..90d6daf 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -447,7 +447,8 @@ static int cap_task_getioprio(struct task_struct *p)
>  	return 0;
>  }
>  
> -static int cap_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
> +static int cap_task_setrlimit(struct task_struct *p, unsigned int resource,
> +		struct rlimit *new_rlim)
>  {
>  	return 0;
>  }
> diff --git a/security/security.c b/security/security.c
> index 0b7f9eb..b2c21dc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -779,9 +779,10 @@ int security_task_getioprio(struct task_struct *p)
>  	return security_ops->task_getioprio(p);
>  }
>  
> -int security_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
> +int security_task_setrlimit(struct task_struct *p, unsigned int resource,
> +		struct rlimit *new_rlim)
>  {
> -	return security_ops->task_setrlimit(resource, new_rlim);
> +	return security_ops->task_setrlimit(p, resource, new_rlim);
>  }
>  
>  int security_task_setscheduler(struct task_struct *p,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 496e626..839622a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3355,16 +3355,17 @@ static int selinux_task_getioprio(struct task_struct *p)
>  	return current_has_perm(p, PROCESS__GETSCHED);
>  }
>  
> -static int selinux_task_setrlimit(unsigned int resource, struct rlimit *new_rlim)
> +static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
> +		struct rlimit *new_rlim)
>  {
> -	struct rlimit *old_rlim = current->signal->rlim + resource;
> +	struct rlimit *old_rlim = p->signal->rlim + resource;
>  
>  	/* Control the ability to change the hard limit (whether
>  	   lowering or raising it), so that the hard limit can
>  	   later be used as a safe reset point for the soft limit
>  	   upon context transitions.  See selinux_bprm_committing_creds. */
>  	if (old_rlim->rlim_max != new_rlim->rlim_max)
> -		return current_has_perm(current, PROCESS__SETRLIMIT);
> +		return current_has_perm(p, PROCESS__SETRLIMIT);
>  
>  	return 0;
>  }

Re: [PATCH 2/6] SECURITY: add task_struct to setrlimit

[James Morris]

On Mon, 31 Aug 2009, Jiri Slaby wrote:

> Add task_struct to task_setrlimit of security_operations to be able to set
> rlimit of different task than current.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Paris <eparis@parisplace.org>

Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

[PATCH 3/6] core: add task_struct to update_rlimit_cpu

[Jiri Slaby]

Add task_struct as a parameter to update_rlimit_cpu to be able to set
rlimit_cpu of different task than current.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 include/linux/posix-timers.h |    2 +-
 kernel/posix-cpu-timers.c    |   10 +++++-----
 kernel/sys.c                 |    2 +-
 security/selinux/hooks.c     |    3 ++-
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 4f71bf4..3e23844 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -117,6 +117,6 @@ void set_process_cpu_timer(struct task_struct *task, unsigned int clock_idx,
 
 long clock_nanosleep_restart(struct restart_block *restart_block);
 
-void update_rlimit_cpu(unsigned long rlim_new);
+void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
 #endif
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 12161f7..a61c625 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -12,16 +12,16 @@
 /*
  * Called after updating RLIMIT_CPU to set timer expiration if necessary.
  */
-void update_rlimit_cpu(unsigned long rlim_new)
+void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new)
 {
 	cputime_t cputime = secs_to_cputime(rlim_new);
-	struct signal_struct *const sig = current->signal;
+	struct signal_struct *const sig = task->signal;
 
 	if (cputime_eq(sig->it[CPUCLOCK_PROF].expires, cputime_zero) ||
 	    cputime_gt(sig->it[CPUCLOCK_PROF].expires, cputime)) {
-		spin_lock_irq(&current->sighand->siglock);
-		set_process_cpu_timer(current, CPUCLOCK_PROF, &cputime, NULL);
-		spin_unlock_irq(&current->sighand->siglock);
+		spin_lock_irq(&task->sighand->siglock);
+		set_process_cpu_timer(task, CPUCLOCK_PROF, &cputime, NULL);
+		spin_unlock_irq(&task->sighand->siglock);
 	}
 }
 
diff --git a/kernel/sys.c b/kernel/sys.c
index ae7be2e..d501f60 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1284,7 +1284,7 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
 	if (new_rlim.rlim_cur == RLIM_INFINITY)
 		goto out;
 
-	update_rlimit_cpu(new_rlim.rlim_cur);
+	update_rlimit_cpu(current, new_rlim.rlim_cur);
 out:
 	return 0;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 839622a..6903784 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2360,7 +2360,8 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
 			initrlim = init_task.signal->rlim + i;
 			rlim->rlim_cur = min(rlim->rlim_max, initrlim->rlim_cur);
 		}
-		update_rlimit_cpu(current->signal->rlim[RLIMIT_CPU].rlim_cur);
+		update_rlimit_cpu(current,
+				current->signal->rlim[RLIMIT_CPU].rlim_cur);
 	}
 }
 
-- 
1.6.3.3

Re: [PATCH 3/6] core: add task_struct to update_rlimit_cpu

[James Morris]

On Mon, 31 Aug 2009, Jiri Slaby wrote:

> Add task_struct as a parameter to update_rlimit_cpu to be able to set
> rlimit_cpu of different task than current.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

Acked-by: James Morris <jmorris@namei.org>

-- 
James Morris
<jmorris@namei.org>

[PATCH 4/6] core: split sys_setrlimit

[Jiri Slaby]

Create setrlimit from sys_setrlimit and declare setrlimit in
the resource header. This is to allow rlimits to be changed not
only by syscall, but later from proc code too.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 include/linux/resource.h |    2 ++
 kernel/sys.c             |   44 ++++++++++++++++++++++++++------------------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/include/linux/resource.h b/include/linux/resource.h
index 40fc7e6..4301d67 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -71,5 +71,7 @@ struct rlimit {
 #include <asm/resource.h>
 
 int getrusage(struct task_struct *p, int who, struct rusage __user *ru);
+int setrlimit(struct task_struct *tsk, unsigned int resource,
+		struct rlimit *new_rlim);
 
 #endif
diff --git a/kernel/sys.c b/kernel/sys.c
index d501f60..919cebe 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1236,41 +1236,38 @@ SYSCALL_DEFINE2(old_getrlimit, unsigned int, resource,
 
 #endif
 
-SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
+int setrlimit(struct task_struct *tsk, unsigned int resource,
+		struct rlimit *new_rlim)
 {
-	struct rlimit new_rlim, *old_rlim;
+	struct rlimit *old_rlim;
 	int retval;
 
-	if (resource >= RLIM_NLIMITS)
-		return -EINVAL;
-	if (copy_from_user(&new_rlim, rlim, sizeof(*rlim)))
-		return -EFAULT;
-	if (new_rlim.rlim_cur > new_rlim.rlim_max)
+	if (new_rlim->rlim_cur > new_rlim->rlim_max)
 		return -EINVAL;
-	old_rlim = current->signal->rlim + resource;
-	if ((new_rlim.rlim_max > old_rlim->rlim_max) &&
+	old_rlim = tsk->signal->rlim + resource;
+	if ((new_rlim->rlim_max > old_rlim->rlim_max) &&
 	    !capable(CAP_SYS_RESOURCE))
 		return -EPERM;
-	if (resource == RLIMIT_NOFILE && new_rlim.rlim_max > sysctl_nr_open)
+	if (resource == RLIMIT_NOFILE && new_rlim->rlim_max > sysctl_nr_open)
 		return -EPERM;
 
-	retval = security_task_setrlimit(current, resource, &new_rlim);
+	retval = security_task_setrlimit(tsk, resource, new_rlim);
 	if (retval)
 		return retval;
 
-	if (resource == RLIMIT_CPU && new_rlim.rlim_cur == 0) {
+	if (resource == RLIMIT_CPU && new_rlim->rlim_cur == 0) {
 		/*
 		 * The caller is asking for an immediate RLIMIT_CPU
 		 * expiry.  But we use the zero value to mean "it was
 		 * never set".  So let's cheat and make it one second
 		 * instead
 		 */
-		new_rlim.rlim_cur = 1;
+		new_rlim->rlim_cur = 1;
 	}
 
-	task_lock(current->group_leader);
-	*old_rlim = new_rlim;
-	task_unlock(current->group_leader);
+	task_lock(tsk->group_leader);
+	*old_rlim = *new_rlim;
+	task_unlock(tsk->group_leader);
 
 	if (resource != RLIMIT_CPU)
 		goto out;
@@ -1281,14 +1278,25 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
 	 * very long-standing error, and fixing it now risks breakage of
 	 * applications, so we live with it
 	 */
-	if (new_rlim.rlim_cur == RLIM_INFINITY)
+	if (new_rlim->rlim_cur == RLIM_INFINITY)
 		goto out;
 
-	update_rlimit_cpu(current, new_rlim.rlim_cur);
+	update_rlimit_cpu(tsk, new_rlim->rlim_cur);
 out:
 	return 0;
 }
 
+SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
+{
+	struct rlimit new_rlim;
+
+	if (resource >= RLIM_NLIMITS)
+		return -EINVAL;
+	if (copy_from_user(&new_rlim, rlim, sizeof(*rlim)))
+		return -EFAULT;
+	return setrlimit(current, resource, &new_rlim);
+}
+
 /*
  * It would make sense to put struct rusage in the task_struct,
  * except that would make the task_struct be *really big*.  After
-- 
1.6.3.3

[PATCH 5/6] core: allow setrlimit to non-current tasks

[Jiri Slaby]

Add locking to allow setrlimit accept task parameter other than
current.

Namely, lock tasklist_lock for read and check whether the task
structure has signal and sighand non-null. Do all the signal
processing under that lock still held.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 kernel/sys.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 919cebe..777a152 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1244,16 +1244,27 @@ int setrlimit(struct task_struct *tsk, unsigned int resource,
 
 	if (new_rlim->rlim_cur > new_rlim->rlim_max)
 		return -EINVAL;
+
+	/* protect tsk->signal and tsk->sighand from disappearing */
+	read_lock_irq(&tasklist_lock);
+	if (!tsk->signal || !tsk->sighand) {
+		retval = -ESRCH;
+		goto unlock;
+	}
 	old_rlim = tsk->signal->rlim + resource;
 	if ((new_rlim->rlim_max > old_rlim->rlim_max) &&
-	    !capable(CAP_SYS_RESOURCE))
-		return -EPERM;
-	if (resource == RLIMIT_NOFILE && new_rlim->rlim_max > sysctl_nr_open)
-		return -EPERM;
+	    !capable(CAP_SYS_RESOURCE)) {
+		retval = -EPERM;
+		goto unlock;
+	}
+	if (resource == RLIMIT_NOFILE && new_rlim->rlim_max > sysctl_nr_open) {
+		retval = -EPERM;
+		goto unlock;
+	}
 
 	retval = security_task_setrlimit(tsk, resource, new_rlim);
 	if (retval)
-		return retval;
+		goto unlock;
 
 	if (resource == RLIMIT_CPU && new_rlim->rlim_cur == 0) {
 		/*
@@ -1283,7 +1294,10 @@ int setrlimit(struct task_struct *tsk, unsigned int resource,
 
 	update_rlimit_cpu(tsk, new_rlim->rlim_cur);
 out:
-	return 0;
+	retval = 0;
+unlock:
+	read_unlock_irq(&tasklist_lock);
+	return retval;
 }
 
 SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
-- 
1.6.3.3

Re: [PATCH 5/6] core: allow setrlimit to non-current tasks

[Oleg Nesterov]

Sorry, can't read these series today. Will try tomorrow.

But at first glance some parts looks suspicious to me,

On 08/31, Jiri Slaby wrote:
>
> @@ -1244,16 +1244,27 @@ int setrlimit(struct task_struct *tsk, unsigned int resource,
>
>  	if (new_rlim->rlim_cur > new_rlim->rlim_max)
>  		return -EINVAL;
> +
> +	/* protect tsk->signal and tsk->sighand from disappearing */
> +	read_lock_irq(&tasklist_lock);

Why _irq? We can take tasklist_lock for reading without disabling irqs.

And. Unless I misread the patch, update_rlimit_cpu() is called before
read_unlock_irq(&tasklist_lock), but update_rlimit_cpu() does
spin_unlock_irq(->siglock) and restores interrupts.

> +	if (!tsk->signal || !tsk->sighand) {

Please don't check !tsk->signal, !tsk->sighand is enough. If
we have ->sighand != NULL (under lock) ->signal must be valid.

But I dislike the fact the patch uses tasklist_lock. Can't
lock_task_sighand() work for you? (of course, in this case
update_rlimit_cpu() should be updated too).

Once again, I didn't actually read this series yet, sorry.

Oleg.

[PATCH 6/6] FS: proc, make limits writable

[Jiri Slaby]

Allow writing strings such as
Max core file size=0:unlimited
to /proc/<pid>/limits to change limits.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 fs/proc/base.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index ea71cae..0d9e3e9 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -129,6 +129,8 @@ struct pid_entry {
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
 
+static ssize_t proc_info_read(struct file * file, char __user * buf,
+			  size_t count, loff_t *ppos);
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
  * and .. links.
@@ -522,6 +524,74 @@ static int proc_pid_limits(struct task_struct *task, char *buffer)
 	return count;
 }
 
+static ssize_t limits_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *ppos)
+{
+	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
+	char str[32 + 1 + 16 + 1 + 16 + 1], *delim, *next;
+	struct rlimit new_rlimit;
+	unsigned int i;
+	int ret;
+
+	if (!task) {
+		count = -ESRCH;
+		goto out;
+	}
+	if (copy_from_user(str, buf, min(count, sizeof(str) - 1))) {
+		count = -EFAULT;
+		goto put_task;
+	}
+
+	str[min(count, sizeof(str) - 1)] = 0;
+
+	delim = strchr(str, '=');
+	if (!delim) {
+		count = -EINVAL;
+		goto put_task;
+	}
+	*delim++ = 0; /* for easy 'str' usage */
+	new_rlimit.rlim_cur = simple_strtoul(delim, &next, 0);
+	if (*next != ':') {
+		if (strncmp(delim, "unlimited:", 10)) {
+			count = -EINVAL;
+			goto put_task;
+		}
+		new_rlimit.rlim_cur = RLIM_INFINITY;
+		next = delim + 9; /* move to ':' */
+	}
+	delim = next + 1;
+	new_rlimit.rlim_max = simple_strtoul(delim, &next, 0);
+	if (*next != 0) {
+		if (strcmp(delim, "unlimited")) {
+			count = -EINVAL;
+			goto put_task;
+		}
+		new_rlimit.rlim_max = RLIM_INFINITY;
+	}
+
+	for (i = 0; i < RLIM_NLIMITS; i++)
+		if (!strcmp(str, lnames[i].name))
+			break;
+	if (i >= RLIM_NLIMITS) {
+		count = -EINVAL;
+		goto put_task;
+	}
+
+	ret = setrlimit(task, i, &new_rlimit);
+	if (ret)
+		count = ret;
+
+put_task:
+	put_task_struct(task);
+out:
+	return count;
+}
+
+static const struct file_operations proc_pid_limits_operations = {
+	.read		= proc_info_read,
+	.write		= limits_write,
+};
+
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 static int proc_pid_syscall(struct task_struct *task, char *buffer)
 {
@@ -2501,7 +2571,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 	INF("auxv",       S_IRUSR, proc_pid_auxv),
 	ONE("status",     S_IRUGO, proc_pid_status),
 	ONE("personality", S_IRUSR, proc_pid_personality),
-	INF("limits",	  S_IRUSR, proc_pid_limits),
+	NOD("limits",	  S_IFREG|S_IRUSR|S_IWUSR, NULL,
+			&proc_pid_limits_operations,
+			{ .proc_read = proc_pid_limits }),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",      S_IRUGO|S_IWUSR, proc_pid_sched_operations),
 #endif
@@ -2836,7 +2908,9 @@ static const struct pid_entry tid_base_stuff[] = {
 	INF("auxv",      S_IRUSR, proc_pid_auxv),
 	ONE("status",    S_IRUGO, proc_pid_status),
 	ONE("personality", S_IRUSR, proc_pid_personality),
-	INF("limits",	 S_IRUSR, proc_pid_limits),
+	NOD("limits",	  S_IFREG|S_IRUSR|S_IWUSR, NULL,
+			&proc_pid_limits_operations,
+			{ .proc_read = proc_pid_limits }),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",     S_IRUGO|S_IWUSR, proc_pid_sched_operations),
 #endif
-- 
1.6.3.3

Re: [PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[James Morris]

On Mon, 31 Aug 2009, Jiri Slaby wrote:

[added lsm list]

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index cf41988..496e626 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2360,7 +2360,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
>  			initrlim = init_task.signal->rlim + i;
>  			rlim->rlim_cur = min(rlim->rlim_max, initrlim->rlim_cur);
>  		}
> -		update_rlimit_cpu(rlim->rlim_cur);
> +		update_rlimit_cpu(current->signal->rlim[RLIMIT_CPU].rlim_cur);

This doesn't look correct to me: the original code determines 
rlim->rlim_cur and then updates current to that.  With your change, this 
value is not used.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[Jiri Slaby]

On 09/01/2009 12:22 AM, James Morris wrote:
> On Mon, 31 Aug 2009, Jiri Slaby wrote:
> 
> [added lsm list]
> 
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index cf41988..496e626 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2360,7 +2360,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
>>  			initrlim = init_task.signal->rlim + i;
>>  			rlim->rlim_cur = min(rlim->rlim_max, initrlim->rlim_cur);
>>  		}
>> -		update_rlimit_cpu(rlim->rlim_cur);
>> +		update_rlimit_cpu(current->signal->rlim[RLIMIT_CPU].rlim_cur);
> 
> This doesn't look correct to me: the original code determines 
> rlim->rlim_cur and then updates current to that.  With your change, this 
> value is not used.

No, the for loop is to alter all limits according to the init limits.

update_rlimit_cpu is called for RLIMIT_CPU to annotate scheduler about
CPU time changes (if any).

BTW this was introduced by f06febc96ba8e0af80bcc3eaec0a109e88275fac
(timers: fix itimer/many thread hang).

Re: [PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[James Morris]

On Tue, 1 Sep 2009, Jiri Slaby wrote:

> > This doesn't look correct to me: the original code determines 
> > rlim->rlim_cur and then updates current to that.  With your change, this 
> > value is not used.
> 
> No, the for loop is to alter all limits according to the init limits.

Ahh, ok, I misread the code.

> 
> update_rlimit_cpu is called for RLIMIT_CPU to annotate scheduler about
> CPU time changes (if any).
> 
> BTW this was introduced by f06febc96ba8e0af80bcc3eaec0a109e88275fac
> (timers: fix itimer/many thread hang).
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/6] SECURITY: selinux, fix update_rlimit_cpu parameter

[James Morris]

On Mon, 31 Aug 2009, Jiri Slaby wrote:

> I posted this one earlier, but for the sake of completeness and
> applicability, reposting.
> --
> 
> Don't pass rlim_cur member of RLIM_NLIMITS-1=RLIMIT_RTTIME limit
> to update_rlimit_cpu() in selinux_bprm_committing_creds.
> 
> Use proper rlim[RLIMIT_CPU].rlim_cur instead.

Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>