Re: [PATCH v3 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

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

New issues:
- [High] The patch rewrites the `-EBUSY` return code from the crypto engine to `-EINPROGRESS`, improperly propagating a fake success state to the caller and breaking the crypto API backpressure mechanism.

Pre-existing issues:
- [Critical] `phmac_do_one_request` triggers a double completion when an asynchronous request fails with an error code.
- [Critical] Unconditional `hwh_advance()` on error paths leads to a double `kunmap_local()` or a NULL pointer dereference panic.
--

--- Patch [6]: [PATCH v3 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine ---
commit 049db089e02c25bd82461143ffc80704186a5ebb
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

When a request is transferred to the engine via
crypto_transfer_hash_request_to_engine() there are two return codes
signaling a successful transfer: EINPROGRESS and EBUSY. However the
correct handling of EBUSY was missing and has been added as a return
code indicating a successful transfer to the crypto engine.

> diff --git a/arch/s390/crypto/phmac_s390.c b/arch/s390/crypto/phmac_s390.c
> index 03ca33ffe6ccf..9d77198b22ff9 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c
> @@ -629,6 +629,8 @@ static int phmac_update(struct ahash_request *req)
>  		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 == -EBUSY)
> +			rc = -EINPROGRESS;

[Severity: High]
Does this break the crypto API backpressure mechanism?

When the engine's queue is full, crypto_enqueue_request() queues the
request to the backlog and returns -EBUSY. Callers like dm-crypt rely on
this -EBUSY signal to apply backpressure and throttle incoming requests.

By unconditionally mutating -EBUSY to -EINPROGRESS, do we deceive the caller
into thinking the request is processing normally, breaking the feedback loop
and allowing unbounded queue growth?

Furthermore, the crypto engine will later invoke the completion callback
with -EINPROGRESS when dequeued. Are callers who received -EINPROGRESS
initially instead of -EBUSY prepared for this state transition?

Would it be safer to decouple the local pending check from the return value?
For example:

if (rc != -EINPROGRESS && rc != -EBUSY)
	atomic_dec(&tfm_ctx->via_engine_ctr);

>  		if (rc != -EINPROGRESS)
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
>  	if (rc != -EINPROGRESS) {
>  		hwh_advance(hwh, rc);

[Severity: Critical]
This is a pre-existing issue, but does unconditional execution of
hwh_advance() on error paths risk a double kunmap_local() or a null
pointer dereference panic?

If a hash operation completes successfully locally but errors out later
(for instance, if -EKEYEXPIRED causes an -ENOSPC engine fallback), the walk
might already be completely consumed and successfully unmapped.

If we then call hwh_advance() on the already-closed walk, does it execute
crypto_hash_walk_done() again on the same address, resulting in a double
kunmap_local()?

Additionally, if the request size is 0 bytes, crypto_hash_walk_first()
might not map any memory at all. Could this trigger a panic by
unconditionally attempting to unmap a null address offset in the error path?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but looking at
phmac_do_one_request(), is there a risk of a double completion when an
asynchronous request fails?

arch/s390/crypto/phmac_s390.c:phmac_do_one_request() {
	...
	crypto_finalize_hash_request(engine, req, rc);
	local_bh_enable();
	return rc;
}

If an error occurs, the driver explicitly finalizes the request by calling
crypto_finalize_hash_request(), which synchronously completes the request
by invoking the caller's callback.

However, the function then returns the negative rc back to the crypto
engine. Seeing a negative return value and not -ENOSPC, will the crypto
engine core incorrectly assume the driver failed to take ownership of the
request and immediately invoke crypto_request_complete() a second time?

Should this function return 0 after explicitly finalizing a request?

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