* [PATCH] KEYS: fix refcount_inc() on zero
@ 2017-05-26 17:37 Mark Rutland
2017-05-26 19:05 ` Kees Cook
2017-05-31 12:20 ` David Howells
0 siblings, 2 replies; 4+ messages in thread
From: Mark Rutland @ 2017-05-26 17:37 UTC (permalink / raw)
To: dhowells
Cc: keyrings, linux-security-module, linux-kernel, Mark Rutland,
David Windsor, Elena Reshetova, Hans Liljestrand, James Morris,
Kees Cook, Peter Zijlstra
If a key's refcount is dropped to zero between key_lookup() peeking at
the refcount and subsequently attempting to increment it, refcount_inc()
will see a zero refcount. Here, refcount_inc() will WARN_ONCE(), and
will *not* increment the refcount, which will remain zero.
Once key_lookup() drops key_serial_lock, it is possible for the key to
be freed behind our back.
This patch uses refcount_inc_not_zero() to perform the peek and
increment atomically. A helper with lockdep annotation is added to
document why this is safe.
Fixes: fff292914d3a2f1e ("security, keys: convert key.usage from atomic_t to refcount_t")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Elena Reshetova <elena.reshetova@intel.com>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
security/keys/key.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d..150f51d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -632,6 +632,12 @@ void key_put(struct key *key)
}
EXPORT_SYMBOL(key_put);
+static bool key_get_not_free(struct key *key)
+{
+ lockdep_assert_held(&key_serial_lock);
+ return refcount_inc_not_zero(&key->usage);
+}
+
/*
* Find a key by its serial number.
*/
@@ -660,14 +666,12 @@ struct key *key_lookup(key_serial_t id)
goto error;
found:
- /* pretend it doesn't exist if it is awaiting deletion */
- if (refcount_read(&key->usage) == 0)
- goto not_found;
-
- /* this races with key_put(), but that doesn't matter since key_put()
- * doesn't actually change the key
+ /*
+ * Pretend it doesn't exist if it is awaiting deletion. This races with
+ * key_put(), but we can peek at the key until we drop key_serial_lock.
*/
- __key_get(key);
+ if (!key_get_not_free(key))
+ goto not_found;
error:
spin_unlock(&key_serial_lock);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: fix refcount_inc() on zero
2017-05-26 17:37 [PATCH] KEYS: fix refcount_inc() on zero Mark Rutland
@ 2017-05-26 19:05 ` Kees Cook
2017-05-31 12:20 ` David Howells
1 sibling, 0 replies; 4+ messages in thread
From: Kees Cook @ 2017-05-26 19:05 UTC (permalink / raw)
To: Mark Rutland
Cc: David Howells, keyrings, linux-security-module, LKML,
David Windsor, Elena Reshetova, Hans Liljestrand, James Morris,
Peter Zijlstra
On Fri, May 26, 2017 at 10:37 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> If a key's refcount is dropped to zero between key_lookup() peeking at
> the refcount and subsequently attempting to increment it, refcount_inc()
> will see a zero refcount. Here, refcount_inc() will WARN_ONCE(), and
> will *not* increment the refcount, which will remain zero.
>
> Once key_lookup() drops key_serial_lock, it is possible for the key to
> be freed behind our back.
>
> This patch uses refcount_inc_not_zero() to perform the peek and
> increment atomically. A helper with lockdep annotation is added to
> document why this is safe.
>
> Fixes: fff292914d3a2f1e ("security, keys: convert key.usage from atomic_t to refcount_t")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: David Windsor <dwindsor@gmail.com>
> Cc: Elena Reshetova <elena.reshetova@intel.com>
> Cc: Hans Liljestrand <ishkamiel@gmail.com>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
Thanks for catching this!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> security/keys/key.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 455c04d..150f51d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -632,6 +632,12 @@ void key_put(struct key *key)
> }
> EXPORT_SYMBOL(key_put);
>
> +static bool key_get_not_free(struct key *key)
> +{
> + lockdep_assert_held(&key_serial_lock);
> + return refcount_inc_not_zero(&key->usage);
> +}
> +
> /*
> * Find a key by its serial number.
> */
> @@ -660,14 +666,12 @@ struct key *key_lookup(key_serial_t id)
> goto error;
>
> found:
> - /* pretend it doesn't exist if it is awaiting deletion */
> - if (refcount_read(&key->usage) == 0)
> - goto not_found;
> -
> - /* this races with key_put(), but that doesn't matter since key_put()
> - * doesn't actually change the key
> + /*
> + * Pretend it doesn't exist if it is awaiting deletion. This races with
> + * key_put(), but we can peek at the key until we drop key_serial_lock.
> */
> - __key_get(key);
> + if (!key_get_not_free(key))
> + goto not_found;
>
> error:
> spin_unlock(&key_serial_lock);
> --
> 1.9.1
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: fix refcount_inc() on zero
2017-05-26 17:37 [PATCH] KEYS: fix refcount_inc() on zero Mark Rutland
2017-05-26 19:05 ` Kees Cook
@ 2017-05-31 12:20 ` David Howells
2017-05-31 12:39 ` Mark Rutland
1 sibling, 1 reply; 4+ messages in thread
From: David Howells @ 2017-05-31 12:20 UTC (permalink / raw)
To: Mark Rutland
Cc: dhowells, keyrings, linux-security-module, linux-kernel,
David Windsor, Elena Reshetova, Hans Liljestrand, James Morris,
Kees Cook, Peter Zijlstra
Mark Rutland <mark.rutland@arm.com> wrote:
> This patch uses refcount_inc_not_zero() to perform the peek and
> increment atomically.
I think the helper is unnecessary. Better to adjust the comment if you really
want to explain it. Anyone editing the code should be that this is inside a
critical section.
> A helper with lockdep annotation is added to document why this is safe.
This doesn't explain why this is safe.
> + /*
> + * Pretend it doesn't exist if it is awaiting deletion. This races with
> + * key_put(), but we can peek at the key until we drop key_serial_lock.
> */
With your change, there is no race with key_put() - so the second sentence is
unnecessary.
I've adjusted your patch - see attached.
David
---
commit f66bf831c45306ebbc28aecd407e238983457251
Author: Mark Rutland <mark.rutland@arm.com>
Date: Fri May 26 18:37:34 2017 +0100
KEYS: fix refcount_inc() on zero
If a key's refcount is dropped to zero between key_lookup() peeking at
the refcount and subsequently attempting to increment it, refcount_inc()
will see a zero refcount. Here, refcount_inc() will WARN_ONCE(), and
will *not* increment the refcount, which will remain zero.
Once key_lookup() drops key_serial_lock, it is possible for the key to
be freed behind our back.
This patch uses refcount_inc_not_zero() to perform the peek and increment
atomically.
Fixes: fff292914d3a2f1e ("security, keys: convert key.usage from atomic_t to refcount_t")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Elena Reshetova <elena.reshetova@intel.com>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d80bbb..d84ee2a87da6 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -660,14 +660,11 @@ struct key *key_lookup(key_serial_t id)
goto error;
found:
- /* pretend it doesn't exist if it is awaiting deletion */
- if (refcount_read(&key->usage) == 0)
- goto not_found;
-
- /* this races with key_put(), but that doesn't matter since key_put()
- * doesn't actually change the key
+ /* A key is allowed to be looked up only if someone still owns a
+ * reference to it - otherwise it's awaiting the gc.
*/
- __key_get(key);
+ if (!refcount_inc_not_zero(&key->usage))
+ goto not_found;
error:
spin_unlock(&key_serial_lock);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: fix refcount_inc() on zero
2017-05-31 12:20 ` David Howells
@ 2017-05-31 12:39 ` Mark Rutland
0 siblings, 0 replies; 4+ messages in thread
From: Mark Rutland @ 2017-05-31 12:39 UTC (permalink / raw)
To: David Howells
Cc: keyrings, linux-security-module, linux-kernel, David Windsor,
Elena Reshetova, Hans Liljestrand, James Morris, Kees Cook,
Peter Zijlstra
On Wed, May 31, 2017 at 01:20:44PM +0100, David Howells wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > This patch uses refcount_inc_not_zero() to perform the peek and
> > increment atomically.
>
> I think the helper is unnecessary. Better to adjust the comment if you really
> want to explain it. Anyone editing the code should be that this is inside a
> critical section.
>
> > A helper with lockdep annotation is added to document why this is safe.
>
> This doesn't explain why this is safe.
>
> > + /*
> > + * Pretend it doesn't exist if it is awaiting deletion. This races with
> > + * key_put(), but we can peek at the key until we drop key_serial_lock.
> > */
>
> With your change, there is no race with key_put() - so the second sentence is
> unnecessary.
Fair enough, on all counts.
> I've adjusted your patch - see attached.
That looks fine to me, thanks!
Mark.
>
> David
> ---
> commit f66bf831c45306ebbc28aecd407e238983457251
> Author: Mark Rutland <mark.rutland@arm.com>
> Date: Fri May 26 18:37:34 2017 +0100
>
> KEYS: fix refcount_inc() on zero
>
> If a key's refcount is dropped to zero between key_lookup() peeking at
> the refcount and subsequently attempting to increment it, refcount_inc()
> will see a zero refcount. Here, refcount_inc() will WARN_ONCE(), and
> will *not* increment the refcount, which will remain zero.
>
> Once key_lookup() drops key_serial_lock, it is possible for the key to
> be freed behind our back.
>
> This patch uses refcount_inc_not_zero() to perform the peek and increment
> atomically.
>
> Fixes: fff292914d3a2f1e ("security, keys: convert key.usage from atomic_t to refcount_t")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Cc: David Windsor <dwindsor@gmail.com>
> Cc: Elena Reshetova <elena.reshetova@intel.com>
> Cc: Hans Liljestrand <ishkamiel@gmail.com>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
>
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 455c04d80bbb..d84ee2a87da6 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -660,14 +660,11 @@ struct key *key_lookup(key_serial_t id)
> goto error;
>
> found:
> - /* pretend it doesn't exist if it is awaiting deletion */
> - if (refcount_read(&key->usage) == 0)
> - goto not_found;
> -
> - /* this races with key_put(), but that doesn't matter since key_put()
> - * doesn't actually change the key
> + /* A key is allowed to be looked up only if someone still owns a
> + * reference to it - otherwise it's awaiting the gc.
> */
> - __key_get(key);
> + if (!refcount_inc_not_zero(&key->usage))
> + goto not_found;
>
> error:
> spin_unlock(&key_serial_lock);
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-31 12:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-26 17:37 [PATCH] KEYS: fix refcount_inc() on zero Mark Rutland
2017-05-26 19:05 ` Kees Cook
2017-05-31 12:20 ` David Howells
2017-05-31 12:39 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).