Re: [PATCH v4 5/7] 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:

Pre-existing issues:
- [High] The patch corrects the handling of the `-EBUSY` return code for the crypto engine in `paes_s390.c` but fails to apply the identical fix to `phmac_s390.c`, leading to state corruption and Use-After-Free.
- [High] The `do_one_request` engine callbacks complete the crypto request twice if a synchronous error occurs during processing, leading to a double completion and Use-After-Free.
--

--- Patch [5]: [PATCH v4 5/7] s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine ---
commit 3e0f47412e8d6356e198b6481361a3ba704da13f
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix handling of EBUSY in PAES when req is pushed to crypto engine
    
This commit adds -EBUSY as a successful return code when transferring
skcipher requests to the crypto engine in paes_s390.c. This prevents
state cleanup on requests that are successfully queued on the backlog.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 03446bfac37f2..2048327c1ffe4 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -494,15 +495,17 @@ 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)
> +			cleanup = false;
> +		else
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but should the same fix be applied
to the hash requests in phmac_s390.c?

For example, in phmac_update():

arch/s390/crypto/phmac_s390.c:phmac_update() {
    ...
	if (rc == 0 || rc == -EKEYEXPIRED) {
		req_ctx->async_op = OP_UPDATE;
		atomic_inc(&tfm_ctx->via_engine_ctr);
		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));
	}
    ...
}

It looks like phmac_update, phmac_final, and phmac_finup also incorrectly
treat -EBUSY as a failure, which could dismantle the state needed by the
engine thread.

[ ... ]

[Severity: High]
This is a pre-existing issue, but could the do_one_request callbacks lead to
a double completion if a synchronous error occurs?

In ecb_paes_do_one_request():

arch/s390/crypto/paes_s390.c:ecb_paes_do_one_request() {
    ...
	crypto_finalize_skcipher_request(engine, req, rc);
	local_bh_enable();
	return rc;
}

If rc is negative, does returning it here after calling
crypto_finalize_skcipher_request() force the engine thread to incorrectly
complete the request a second time?

Could this also affect cbc_paes_do_one_request(), ctr_paes_do_one_request(),
xts_paes_do_one_request(), and phmac_do_one_request()?

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