Re: [PATCHv2] crypto: amcc - fix missing DMA memory barriers in descriptor handling

[Herbert Xu]

On Mon, Aug 10, 2026 at 09:47:38PM -0700, Rosen Penev wrote:
> Fix two memory ordering bugs in the AMCC crypto driver:
> 
> 1. In crypto4xx_bh_tasklet_cb(), add a dma_rmb() between reading the
>    PD_CTL_PE_DONE bit (with READ_ONCE) and reading the descriptor and DMA
>    buffer data in crypto4xx_pd_done().  Without it, the CPU on a
>    weakly-ordered architecture could read stale descriptor data before the
>    hardware's writes are globally visible.
> 
> 2. In crypto4xx_build_pd(), add a dma_wmb() before writing
>    PD_CTL_HOST_READY to ensure all descriptor and SA data is visible to
>    the device before the ownership handover bit.  Also fix the descriptor
>    field ordering: pd_ctl_len.w must be written before pd_ctl.w (which
>    contains HOST_READY), not after, to prevent the hardware from fetching
>    uninitialized length data.
> 
> Fixes: f6c48b76daa6 (crypto: amcc - Add crypto4xx-aead cryptographic
>   AEAD accelerator driver)
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>  v2: add descriptions for write barriers.
>  drivers/crypto/amcc/crypto4xx_core.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
> index fd010bfb7020..4b0ce165c198 100644
> --- a/drivers/crypto/amcc/crypto4xx_core.c
> +++ b/drivers/crypto/amcc/crypto4xx_core.c
> @@ -876,11 +876,17 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
>  		}
>  	}
>  
> +	pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
> +	pd_uinfo->state = PD_ENTRY_INUSE | (is_busy ? PD_ENTRY_BUSY : 0);
> +
> +	/* make the pd_ctl_len and pd_uinfo->state writes above visible to
> +	 * the device before the HOST_READY handover write below, so the
> +	 * device never fetches a descriptor with stale length/state bits
> +	 */
> +	dma_wmb();
>  	pd->pd_ctl.w = PD_CTL_HOST_READY |
>  		((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
>  			PD_CTL_HASH_FINAL : 0);
> -	pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
> -	pd_uinfo->state = PD_ENTRY_INUSE | (is_busy ? PD_ENTRY_BUSY : 0);
>  
>  	wmb();
>  	/* write any value to push engine to read a pd */

I would've thought that the hardware is only able to start reading
after this wmb() and the subsequent writes.  If this isn't the case
then plesae explain how the whole sequence works.

Thanks,
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt