[PATCH] Fix reproducible SMP crash in security/keys/key.c

[PATCH] Fix reproducible SMP crash in security/keys/key.c

[Jani Jaakkola]

SMP race handling is broken in key_user_lookup() in security/keys/key.c
(if CONFIG_KEYS is set to 'y'). This came up on our Samba servers, but is
not restricted to samba, though samba is probably the only software which
is likely to trigger this repeatedly (and it did happen allready four 
times here in University of Helsinki, CS department).

However, it only takes two setreuid() calls at the same instant, so this
may be responsible for some other mysterious random crashes.

This is the same bug which was previously raported to LKML here (found by 
google):
http://www.ussg.iu.edu/hypermail/linux/kernel/0502.2/0521.html

Here is a small test program, which can be used to trigger the bug and 
crash the machine where it is run. It might take a few seconds:

#include<unistd.h>
#include<stdio.h>
int main() {
        int i;
        fork();
        while(1) {
                for(i=0;i<60000;i++) { setreuid(i,0); } 
                putchar('.'); fflush(stdout);
        };
}

The (rather obvious) problem is that key_user_lookup() does not properly 
re-initialize the user lookup if there was a race.

This patch applies to vanilla 2.6.11.7 and latest fedora kernel
2.6.11-1.14_FC3. When applied, the test program runs just fine (and does
nothing useful).

Please Cc: any replies to me, since I am not a regular kernel hacker and 
do not subscribe LKML.

Signed-Off-By: Jani Jaakkola <jjaakkol@cs.helsinki.fi>

--- linux-2.6.11/security/keys/key.c.orig       Wed Mar  2 09:38:25 2005
+++ linux-2.6.11/security/keys/key.c    Tue Apr 12 18:19:50 2005
@@ -56,10 +56,12 @@
 struct key_user *key_user_lookup(uid_t uid)
 {
        struct key_user *candidate = NULL, *user;
-       struct rb_node *parent = NULL;
-       struct rb_node **p = &key_user_tree.rb_node;
+       struct rb_node *parent;
+       struct rb_node **p;
 
  try_again:
+       parent = NULL;
+       p = &key_user_tree.rb_node;
        spin_lock(&key_user_lock);
 
        /* search the tree for a user record with a matching UID */

Re: [PATCH] Fix reproducible SMP crash in security/keys/key.c

[Alexander Nyberg]

tis 2005-04-12 klockan 21:58 +0300 skrev Jani Jaakkola:
> SMP race handling is broken in key_user_lookup() in security/keys/key.c
> (if CONFIG_KEYS is set to 'y'). This came up on our Samba servers, but is
> not restricted to samba, though samba is probably the only software which
> is likely to trigger this repeatedly (and it did happen allready four 
> times here in University of Helsinki, CS department).
> 
> However, it only takes two setreuid() calls at the same instant, so this
> may be responsible for some other mysterious random crashes.
> 
> This is the same bug which was previously raported to LKML here (found by 
> google):
> http://www.ussg.iu.edu/hypermail/linux/kernel/0502.2/0521.html
> 
> Here is a small test program, which can be used to trigger the bug and 
> crash the machine where it is run. It might take a few seconds:
> 
> #include<unistd.h>
> #include<stdio.h>
> int main() {
>         int i;
>         fork();
>         while(1) {
>                 for(i=0;i<60000;i++) { setreuid(i,0); } 
>                 putchar('.'); fflush(stdout);
>         };
> }
> 
> The (rather obvious) problem is that key_user_lookup() does not properly 
> re-initialize the user lookup if there was a race.
> 
> This patch applies to vanilla 2.6.11.7 and latest fedora kernel
> 2.6.11-1.14_FC3. When applied, the test program runs just fine (and does
> nothing useful).

A fix went into mainline for this two months ago (post 2.6.11), but I
probably should have sent it into -stable aswell.

For your own sake always use the latest kernel when looking at
problems/fixes, things move fast around here :)

Re: [PATCH] Fix reproducible SMP crash in security/keys/key.c

[Andrew Morton]

Jani Jaakkola <jjaakkol@cs.Helsinki.FI> wrote:
>
> SMP race handling is broken in key_user_lookup() in security/keys/key.c

This was fixed post-2.6.11.  Can you confirm that 2.6.12-rc2 works OK?

This is the patch we used.  It should go into -stable if it's not already
there.


From: Alexander Nyberg <alexn@dsv.su.se>

I looked at some of the oops reports against keyrings, I think the problem
is that the search isn't restarted after dropping the key_user_lock, *p
will still be NULL when we get back to try_again and look through the tree.

It looks like the intention was that the search start over from scratch.

Signed-off-by: Alexander Nyberg <alexn@dsv.su.se>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/security/keys/key.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN security/keys/key.c~race-against-parent-deletion-in-key_user_lookup security/keys/key.c
--- 25/security/keys/key.c~race-against-parent-deletion-in-key_user_lookup	2005-03-10 00:38:38.000000000 -0800
+++ 25-akpm/security/keys/key.c	2005-03-10 00:38:38.000000000 -0800
@@ -57,9 +57,10 @@ struct key_user *key_user_lookup(uid_t u
 {
 	struct key_user *candidate = NULL, *user;
 	struct rb_node *parent = NULL;
-	struct rb_node **p = &key_user_tree.rb_node;
+	struct rb_node **p;
 
  try_again:
+	p = &key_user_tree.rb_node;
 	spin_lock(&key_user_lock);
 
 	/* search the tree for a user record with a matching UID */
_

Re: [stable] Re: [PATCH] Fix reproducible SMP crash in security/keys/key.c

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Jani Jaakkola <jjaakkol@cs.Helsinki.FI> wrote:
> >
> > SMP race handling is broken in key_user_lookup() in security/keys/key.c
> 
> This was fixed post-2.6.11.  Can you confirm that 2.6.12-rc2 works OK?
> 
> This is the patch we used.  It should go into -stable if it's not already
> there.

No, it's not in -stable, queued up, thanks.
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Fix reproducible SMP crash in security/keys/key.c

[David Howells]

Jani Jaakkola <jjaakkol@cs.Helsinki.FI> wrote:

> SMP race handling is broken in key_user_lookup() in security/keys/key.c
> (if CONFIG_KEYS is set to 'y').

A patch very much like the one you proposed is already resident in the latest
-rc kernels. Thanks anyway:-)

David