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

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

Pre-existing issues:
- [Critical] The wrapper functions (e.g., ecb_paes_crypt) incorrectly treat a return value of -EBUSY from crypto_transfer_skcipher_request_to_engine() as an enqueue failure, leading to a Use-After-Free when the crypto engine eventually processes the request.
- [Critical] The async request handler returns an error code after already finalizing the request, causing the crypto engine to complete the request a second time.
--

--- Patch [1]: [PATCH v2 1/6] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit 8ebdb4de82e72b4c0a21b2b0f1f7cb80c8245b4f
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

This commit adds immediate error checks after skcipher_walk_done()
in PAES algorithms to prevent error masking. It also reworks how
walk->nbytes is checked to avoid double de-allocation of walk resources.

Cc: [email protected] # 6.16+

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 8cfe6166c193c..ccc7da106c3a0 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[ ... ]

> @@ -495,7 +498,7 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && walk->nbytes)
>  		skcipher_walk_done(walk, rc);

[Severity: Critical]
This is a pre-existing issue, but does this code properly handle backlog
queueing?

If crypto_transfer_skcipher_request_to_engine() returns -EBUSY when the
hardware queue reaches its soft limit, the request is still enqueued.

Because -EBUSY != -EINPROGRESS, this error path will execute and call
skcipher_walk_done(), destroying the scatterlist walk and zeroing
parameters.

Since the engine will eventually process the enqueued request, could this
result in a use-after-free when the engine accesses the destroyed walk
structure? This pattern also appears in cbc_paes_crypt(), ctr_paes_crypt(),
and xts_paes_crypt().

>  
>  out:

[ ... ]

> @@ -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);
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but does returning the error code at the end
of this function cause the crypto engine to double-complete the request?

In ecb_paes_do_one_request() and the other async handlers, if
ecb_paes_do_crypt() returns a fatal error, the code finalizes the request
and later returns the error code:

    crypto_finalize_skcipher_request(engine, req, rc);
    local_bh_enable();
    return rc;

The crypto_pump_requests() loop in the engine framework interprets any
returned error other than -ENOSPC as an execution failure and manually calls
crypto_request_complete() a second time.

Could this double completion lead to a use-after-free if the caller frees
the request after the first callback?

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