message queue limits

message queue limits

[Ulrich Drepper]

Something has to change in the way message queues are created.
Currently it is possible for an unprivileged user to exhaust all mq
slots so that only root can create a few more.  Any other unprivileged
user has no change to create anything.

I think it is necessary to create a per-user limit instead of a
system-wide limit.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: message queue limits

[Bill Davidsen]

Ulrich Drepper wrote:
> Something has to change in the way message queues are created.
> Currently it is possible for an unprivileged user to exhaust all mq
> slots so that only root can create a few more.  Any other unprivileged
> user has no change to create anything.
> 
> I think it is necessary to create a per-user limit instead of a
> system-wide limit.

I think you mean "in addition to" rather than "instead of" here, one 
limit would keep one user from hogging the resource, the other would 
keep the resource from exhausting {whatever runs out first}.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: message queue limits

[Marcelo Tosatti]

On Sun, Apr 11, 2004 at 10:48:28PM -0700, Ulrich Drepper wrote:
> Something has to change in the way message queues are created.
> Currently it is possible for an unprivileged user to exhaust all mq
> slots so that only root can create a few more.  Any other unprivileged
> user has no change to create anything.
> 
> I think it is necessary to create a per-user limit instead of a
> system-wide limit.

Actually, there is no infrastructure to account for per-UID limits right now AFAICS 
(please someone correct me) at ALL. We need to account and limit for per-user

- pending signals
- message queues

And all other current limits which are per "struct task".

There is CKRM available, but I suppose its not easily mergeable and not
finished yet.

There was an effort to create simple per-user limits infrastructure called
"userbeans" at some point in 2.3.x development. Maybe it needs to be
resurrected?

This is bad.

Re: message queue limits

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> On Sun, Apr 11, 2004 at 10:48:28PM -0700, Ulrich Drepper wrote:
>  > Something has to change in the way message queues are created.
>  > Currently it is possible for an unprivileged user to exhaust all mq
>  > slots so that only root can create a few more.  Any other unprivileged
>  > user has no change to create anything.
>  > 
>  > I think it is necessary to create a per-user limit instead of a
>  > system-wide limit.
> 
>  Actually, there is no infrastructure to account for per-UID limits right now AFAICS 
>  (please someone correct me) at ALL. We need to account and limit for per-user
> 
>  - pending signals
>  - message queues

The stuff in kernel/user.c may be sufficient for this.

Re: message queue limits

[Marcelo Tosatti]

On Thu, Apr 15, 2004 at 12:24:11PM -0700, Andrew Morton wrote:
> Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> >
> > On Sun, Apr 11, 2004 at 10:48:28PM -0700, Ulrich Drepper wrote:
> >  > Something has to change in the way message queues are created.
> >  > Currently it is possible for an unprivileged user to exhaust all mq
> >  > slots so that only root can create a few more.  Any other unprivileged
> >  > user has no change to create anything.
> >  > 
> >  > I think it is necessary to create a per-user limit instead of a
> >  > system-wide limit.
> > 
> >  Actually, there is no infrastructure to account for per-UID limits right now AFAICS 
> >  (please someone correct me) at ALL. We need to account and limit for per-user
> > 
> >  - pending signals
> >  - message queues
> 
> The stuff in kernel/user.c may be sufficient for this.

Oh, sweat! I'll try adding a "atomic_t signal_pending" to "user_struct" 
to be checked at send_signal(), and then go for message queue limiting.

Something which sucks is to add a atomic read/inc at each send signal operation.

Can we avoid the locking in some way?

Re: message queue limits

[Marcelo Tosatti]

On Thu, Apr 15, 2004 at 04:54:30PM -0300, Marcelo Tosatti wrote:
> On Thu, Apr 15, 2004 at 12:24:11PM -0700, Andrew Morton wrote:
> > Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> > >
> > > On Sun, Apr 11, 2004 at 10:48:28PM -0700, Ulrich Drepper wrote:
> > >  > Something has to change in the way message queues are created.
> > >  > Currently it is possible for an unprivileged user to exhaust all mq
> > >  > slots so that only root can create a few more.  Any other unprivileged
> > >  > user has no change to create anything.
> > >  > 
> > >  > I think it is necessary to create a per-user limit instead of a
> > >  > system-wide limit.
> > > 
> > >  Actually, there is no infrastructure to account for per-UID limits right now AFAICS 
> > >  (please someone correct me) at ALL. We need to account and limit for per-user
> > > 
> > >  - pending signals
> > >  - message queues
> > 
> > The stuff in kernel/user.c may be sufficient for this.
> 
> Oh, sweat! I'll try adding a "atomic_t signal_pending" to "user_struct" 
> to be checked at send_signal(), and then go for message queue limiting.

