[PATCH] blk-crypto: fix NULL deref and UAF in __blk_crypto_evict_key()

[PATCH] blk-crypto: fix NULL deref and UAF in __blk_crypto_evict_key()

[Baokun Li]

Sashiko reported a possible NULL pointer dereference, triggered by the
following flow:

  dm-inlinecrypt ctor            # first use of the fallback
   ├─ blk_crypto_init_key()      # succeeds, ctx->key.size != 0
   ├─ blk_crypto_start_using_key()
   │   ├─ return -EOPNOTSUPP     # uninit profile, will NULL deref
   │   └─ blk_crypto_fallback_start_using_key()
   │      └─ blk_crypto_fallback_init() fails
   │         └─ kfree(profile)   # not NULLed, will UAF
   ├─ goto bad
   └─ inlinecrypt_dtr()          # evicts whenever key.size is set
      └─ blk_crypto_evict_key()
         └─ blk_crypto_config_supported_natively() = false
            └─ blk_crypto_fallback_evict_key()
               └─ __blk_crypto_evict_key(blk_crypto_fallback_profile)
                  └─ profile->num_slots # BOOM!!!

This issue can be reproduced using the following commands:

  dd if=/dev/zero of=/tmp/backing bs=1M count=64
  LOOP=$(losetup -f --show /tmp/backing)
  SZ=$(blockdev --getsz $LOOP)
  KEY=$(head -c 64 /dev/urandom | od -An -tx1 | tr -d ' \n')
  dmsetup create ic --table "0 $SZ inlinecrypt aes-xts-plain64 \
    $KEY 0 $LOOP 0 1 keytype:hw-wrapped"

And the following oops is triggered:

  ==================================================================
  blk-crypto: loop0: no support for wrapped keys
  Oops: general protection fault, probably for non-canonical address
        0xdffffc000000000b: 0000 [#1] SMP KASAN PTI
  KASAN: null-ptr-deref in range [0x0000000000000058-0x000000000000005f]
  CPU: 5 UID: 0 PID: 901 Comm: dmsetup 7.3.0-rc3 #1 PREEMPT(lazy)
  RIP: 0010:__blk_crypto_evict_key+0x2e/0x3b0
  Call Trace:
   blk_crypto_evict_key+0x24c/0x320
   inlinecrypt_dtr+0xf6/0x160
   inlinecrypt_ctr+0x2d3/0xbd0
   dm_table_add_target+0x661/0xf40
   table_load+0x1c4/0x490
   ctl_ioctl+0x32f/0x5f0
   dm_ctl_ioctl+0xe/0x20
  ==================================================================

Fix this by checking the profile for NULL in __blk_crypto_evict_key()
to avoid the NULL pointer dereference, and by clearing the profile
pointer after it is freed to avoid the use-after-free.

Fixes: e7f57d2c47e2 ("dm-inlinecrypt: add target for inline block device encryption")
Cc: stable@vger.kernel.org # v7.2+
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://patch.msgid.link/sashiko-outbox-167341@kernel.org
Signed-off-by: Baokun Li <libaokun@linux.alibaba.com>
---
 block/blk-crypto-fallback.c | 1 +
 block/blk-crypto-profile.c  | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 2a8f40a65158..b7890ee1951c 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -605,6 +605,7 @@ static int blk_crypto_fallback_init(void)
 	blk_crypto_profile_destroy(blk_crypto_fallback_profile);
 fail_free_profile:
 	kfree(blk_crypto_fallback_profile);
+	blk_crypto_fallback_profile = NULL;
 fail_free_bioset:
 	bioset_exit(&enc_bio_set);
 out:
diff --git a/block/blk-crypto-profile.c b/block/blk-crypto-profile.c
index 53126c091b0b..1c5feca3b856 100644
--- a/block/blk-crypto-profile.c
+++ b/block/blk-crypto-profile.c
@@ -346,6 +346,9 @@ int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
 	struct blk_crypto_keyslot *slot;
 	int err;
 
+	if (!profile)
+		return 0;
+
 	if (profile->num_slots == 0) {
 		if (profile->ll_ops.keyslot_evict) {
 			blk_crypto_hw_enter(profile);

base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
-- 
2.43.7

Re: [PATCH] blk-crypto: fix NULL deref and UAF in __blk_crypto_evict_key()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] block: concurrent UAF in __blk_crypto_evict_key() via lockless global read
--

commit e07e1c7433e60b079101f4843a8706bcb93632f8
Author: Baokun Li <libaokun@linux.alibaba.com>
blk-crypto: fix NULL deref and UAF in __blk_crypto_evict_key()

This patch attempts to resolve a NULL pointer dereference and a
use-after-free by checking for a NULL profile in __blk_crypto_evict_key()
and clearing the fallback profile pointer after freeing it on the error
path.

Closes: https://patch.msgid.link/sashiko-outbox-167341@kernel.org

> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index 2a8f40a65158b..b7890ee1951c7 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
[ ... ]
> @@ -605,6 +605,7 @@ static int blk_crypto_fallback_init(void)
>  	blk_crypto_profile_destroy(blk_crypto_fallback_profile);
>  fail_free_profile:
>  	kfree(blk_crypto_fallback_profile);
> +	blk_crypto_fallback_profile = NULL;

[Severity: High]
Does this change leave a concurrent use-after-free window open?

In blk_crypto_fallback_init(), the global pointer is published before
initialization is complete:

    blk_crypto_fallback_profile = kzalloc_obj(*blk_crypto_fallback_profile);

If another thread concurrently executes blk_crypto_fallback_evict_key(),
it can locklessly read the global blk_crypto_fallback_profile. If the
initialization thread encounters an error and frees the profile:

    kfree(blk_crypto_fallback_profile);

Could the evicting thread end up reading the pointer before the kfree()
occurs and subsequently dereferencing the freed memory? Setting the pointer
to NULL after it is freed might not prevent a data race if the pointer was
already copied by the evicting thread.

>  fail_free_bioset:
>  	bioset_exit(&enc_bio_set);
>  out:
> diff --git a/block/blk-crypto-profile.c b/block/blk-crypto-profile.c
> index 53126c091b0be..1c5feca3b856a 100644
> --- a/block/blk-crypto-profile.c
> +++ b/block/blk-crypto-profile.c
> @@ -346,6 +346,9 @@ int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
>  	struct blk_crypto_keyslot *slot;
>  	int err;
>  
> +	if (!profile)
> +		return 0;
> +
>  	if (profile->num_slots == 0) {

[Severity: High]
Because the profile pointer is passed in after being read locklessly by the
caller, could this pointer still point to freed memory if the initialization
thread freed it concurrently?

If so, the new NULL check would be bypassed, and reading profile->num_slots
would trigger a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013095106.881914-1-libaokun@linux.alibaba.com?part=1