Re: [PATCH v2 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 incorrectly skips cleanup on all -EBUSY return codes, failing to distinguish between asynchronous enqueue success and synchronous hardware errors.

Pre-existing issues:
- [Critical] The handling of -EBUSY when transferring hash requests to the crypto engine is broken in HMAC algorithms, leading to memory corruption and use-after-free.
--

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

This commit addresses a missing check for -EBUSY when a request is
successfully transferred to the crypto engine backlog in the PAES algorithms.
It updates the return code checks to treat -EBUSY similarly to -EINPROGRESS to
avoid incorrectly cleaning up the request while it is still queued.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..b60696c49fccf 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,15 +494,15 @@ 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 != -EINPROGRESS)
> +		if (rc != -EINPROGRESS && rc != -EBUSY)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS && walk->nbytes)
> +	if (rc != -EINPROGRESS && rc != -EBUSY && walk->nbytes)
>  		skcipher_walk_done(walk, rc);
>  
>  out:
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && rc != -EBUSY)
>  		memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));

[Severity: High]
Does this bypass cleanup if ecb_paes_do_crypt() fails synchronously?

Looking at ecb_paes_crypt(), ecb_paes_do_crypt() can fail synchronously and
return -EBUSY due to hardware key conversion failures:

    if (!atomic_read(&ctx->via_engine_ctr)) {
        rc = ecb_paes_do_crypt(ctx, req_ctx, tested, false);
        if (rc == 0)
            goto out;
    }

If rc is -EBUSY, the fallback block is skipped:

    if (rc == 0 || rc == -EKEYEXPIRED) {
        atomic_inc(&ctx->via_engine_ctr);
        rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
        ...

Then we reach the updated cleanup logic, which now skips execution when rc is
-EBUSY. Could this lead to leaked resources from skcipher_walk_done() and an
indefinite wait, since the caller interprets -EBUSY as a successful queue to the
engine but no callback will ever arrive?

Similar logic is updated in cbc_paes_crypt(), ctr_paes_crypt(), and
xts_paes_crypt().


This is a pre-existing issue, but I noticed a similar pattern in phmac_s390.c
that might need addressing.

arch/s390/crypto/phmac_s390.c:phmac_update()
    rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
    if (rc != -EINPROGRESS)
        atomic_dec(&tfm_ctx->via_engine_ctr);
    ...
    if (rc != -EINPROGRESS) {
        hwh_advance(hwh, rc);
        memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
    }

[Severity: Critical]
If crypto_transfer_hash_request_to_engine() returns -EBUSY when the request is
successfully added to the backlog queue, will this incorrectly treat it as a
failure?

If so, this could unmap the scatterlist via hwh_advance() and zero the hash
context via memzero_explicit() while the crypto engine later operates on it
asynchronously.

Is it necessary to apply the same -EBUSY handling fixes to the HMAC algorithms?

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