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

Either way... I know nothing about security/key, but grep -w key_put finds

* When it is no longer required, the key should be released using::

void key_put(struct key *key);

Or::

void key_ref_put(key_ref_t key_ref);

These can be called from interrupt context. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

in Documentation/security/keys/core.rst

and since key_user_put() takes key_user_lock with irqs enabled, key_put()->key_user_put() doesn't look correct...

Oleg.

On 03/18, Oleg Nesterov wrote:

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.