Re: [PATCH v3 5/6] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine

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

New issues:
- [High] The patch masks the `-EBUSY` backpressure signal by converting it to `-EINPROGRESS`, which breaks the crypto API's queue limit enforcement.

Pre-existing issues:
- [High] The module teardown in `paes_s390_fini` attempts to stop the crypto engine before unregistering the algorithms, potentially leaking the engine's kworker and causing a Use-After-Free.
--

--- Patch [5]: [PATCH v3 5/6] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine ---
commit 1a57782d904dcde97b280595a8b6669541d15c94
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine

When a request is transferred to the engine via
crypto_transfer_skcipher_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.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..1dd48f7a1bdf2 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,6 +494,8 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does overwriting -EBUSY with -EINPROGRESS break the crypto API's queue limit
enforcement?

Callers like dm-crypt or IPsec rely on -EBUSY to signal that a request with
CRYPTO_TFM_REQ_MAY_BACKLOG was successfully enqueued, but the queue has
reached its maximum capacity.

By returning -EINPROGRESS instead, callers are falsely informed that the queue
is not full. This defeats the backpressure mechanism and allows callers to
continuously pump requests into the driver, potentially leading to unbounded
growth of the crypto engine queue and memory exhaustion.

Would it be better to preserve the -EBUSY return value but check
if (rc != -EINPROGRESS && rc != -EBUSY) before executing the local teardown
paths?

[ ... ]
> @@ -763,6 +765,8 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for CBC mode in the same way?

[ ... ]
> @@ -1089,6 +1093,8 @@ static int ctr_paes_crypt(struct skcipher_request *req)
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for CTR mode?

[ ... ]
> @@ -1529,6 +1535,8 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
>  	if (rc == 0 || rc == -EKEYEXPIRED) {
>  		atomic_inc(&ctx->via_engine_ctr);
>  		rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
> +		if (rc == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this code hide the backlog signal for XTS mode?

As a general observation on the teardown path:

[Severity: High]
This isn't a bug introduced by this patch, but does paes_s390_fini() leak the
engine's kworker and cause a use-after-free by stopping the crypto engine
before unregistering the algorithms?

arch/s390/crypto/paes_s390.c:paes_s390_fini() {
	if (paes_crypto_engine) {
		crypto_engine_stop(paes_crypto_engine);
		crypto_engine_exit(paes_crypto_engine);
	}
	__crypto_unregister_skcipher(&ctr_paes_alg);
	...
}

If module initialization fails, paes_s390_fini() could be called while earlier
algorithms are already registered. In this window, another thread can
concurrently allocate a tfm and submit requests.

Because crypto_engine_stop() times out after 10 seconds if requests keep
arriving, it returns -EBUSY, causing crypto_engine_exit() to abort without
destroying the kworker thread.

After the algorithms are unregistered and the module memory is freed, the
surviving kworker will eventually process the queued requests and jump to the
unloaded do_one_request function, resulting in a kernel panic.

Should the algorithms be unregistered first to prevent new TFMs from being
allocated before tearing down the crypto engine?

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