[RFC][PATCH 00/15] KEYS: Fixes

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Hi Eric,

Are you okay with my changes/substitution of your key instantiation-state
patches?

David
---
Here's a collection of fixes for Linux keyrings, mostly thanks to Eric
Biggers, including:

 (1) Fix a bunch of places where kernel drivers may access revoked user-type
     keys and don't do it correctly.

 (2) Fix some ecryptfs bits.

 (3) Fix big_key to require CONFIG_CRYPTO.

 (4) Fix a couple of bugs in the asymmetric key type.

 (5) Fix a race between updating and finding negative keys.

 (6) Prevent add_key() from updating uninstantiated keys.

 (7) Make loading of key flags and expiry time atomic when not holding locks.

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes

David
---
Arnd Bergmann (1):
      security/keys: BIG_KEY requires CONFIG_CRYPTO

David Howells (2):
      KEYS: Fix race between updating and finding a negative key
      KEYS: don't let add_key() update an uninstantiated key

Eric Biggers (10):
      KEYS: encrypted: fix dereference of NULL user_key_payload
      FS-Cache: fix dereference of NULL user_key_payload
      lib/digsig: fix dereference of NULL user_key_payload
      fscrypt: fix dereference of NULL user_key_payload
      ecryptfs: fix dereference of NULL user_key_payload
      ecryptfs: fix out-of-bounds read of key payload
      ecryptfs: move key payload accessor functions into keystore.c
      KEYS: load key flags and expiry time atomically in key_validate()
      KEYS: load key flags and expiry time atomically in keyring_search_iterator()
      KEYS: load key flags and expiry time atomically in proc_keys_show()

Lee, Chun-Yi (2):
      KEYS: Fix the wrong index when checking the existence of second id
      KEYS: checking the input id parameters before finding asymmetric key


 crypto/asymmetric_keys/asymmetric_type.c |    4 +-
 fs/crypto/keyinfo.c                      |    5 ++
 fs/ecryptfs/ecryptfs_kernel.h            |   44 ------------------
 fs/ecryptfs/keystore.c                   |   73 ++++++++++++++++++++++++++++++
 fs/fscache/object-list.c                 |    7 +++
 include/linux/key.h                      |   37 +++++++++------
 lib/digsig.c                             |    6 ++
 security/keys/Kconfig                    |    1 
 security/keys/encrypted-keys/encrypted.c |    9 +++-
 security/keys/gc.c                       |    7 ++-
 security/keys/key.c                      |   37 +++++++++++----
 security/keys/keyctl.c                   |    9 ++--
 security/keys/keyring.c                  |   12 +++--
 security/keys/permission.c               |    7 ++-
 security/keys/proc.c                     |   35 +++++++++-----
 security/keys/process_keys.c             |    8 ++-
 security/keys/request_key.c              |    7 +--
 security/keys/trusted.c                  |    2 -
 security/keys/user_defined.c             |    2 -
 19 files changed, 201 insertions(+), 111 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Sorry, I didn't manage to include the patches because stgit doesn't consider:

    Cc: <stable@vger.kernel.org>    [v2.6.38+]

to be a valid email :-/

submitting-patches.rst and stable-kernel-rules.rst state this:

    Cc: stable at vger.kernel.org

or this:

    Cc: <stable@vger.kernel.org> # 3.3.x

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[Eric Biggers]

Hi David, a few comments:

On Thu, Oct 12, 2017 at 05:17:27PM +0100, David Howells wrote:
> ---
> Arnd Bergmann (1):
>       security/keys: BIG_KEY requires CONFIG_CRYPTO

Doesn't this need 'Cc: <stable@vger.kernel.org>	[v4.9+]',
since the commit it fixes is there?

> 
> David Howells (2):
>       KEYS: Fix race between updating and finding a negative key

I guess the 'state' variable is fine, though it makes this patch (which will
need to be backported) a tad larger.  I'd also prefer a bit more information in
the commit message about the problem being solved.  But anyway, there are also
some problems in how READ_ONCE() and memory barriers are used (or not used) in
the new/changed code, and there is one other bug:

>  static inline bool key_is_instantiated(const struct key *key)
>  {
> -       return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
> -               !test_bit(KEY_FLAG_NEGATIVE, &key->flags);
> +       return key->state == KEY_IS_INSTANTIATED;
> +}

This should use 'smp_load_acquire(&key->state) == KEY_IS_INSTANTIATED', since
some ->describe() methods expect to access the payload after this.  Yes, it's no
worse than before, but as long as the line of code is being replaced we might as
well get it right...

> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 87cb260e4890..1578f671a213 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -129,14 +129,15 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>        while (!list_empty(keys)) {
>                struct key *key =
>                        list_entry(keys->next, struct key, graveyard_link);
> +              short state = READ_ONCE(key->state);
> +

Here the key has no more references, so nothing can be changing the state.
Thus, the READ_ONCE() isn't actually needed.

> +/*
> + * Change the key state to being instantiated.
> + */
> +static void mark_key_instantiated(struct key *key, int reject_error)
> +{
> +       smp_wmb(); /* Commit the payload before setting the state */
> +       key->state = (reject_error < 0) ? reject_error : KEY_IS_INSTANTIATED;
> +}
> +

smp_store_release() would make this simpler as well as guarantee that the write
is atomic.

> +       ret = key->state;
> +       if (ret < 0)
> +               goto error2; /* Negatively instantiated */

Not too important in practice (as this is constantly gotten wrong all over the
kernel, and compilers play nice enough to make it not a huge deal), but
READ_ONCE(key->state) will guarantee that the read of 'key->state' is atomic and
not e.g. done byte-by-byte.


> -       if (key->type->describe)
> +       if (key->type->describe) {
> +               smp_rmb(); /* Order access to payload after state set. */
>		  key->type->describe(key, m);
> +       }

This is the wrong place for this memory barrier.  The state is checked
separately in ->describe() and it may have changed between when it was shown in
proc_keys_show() vs. when it is checked in ->describe().  We can't actually make
these two access to ->state consistent with respect to each other right now.
The most we can do is use smp_load_acquire() in key_is_instantiated() so that at
least ->describe() isn't broken by itself.  So the change here is pointless.

> -       if (!(lflags & KEY_LOOKUP_PARTIAL) &&
> -           !test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
> -               goto invalid_key;
> +       if (!(lflags & KEY_LOOKUP_PARTIAL)) {
> +               if (key->state != KEY_IS_INSTANTIATED)
> +                       goto invalid_key;
> +               smp_rmb(); /* Order access to payload after state set. */
> +       }

This should be simply:

	if (!(lflags & KEY_LOOKUP_PARTIAL) && !key_is_instantiated(key))
		goto invalid_key;

> @@ -595,10 +595,9 @@ int wait_for_key_construction(struct key *key, bool intr)
>                          intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
>        if (ret)
>                return -ERESTARTSYS;
> -       if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
> -               smp_rmb();
> -               return key->reject_error;
> -       }
> +       ret = READ_ONCE(key->state);
> +       if (ret < 0)
> +               return ret;
>        return key_validate(key);
> }
> EXPORT_SYMBOL(wait_for_key_construction);

smp_load_acquire() rather than READ_ONCE(), in case the caller uses this as an
indication that it is safe to access the payload.

> diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
> index 3d8c68eba516..9afa64817d4f 100644
> --- a/security/keys/user_defined.c
> +++ b/security/keys/user_defined.c
> @@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
> 
>        /* attach the new data, displacing the old */
>        key->expiry = prep->expiry;
>-       if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
>+       if (key->state < 0)
>                zap = dereference_key_locked(key);
>        rcu_assign_keypointer(key, prep->payload.data[0]);
>        prep->payload.data[0] = NULL;

This is backwards; it should be 'if (!key_is_negative(key))'.

>       KEYS: don't let add_key() update an uninstantiated key
> 

>
> +       if (test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags)) {
> +               ret = wait_for_key_construction(key_ref_to_ptr(key_ref), true);
> +               if (ret < 0) {
> +                       key_ref_put(key_ref);
> +                       key_ref = ERR_PTR(ret);
> +                       goto error_free_prep;
> +               }
> +       }
> +

'key' is NULL here.  It should be 'key_ref_to_ptr(key_ref)'.


>       KEYS: load key flags and expiry time atomically in keyring_search_iterator()

The commit message refers to both flags and expiry time, but now it only
actually updates how the expiry time is loaded, as the flags were already done
by "KEYS: Fix race between updating and finding a negative key".

>       KEYS: load key flags and expiry time atomically in proc_keys_show()

The commit message refers to negative ('N') but not instantiated ('I') as the
example, but that is no longer a valid example since this patch comes after
"KEYS: Fix race between updating and finding a negative key".

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Hi Eric,

Are you okay with my changes/substitution of your key instantiation-state
patches?

David
---
Here's a collection of fixes for Linux keyrings, mostly thanks to Eric
Biggers, including:

 (1) Fix a bunch of places where kernel drivers may access revoked user-type
     keys and don't do it correctly.

 (2) Fix some ecryptfs bits.

 (3) Fix big_key to require CONFIG_CRYPTO.

 (4) Fix a couple of bugs in the asymmetric key type.

 (5) Fix a race between updating and finding negative keys.

 (6) Prevent add_key() from updating uninstantiated keys.

 (7) Make loading of key flags and expiry time atomic when not holding locks.

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes

David
---
Arnd Bergmann (1):
      security/keys: BIG_KEY requires CONFIG_CRYPTO

David Howells (2):
      KEYS: Fix race between updating and finding a negative key
      KEYS: don't let add_key() update an uninstantiated key

Eric Biggers (10):
      KEYS: encrypted: fix dereference of NULL user_key_payload
      FS-Cache: fix dereference of NULL user_key_payload
      lib/digsig: fix dereference of NULL user_key_payload
      fscrypt: fix dereference of NULL user_key_payload
      ecryptfs: fix dereference of NULL user_key_payload
      ecryptfs: fix out-of-bounds read of key payload
      ecryptfs: move key payload accessor functions into keystore.c
      KEYS: load key flags and expiry time atomically in key_validate()
      KEYS: load key flags and expiry time atomically in keyring_search_iterator()
      KEYS: load key flags and expiry time atomically in proc_keys_show()

