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

Pre-existing issues:
- [Critical] Thread-local memory mapping (kmap_local_page) is leaked across an asynchronous boundary, leading to an invalid cross-thread kunmap_local.
- [High] Double completion of asynchronous requests on error paths in `phmac_do_one_request`.
- [High] Persistent hardware busy state causes the driver to complete the request with `-EBUSY`, which the crypto API swallows, leading to an infinite wait.
--

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

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

This commit fixes the handling of the -EBUSY return code when a request is
transferred to the crypto engine via crypto_transfer_hash_request_to_engine().
It adds -EBUSY as a successful transfer indicator alongside -EINPROGRESS,
preventing premature cleanup of the request context.

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

[ ... ]

> @@ -629,12 +632,15 @@ 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 != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;

[Severity: Critical]
This is a pre-existing issue, but does setting cleanup = false here leak a
thread-local memory mapping across an asynchronous boundary?

When the submitter thread maps the scatterlist page using kmap_local_page()
via hwh_prepare() calling into crypto_hash_walk_first(), the map is strictly
thread-local. If the request is pushed to the engine and the submitter thread
returns, the local memory remains mapped in its thread context.

Later, the crypto_engine kworker thread processes the queued request in
arch/s390/crypto/phmac_s390.c:phmac_do_one_request(), calling
crypto_hash_walk_done(), which invokes kunmap_local() cross-thread. Can this
cause a kernel BUG or panic by violating the thread-locality requirements of
kmap_local_page()?

> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS) {
> -		hwh_advance(hwh, rc);
> +	if (cleanup) {
> +		if (hwh->walkbytes > 0)
> +			hwh_advance(hwh, rc);
>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>  	}
>  

[ ... ]

> @@ -667,12 +674,14 @@ static int phmac_final(struct ahash_request *req)
>  		req_ctx->async_op = OP_FINAL;
>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
> -		if (rc != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;

[Severity: High]
This is a pre-existing issue, but does the driver properly translate persistent
-EBUSY hardware errors before completion?

If the hardware persistently returns -EBUSY when deriving a protected key in
arch/s390/crypto/phmac_s390.c:convert_key(), it exhausts its retries and
returns -EBUSY. This propagates to phmac_do_one_request(), which explicitly
finalizes the request:

    crypto_finalize_hash_request(engine, req, rc);

Since the crypto API's completion wrapper ahash_op_done() explicitly swallows
-EBUSY because it treats it as a queueing status, completing a request with
-EBUSY prevents the final completion callback from firing. Could this cause
callers using crypto_wait_req() to block forever?

> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}
>  
>  out:
> -	if (rc != -EINPROGRESS)
> +	if (cleanup)
>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>  	pr_debug("rc=%d\n", rc);
>  	return rc;

[ ... ]

> @@ -716,15 +726,17 @@ static int phmac_finup(struct ahash_request *req)
>  		/* req->async_op has been set to either OP_FINUP or OP_FINAL */
>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, req);
> -		if (rc != -EINPROGRESS)
> +		if (rc == -EINPROGRESS || rc == -EBUSY)
> +			cleanup = false;
> +		else
>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>  	}

[Severity: High]
This is a pre-existing issue, but is there a risk of double completion on error
paths in arch/s390/crypto/phmac_s390.c:phmac_do_one_request()?

If hardware execution fails and returns an error like -EIO,
phmac_do_one_request() manually completes the request:

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

By returning the negative rc back to the crypto_engine core, and because the
engine allocates with retry_support=true, the core in crypto_pump_requests()
will treat this as a failure it must handle. Does this trigger an automatic
fallback crypto_request_complete() that invokes the user's completion callback
a second time?

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