So here goes the signal pending limitation. The default value is "max_signals/2", 
which I think is good enough.

I'll go see the message queue limits tomorrow.

This adds a new "RLIMIT_SIGPENDING" limit, which is used to limit
per-uid pending signals. Currently an unpriviledged user can queue 
more than maximum of allowed signals and cause overall system 
malfunction.


diff -Nur linux-2.6.5.org/include/asm-alpha/resource.h linux-2.6.5/include/asm-alpha/resource.h
--- linux-2.6.5.org/include/asm-alpha/resource.h	2004-04-15 11:13:28.000000000 -0300
+++ linux-2.6.5/include/asm-alpha/resource.h	2004-04-15 18:19:34.543837360 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_NPROC	8		/* max number of processes */
 #define RLIMIT_MEMLOCK	9		/* max locked-in-memory address space */
 #define RLIMIT_LOCKS   10              /* maximum file locks held */
+#define RLIMIT_SIGPENDING	 11	/* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.  Fine, it's unsigned, but
diff -Nur linux-2.6.5.org/include/asm-arm/resource.h linux-2.6.5/include/asm-arm/resource.h
--- linux-2.6.5.org/include/asm-arm/resource.h	2004-04-15 11:13:27.000000000 -0300
+++ linux-2.6.5/include/asm-arm/resource.h	2004-04-15 18:03:27.731815128 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING        11     /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-arm26/resource.h linux-2.6.5/include/asm-arm26/resource.h
--- linux-2.6.5.org/include/asm-arm26/resource.h	2004-04-15 11:13:30.000000000 -0300
+++ linux-2.6.5/include/asm-arm26/resource.h	2004-04-15 18:00:33.386319672 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING	11	/* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-cris/resource.h linux-2.6.5/include/asm-cris/resource.h
--- linux-2.6.5.org/include/asm-cris/resource.h	2004-04-15 11:13:29.000000000 -0300
+++ linux-2.6.5/include/asm-cris/resource.h	2004-04-15 18:03:40.023946440 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS   10              /* maximum file locks held */
+#define RLIMIT_SIGPENDING        11     /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-h8300/resource.h linux-2.6.5/include/asm-h8300/resource.h
--- linux-2.6.5.org/include/asm-h8300/resource.h	2004-04-15 11:13:29.000000000 -0300
+++ linux-2.6.5/include/asm-h8300/resource.h	2004-04-15 18:03:46.904900376 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING        11     /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-i386/resource.h linux-2.6.5/include/asm-i386/resource.h
--- linux-2.6.5.org/include/asm-i386/resource.h	2004-04-15 11:13:28.000000000 -0300
+++ linux-2.6.5/include/asm-i386/resource.h	2004-04-15 18:04:01.836630408 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING 11		/* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-ia64/resource.h linux-2.6.5/include/asm-ia64/resource.h
--- linux-2.6.5.org/include/asm-ia64/resource.h	2004-04-15 11:13:23.000000000 -0300
+++ linux-2.6.5/include/asm-ia64/resource.h	2004-04-15 18:04:09.232506064 -0300
@@ -23,8 +23,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING        11     /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-m68k/resource.h linux-2.6.5/include/asm-m68k/resource.h
--- linux-2.6.5.org/include/asm-m68k/resource.h	2004-04-15 11:13:25.000000000 -0300
+++ linux-2.6.5/include/asm-m68k/resource.h	2004-04-15 18:04:17.941182144 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING        11     /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-mips/resource.h linux-2.6.5/include/asm-mips/resource.h
--- linux-2.6.5.org/include/asm-mips/resource.h	2004-04-15 11:13:21.000000000 -0300
+++ linux-2.6.5/include/asm-mips/resource.h	2004-04-15 18:05:04.764063984 -0300
@@ -23,8 +23,9 @@
 #define RLIMIT_NPROC 8			/* max number of processes */
 #define RLIMIT_MEMLOCK 9		/* max locked-in-memory address space */
 #define RLIMIT_LOCKS 10			/* maximum file locks held */
+#define RLIMIT_SIGPENDING	11 	/* max number of pending signals */
 
