linux-kernel - Re: [PATCH] crypto: api - Allow delayed algorithm destruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf7e20a2-dc30-4940-9abe-bbf5ea1ac413@linux.dev>
Date: Thu, 10 Apr 2025 19:24:25 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Horia Geantă <horia.geanta@....com>,
 Pankaj Gupta <pankaj.gupta@....com>, Gaurav Jain <gaurav.jain@....com>,
 linux-crypto@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 linux-kernel@...r.kernel.org,
 Valentin Ciocoi Radulescu <valentin.ciocoi@....com>
Subject: Re: [PATCH] crypto: api - Allow delayed algorithm destruction

On 4/8/25 23:29, Herbert Xu wrote:
> On Wed, Apr 09, 2025 at 10:58:04AM +0800, Herbert Xu wrote:
>>
>> What I'll do is make the crypto_unregister call wait for the users
>> to go away.  That matches how the network device unregistration works
>> and hopefully should solve this problem.  But keep your eyes for
>> dead locks that used to plague netdev unregistration :)
> 
> That was a dumb idea.  All it would do is make the shutdown hang.
> So here is a different tack.  Let the algorithms stick around,
> by allocating them dynamically instead.  Then we could simply
> kfree them when the user finally disappears (if ever).
> 
> Note to make this work, caam needs to be modified to allocate the
> algorithms dynamically (kmemdup should work), and add a cra_destroy
> function to kfree the memory.
> 
> ---8<---
> The current algorithm unregistration mechanism originated from
> software crypto.  The code relies on module reference counts to
> stop in-use algorithms from being unregistered.  Therefore if
> the unregistration function is reached, it is assumed that the
> module reference count has hit zero and thus the algorithm reference
> count should be exactly 1.
> 
> This is completely broken for hardware devices, which can be
> unplugged at random.
> 
> Fix this by allowing algorithms to be destroyed later if a destroy
> callback is provided.
> 
> Reported-by: Sean Anderson <sean.anderson@...ux.dev>
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 5b8a4c787387..f368c0dc0d6d 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -481,10 +481,10 @@ void crypto_unregister_alg(struct crypto_alg *alg)
>  	if (WARN(ret, "Algorithm %s is not registered", alg->cra_driver_name))
>  		return;
>  
> -	if (WARN_ON(refcount_read(&alg->cra_refcnt) != 1))
> -		return;
> -
> -	if (alg->cra_type && alg->cra_type->destroy)
> +	if (alg->cra_destroy)
> +		crypto_alg_put(alg);
> +	else if (!WARN_ON(refcount_read(&alg->cra_refcnt) != 1) &&
> +		 alg->cra_type && alg->cra_type->destroy)
>  		alg->cra_type->destroy(alg);
>  
>  	crypto_remove_final(&list);

The above patch didn't apply cleanly. I seem to be missing cra_type. What
tree should I test with?

--Sean