* [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails
@ 2017-04-01 18:54 Eric Biggers
2017-04-03 15:42 ` David Howells
0 siblings, 1 reply; 3+ messages in thread
From: Eric Biggers @ 2017-04-01 18:54 UTC (permalink / raw)
To: keyrings; +Cc: David Howells, linux-kernel, Eric Biggers
From: Eric Biggers <ebiggers@google.com>
In join_session_keyring(), if install_session_keyring_to_cred() were to
fail, we would leak the keyring reference, just like in the bug fixed by
commit 23567fd052a9 ("KEYS: Fix keyring ref leak in
join_session_keyring()"). Fortunately this cannot happen currently, but
we really should be more careful. Do this by adding and using a new
error label at which the keyring reference is dropped.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
security/keys/process_keys.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index b6fdd22205b1..fc60f998c74d 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -799,15 +799,14 @@ long join_session_keyring(const char *name)
ret = PTR_ERR(keyring);
goto error2;
} else if (keyring == new->session_keyring) {
- key_put(keyring);
ret = 0;
- goto error2;
+ goto error3;
}
/* we've got a keyring - now to install it */
ret = install_session_keyring_to_cred(new, keyring);
if (ret < 0)
- goto error2;
+ goto error3;
commit_creds(new);
mutex_unlock(&key_session_mutex);
@@ -817,6 +816,8 @@ long join_session_keyring(const char *name)
okay:
return ret;
+error3:
+ key_put(keyring);
error2:
mutex_unlock(&key_session_mutex);
error:
--
2.12.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails
2017-04-01 18:54 [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails Eric Biggers
@ 2017-04-03 15:42 ` David Howells
2017-04-03 17:48 ` Eric Biggers
0 siblings, 1 reply; 3+ messages in thread
From: David Howells @ 2017-04-03 15:42 UTC (permalink / raw)
To: Eric Biggers; +Cc: dhowells, keyrings, linux-kernel, Eric Biggers
Eric Biggers <ebiggers3@gmail.com> wrote:
> +error3:
> + key_put(keyring);
> error2:
> mutex_unlock(&key_session_mutex);
I would prefer the put to be outside of the locked section. How about the
attached instead?
David
---
commit 5f9e6fc4fdbbf9ada9bd03640b1c3ca50a3e5415
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Apr 1 11:54:16 2017 -0700
KEYS: put keyring if install_session_keyring_to_cred() fails
In join_session_keyring(), if install_session_keyring_to_cred() were to
fail, we would leak the keyring reference, just like in the bug fixed by
commit 23567fd052a9 ("KEYS: Fix keyring ref leak in
join_session_keyring()"). Fortunately this cannot happen currently, but
we really should be more careful. Do this by adding and using a new
error label at which the keyring reference is dropped.
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 44451af828c0..8e98e5d1b5fb 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -793,21 +793,20 @@ long join_session_keyring(const char *name)
KEY_ALLOC_IN_QUOTA, NULL, NULL);
if (IS_ERR(keyring)) {
ret = PTR_ERR(keyring);
- goto error2;
+ goto error_unlock_no_keyring;
}
} else if (IS_ERR(keyring)) {
ret = PTR_ERR(keyring);
- goto error2;
+ goto error_unlock_no_keyring;
} else if (keyring == new->session_keyring) {
- key_put(keyring);
ret = 0;
- goto error2;
+ goto error_unlock;
}
/* we've got a keyring - now to install it */
ret = install_session_keyring_to_cred(new, keyring);
if (ret < 0)
- goto error2;
+ goto error_unlock;
commit_creds(new);
mutex_unlock(&key_session_mutex);
@@ -817,8 +816,11 @@ long join_session_keyring(const char *name)
okay:
return ret;
-error2:
+error_unlock_no_keyring:
+ keyring = NULL;
+error_unlock:
mutex_unlock(&key_session_mutex);
+ key_put(keyring);
error:
abort_creds(new);
return ret;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails
2017-04-03 15:42 ` David Howells
@ 2017-04-03 17:48 ` Eric Biggers
0 siblings, 0 replies; 3+ messages in thread
From: Eric Biggers @ 2017-04-03 17:48 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-kernel, Eric Biggers
Hi David,
On Mon, Apr 03, 2017 at 04:42:05PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
>
> > +error3:
> > + key_put(keyring);
> > error2:
> > mutex_unlock(&key_session_mutex);
>
> I would prefer the put to be outside of the locked section. How about the
> attached instead?
[...]
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 44451af828c0..8e98e5d1b5fb 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -793,21 +793,20 @@ long join_session_keyring(const char *name)
> KEY_ALLOC_IN_QUOTA, NULL, NULL);
> if (IS_ERR(keyring)) {
> ret = PTR_ERR(keyring);
> - goto error2;
> + goto error_unlock_no_keyring;
> }
> } else if (IS_ERR(keyring)) {
> ret = PTR_ERR(keyring);
> - goto error2;
> + goto error_unlock_no_keyring;
> } else if (keyring == new->session_keyring) {
> - key_put(keyring);
> ret = 0;
> - goto error2;
> + goto error_unlock;
> }
>
> /* we've got a keyring - now to install it */
> ret = install_session_keyring_to_cred(new, keyring);
> if (ret < 0)
> - goto error2;
> + goto error_unlock;
>
> commit_creds(new);
> mutex_unlock(&key_session_mutex);
> @@ -817,8 +816,11 @@ long join_session_keyring(const char *name)
> okay:
> return ret;
>
> -error2:
> +error_unlock_no_keyring:
> + keyring = NULL;
> +error_unlock:
> mutex_unlock(&key_session_mutex);
> + key_put(keyring);
> error:
> abort_creds(new);
> return ret;
I guess I'm okay with either patch, but I think jumping to the error labels in
the way I proposed is more straightforward since it keeps things in the logical
order. And I doubt there's much advantage in moving key_put() outside of the
mutex, given that key_session_mutex is only used for joining session keyrings so
shouldn't be contended, and key_put() hardly does any work anyway.
- Eric
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-03 17:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-01 18:54 [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails Eric Biggers
2017-04-03 15:42 ` David Howells
2017-04-03 17:48 ` Eric Biggers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.