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

[sashiko-bot@kernel.org]

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