Lee, Chun-Yi (2):
      KEYS: Fix the wrong index when checking the existence of second id
      KEYS: checking the input id parameters before finding asymmetric key


 crypto/asymmetric_keys/asymmetric_type.c |    4 +-
 fs/crypto/keyinfo.c                      |    5 ++
 fs/ecryptfs/ecryptfs_kernel.h            |   44 ------------------
 fs/ecryptfs/keystore.c                   |   73 ++++++++++++++++++++++++++++++
 fs/fscache/object-list.c                 |    7 +++
 include/linux/key.h                      |   37 +++++++++------
 lib/digsig.c                             |    6 ++
 security/keys/Kconfig                    |    1 
 security/keys/encrypted-keys/encrypted.c |    9 +++-
 security/keys/gc.c                       |    7 ++-
 security/keys/key.c                      |   37 +++++++++++----
 security/keys/keyctl.c                   |    9 ++--
 security/keys/keyring.c                  |   12 +++--
 security/keys/permission.c               |    7 ++-
 security/keys/proc.c                     |   35 +++++++++-----
 security/keys/process_keys.c             |    8 ++-
 security/keys/request_key.c              |    7 +--
 security/keys/trusted.c                  |    2 -
 security/keys/user_defined.c             |    2 -
 19 files changed, 201 insertions(+), 111 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> >  static inline bool key_is_instantiated(const struct key *key)
> ...
> This should use 'smp_load_acquire(&key->state) == KEY_IS_INSTANTIATED',

I made a key_read_state() wrapper function that does this and then used it in
a bunch of places that read it, including this one.

> > +              short state = READ_ONCE(key->state);
> 
> Here the key has no more references, so nothing can be changing the state.
> Thus, the READ_ONCE() isn't actually needed.

Changed.

> > +static void mark_key_instantiated(struct key *key, int reject_error)
> > +{
> > +       smp_wmb(); /* Commit the payload before setting the state */
> > +       key->state = (reject_error < 0) ? reject_error : KEY_IS_INSTANTIATED;
> > +}
> > +
> 
> smp_store_release() would make this simpler as well as guarantee that the
> write is atomic.

Changed.

> > +       ret = key->state;
> > +       if (ret < 0)
> > +               goto error2; /* Negatively instantiated */
> 
> Not too important in practice (as this is constantly gotten wrong all over
> the kernel, and compilers play nice enough to make it not a huge deal), but
> READ_ONCE(key->state) will guarantee that the read of 'key->state' is atomic
> and not e.g. done byte-by-byte.

Changed to use key_read_state().

> > -       if (key->type->describe)
> > +       if (key->type->describe) {
> > +               smp_rmb(); /* Order access to payload after state set. */
> >		  key->type->describe(key, m);
> > +       }
> 
> This is the wrong place for this memory barrier.  The state is checked
> separately in ->describe() and it may have changed between when it was shown
> in proc_keys_show() vs. when it is checked in ->describe().  We can't
> actually make these two access to ->state consistent with respect to each
> other right now.  The most we can do is use smp_load_acquire() in
> key_is_instantiated() so that at least ->describe() isn't broken by itself.
> So the change here is pointless.

Feasibly.  I've changed the preceding:

	state = READ_ONCE(key->state);

into:

	state = key_read_state(key);

and got rid of smp_rmb() anyway, but you're right, ->describe() needs to
recheck the state if it might want to touch it.

I wonder if I should pass the state into ->describe()?

Possibly I shouldn't be calling ->describe() if the key isn't instantiated.

> > +       if (!(lflags & KEY_LOOKUP_PARTIAL)) {
> > +               if (key->state != KEY_IS_INSTANTIATED)
> > +                       goto invalid_key;
> > +               smp_rmb(); /* Order access to payload after state set. */
> > +       }
> 
> This should be simply:
> 
> 	if (!(lflags & KEY_LOOKUP_PARTIAL) && !key_is_instantiated(key))
> 		goto invalid_key;

Changed.

> > +       ret = READ_ONCE(key->state);
> > +       if (ret < 0)
> > +               return ret;
> >        return key_validate(key);
> > }
> > EXPORT_SYMBOL(wait_for_key_construction);
> 
> smp_load_acquire() rather than READ_ONCE(), in case the caller uses this as an
> indication that it is safe to access the payload.

Changed to use key_read_state().

> >-       if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
> >+       if (key->state < 0)
> >                zap = dereference_key_locked(key);
> >        rcu_assign_keypointer(key, prep->payload.data[0]);
> >        prep->payload.data[0] = NULL;
> 
> This is backwards; it should be 'if (!key_is_negative(key))'.

Actually, I think it should be 'if (key_is_instantiated())'