-#define RLIM_NLIMITS 11			/* Number of limit flavors.  */
+#define RLIM_NLIMITS 12			/* Number of limit flavors.  */
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-parisc/resource.h linux-2.6.5/include/asm-parisc/resource.h
--- linux-2.6.5.org/include/asm-parisc/resource.h	2004-04-15 11:13:19.000000000 -0300
+++ linux-2.6.5/include/asm-parisc/resource.h	2004-04-15 18:05:20.939604928 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-ppc/resource.h linux-2.6.5/include/asm-ppc/resource.h
--- linux-2.6.5.org/include/asm-ppc/resource.h	2004-04-15 11:13:25.000000000 -0300
+++ linux-2.6.5/include/asm-ppc/resource.h	2004-04-15 18:05:30.876094352 -0300
@@ -12,8 +12,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit(?) */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-ppc64/resource.h linux-2.6.5/include/asm-ppc64/resource.h
--- linux-2.6.5.org/include/asm-ppc64/resource.h	2004-04-15 11:13:20.000000000 -0300
+++ linux-2.6.5/include/asm-ppc64/resource.h	2004-04-15 18:05:39.288815424 -0300
@@ -21,8 +21,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit(?) */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-s390/resource.h linux-2.6.5/include/asm-s390/resource.h
--- linux-2.6.5.org/include/asm-s390/resource.h	2004-04-15 11:13:29.000000000 -0300
+++ linux-2.6.5/include/asm-s390/resource.h	2004-04-15 18:05:48.191462016 -0300
@@ -24,8 +24,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
-  
-#define RLIM_NLIMITS	11
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
+
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-sh/resource.h linux-2.6.5/include/asm-sh/resource.h
--- linux-2.6.5.org/include/asm-sh/resource.h	2004-04-15 11:13:30.000000000 -0300
+++ linux-2.6.5/include/asm-sh/resource.h	2004-04-15 18:05:56.156251184 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 #ifdef __KERNEL__
 
diff -Nur linux-2.6.5.org/include/asm-sparc/resource.h linux-2.6.5/include/asm-sparc/resource.h
--- linux-2.6.5.org/include/asm-sparc/resource.h	2004-04-15 11:13:27.000000000 -0300
+++ linux-2.6.5/include/asm-sparc/resource.h	2004-04-15 18:06:03.934068776 -0300
@@ -22,8 +22,9 @@
 #define RLIMIT_MEMLOCK  8               /* max locked-in-memory address space */
 #define RLIMIT_AS       9               /* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-sparc64/resource.h linux-2.6.5/include/asm-sparc64/resource.h
--- linux-2.6.5.org/include/asm-sparc64/resource.h	2004-04-15 11:13:29.000000000 -0300
+++ linux-2.6.5/include/asm-sparc64/resource.h	2004-04-15 18:06:12.013840464 -0300
@@ -22,8 +22,9 @@
 #define RLIMIT_MEMLOCK  8               /* max locked-in-memory address space */
 #define RLIMIT_AS       9               /* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-v850/resource.h linux-2.6.5/include/asm-v850/resource.h
--- linux-2.6.5.org/include/asm-v850/resource.h	2004-04-15 11:13:30.000000000 -0300
+++ linux-2.6.5/include/asm-v850/resource.h	2004-04-15 18:06:26.557629472 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/asm-x86_64/resource.h linux-2.6.5/include/asm-x86_64/resource.h
--- linux-2.6.5.org/include/asm-x86_64/resource.h	2004-04-15 11:13:23.000000000 -0300
+++ linux-2.6.5/include/asm-x86_64/resource.h	2004-04-15 18:06:38.944746344 -0300
@@ -16,8 +16,9 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING       11      /* max number of pending signals */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	12
 
 /*
  * SuS says limits have to be unsigned.
diff -Nur linux-2.6.5.org/include/linux/sched.h linux-2.6.5/include/linux/sched.h
--- linux-2.6.5.org/include/linux/sched.h	2004-04-15 11:13:23.000000000 -0300
+++ linux-2.6.5/include/linux/sched.h	2004-04-15 16:36:22.000000000 -0300
@@ -296,6 +296,7 @@
 	atomic_t __count;	/* reference count */
 	atomic_t processes;	/* How many processes does this user have? */
 	atomic_t files;		/* How many open files does this user have? */
+	atomic_t signal_pending; /* How many pending signals this user have? */
 
 	/* Hash table maintenance information */
 	struct list_head uidhash_list;
