Re: [PATCH] keys: Fix UAF in key_put()

On 03/18, David Howells wrote:

--- a/security/keys/key.c +++ b/security/keys/key.c @@ -645,21 +645,30 @@ EXPORT_SYMBOL(key_reject_and_link); */ void key_put(struct key *key) {

• int quota_flag;
• unsigned short len;
• struct key_user *user;
• if (key) { key_check(key);

• quota_flag = test_bit(KEY_FLAG_IN_QUOTA, &key->flags);
  

• len = key->quotalen;
  

• user = key->user;
  

• refcount_inc(&user->usage);
  

  if (refcount_dec_and_test(&key->usage)) { unsigned long flags;

/* deal with the user's key tracking and quota */

• 	if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
  

• 		spin_lock_irqsave(&key->user->lock, flags);
  

• 		key->user->qnkeys--;
  

• 		key->user->qnbytes -= key->quotalen;
  

• 		spin_unlock_irqrestore(&key->user->lock, flags);
  

• 	if (quota_flag) {
  

• 		spin_lock_irqsave(&user->lock, flags);
  

• 		user->qnkeys--;
  

• 		user->qnbytes -= len;
  

• 		spin_unlock_irqrestore(&user->lock, flags);
  }
  schedule_work(&key_gc_work);
  

  }

• key_user_put(user);
  

Do we really need the unconditional refcount_inc / key_user_put ?

void key_put(struct key *key) { if (key) { struct key_user *user = NULL; unsigned short len;

key_check(key);

if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) { len = key->quotalen; user = key->user; refcount_inc(&user->usage); }

if (refcount_dec_and_test(&key->usage)) { unsigned long flags;

/* deal with the user's key tracking and quota */ if (user) { spin_lock_irqsave(&user->lock, flags); user->qnkeys--; user->qnbytes -= len; spin_unlock_irqrestore(&user->lock, flags); } schedule_work(&key_gc_work); }

if (user) key_user_put(user); } }

looks a bit more clear/simple to me...

Oleg.