> > +       if (test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags)) {
> ...
> 'key' is NULL here.  It should be 'key_ref_to_ptr(key_ref)'.

Fixed.

David
---
commit 21fe21748b709710fbc78536a6d68bb4df8297ad
Author: David Howells <dhowells@redhat.com>
Date:   Wed Oct 4 16:43:25 2017 +0100

    KEYS: Fix race between updating and finding a negative key
    
    Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
    error into one field such that:
    
     (1) The instantiation state can be modified/read atomically.
    
     (2) The error can be accessed atomically with the state.
    
     (3) The error isn't stored unioned with the payload pointers.
    
    This deals with the problem that the state is spread over three different
    objects (two bits and a separate variable) and reading or updating them
    atomically isn't practical, given that not only can uninstantiated keys
    change into instantiated or rejected keys, but rejected keys can also turn
    into instantiated keys - and someone accessing the key might not be using
    any locking.
    
    The main side effect of this problem is that what was held in the payload
    may change, depending on the state.  For instance, you might observe the
    key to be in the rejected state.  You then read the cached error, but if
    the key semaphore wasn't locked, the key might've become instantiated
    between the two reads - and you might now have something in hand that isn't
    actually an error code.
    
    Additionally, barriering is included:
    
     (1) Order payload-set before state-set during instantiation.
    
     (2) Order state-read before payload-read when using the key.
    
    Further separate barriering is necessary if RCU is being used to access the
    payload content after reading the payload pointers.
    
    Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
    Cc: <stable@vger.kernel.org>    [v4.4+]
    Reported-by: Eric Biggers <ebiggers@google.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..eae10f302b44 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -138,6 +138,11 @@ struct key_restriction {
 	struct key_type *keytype;
 };
 
+enum key_state {
+	KEY_IS_UNINSTANTIATED,
+	KEY_IS_INSTANTIATED,
+};
+
 /*****************************************************************************/
 /*
  * authentication token / access credential / keyring
@@ -169,6 +174,7 @@ struct key {
 						 * - may not match RCU dereferenced payload
 						 * - payload should contain own length
 						 */
+	short			state;		/* Key state (+) or rejection error (-) */
 
 #ifdef KEY_DEBUGGING
 	unsigned		magic;
@@ -176,18 +182,16 @@ struct key {
 #endif
 
 	unsigned long		flags;		/* status flags (change with bitops) */
-#define KEY_FLAG_INSTANTIATED	0	/* set if key has been instantiated */
-#define KEY_FLAG_DEAD		1	/* set if key type has been deleted */
-#define KEY_FLAG_REVOKED	2	/* set if key had been revoked */
-#define KEY_FLAG_IN_QUOTA	3	/* set if key consumes quota */
-#define KEY_FLAG_USER_CONSTRUCT	4	/* set if key is being constructed in userspace */
-#define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
-#define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
-#define KEY_FLAG_BUILTIN	8	/* set if key is built in to the kernel */
-#define KEY_FLAG_ROOT_CAN_INVAL	9	/* set if key can be invalidated by root without permission */
-#define KEY_FLAG_KEEP		10	/* set if key should not be removed */
-#define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
+#define KEY_FLAG_DEAD		0	/* set if key type has been deleted */
+#define KEY_FLAG_REVOKED	1	/* set if key had been revoked */
+#define KEY_FLAG_IN_QUOTA	2	/* set if key consumes quota */
+#define KEY_FLAG_USER_CONSTRUCT	3	/* set if key is being constructed in userspace */
+#define KEY_FLAG_ROOT_CAN_CLEAR	4	/* set if key can be cleared by root without permission */
+#define KEY_FLAG_INVALIDATED	5	/* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN	6	/* set if key is built in to the kernel */
+#define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
+#define KEY_FLAG_KEEP		8	/* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
@@ -213,7 +217,6 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
@@ -353,6 +356,12 @@ extern void key_set_timeout(struct key *, unsigned);
 #define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
 #define	KEY_NEED_ALL	0x3f	/* All the above permissions */
 
+static inline short key_read_state(const struct key *key)
+{
+	/* Barrier versus mark_key_instantiated(). */
+	return smp_load_acquire(&key->state);
+}
+
 /**
  * key_is_instantiated - Determine if a key has been positively instantiated
  * @key: The key to check.
@@ -362,8 +371,12 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	return key_read_state(key) == KEY_IS_INSTANTIATED;
+}
+
+static inline bool key_is_negative(const struct key *key)
+{
+	return key_read_state(key) < 0;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 535db141f4da..d92cbf9687c3 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -854,7 +854,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	size_t datalen = prep->datalen;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 87cb260e4890..e7aeecbf7f19 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -129,14 +129,15 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 	while (!list_empty(keys)) {
 		struct key *key =
 			list_entry(keys->next, struct key, graveyard_link);
+		short state = key->state;
+
 		list_del(&key->graveyard_link);
 
 		kdebug("- %u", key->serial);
 		key_check(key);
 
 		/* Throw away the key data if the key is instantiated */
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		    !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
+		if (state == KEY_IS_INSTANTIATED &&
 		    key->type->destroy)
 			key->type->destroy(key);
 
@@ -151,7 +152,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		}
 
 		atomic_dec(&key->user->nkeys);
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
+		if (state == KEY_IS_INSTANTIATED)
 			atomic_dec(&key->user->nikeys);
 
 		key_user_put(key->user);
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..cdd8fea8ddc4 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -402,6 +402,18 @@ int key_payload_reserve(struct key *key, size_t datalen)
 EXPORT_SYMBOL(key_payload_reserve);
 
 /*
+ * Change the key state to being instantiated.
+ */
+static void mark_key_instantiated(struct key *key, int reject_error)
+{
+	/* Commit the payload before setting the state; barrier versus
+	 * key_read_state().
+	 */
+	smp_store_release(&key->state,
+			  (reject_error < 0) ? reject_error : KEY_IS_INSTANTIATED);
+}
+
+/*
  * Instantiate a key and link it into the target keyring atomically.  Must be
  * called with the target keyring's semaphore writelocked.  The target key's
  * semaphore need not be locked as instantiation is serialised by
@@ -424,14 +436,14 @@ static int __key_instantiate_and_link(struct key *key,
 	mutex_lock(&key_construction_mutex);
 
 	/* can't instantiate twice */
-	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* instantiate the key */
 		ret = key->type->instantiate(key, prep);
 
 		if (ret == 0) {
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
-			set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+			mark_key_instantiated(key, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
 				awaken = 1;
@@ -577,13 +589,10 @@ int key_reject_and_link(struct key *key,
 	mutex_lock(&key_construction_mutex);
 
 	/* can't instantiate twice */
-	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
-		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
-		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+		mark_key_instantiated(key, -error);
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;
 		key_schedule_gc(key->expiry + key_gc_delay);
@@ -752,8 +761,8 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 
 	ret = key->type->update(key, prep);
 	if (ret == 0)
-		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		/* Updating a negative key positively instantiates it */
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -986,8 +995,8 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 
 	ret = key->type->update(key, &prep);
 	if (ret == 0)
-		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		/* Updating a negative key positively instantiates it */
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..698309a8502e 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -766,10 +766,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		ret = -ENOKEY;
-		goto error2;
-	}
+	ret = key_read_state(key);
+	if (ret < 0)
+		goto error2; /* Negatively instantiated */
 
 	/* see if we can read it directly */
 	ret = key_permission(key_ref, KEY_NEED_READ);
@@ -901,7 +900,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 		atomic_dec(&key->user->nkeys);
 		atomic_inc(&newowner->nkeys);
 
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+		if (key->state == KEY_IS_INSTANTIATED) {
 			atomic_dec(&key->user->nikeys);
 			atomic_inc(&newowner->nikeys);
 		}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..816948abff89 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -553,7 +553,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 {
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
-	unsigned long kflags = key->flags;
+	unsigned long kflags = READ_ONCE(key->flags);
+	short state = READ_ONCE(key->state);
 
 	kenter("{%d}", key->serial);
 
@@ -597,9 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
-		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
-			smp_rmb();
-			ctx->result = ERR_PTR(key->reject_error);
+		if (state < 0) {
+			ctx->result = ERR_PTR(state);
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/proc.c b/security/keys/proc.c
index de834309d100..9510822c4d96 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	unsigned long timo;
 	key_ref_t key_ref, skey_ref;
 	char xbuf[16];
+	short state;
 	int rc;
 
 	struct keyring_search_context ctx = {
@@ -236,17 +237,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
+	state = key_read_state(key);
+
 #define showflag(KEY, LETTER, FLAG) \
 	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
 
 	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
 		   key->serial,
-		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
+		   state == KEY_IS_INSTANTIATED ? 'I' : '-',
 		   showflag(key, 'R', KEY_FLAG_REVOKED),
 		   showflag(key, 'D', KEY_FLAG_DEAD),
 		   showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
 		   showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(key, 'N', KEY_FLAG_NEGATIVE),
+		   state < 0 ? 'N' : '-',
 		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 293d3598153b..5a8b985d1d5f 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -729,8 +729,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 	}
 
 	ret = -EIO;
-	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
-	    !test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
+	if (!(lflags & KEY_LOOKUP_PARTIAL) && !key_is_instantiated(key))
 		goto invalid_key;
 
 	/* check the permissions */
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..e8036cd0ad54 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -595,10 +595,9 @@ int wait_for_key_construction(struct key *key, bool intr)
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+	ret = key_read_state(key);
+	if (ret < 0)
+		return ret;
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	char *datablob;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	p = key->payload.data[0];
 	if (!p->migratable)
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 3d8c68eba516..d993d030a644 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
 	/* attach the new data, displacing the old */
 	key->expiry = prep->expiry;
-	if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_instantiated(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[Eric Biggers]

On Fri, Oct 13, 2017 at 04:39:28PM +0100, David Howells wrote:
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 87cb260e4890..e7aeecbf7f19 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -129,14 +129,15 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>  	while (!list_empty(keys)) {
>  		struct key *key =
>  			list_entry(keys->next, struct key, graveyard_link);
> +		short state = key->state;
> +
>  		list_del(&key->graveyard_link);
>  
>  		kdebug("- %u", key->serial);
>  		key_check(key);
>  
>  		/* Throw away the key data if the key is instantiated */
> -		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
> -		    !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
> +		if (state == KEY_IS_INSTANTIATED &&
>  		    key->type->destroy)
>  			key->type->destroy(key);

Nit: put the two checks on the same line.

>  
> @@ -151,7 +152,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>  		}
>  
>  		atomic_dec(&key->user->nkeys);
> -		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
> +		if (state == KEY_IS_INSTANTIATED)
>  			atomic_dec(&key->user->nikeys);

This changes the behavior.  Previously ->nikeys counted both negatively and
positively instantiated keys, while this change implies that it now will only
count positively instantiated keys.  I think you meant 'state !=
KEY_IS_UNINSTANTIATED'?  Renaming KEY_IS_INSTANTIATED to KEY_IS_POSITIVE or
KEY_IS_POSITIVELY_INSTANTIATED also might help reduce this confusion.

> @@ -901,7 +900,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  		atomic_dec(&key->user->nkeys);
>  		atomic_inc(&newowner->nkeys);
>  
> -		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
> +		if (key->state == KEY_IS_INSTANTIATED) {
>  			atomic_dec(&key->user->nikeys);
>  			atomic_inc(&newowner->nikeys);
>  		}

Same problem: ->nikeys was previously counting negatively instantiated keys too.
Now it isn't.  Shouldn't it test 'key->state != KEY_IS_UNINSTANTIATED'?

> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index de834309d100..9510822c4d96 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
>  	unsigned long timo;
>  	key_ref_t key_ref, skey_ref;
>  	char xbuf[16];
> +	short state;
>  	int rc;
>  
>  	struct keyring_search_context ctx = {
> @@ -236,17 +237,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
>  			sprintf(xbuf, "%luw", timo / (60*60*24*7));
>  	}
>  
> +	state = key_read_state(key);
> +
>  #define showflag(KEY, LETTER, FLAG) \
>  	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
>  
>  	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
>  		   key->serial,
> -		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
> +		   state == KEY_IS_INSTANTIATED ? 'I' : '-',

Similar problem.  Previously 'I' was shown for negatively instantiated keys; now
it's not.  Shouldn't it test 'state != KEY_IS_UNINSTANTIATED'?

> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 293d3598153b..5a8b985d1d5f 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -729,8 +729,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
>  	}
>  
>  	ret = -EIO;
> -	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
> -	    !test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
> +	if (!(lflags & KEY_LOOKUP_PARTIAL) && !key_is_instantiated(key))
>  		goto invalid_key;

Similar problem again.  Previously this allowed negatively instantiated keys
through whereas now it only allows positively instantiated keys.  Is that
intentional?

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> > -		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
> > +		if (state == KEY_IS_INSTANTIATED)
> >  			atomic_dec(&key->user->nikeys);
> 
> This changes the behavior.  Previously ->nikeys counted both negatively and
> positively instantiated keys, while this change implies that it now will only
> count positively instantiated keys.  I think you meant 'state !=
> KEY_IS_UNINSTANTIATED'?  Renaming KEY_IS_INSTANTIATED to KEY_IS_POSITIVE or
> KEY_IS_POSITIVELY_INSTANTIATED also might help reduce this confusion.

Yeah - I think I'm confusing myself by overloading 'instantiated' in my mind.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[David Howells]

Okay, I've fixed those issues, I think.  I've renamed the instantiation labels
to positive.

Thanks,
David
---
commit f23f3bb0ba3be44e775ac74148157136b919e3b0
Author: David Howells <dhowells@redhat.com>
Date:   Wed Oct 4 16:43:25 2017 +0100

    KEYS: Fix race between updating and finding a negative key
    
    Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
    error into one field such that:
    
     (1) The instantiation state can be modified/read atomically.
    
     (2) The error can be accessed atomically with the state.
    
     (3) The error isn't stored unioned with the payload pointers.
    
    This deals with the problem that the state is spread over three different
    objects (two bits and a separate variable) and reading or updating them
    atomically isn't practical, given that not only can uninstantiated keys
    change into instantiated or rejected keys, but rejected keys can also turn
    into instantiated keys - and someone accessing the key might not be using
    any locking.
    
    The main side effect of this problem is that what was held in the payload
    may change, depending on the state.  For instance, you might observe the
    key to be in the rejected state.  You then read the cached error, but if
    the key semaphore wasn't locked, the key might've become instantiated
    between the two reads - and you might now have something in hand that isn't
    actually an error code.
    
    The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error
    code if the key is negatively instantiated.  The key_is_instantiated()
    function is replaced with key_is_positive() to avoid confusion as negative
    keys are also 'instantiated'.
    
    Additionally, barriering is included:
    
     (1) Order payload-set before state-set during instantiation.
    
     (2) Order state-read before payload-read when using the key.
    
    Further separate barriering is necessary if RCU is being used to access the
    payload content after reading the payload pointers.
    
    Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
    Cc: stable at vger.kernel.org # v4.4+
    Reported-by: Eric Biggers <ebiggers@google.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..8a15cabe928d 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -138,6 +138,11 @@ struct key_restriction {
 	struct key_type *keytype;
 };
 
+enum key_state {
+	KEY_IS_UNINSTANTIATED,
+	KEY_IS_POSITIVE,		/* Positively instantiated */
+};
+
 /*****************************************************************************/
 /*
  * authentication token / access credential / keyring
@@ -169,6 +174,7 @@ struct key {
 						 * - may not match RCU dereferenced payload
 						 * - payload should contain own length
 						 */
+	short			state;		/* Key state (+) or rejection error (-) */
 
 #ifdef KEY_DEBUGGING
 	unsigned		magic;
@@ -176,18 +182,16 @@ struct key {
 #endif
 
 	unsigned long		flags;		/* status flags (change with bitops) */
-#define KEY_FLAG_INSTANTIATED	0	/* set if key has been instantiated */
-#define KEY_FLAG_DEAD		1	/* set if key type has been deleted */
-#define KEY_FLAG_REVOKED	2	/* set if key had been revoked */
-#define KEY_FLAG_IN_QUOTA	3	/* set if key consumes quota */
-#define KEY_FLAG_USER_CONSTRUCT	4	/* set if key is being constructed in userspace */
-#define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
-#define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
-#define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
-#define KEY_FLAG_BUILTIN	8	/* set if key is built in to the kernel */
-#define KEY_FLAG_ROOT_CAN_INVAL	9	/* set if key can be invalidated by root without permission */
-#define KEY_FLAG_KEEP		10	/* set if key should not be removed */
-#define KEY_FLAG_UID_KEYRING	11	/* set if key is a user or user session keyring */
+#define KEY_FLAG_DEAD		0	/* set if key type has been deleted */
+#define KEY_FLAG_REVOKED	1	/* set if key had been revoked */
+#define KEY_FLAG_IN_QUOTA	2	/* set if key consumes quota */
+#define KEY_FLAG_USER_CONSTRUCT	3	/* set if key is being constructed in userspace */
+#define KEY_FLAG_ROOT_CAN_CLEAR	4	/* set if key can be cleared by root without permission */
+#define KEY_FLAG_INVALIDATED	5	/* set if key has been invalidated */
+#define KEY_FLAG_BUILTIN	6	/* set if key is built in to the kernel */
+#define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
+#define KEY_FLAG_KEEP		8	/* set if key should not be removed */
+#define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
@@ -213,7 +217,6 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
@@ -353,17 +356,27 @@ extern void key_set_timeout(struct key *, unsigned);
 #define	KEY_NEED_SETATTR 0x20	/* Require permission to change attributes */
 #define	KEY_NEED_ALL	0x3f	/* All the above permissions */
 
+static inline short key_read_state(const struct key *key)
+{
+	/* Barrier versus mark_key_instantiated(). */
+	return smp_load_acquire(&key->state);
+}
+
 /**
- * key_is_instantiated - Determine if a key has been positively instantiated
+ * key_is_positive - Determine if a key has been positively instantiated
  * @key: The key to check.
  *
  * Return true if the specified key has been positively instantiated, false
  * otherwise.
  */
-static inline bool key_is_instantiated(const struct key *key)
+static inline bool key_is_positive(const struct key *key)
+{
+	return key_read_state(key) == KEY_IS_POSITIVE;
+}
+
+static inline bool key_is_negative(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	return key_read_state(key) < 0;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 8737412c7b27..e1d4d898a007 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -224,7 +224,7 @@ static int dns_resolver_match_preparse(struct key_match_data *match_data)
 static void dns_resolver_describe(const struct key *key, struct seq_file *m)
 {
 	seq_puts(m, key->description);
-	if (key_is_instantiated(key)) {
+	if (key_is_positive(key)) {
 		int err = PTR_ERR(key->payload.data[dns_key_error]);
 
 		if (err)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index e607830b6154..929e14978c42 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -247,7 +247,7 @@ void big_key_revoke(struct key *key)
 
 	/* clear the quota */
 	key_payload_reserve(key, 0);
-	if (key_is_instantiated(key) &&
+	if (key_is_positive(key) &&
 	    (size_t)key->payload.data[big_key_len] > BIG_KEY_FILE_THRESHOLD)
 		vfs_truncate(path, 0);
 }
@@ -279,7 +279,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
 
 	seq_puts(m, key->description);
 
-	if (key_is_instantiated(key))
+	if (key_is_positive(key))
 		seq_printf(m, ": %zu [%s]",
 			   datalen,
 			   datalen > BIG_KEY_FILE_THRESHOLD ? "file" : "buff");
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 535db141f4da..d92cbf9687c3 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -854,7 +854,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	size_t datalen = prep->datalen;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 87cb260e4890..f01d48cb3de1 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -129,15 +129,15 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 	while (!list_empty(keys)) {
 		struct key *key =
 			list_entry(keys->next, struct key, graveyard_link);
+		short state = key->state;
+
 		list_del(&key->graveyard_link);
 
 		kdebug("- %u", key->serial);
 		key_check(key);
 
 		/* Throw away the key data if the key is instantiated */
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		    !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
-		    key->type->destroy)
+		if (state == KEY_IS_POSITIVE && key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);
@@ -151,7 +151,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		}
 
 		atomic_dec(&key->user->nkeys);
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
+		if (state != KEY_IS_UNINSTANTIATED)
 			atomic_dec(&key->user->nikeys);
 
 		key_user_put(key->user);
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..9385e7cc710f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -402,6 +402,18 @@ int key_payload_reserve(struct key *key, size_t datalen)
 EXPORT_SYMBOL(key_payload_reserve);
 
 /*
+ * Change the key state to being instantiated.
+ */
+static void mark_key_instantiated(struct key *key, int reject_error)
+{
+	/* Commit the payload before setting the state; barrier versus
+	 * key_read_state().
+	 */
+	smp_store_release(&key->state,
+			  (reject_error < 0) ? reject_error : KEY_IS_POSITIVE);
+}
+
+/*
  * Instantiate a key and link it into the target keyring atomically.  Must be
  * called with the target keyring's semaphore writelocked.  The target key's
  * semaphore need not be locked as instantiation is serialised by
@@ -424,14 +436,14 @@ static int __key_instantiate_and_link(struct key *key,
 	mutex_lock(&key_construction_mutex);
 
 	/* can't instantiate twice */
-	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* instantiate the key */
 		ret = key->type->instantiate(key, prep);
 
 		if (ret == 0) {
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
-			set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+			mark_key_instantiated(key, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
 				awaken = 1;
@@ -577,13 +589,10 @@ int key_reject_and_link(struct key *key,
 	mutex_lock(&key_construction_mutex);
 
 	/* can't instantiate twice */
-	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
-		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
-		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+		mark_key_instantiated(key, -error);
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;
 		key_schedule_gc(key->expiry + key_gc_delay);
@@ -752,8 +761,8 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 
 	ret = key->type->update(key, prep);
 	if (ret == 0)
-		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		/* Updating a negative key positively instantiates it */
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -986,8 +995,8 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 
 	ret = key->type->update(key, &prep);
 	if (ret == 0)
-		/* updating a negative key instantiates it */
-		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+		/* Updating a negative key positively instantiates it */
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..76d22f726ae4 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -766,10 +766,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		ret = -ENOKEY;
-		goto error2;
-	}
+	ret = key_read_state(key);
+	if (ret < 0)
+		goto error2; /* Negatively instantiated */
 
 	/* see if we can read it directly */
 	ret = key_permission(key_ref, KEY_NEED_READ);
@@ -901,7 +900,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 		atomic_dec(&key->user->nkeys);
 		atomic_inc(&newowner->nkeys);
 
-		if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
+		if (key->state != KEY_IS_UNINSTANTIATED) {
 			atomic_dec(&key->user->nikeys);
 			atomic_inc(&newowner->nikeys);
 		}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..06173b091a74 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -414,7 +414,7 @@ static void keyring_describe(const struct key *keyring, struct seq_file *m)
 	else
 		seq_puts(m, "[anon]");
 
-	if (key_is_instantiated(keyring)) {
+	if (key_is_positive(keyring)) {
 		if (keyring->keys.nr_leaves_on_tree != 0)
 			seq_printf(m, ": %lu", keyring->keys.nr_leaves_on_tree);
 		else
@@ -553,7 +553,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 {
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
-	unsigned long kflags = key->flags;
+	unsigned long kflags = READ_ONCE(key->flags);
+	short state = READ_ONCE(key->state);
 
 	kenter("{%d}", key->serial);
 
@@ -597,9 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
 
 	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
 		/* we set a different error code if we pass a negative key */
-		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
-			smp_rmb();
-			ctx->result = ERR_PTR(key->reject_error);
+		if (state < 0) {
+			ctx->result = ERR_PTR(state);
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/proc.c b/security/keys/proc.c
index de834309d100..4089ce1f7757 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	unsigned long timo;
 	key_ref_t key_ref, skey_ref;
 	char xbuf[16];
+	short state;
 	int rc;
 
 	struct keyring_search_context ctx = {
@@ -236,17 +237,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
+	state = key_read_state(key);
+
 #define showflag(KEY, LETTER, FLAG) \
 	(test_bit(FLAG,	&(KEY)->flags) ? LETTER : '-')
 
 	seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
 		   key->serial,
-		   showflag(key, 'I', KEY_FLAG_INSTANTIATED),
+		   state != KEY_IS_UNINSTANTIATED ? 'I' : '-',
 		   showflag(key, 'R', KEY_FLAG_REVOKED),
 		   showflag(key, 'D', KEY_FLAG_DEAD),
 		   showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
 		   showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
-		   showflag(key, 'N', KEY_FLAG_NEGATIVE),
+		   state < 0 ? 'N' : '-',
 		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 293d3598153b..740affd65ee9 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -730,7 +730,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 
 	ret = -EIO;
 	if (!(lflags & KEY_LOOKUP_PARTIAL) &&
-	    !test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
+	    key_read_state(key) == KEY_IS_UNINSTANTIATED)
 		goto invalid_key;
 
 	/* check the permissions */
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..e8036cd0ad54 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -595,10 +595,9 @@ int wait_for_key_construction(struct key *key, bool intr)
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+	ret = key_read_state(key);
+	if (ret < 0)
+		return ret;
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 6ebf1af8fce9..424e1d90412e 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -73,7 +73,7 @@ static void request_key_auth_describe(const struct key *key,
 
 	seq_puts(m, "key:");
 	seq_puts(m, key->description);
-	if (key_is_instantiated(key))
+	if (key_is_positive(key))
 		seq_printf(m, " pid:%d ci:%zu", rka->pid, rka->callout_len);
 }
 
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	char *datablob;
 	int ret = 0;
 
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_negative(key))
 		return -ENOKEY;
 	p = key->payload.data[0];
 	if (!p->migratable)
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 3d8c68eba516..9f558bedba23 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
 	/* attach the new data, displacing the old */
 	key->expiry = prep->expiry;
-	if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+	if (key_is_positive(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;
@@ -162,7 +162,7 @@ EXPORT_SYMBOL_GPL(user_destroy);
 void user_describe(const struct key *key, struct seq_file *m)
 {
 	seq_puts(m, key->description);
-	if (key_is_instantiated(key))
+	if (key_is_positive(key))
 		seq_printf(m, ": %u", key->datalen);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 00/15] KEYS: Fixes

[Eric Biggers]

On Mon, Oct 16, 2017 at 11:27:22PM +0100, David Howells wrote:
> Okay, I've fixed those issues, I think.  I've renamed the instantiation labels
> to positive.
> 
> Thanks,
> David
> ---
> commit f23f3bb0ba3be44e775ac74148157136b919e3b0
> Author: David Howells <dhowells@redhat.com>
> Date:   Wed Oct 4 16:43:25 2017 +0100
> 
>     KEYS: Fix race between updating and finding a negative key
>     
>     Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
>     error into one field such that:
>     
>      (1) The instantiation state can be modified/read atomically.
>     
>      (2) The error can be accessed atomically with the state.
>     
>      (3) The error isn't stored unioned with the payload pointers.
>     
>     This deals with the problem that the state is spread over three different
>     objects (two bits and a separate variable) and reading or updating them
>     atomically isn't practical, given that not only can uninstantiated keys
>     change into instantiated or rejected keys, but rejected keys can also turn
>     into instantiated keys - and someone accessing the key might not be using
>     any locking.
>     
>     The main side effect of this problem is that what was held in the payload
>     may change, depending on the state.  For instance, you might observe the
>     key to be in the rejected state.  You then read the cached error, but if
>     the key semaphore wasn't locked, the key might've become instantiated
>     between the two reads - and you might now have something in hand that isn't
>     actually an error code.
>     
>     The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error
>     code if the key is negatively instantiated.  The key_is_instantiated()
>     function is replaced with key_is_positive() to avoid confusion as negative
>     keys are also 'instantiated'.
>     
>     Additionally, barriering is included:
>     
>      (1) Order payload-set before state-set during instantiation.
>     
>      (2) Order state-read before payload-read when using the key.
>     
>     Further separate barriering is necessary if RCU is being used to access the
>     payload content after reading the payload pointers.
>     
>     Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
>     Cc: stable at vger.kernel.org # v4.4+
>     Reported-by: Eric Biggers <ebiggers@google.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
> 

This looks good now; feel free to add

	Reviewed-by: Eric Biggers <ebiggers@google.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html