Re: [PATCH v1 1/3] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

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

Pre-existing issues:
- [High] The crypto engine async callback functions (e.g., ecb_paes_do_one_request, cbc_paes_do_one_request) double-complete requests upon failure.
--

--- Patch [1]: [PATCH v1 1/3] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit 9fb1966e4b16413a77ebe1ccd447f231c3b30ec4
Author: Harald Freudenberger <[email protected]>
Subject: s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

This commit adds immediate error checks after skcipher_walk_done in the
main processing loops for PAES algorithms. It ensures walk errors are
propagated properly instead of being masked, and checks walk->nbytes to
avoid double de-allocation.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
[ ... ]
> @@ -558,7 +561,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
>  		cond_resched();
>  		pr_debug("rescheduling request\n");
>  		return -ENOSPC;
> -	} else if (rc) {
> +	} else if (rc && walk->nbytes) {
>  		skcipher_walk_done(walk, rc);
>  	}
>  
>  	memzero_explicit(&req_ctx->param, sizeof(req_ctx->param));
>  	pr_debug("request complete with rc=%d\n", rc);
>  	local_bh_disable();
>  	atomic_dec(&ctx->via_engine_ctr);
>  	crypto_finalize_skcipher_request(engine, req, rc);
>  	local_bh_enable();
>  	return rc;

[Severity: High]
This is a pre-existing issue, but can returning a negative rc here cause a
double-completion of the request?

If an error occurs during cryptographic processing (for example, if
paes_convert_key() fails and returns a negative rc), the code calls
crypto_finalize_skcipher_request() which completes the request once.

It then returns that same negative rc to the caller. Does the crypto engine
core (in crypto_pump_requests) see this negative return value and execute its
own error path, completing the exact same request a second time?

Could this result in a use-after-free or double-free, since callers might
drop references multiple times when a request is completed twice? This
identical pattern appears to exist across all the PAES cipher modes
(ecb_paes_do_one_request, cbc_paes_do_one_request, ctr_paes_do_one_request,
and xts_paes_do_one_request).

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