Re: next-20170510 refcount_inc() on zero / use-after-free in key_lookup()

[Mark Rutland <mark.rutland@arm.com>]

On Fri, May 12, 2017 at 04:29:39PM +0100, David Howells wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > From a quick look at key_lookup(), the following looks very suspicious:
> > 
> > found:
> >         /* pretend it doesn't exist if it is awaiting deletion */
> >         if (refcount_read(&key->usage) == 0)
> >                 goto not_found;
> > 
> >         /* this races with key_put(), but that doesn't matter since key_put()
> >          * doesn't actually change the key
> >          */
> >         __key_get(key);
> > 
> > ... as if we can race with key_put(), we can see a zero refcount here,
> > and the race *does* matter.
> 
> No, it doesn't.
> 
> If key_put() reduces a refcount to 0, it doesn't do anything other than poke
> the gc thread:
> 
> 	void key_put(struct key *key)
> 	{
> 		if (key) {
> 			key_check(key);
> 
> 			if (refcount_dec_and_test(&key->usage))
> 				schedule_work(&key_gc_work);
> 		}
> 	}
> 
> in particular, no indication of the reduced key is passed.
> 
> The gc thread scans the entire key serial tree under the key_serial_lock
> looking for keys that are no longer ref'd.  No one else is allowed to remove
> keys from the tree.  This means that the gc thread can safely leave a cursor
> pointing into the midst of the tree with no locks held whilst it yields to the
> scheduler.
> 
> The code you quoted above in key_lookup() is inside the key_serial_lock, so it
> prevents the gc thread from culling a key when it resurrects it.
> 
> So the problem isn't the key code, it's the refcount code.

Sure, there's no actual use-after-free here.

Sorry for the misleading title.

> As I've said before, the refcount code needs an increment op that permits
> inc-from-0.  In this case, it's perfectly okay.

Given that there's currently an attempt to bail out on a zero refcount courtesy
of the refcount_read(), can't we do something like the below?

Thanks,
Mark.

diff --git a/include/linux/key.h b/include/linux/key.h
index 78e25aa..1e68ae2 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -248,6 +248,14 @@ extern struct key *key_alloc(struct key_type *type,
 extern void key_invalidate(struct key *key);
 extern void key_put(struct key *key);
 
+static inline struct key *__key_get_notzero(struct key *key)
+{
+       if (refcount_inc_not_zero(&key->usage))
+               return key;
+
+       return NULL;
+}
+
 static inline struct key *__key_get(struct key *key)
 {
        refcount_inc(&key->usage);
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d..f375cc6 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -661,14 +661,9 @@ struct key *key_lookup(key_serial_t id)
 
 found:
        /* pretend it doesn't exist if it is awaiting deletion */
-       if (refcount_read(&key->usage) == 0)
+       if (!__key_get_notzero(key))
                goto not_found;
 
-       /* this races with key_put(), but that doesn't matter since key_put()
-        * doesn't actually change the key
-        */
-       __key_get(key);
-
 error:
        spin_unlock(&key_serial_lock);
        return key;