diff -Nur linux-2.6.5.org/kernel/signal.c linux-2.6.5/kernel/signal.c
--- linux-2.6.5.org/kernel/signal.c	2004-04-15 11:14:05.000000000 -0300
+++ linux-2.6.5/kernel/signal.c	2004-04-15 17:52:49.000000000 -0300
@@ -268,10 +268,13 @@
 {
 	struct sigqueue *q = 0;
 
-	if (atomic_read(&nr_queued_signals) < max_queued_signals)
+	if (atomic_read(&nr_queued_signals) < max_queued_signals &&
+	    atomic_read(&current->user->signal_pending) <= 
+			current->rlim[RLIMIT_SIGPENDING].rlim_cur)
 		q = kmem_cache_alloc(sigqueue_cachep, GFP_ATOMIC);
 	if (q) {
 		atomic_inc(&nr_queued_signals);
+		atomic_inc(&current->user->signal_pending);
 		INIT_LIST_HEAD(&q->list);
 		q->flags = 0;
 		q->lock = 0;
@@ -285,6 +288,7 @@
 		return;
 	kmem_cache_free(sigqueue_cachep, q);
 	atomic_dec(&nr_queued_signals);
+	atomic_dec(&current->user->signal_pending);
 }
 
 static void flush_sigqueue(struct sigpending *queue)
@@ -699,11 +703,14 @@
 	   make sure at least one signal gets delivered and don't
 	   pass on the info struct.  */
 
-	if (atomic_read(&nr_queued_signals) < max_queued_signals)
+	if (atomic_read(&nr_queued_signals) < max_queued_signals &&
+		atomic_read(&current->user->signal_pending) <=
+			current->rlim[RLIMIT_SIGPENDING].rlim_cur) 
 		q = kmem_cache_alloc(sigqueue_cachep, GFP_ATOMIC);
 
 	if (q) {
 		atomic_inc(&nr_queued_signals);
+		atomic_inc(&current->user->signal_pending);
 		q->flags = 0;
 		list_add_tail(&q->list, &signals->list);
 		switch ((unsigned long) info) {
@@ -2552,5 +2559,8 @@
 				  0, NULL, NULL);
 	if (!sigqueue_cachep)
 		panic("signals_init(): cannot create sigqueue SLAB cache");
+
+        init_task.rlim[RLIMIT_SIGPENDING].rlim_cur = max_queued_signals/2;
+        init_task.rlim[RLIMIT_SIGPENDING].rlim_max = max_queued_signals/2;
 }
 
diff -Nur linux-2.6.5.org/kernel/user.c linux-2.6.5/kernel/user.c
--- linux-2.6.5.org/kernel/user.c	2004-04-15 11:14:05.000000000 -0300
+++ linux-2.6.5/kernel/user.c	2004-04-15 18:20:03.379453680 -0300
@@ -30,7 +30,8 @@
 struct user_struct root_user = {
 	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
-	.files		= ATOMIC_INIT(0)
+	.files		= ATOMIC_INIT(0),
+	.signal_pending = ATOMIC_INIT(0)
 };
 
 /*

Re: message queue limits

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> This adds a new "RLIMIT_SIGPENDING" limit, which is used to limit
> per-uid pending signals. Currently an unpriviledged user can queue 
> more than maximum of allowed signals and cause overall system 
> malfunction.

So now it takes two users to gang up and do the same thing.  We should
either exempt root from the global check or simply remove the global limit
altogether.

Is it possible for a process to do setuid() with outstanding signals?  If
so, they may end up with a negative current->user->signal_pending?

You need to initialise ->signal_pending in alloc_uid().

What are you doing for testing of this?

Thanks.

Re: message queue limits

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]


Hi Andrew!

On Thu, Apr 15, 2004 at 03:54:08PM -0700, Andrew Morton wrote:
> Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> >
> > This adds a new "RLIMIT_SIGPENDING" limit, which is used to limit
> > per-uid pending signals. Currently an unpriviledged user can queue 
> > more than maximum of allowed signals and cause overall system 
> > malfunction.
> 
> So now it takes two users to gang up and do the same thing.  

Decrease rlim_cur then. Usually people dont have several accounts 
on the same box.

> We should either exempt root from the global check or simply remove the global > limit
> altogether.

Then allow for unlimited pending signals? Are you sure?

> Is it possible for a process to do setuid() with outstanding signals?  If
> so, they may end up with a negative current->user->signal_pending?

Nice catch, root can do that and I think current->user->signal_pending can get
negative. Not completly sure though.

> You need to initialise ->signal_pending in alloc_uid().

--- signal.c.orig	2004-04-15 20:44:17.458438104 -0300
+++ signal.c	2004-04-15 20:45:36.850368696 -0300
@@ -288,7 +288,8 @@
 		return;
 	kmem_cache_free(sigqueue_cachep, q);
 	atomic_dec(&nr_queued_signals);
-	atomic_dec(&current->user->signal_pending);
+	if (atomic_read(&current->user->signal_pending) > 0)
+		atomic_dec(&current->user->signal_pending);
 }
 
 static void flush_sigqueue(struct sigpending *queue)
--- user.c.orig	2004-04-15 20:44:20.395991528 -0300
+++ user.c	2004-04-15 20:44:37.069456776 -0300
@@ -98,6 +98,7 @@
 		atomic_set(&new->__count, 1);
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
+		atomic_set(&new->signal_pending, 0);
 
 		/*
 		 * Before adding this, check whether we raced

> What are you doing for testing of this?

Simple app posted by Nikita (attached) together with MySQL and 
sql-bench for creating mysql threads.
The setuid() was added by me now.


[-- Attachment #2: signal.c --]
[-- Type: text/plain, Size: 921 bytes --]

#include <signal.h>
#include <unistd.h>
#include <stdlib.h>
                                                                                                                                                                                    
int main()
{
        sigset_t set;
        int i;
        pid_t pid;
                                                                                                                                                                                    
        sigemptyset(&set);
        sigaddset(&set, 40);
        sigprocmask(SIG_BLOCK, &set, 0);
                                                                                                                                                                                    
        pid = getpid();
        for (i = 0; i < 1024; i++)
                kill(pid, 40);

	setuid(500);

        while (1)
                sleep(1);
}

Re: message queue limits

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> 
> Hi Andrew!
> 
> On Thu, Apr 15, 2004 at 03:54:08PM -0700, Andrew Morton wrote:
> > Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> > >
> > > This adds a new "RLIMIT_SIGPENDING" limit, which is used to limit
> > > per-uid pending signals. Currently an unpriviledged user can queue 
> > > more than maximum of allowed signals and cause overall system 
> > > malfunction.
> > 
> > So now it takes two users to gang up and do the same thing.  
> 
> Decrease rlim_cur then. Usually people dont have several accounts 
> on the same box.

You underestimate the deviousness of university students.

> > We should either exempt root from the global check or simply remove the global > limit
> > altogether.
> 
> Then allow for unlimited pending signals? Are you sure?

I think so.  If

	(number of users * rlimit(RLIMIT_SIGPENDING) * sizeof(struct sigqueue))

is a significant proportion of available lowmem then someone screwed up.

Given that we're currently allowing 1024 sigqueues kernel-wide, it seems
reasonable to permit 128 per user, and infinity for root.  At 144 bytes
each, that's 17k of pinned memory per user.  No big deal.

In fact I'd be inclined to allow 1024 per user as a default, just to be
sure that existing installations will not break.  Think: a machine running
a single application which wants all 1024.

> > Is it possible for a process to do setuid() with outstanding signals?  If
> > so, they may end up with a negative current->user->signal_pending?
> 
> Nice catch, root can do that and I think current->user->signal_pending can get
> negative. Not completly sure though.

Me not sure either.  I've noticed that anything with "signal" in the
subject seems to cause kernel developers' "Reply" buttons to malfunction. 
Funny, that.

> > You need to initialise ->signal_pending in alloc_uid().
> 
> --- signal.c.orig	2004-04-15 20:44:17.458438104 -0300
> +++ signal.c	2004-04-15 20:45:36.850368696 -0300
> @@ -288,7 +288,8 @@
>  		return;
>  	kmem_cache_free(sigqueue_cachep, q);
>  	atomic_dec(&nr_queued_signals);
> -	atomic_dec(&current->user->signal_pending);
> +	if (atomic_read(&current->user->signal_pending) > 0)
> +		atomic_dec(&current->user->signal_pending);
>  }

hrm.  I guess that's OK.  It does mean that there's a window in which the
user won't be able to queue as many signals as he expected to, but he was
doing weird stuff anyway.

It needs a comment though.

> > What are you doing for testing of this?
> 
> Simple app posted by Nikita (attached) together with MySQL and 
> sql-bench for creating mysql threads.
> The setuid() was added by me now.

OK, but you haven't tested dropping the rlimit?

Hey, I just discovered something.  In both bash and zsh you can do

	ulimit 4 1024

and it sets RLIMIT_CORE for you.  How very sensible.

Re: message queue limits

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 4837 bytes --]


On Thu, Apr 15, 2004 at 06:48:25PM -0700, Andrew Morton wrote:
> > On Thu, Apr 15, 2004 at 03:54:08PM -0700, Andrew Morton wrote:
> > > Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> > > >
> > > > This adds a new "RLIMIT_SIGPENDING" limit, which is used to limit
> > > > per-uid pending signals. Currently an unpriviledged user can queue 
> > > > more than maximum of allowed signals and cause overall system 
> > > > malfunction.
> > > 
> > > So now it takes two users to gang up and do the same thing.  
> > 
> > Decrease rlim_cur then. Usually people dont have several accounts 
> > on the same box.
> 
> You underestimate the deviousness of university students.
> 
> > > We should either exempt root from the global check or simply remove the global > limit
> > > altogether.
> > 
> > Then allow for unlimited pending signals? Are you sure?
> 
> I think so.  If
> 
> 	(number of users * rlimit(RLIMIT_SIGPENDING) * sizeof(struct sigqueue))
> 
> is a significant proportion of available lowmem then someone screwed up.
> 
> Given that we're currently allowing 1024 sigqueues kernel-wide, it seems
> reasonable to permit 128 per user, and infinity for root.  At 144 bytes
> each, that's 17k of pinned memory per user.  No big deal.

I'm not sure about "infinity" for root. That allows bad behaving apps (I know, very 
odd, but still) to exhaust system memory. I know, you can screwup as root anyway, but...

My personal preference is to keep a global limit (can be much higher than the  
per-user, like 131072). 

Are you sure about the removal of global limit ?

> In fact I'd be inclined to allow 1024 per user as a default, just to be
> sure that existing installations will not break.  Think: a machine running
> a single application which wants all 1024.

Indeed, 1024 is the default right now and we should keep it like that.

> > > Is it possible for a process to do setuid() with outstanding signals?  If
> > > so, they may end up with a negative current->user->signal_pending?
> > 
> > Nice catch, root can do that and I think current->user->signal_pending can get
> > negative. Not completly sure though.
> 
> Me not sure either.  I've noticed that anything with "signal" in the
> subject seems to cause kernel developers' "Reply" buttons to malfunction. 
> Funny, that.

Hehe.

> > > You need to initialise ->signal_pending in alloc_uid().
> > 
> > --- signal.c.orig	2004-04-15 20:44:17.458438104 -0300
> > +++ signal.c	2004-04-15 20:45:36.850368696 -0300
> > @@ -288,7 +288,8 @@
> >  		return;
> >  	kmem_cache_free(sigqueue_cachep, q);
> >  	atomic_dec(&nr_queued_signals);
> > -	atomic_dec(&current->user->signal_pending);
> > +	if (atomic_read(&current->user->signal_pending) > 0)
> > +		atomic_dec(&current->user->signal_pending);
> >  }
> 
> hrm.  I guess that's OK.  It does mean that there's a window in which the
> user won't be able to queue as many signals as he expected to, but he was
> doing weird stuff anyway.
> 
> It needs a comment though.

Now after a while I think that transferring the pending count 
at switch_uid() to the new "user_struct" may make sense. This way
we can remove the "if(atomic_read(&current->user->signal_pending)" 
in __free_sigqueue. Two options:

- setuid/friends can fail if the transferred pending signals plus
the current pending signals for the new user is higher than
rlimit[SIGPENDING]

- copy the pending signal count to the new user_struct->signal_pending
without checking for limits (so the signal_pending for this new 
user will be higher than the rlimit[SIGPENDING] and new sigqueue 
allocations will fail.

But maybe thats just "too much care" about this rare 
"setuid() with signal pending" case?

What you think?

> > > What are you doing for testing of this?
> > 
> > Simple app posted by Nikita (attached) together with MySQL and 
> > sql-bench for creating mysql threads.
> > The setuid() was added by me now.
> 
> OK, but you haven't tested dropping the rlimit?

I did now, it works. Look at the attached test program.

> Hey, I just discovered something.  In both bash and zsh you can do
> 
> 	ulimit 4 1024
> 
> and it sets RLIMIT_CORE for you.  How very sensible.

Not here: 

[root@yage root]# ulimit -a
core file size        (blocks, -c) 0
data seg size         (kbytes, -d) unlimited
file size             (blocks, -f) unlimited
max locked memory     (kbytes, -l) unlimited
max memory size       (kbytes, -m) unlimited
<snip>
[root@yage root]# ulimit 4 1024
[root@yage root]# ulimit -a
core file size        (blocks, -c) 0
data seg size         (kbytes, -d) unlimited
file size             (blocks, -f) 4
max locked memory     (kbytes, -l) unlimited
max memory size       (kbytes, -m) unlimited

It sets file size to 4:

GNU bash, version 2.05b.0(1)-release (i686-pc-linux-gnu)

Ah, the "files" member of struct user is unused. What is going on?


[-- Attachment #2: getrlimit.c --]
[-- Type: text/plain, Size: 366 bytes --]

#include <sys/time.h>
#include <sys/resource.h>
#include <unistd.h>

#define RLIMIT_SIGPENDING 11

void main() {
	struct rlimit rlim;

	if(getrlimit(RLIMIT_SIGPENDING, &rlim)) {
		perror();
	}

	rlim.rlim_cur = 128;

	setrlimit(RLIMIT_SIGPENDING, &rlim);

	printf("cur: %d\n", rlim.rlim_cur);
	printf("max: %d\n", rlim.rlim_max);

	execve("signal", NULL, NULL);

}

Re: message queue limits

[Marcelo Tosatti]

On Thu, Apr 15, 2004 at 05:55:57PM +0200, Manfred Spraul wrote:
> Marcelo Tosatti wrote:
> 
> >On Sun, Apr 11, 2004 at 10:48:28PM -0700, Ulrich Drepper wrote:
> > 
> >
> >>Something has to change in the way message queues are created.
> >>Currently it is possible for an unprivileged user to exhaust all mq
> >>slots so that only root can create a few more.  Any other unprivileged
> >>user has no change to create anything.
> >>
> >>I think it is necessary to create a per-user limit instead of a
> >>system-wide limit.
> >>   
> >>
> >
> >Manfred, 
> >
> >Are you looking into this already? I'm doing so for queued signals 
> >(max_nr_signals global limit).
> > 
> >
> Ahm, not really. Actually:
> I had noticed the queued signal report on lkml and decided to wait for 
> the patch that fixes the signal limit. I assume I'll be able to clone 
> the fix and apply it to ipc/mqueue.c.

Ulrich, Andrew, Manfred,

This should be working, but for some reason rlim[RLIMIT_MSGQUEUE].rlim_cur of
all tasks is 0, remembering it sets init_tasks's value at ipc/mqueue.c's __init function:

	init_task.rlim[RLIMIT_MSGQUEUE].rlim_cur = 64;
	init_task.rlim[RLIMIT_MSGQUEUE].rlim_max = 64;

Probably something stupid.

It may be interesting to move the mqueue pending count to the new task at "setuid()", 

diff -Nur linux-2.6.5.org/include/linux/sched.h linux-2.6.5/include/linux/sched.h
--- linux-2.6.5.org/include/linux/sched.h	2004-04-15 19:20:57.000000000 -0300
+++ linux-2.6.5/include/linux/sched.h	2004-04-15 20:25:18.000000000 -0300
@@ -310,6 +310,9 @@
 	atomic_t __count;	/* reference count */
 	atomic_t processes;	/* How many processes does this user have? */
 	atomic_t files;		/* How many open files does this user have? */
+	atomic_t signal_pending; /* How many pending signals does this user have? */
+	/* protected by mq_lock 	*/
+	int msg_queues; 	/* How many message queues does this user have? */
 
 	/* Hash table maintenance information */
 	struct list_head uidhash_list;
diff -Nur linux-2.6.5.org/ipc/mqueue.c linux-2.6.5/ipc/mqueue.c
--- linux-2.6.5.org/ipc/mqueue.c	2004-04-15 19:20:57.000000000 -0300
+++ linux-2.6.5/ipc/mqueue.c	2004-04-16 10:07:13.000000000 -0300
@@ -43,7 +43,7 @@
 #define CTL_MSGSIZEMAX 	4
 
 /* default values */
-#define DFLT_QUEUESMAX	64	/* max number of message queues */
+#define DFLT_QUEUESMAX	256		/* max number of message queues */
 #define DFLT_MSGMAX 	40	/* max number of messages in each queue */
 #define HARD_MSGMAX 	(131072/sizeof(void*))
 #define DFLT_MSGSIZEMAX 16384	/* max message size */
@@ -217,6 +217,14 @@
 
 	spin_lock(&mq_lock);
 	queues_count--;
+
+	/* 
+	 * If we create mqueues, then "setuid()", and destroy 
+	 * mqueues later on (with the new uid), msg_queues 
+	 * can get negative.
+	 */
+	if (current->user->msg_queues > 0)
+		current->user->msg_queues--;
 	spin_unlock(&mq_lock);
 }
 
