Re: [PATCH 1/3] dma: fsl_raid: fix endianness of in-memory descriptor stores

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch correctly fixes the endianness of `struct fsl_re_cmpnd_frame` and `struct fsl_re_hw_desc` by wrapping assignments in `cpu_to_be32()`, but completely misses the same class of bug for `cdb32` fields in the Command Descriptor Block structs (`fsl_re_xor_cdb`, `fsl_re_pq_cdb`, `fsl_re_move_cdb`).
- [High] Uninitialized kernel DMA pool memory is passed directly to the hardware for Command Descriptor Blocks (CDBs) and compound frames.
- [High] A DMA length of exactly 1MB (1024*1024) is allowed by `FSL_RE_MAX_DATA_LEN` but is truncated to 0 when bitwise-ANDed with `FSL_RE_CF_LENGTH_MASK`, resulting in a silent 0-byte transfer.
--

--- Patch [1]: [PATCH 1/3] dma: fsl_raid: fix endianness of in-memory descriptor stores ---
commit b79b0fb376ec1f7e303159ca0f7c486730ae5496
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: fix endianness of in-memory descriptor stores

This commit updates the driver to wrap in-memory descriptor fields in
cpu_to_be32() to resolve base-type mismatches and endianness issues. It
also simplifies the final-frame bit assignment in fill_cfd_frame().

Link: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..888f55b672a58 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -242,9 +242,9 @@ static void fill_cfd_frame(struct fsl_re_cmpnd_frame *cf, u8 index,
>  	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;

[Severity: High]
This is a pre-existing issue, but can this truncate a legitimate 1MB
transfer down to 0 bytes?

If a caller issues a DMA request of exactly FSL_RE_MAX_DATA_LEN (which
is 1024*1024, or 0x100000), it passes the bounds checks. However,
FSL_RE_CF_LENGTH_MASK is 0x000FFFFF, so the bitwise-AND operation here
would evaluate to 0.

Does this result in a silent 0-byte transfer instead of the intended
1MB transfer?

>  
>  	efrl |= final << FSL_RE_CF_FINAL_SHIFT;
> -	cf[index].efrl32 = efrl;
> -	cf[index].addr_high = upper_32_bits(addr);
> -	cf[index].addr_low = lower_32_bits(addr);
> +	cf[index].efrl32 = cpu_to_be32(efrl);
> +	cf[index].addr_high = cpu_to_be32(upper_32_bits(addr));
> +	cf[index].addr_low = cpu_to_be32(lower_32_bits(addr));
>  }

[Severity: High]
This is a pre-existing issue, but does this patch also need to fix the
endianness of the cdb32 fields in the Command Descriptor Block structs?

When preparing a DMA transfer in fsl_re_prep_dma_genq(), fsl_re_prep_dma_pq(),
or fsl_re_prep_dma_memcpy(), the CPU-endian cdb variable is directly
assigned to the __be32 cdb32 field in the command descriptor block:

drivers/dma/fsl_raid.c:fsl_re_prep_dma_genq() {
    ...
    xor = desc->cdb_addr;
    xor->cdb32 = cdb;
    ...
}

Will the hardware receive byte-swapped opcodes and configuration flags
on little-endian platforms?

>  
>  static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
> @@ -256,9 +256,10 @@ static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
>  	dma_async_tx_descriptor_init(&desc->async_tx, &re_chan->chan);
>  	INIT_LIST_HEAD(&desc->node);
>  
> -	desc->hwdesc.fmt32 = FSL_RE_FRAME_FORMAT << FSL_RE_HWDESC_FMT_SHIFT;
> -	desc->hwdesc.lbea32 = upper_32_bits(paddr);
> -	desc->hwdesc.addr_low = lower_32_bits(paddr);
> +	desc->hwdesc.fmt32 = cpu_to_be32(FSL_RE_FRAME_FORMAT <<
> +					  FSL_RE_HWDESC_FMT_SHIFT);
> +	desc->hwdesc.lbea32 = cpu_to_be32(upper_32_bits(paddr));
> +	desc->hwdesc.addr_low = cpu_to_be32(lower_32_bits(paddr));
>  	desc->cf_addr = cf;
>  	desc->cf_paddr = paddr;

[Severity: High]
This is a pre-existing issue, but does the hardware receive uninitialized
memory here?

The DMA pool allocation for the compound frame and CDB buffer in
fsl_re_chan_alloc_desc() uses dma_pool_alloc() without zeroing the buffer:

drivers/dma/fsl_raid.c:fsl_re_chan_alloc_desc() {
    ...
    cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT, &paddr);
    ...
}

Since functions like fill_cfd_frame() and fsl_re_init_desc() only populate
specific fields, could stale data in uninitialized fields like rbro32 or
dpi_dest_spec cause unpredictable hardware behavior when passed to the
DMA engine?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714233855.870797-1-rosenp@gmail.com?part=1