Re: [PATCH v2 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 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] API Violation: kmap_local_page() mapped in one thread and unmapped in another
- [High] Double-Unmap / Memory Corruption in `phmac_finup` error path
--

--- Patch [6]: [PATCH v2 6/6] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine ---
commit 937135347da9f51601b4d25da73d8f9de9ecb6c8
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..adf14553ad392 100644
> --- a/arch/s390/crypto/phmac_s390.c
> +++ b/arch/s390/crypto/phmac_s390.c
> @@ -629,11 +629,11 @@ 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);

[Severity: Critical]
This is a pre-existing issue, but does deferring this request to the crypto
engine kworker thread cause a cross-thread kmap_local_page() violation?

Earlier in phmac_update(), hwh_prepare() maps memory into the local task.
If the request is deferred to the async engine, the local map context will
be leaked, causing kunmap_local() corruption when it is unmapped in the
worker thread.

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

[ ... ]

> @@ -716,15 +716,15 @@ 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);

[Severity: Critical]
This is a pre-existing issue, but does deferring this request to the crypto
engine kworker thread cause a cross-thread kmap_local_page() violation?

Similar to phmac_update(), hwh_prepare() maps memory into the local task.
If the request is deferred to the async engine, the local map context will
be leaked, causing kunmap_local() corruption when it is unmapped in the
worker thread.

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

[Severity: High]
This isn't a bug introduced by this patch, but could unconditionally calling
hwh_advance() here on an engine fallback error path cause a double-unmap?

On a fallback failure, unconditionally calling hwh_advance() causes
crypto_hash_walk_done() to call kunmap_local() on already unmapped
pointers, potentially resulting in memory corruption.

>  
>  out:

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