mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: David Howells <dhowells@redhat.com>
To: torvalds@osdl.org, akpm@linux-foundation.org
Cc: keyrings@linux-nfs.org, linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, David Howells <dhowells@redhat.com>
Subject: [PATCH 4/7] KEYS: call_sbin_request_key() must write lock keyrings before modifying them
Date: Fri, 30 Apr 2010 14:32:23 +0100	[thread overview]
Message-ID: <20100430133223.2126.77304.stgit@warthog.procyon.org.uk> (raw)
In-Reply-To: <20100430133208.2126.56765.stgit@warthog.procyon.org.uk>

call_sbin_request_key() creates a keyring and then attempts to insert a link to
the authorisation key into that keyring, but does so without holding a write
lock on the keyring semaphore.

It will normally get away with this because it hasn't told anyone that the
keyring exists yet.  The new keyring, however, has had its serial number
published, which means it can be accessed directly by that handle.

This was found by a previous patch that adds RCU lockdep checks to the code
that reads the keyring payload pointer, which includes a check that the keyring
semaphore is actually locked.

Without this patch, the following command:

	keyctl request2 user b a @s

will provoke the following lockdep warning is displayed in dmesg:

	===================================================
	[ INFO: suspicious rcu_dereference_check() usage. ]
	---------------------------------------------------
	security/keys/keyring.c:727 invoked rcu_dereference_check() without protection!

	other info that might help us debug this:


	rcu_scheduler_active = 1, debug_locks = 0
	2 locks held by keyctl/2076:
	 #0:  (key_types_sem){.+.+.+}, at: [<ffffffff811a5b29>] key_type_lookup+0x1c/0x71
	 #1:  (keyring_serialise_link_sem){+.+.+.}, at: [<ffffffff811a6d1e>] __key_link+0x4d/0x3c5

	stack backtrace:
	Pid: 2076, comm: keyctl Not tainted 2.6.34-rc6-cachefs #54
	Call Trace:
	 [<ffffffff81051fdc>] lockdep_rcu_dereference+0xaa/0xb2
	 [<ffffffff811a6d1e>] ? __key_link+0x4d/0x3c5
	 [<ffffffff811a6e6f>] __key_link+0x19e/0x3c5
	 [<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
	 [<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
	 [<ffffffff811aa0dc>] call_sbin_request_key+0xe7/0x33b
	 [<ffffffff8139376a>] ? mutex_unlock+0x9/0xb
	 [<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
	 [<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
	 [<ffffffff811aa6fa>] ? request_key_auth_new+0x1c2/0x23c
	 [<ffffffff810aaf15>] ? cache_alloc_debugcheck_after+0x108/0x173
	 [<ffffffff811a9d00>] ? request_key_and_link+0x146/0x300
	 [<ffffffff810ac568>] ? kmem_cache_alloc+0xe1/0x118
	 [<ffffffff811a9e45>] request_key_and_link+0x28b/0x300
	 [<ffffffff811a89ac>] sys_request_key+0xf7/0x14a
	 [<ffffffff81052c0b>] ? trace_hardirqs_on_caller+0x10c/0x130
	 [<ffffffff81394fb9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
	 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/request_key.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index d737cea..d8c1a6a 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -94,7 +94,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 	}
 
 	/* attach the auth key to the session keyring */
-	ret = __key_link(keyring, authkey);
+	ret = key_link(keyring, authkey);
 	if (ret < 0)
 		goto error_link;
 


  parent reply	other threads:[~2010-04-30 17:17 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-04-30 13:32 [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys David Howells
2010-04-30 13:32 ` [PATCH 2/7] KEYS: find_keyring_by_name() can gain access to a freed keyring David Howells
2010-05-03 22:14   ` Serge E. Hallyn
2010-04-30 13:32 ` [PATCH 3/7] KEYS: Use RCU dereference wrappers in keyring key type code David Howells
2010-05-03 22:30   ` Serge E. Hallyn
2010-05-04 13:00   ` David Howells
2010-04-30 13:32 ` David Howells [this message]
2010-04-30 13:32 ` [PATCH 5/7] KEYS: keyring_serialise_link_sem is only needed for keyring->keyring links David Howells
2010-04-30 13:32 ` [PATCH 6/7] KEYS: Better handling of errors from construct_alloc_key() David Howells
2010-04-30 13:32 ` [PATCH 7/7] KEYS: Do preallocation for __key_link() David Howells
2010-05-03 22:04 ` [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys Serge E. Hallyn
2010-05-04 12:48 ` David Howells
2010-05-06  2:45 ` James Morris
2010-05-06 10:38 ` David Howells
2010-05-06 12:25   ` James Morris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100430133223.2126.77304.stgit@warthog.procyon.org.uk \
    --to=dhowells@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=keyrings@linux-nfs.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=torvalds@osdl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®