Re: [PATCH v3 1/4] s390/crypto: Replace cond_resched() with msleep(1)

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free / Incorrect Backlog Handling due to improper `-EBUSY` check
- [High] Double completion of crypto requests on error paths leading to Use-After-Free
- [Medium] Unreachable error handling logic for `-EKEYEXPIRED` in `phmac_do_one_request`
--

--- Patch [1]: [PATCH v3 1/4] s390/crypto: Replace cond_resched() with msleep(1) ---
commit b1e4ce79023817dc16f19a077b044e708fcff721
Author: Heiko Carstens <hca@linux.ibm.com>

s390/crypto: Replace cond_resched() with msleep(1)

This patch replaces cond_resched calls in the s390 crypto code with msleep(1).
This ensures that the current task is scheduled away as originally intended,
since cond_resched is compiled away to a no-op under newer scheduling
preemption modes.

Link: commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

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

[Severity: High]
This is a pre-existing issue, but does the crypto request entry function
ecb_paes_crypt() properly handle backlogged requests?

When a caller submits a request with CRYPTO_TFM_REQ_MAY_BACKLOG and the
crypto engine queue is full, crypto_enqueue_request() successfully enqueues
the request to the backlog and returns -EBUSY.

However, ecb_paes_crypt() appears to treat -EBUSY as an error because it
only checks for rc != -EINPROGRESS:

ecb_paes_crypt() {
    rc = crypto_transfer_skcipher_request_to_engine(paes_crypto_engine, req);
    if (rc != -EINPROGRESS)
        atomic_dec(&ctx->via_engine_ctr);
    ...
    if (rc != -EINPROGRESS)
        skcipher_walk_done(walk, rc);
}

This prematurely tears down the request state while the request remains
queued in the engine. When the engine eventually processes this request,
could it execute with freed or zeroed structures and corrupt memory?

> @@ -555,7 +555,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
>  		 * To avoid immediately re-invocation of this callback,
>  		 * tell the scheduler to voluntarily give up the CPU here.
>  		 */
> -		cond_resched();
> +		msleep(1);
>  		pr_debug("rescheduling request\n");
>  		return -ENOSPC;
>  	} else if (rc) {

[Severity: High]
This is a pre-existing issue, but does ecb_paes_do_one_request() accidentally
trigger a double completion on error paths?

If ecb_paes_do_crypt() fails and returns a negative error code like -EINVAL,
the code falls through and calls crypto_finalize_skcipher_request(), which
completes the request.

ecb_paes_do_one_request() then returns that same negative error code back to
crypto_pump_requests() in the crypto engine. When crypto_pump_requests() sees
a negative return code that is not -ENOSPC, it branches to its error path
and calls crypto_request_complete() a second time on the exact same request.

Could this lead to a use-after-free if the user completion callback
already freed the request context during the first invocation?

[ ... ]

> diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
> index 03ca33ffe6ccf..da76c7beb1847 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c

[Severity: High]
This is a pre-existing issue, but does phmac_update() share the same
backlog handling regression as ecb_paes_crypt()?

It also checks only for rc != -EINPROGRESS and wrongly cleans up the
request via hwh_advance() on -EBUSY, even though the request was successfully
enqueued to the backlog.

> @@ -895,7 +895,7 @@ static int phmac_do_one_request(struct crypto_engine *engine, void *areq)
>  			 * tell scheduler to voluntarily give up the CPU here.
>  			 */
>  			pr_debug("rescheduling request\n");
> -			cond_resched();
> +			msleep(1);

[Severity: Medium]
This is a pre-existing issue, but is this error handling block for
-EKEYEXPIRED ever actually reachable?

When phmac_kmac_update() and phmac_kmac_final() are called with maysleep
set to true, they do not seem to proactively check tfm_ctx->pk_state to
return -EKEYEXPIRED.

Instead, if the key is expired, they synchronously call phmac_convert_key()
and retry the operation.

Because they handle conversions synchronously in this path, they never
return -EKEYEXPIRED, which would make this entire error handling block
dead code. Is there another path where -EKEYEXPIRED could be returned
here?

>  			return -ENOSPC;
>  		} else if (rc) {
>  			hwh_advance(hwh, rc);

[Severity: High]
This is a pre-existing issue, but just like ecb_paes_do_one_request(), does
this hash error path also cause a double completion?

If rc is a negative error code like -EINVAL, crypto_finalize_hash_request()
is called in phmac_do_one_request(), and then crypto_request_complete()
is called again in the crypto engine error path.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730052907.2607026-1-hca@linux.ibm.com?part=1