diff mbox series

[v2,2/4] blk-crypto: make blk_crypto_evict_key() more robust

Message ID 20230308193645.114069-3-ebiggers@kernel.org (mailing list archive)
State Superseded
Headers show
Series Fix blk-crypto keyslot race condition | expand

Commit Message

Eric Biggers March 8, 2023, 7:36 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

If blk_crypto_evict_key() sees that the key is still in-use (due to a
bug) or that ->keyslot_evict failed, it currently just returns an error
while leaving the key linked into the keyslot management structures.

However, blk_crypto_evict_key() is only called in contexts such as inode
eviction where failure is not an option.  So actually the caller
proceeds with freeing the blk_crypto_key regardless of the return value
of blk_crypto_evict_key().

These two assumptions don't match, and the result is that there can be a
use-after-free in blk_crypto_reprogram_all_keys() after one of these
errors occurs.  (Note, these errors *shouldn't* happen; we're just
talking about what happens if they do anyway.)

Fix this by making blk_crypto_evict_key() unlink the key from the
keyslot management structures even on failure.

Fixes: 1b2628397058 ("block: Keyslot Manager for Inline Encryption")
Cc: stable@vger.kernel.org
Reviewed-by: Nathan Huckleberry <nhuck@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 block/blk-crypto-profile.c | 50 +++++++++++++++-----------------------
 block/blk-crypto.c         | 23 +++++++++++-------
 2 files changed, 33 insertions(+), 40 deletions(-)

Comments

Christoph Hellwig March 15, 2023, 4:23 p.m. UTC | #1 
On Wed, Mar 08, 2023 at 11:36:43AM -0800, Eric Biggers wrote:
>  			   const struct blk_crypto_key *key)
> @@ -389,22 +377,22 @@ int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
>  
>  	blk_crypto_hw_enter(profile);
>  	slot = blk_crypto_find_keyslot(profile, key);
> +	if (slot) {

Isn't !slot also an error condition that we should warn on?

> +		if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
> +			/* BUG: key is still in use by I/O */
> +			err = -EBUSY;

Either way two gotos to jump forward for the error case would improve
the readability of the code a fair bit I think.

> +		} else {
> +			err = profile->ll_ops.keyslot_evict(
> +					profile, key,
> +					blk_crypto_keyslot_index(slot));
> +		}
> +		/*
> +		 * Callers may free the key even on error, so unlink the key
> +		 * from the hash table and clear slot->key even on error.
> +		 */
> +		hlist_del(&slot->hash_node);
> +		slot->key = NULL;
>  	}
>  	blk_crypto_hw_exit(profile);
>  	return err;

Also given your above accurate description of the calling context,
what is the point of even returning an error here vs just logging
an error message?
Eric Biggers March 15, 2023, 6:26 p.m. UTC | #2 
On Wed, Mar 15, 2023 at 09:23:12AM -0700, Christoph Hellwig wrote:
> On Wed, Mar 08, 2023 at 11:36:43AM -0800, Eric Biggers wrote:
> >  			   const struct blk_crypto_key *key)
> > @@ -389,22 +377,22 @@ int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
> >  
> >  	blk_crypto_hw_enter(profile);
> >  	slot = blk_crypto_find_keyslot(profile, key);
> > +	if (slot) {
> 
> Isn't !slot also an error condition that we should warn on?

No, definitely not.  Keys are not guaranteed to be in a keyslot.  I'll add a
comment here.

> 
> > +		if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
> > +			/* BUG: key is still in use by I/O */
> > +			err = -EBUSY;
> 
> Either way two gotos to jump forward for the error case would improve
> the readability of the code a fair bit I think.

I slightly prefer it without the gotos, but sure I'll do it that way.

> > +		} else {
> > +			err = profile->ll_ops.keyslot_evict(
> > +					profile, key,
> > +					blk_crypto_keyslot_index(slot));
> > +		}
> > +		/*
> > +		 * Callers may free the key even on error, so unlink the key
> > +		 * from the hash table and clear slot->key even on error.
> > +		 */
> > +		hlist_del(&slot->hash_node);
> > +		slot->key = NULL;
> >  	}
> >  	blk_crypto_hw_exit(profile);
> >  	return err;
> 
> Also given your above accurate description of the calling context,
> what is the point of even returning an error here vs just logging
> an error message?

Yes, I was thinking about that too.  I'll add a patch that makes
blk_crypto_evict_key() log errors and return void.

- Eric
diff mbox series

Patch

diff --git a/block/blk-crypto-profile.c b/block/blk-crypto-profile.c
index 0307fb0d95d3..1b20ead59f39 100644
--- a/block/blk-crypto-profile.c
+++ b/block/blk-crypto-profile.c
@@ -354,22 +354,10 @@  bool __blk_crypto_cfg_supported(struct blk_crypto_profile *profile,
 	return true;
 }
 
-/**
- * __blk_crypto_evict_key() - Evict a key from a device.
- * @profile: the crypto profile of the device
- * @key: the key to evict.  It must not still be used in any I/O.
- *
- * If the device has keyslots, this finds the keyslot (if any) that contains the
- * specified key and calls the driver's keyslot_evict function to evict it.
- *
- * Otherwise, this just calls the driver's keyslot_evict function if it is
- * implemented, passing just the key (without any particular keyslot).  This
- * allows layered devices to evict the key from their underlying devices.
- *
- * Context: Process context. Takes and releases profile->lock.
- * Return: 0 on success or if there's no keyslot with the specified key, -EBUSY
- *	   if the keyslot is still in use, or another -errno value on other
- *	   error.
+/*
+ * This is an internal function that evicts a key from an inline encryption
+ * device that can be either a real device or the blk-crypto-fallback "device".
+ * It is used only by blk_crypto_evict_key(); see that function for details.
  */
 int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
 			   const struct blk_crypto_key *key)
@@ -389,22 +377,22 @@  int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
 
 	blk_crypto_hw_enter(profile);
 	slot = blk_crypto_find_keyslot(profile, key);
-	if (!slot)
-		goto out_unlock;
-
-	if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
-		err = -EBUSY;
-		goto out_unlock;
+	if (slot) {
+		if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
+			/* BUG: key is still in use by I/O */
+			err = -EBUSY;
+		} else {
+			err = profile->ll_ops.keyslot_evict(
+					profile, key,
+					blk_crypto_keyslot_index(slot));
+		}
+		/*
+		 * Callers may free the key even on error, so unlink the key
+		 * from the hash table and clear slot->key even on error.
+		 */
+		hlist_del(&slot->hash_node);
+		slot->key = NULL;
 	}
-	err = profile->ll_ops.keyslot_evict(profile, key,
-					    blk_crypto_keyslot_index(slot));
-	if (err)
-		goto out_unlock;
-
-	hlist_del(&slot->hash_node);
-	slot->key = NULL;
-	err = 0;
-out_unlock:
 	blk_crypto_hw_exit(profile);
 	return err;
 }
diff --git a/block/blk-crypto.c b/block/blk-crypto.c
index d0c7feb447e9..4e26fac64199 100644
--- a/block/blk-crypto.c
+++ b/block/blk-crypto.c
@@ -399,17 +399,22 @@  int blk_crypto_start_using_key(struct block_device *bdev,
 }
 
 /**
- * blk_crypto_evict_key() - Evict a key from any inline encryption hardware
- *			    it may have been programmed into
- * @bdev: The block_device who's associated inline encryption hardware this key
- *     might have been programmed into
- * @key: The key to evict
+ * blk_crypto_evict_key() - Evict a blk_crypto_key from a block_device
+ * @bdev: a block_device on which I/O using the key may have been done
+ * @key: the key to evict
  *
- * Upper layers (filesystems) must call this function to ensure that a key is
- * evicted from any hardware that it might have been programmed into.  The key
- * must not be in use by any in-flight IO when this function is called.
+ * For a given block_device, this function removes the given blk_crypto_key from
+ * the keyslot management structures and evicts it from any underlying hardware
+ * keyslot(s) or blk-crypto-fallback keyslot it may have been programmed into.
  *
- * Return: 0 on success or if the key wasn't in any keyslot; -errno on error.
+ * Upper layers must call this before freeing the blk_crypto_key.  It must be
+ * called for every block_device the key may have been used on.  The key must no
+ * longer be in use by any I/O when this function is called.
+ *
+ * Context: May sleep.
+ * Return: 0 on success or if the key wasn't in any keyslot; -errno if the key
+ *	   failed to be evicted from a keyslot or is still in-use.  Even on
+ *	   "failure", the key is removed from the keyslot management structures.
  */
 int blk_crypto_evict_key(struct block_device *bdev,
 			 const struct blk_crypto_key *key)