@@ -225,13 +233,16 @@
 {
 	struct inode *inode;
 	int error;
+	struct task_struct *p = current;
 
 	spin_lock(&mq_lock);
-	if (queues_count >= queues_max && !capable(CAP_SYS_RESOURCE)) {
+	if (queues_count >= queues_max && !capable(CAP_SYS_RESOURCE) &&
+	  p->user->msg_queues >= p->rlim[RLIMIT_MSGQUEUE].rlim_cur)
 		error = -ENOSPC;
 		goto out_lock;
-	}
+
 	queues_count++;
+	p->user->msg_queues++;
 	spin_unlock(&mq_lock);
 
 	inode = mqueue_get_inode(dir->i_sb, mode);
@@ -239,6 +250,7 @@
 		error = -ENOMEM;
 		spin_lock(&mq_lock);
 		queues_count--;
+		p->user->msg_queues--;
 		goto out_lock;
 	}
 
@@ -1202,6 +1214,9 @@
 	/* internal initialization - not common for vfs */
 	queues_count = 0;
 	spin_lock_init(&mq_lock);
+	
+	init_task.rlim[RLIMIT_MSGQUEUE].rlim_cur = 64;
+	init_task.rlim[RLIMIT_MSGQUEUE].rlim_max = 64;
 
 	return 0;
 
