Re: [PATCH v5 6/8] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect module teardown order in `s390_phmac_exit` leads to kthread leaks and Use-After-Free of the `crypto_engine`.
--

--- Patch [6]: [PATCH v5 6/8] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine ---
commit c74c807c2aeebffc7e981ca45b5b377727342ad1
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine
    
When a request is transferred to the engine via
crypto_transfer_hash_request_to_engine() there are two return codes
signaling a successful transfer: EINPROGRESS and EBUSY. However the
correct handling of EBUSY was missing and has been added as a return
code indicating a successful transfer to the crypto engine.

Cc: [email protected] # 6.17+

> diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
> index 03ca33ffe6ccf..30c8a59af1a4e 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c

[ ... ]

> @@ -716,15 +726,17 @@ static int phmac_finup(struct ahash_request *req)
>  		/* req->async_op has been set to either OP_FINUP or OP_FINAL */
>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
> -		if (rc != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;
> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (cleanup && hwh->walkbytes > 0)
>  		hwh_advance(hwh, rc);
>  
>  out:
> -	if (rc != -EINPROGRESS)
> +	if (cleanup)
>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>  	pr_debug("rc=%d\n", rc);
>  	return rc;

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential module teardown order problem in s390_phmac_exit() in 
arch/s390/crypto/phmac_s390.c:

    if (phmac_crypto_engine) {
        crypto_engine_stop(phmac_crypto_engine);
        crypto_engine_exit(phmac_crypto_engine);
    }

    for (i = ARRAY_SIZE(phmac_algs) - 1; i >= 0; i--) {
        phmac = &phmac_algs[i];
        if (phmac->registered)
            crypto_engine_unregister_ahash(&phmac->alg);
    }

Could this lead to kthread leaks and a use-after-free of the crypto_engine?

If s390_phmac_init() fails to register a subsequent algorithm after
successfully registering the first, it jumps to the error path and
calls s390_phmac_exit().

Because the first algorithm is already registered and the module is in
MODULE_STATE_COMING, a concurrent userspace program can successfully
allocate it and continuously submit hash requests.

Since s390_phmac_exit() calls crypto_engine_stop() before unregistering
the algorithms, the continuous stream of requests keeps the engine busy.
This can cause crypto_engine_stop() to time out and return -EBUSY,
which leads to crypto_engine_exit() returning without destroying the
kworker thread.

Finally, misc_deregister() destroys the device, freeing the devm-managed
crypto_engine struct, and the module is subsequently unloaded.

Would the still-running kworker thread then access freed memory and
execute freed module code, potentially causing a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6