[PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[David Howells]

The request_key() system call and request_key_and_link() should make a link
from an existing key to the destination keyring (if supplied), not just from a
new key to the destination keyring.

This can be tested by:

	ring=`keyctl newring fred @s`
	keyctl request2 user debug:a a
	keyctl request user debug:a $ring
	keyctl list $ring

If it says:

	keyring is empty

then it didn't work.  If it shows something like:

	1 key in keyring:
	1070462727: --alswrv     0     0 user: debug:a

then it did.

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

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

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index ea97c31..d737cea 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -339,8 +339,10 @@ static int construct_alloc_key(struct key_type *type,
 
 key_already_present:
 	mutex_unlock(&key_construction_mutex);
-	if (dest_keyring)
+	if (dest_keyring) {
+		__key_link(dest_keyring, key_ref_to_ptr(key_ref));
 		up_write(&dest_keyring->sem);
+	}
 	mutex_unlock(&user->cons_lock);
 	key_put(key);
 	*_key = key = key_ref_to_ptr(key_ref);
@@ -431,6 +433,11 @@ struct key *request_key_and_link(struct key_type *type,
 
 	if (!IS_ERR(key_ref)) {
 		key = key_ref_to_ptr(key_ref);
+		if (dest_keyring) {
+			construct_get_dest_keyring(&dest_keyring);
+			key_link(dest_keyring, key);
+			key_put(dest_keyring);
+		}
 	} else if (PTR_ERR(key_ref) != -EAGAIN) {
 		key = ERR_CAST(key_ref);
 	} else  {

Re: [PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[Andrew Morton]

On Mon, 26 Apr 2010 16:44:41 +0100
David Howells <dhowells@redhat.com> wrote:

> The request_key() system call and request_key_and_link() should make a link
> from an existing key to the destination keyring (if supplied), not just from a
> new key to the destination keyring.
> 
> This can be tested by:
> 
> 	ring=`keyctl newring fred @s`
> 	keyctl request2 user debug:a a
> 	keyctl request user debug:a $ring
> 	keyctl list $ring
> 
> If it says:
> 
> 	keyring is empty
> 
> then it didn't work.  If it shows something like:
> 
> 	1 key in keyring:
> 	1070462727: --alswrv     0     0 user: debug:a
> 
> then it did.
> 

I don't think I understand that description.  Are you saying that the
patch switches things from the first example output into the second case?

Is the fix needed in -stable?

Re: [PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> I don't think I understand that description.  Are you saying that the
> patch switches things from the first example output into the second case?

request_key() system call is meant to recursively search all your keyrings for
the key you desire, and, optionally, if it doesn't exist, call out to userspace
to create one for you.

If request_key() finds or creates a key, it should, optionally, create a link
to that key from the destination keyring specified.

Therefore, if, after a successful call to request_key() with a desination
keyring specified, you see the destination keyring empty, the code didn't work
correctly.

If you see the found key in the keyring, then it did - which is what the patch
is required for.

> Is the fix needed in -stable?

Yes.

David

Re: [PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[David Howells]

Vegard Nossum <vegard.nossum@gmail.com> wrote:

> 1. Is it correct to return -EINPROGRESS in this case?

Yes - it prevents construct_key_and_link() from going on to call
construct_key().  The key in question is returned in *_key.

However, I don't think I handle other errors coming back from
construct_alloc_key() correctly.

> 2. (Why) Shouldn't the return value of __key_link() be checked?

It should.  In fact, I should probably precheck and preallocate the keyring to
make sure I don't get errors after allocating the new key.

> 3. In __key_link(), shouldn't rcu_dereference() be used when accessing
> keyring->payload.subscriptions?

No.  We have to hold a write lock on the keyring semaphore if we're going to
call __key_link() to add something to it.

We should, however, call rcu_dereference_protected().

You should find a patch for this in your inbox.

David

Re: [PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[Vegard Nossum]

On 26 April 2010 16:59, David Howells <dhowells@redhat.com> wrote:
> The request_key() system call and request_key_and_link() should make a link
> from an existing key to the destination keyring (if supplied), not just from a
> new key to the destination keyring.
>
> This can be tested by:
>
>        ring=`keyctl newring fred @s`
>        keyctl request2 user debug:a a
>        keyctl request user debug:a $ring
>        keyctl list $ring
>
> If it says:
>
>        keyring is empty
>
> then it didn't work.  If it shows something like:
>
>        1 key in keyring:
>        1070462727: --alswrv     0     0 user: debug:a
>
> then it did.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  security/keys/request_key.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index ea97c31..d737cea 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -339,8 +339,10 @@ static int construct_alloc_key(struct key_type *type,
>
>  key_already_present:
>        mutex_unlock(&key_construction_mutex);
> -       if (dest_keyring)
> +       if (dest_keyring) {
> +               __key_link(dest_keyring, key_ref_to_ptr(key_ref));
>                up_write(&dest_keyring->sem);
> +       }
>        mutex_unlock(&user->cons_lock);
>        key_put(key);
>        *_key = key = key_ref_to_ptr(key_ref);

Hi,

Just a few questions (if you don't mind).

1. Is it correct to return -EINPROGRESS in this case?

2. (Why) Shouldn't the return value of __key_link() be checked?

3. In __key_link(), shouldn't rcu_dereference() be used when accessing
keyring->payload.subscriptions?

Thanks,


Vegard

[PATCH] KEYS: The request_key() syscall should link an existing key to the dest keyring

[David Howells]

The request_key() system call and request_key_and_link() should make a link
from an existing key to the destination keyring (if supplied), not just from a
new key to the destination keyring.

This can be tested by:

	ring=`keyctl newring fred @s`
	keyctl request2 user debug:a a
	keyctl request user debug:a $ring
	keyctl list $ring

If it says:

	keyring is empty

then it didn't work.  If it shows something like:

	1 key in keyring:
	1070462727: --alswrv     0     0 user: debug:a

then it did.

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

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

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index ea97c31..d737cea 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -339,8 +339,10 @@ static int construct_alloc_key(struct key_type *type,
 
 key_already_present:
 	mutex_unlock(&key_construction_mutex);
-	if (dest_keyring)
+	if (dest_keyring) {
+		__key_link(dest_keyring, key_ref_to_ptr(key_ref));
 		up_write(&dest_keyring->sem);
+	}
 	mutex_unlock(&user->cons_lock);
 	key_put(key);
 	*_key = key = key_ref_to_ptr(key_ref);
@@ -431,6 +433,11 @@ struct key *request_key_and_link(struct key_type *type,
 
 	if (!IS_ERR(key_ref)) {
 		key = key_ref_to_ptr(key_ref);
+		if (dest_keyring) {
+			construct_get_dest_keyring(&dest_keyring);
+			key_link(dest_keyring, key);
+			key_put(dest_keyring);
+		}
 	} else if (PTR_ERR(key_ref) != -EAGAIN) {
 		key = ERR_CAST(key_ref);
 	} else  {