diff -Nur linux-2.6.5.org/kernel/user.c linux-2.6.5/kernel/user.c
--- linux-2.6.5.org/kernel/user.c	2004-04-15 11:14:05.000000000 -0300
+++ linux-2.6.5/kernel/user.c	2004-04-16 10:14:00.000000000 -0300
@@ -30,7 +30,9 @@
 struct user_struct root_user = {
 	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
-	.files		= ATOMIC_INIT(0)
+	.files		= ATOMIC_INIT(0),
+	.signal_pending = ATOMIC_INIT(0),
+	.msg_queues = 0
 };
 
 /*
@@ -98,6 +100,9 @@
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
 
+		new->msg_queues = 0;
+		
+
 		/*
 		 * Before adding this, check whether we raced
 		 * on adding the same user already..
--- linux-2.6.5.org/include/asm-i386/resource.h	2004-04-15 11:13:28.000000000 -0300
+++ linux-2.6.5/include/asm-i386/resource.h	2004-04-15 19:26:25.000000000 -0300
@@ -16,8 +16,10 @@
 #define RLIMIT_MEMLOCK	8		/* max locked-in-memory address space */
 #define RLIMIT_AS	9		/* address space limit */
 #define RLIMIT_LOCKS	10		/* maximum file locks held */
+#define RLIMIT_SIGPENDING 11		/* max number of pending signals */
+#define RLIMIT_MSGQUEUE 12		/* max number of POSIX msg queues */
 
-#define RLIM_NLIMITS	11
+#define RLIM_NLIMITS	13
 
 /*
  * SuS says limits have to be unsigned.

Re: message queue limits

[Andrew Morton]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
>
> This should be working, but for some reason rlim[RLIMIT_MSGQUEUE].rlim_cur of
> all tasks is 0, remembering it sets init_tasks's value at ipc/mqueue.c's __init function:
> 
> 	init_task.rlim[RLIMIT_MSGQUEUE].rlim_cur = 64;
> 	init_task.rlim[RLIMIT_MSGQUEUE].rlim_max = 64;

init_task is the task_struct for process 0, "swapper".  But by the time we
run the initcalls, process 1 ("init") is up and running.

So by the time you execute these assignments, you're changing the limits on
a process which will never again create any children.

It's a bit hacky, but you could do

	BUG_ON(current->pid != 1);
	current->rlim[RLIMIT_MSGQUEUE].rlim_cur = 64;

but longer-term these initialisations should be moved into
include/asm-foo/reousrce.h:INIT_RLIMITS