Re: [PATCH v2 20/23] dmaengine: sdxi: Encode nop, copy, and interrupt descriptors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] User-triggerable WARN_ON_ONCE() for data-path validation poses a Denial of Service risk.
- [Medium] Integer overflow in buffer overlap calculation allows invalid configurations to bypass validation.
- [Low] Redundant clamp_val() operation acts as dead code.
--

commit cfd7b5f9b8481ef46337c6e93c25ab0383c6dbcc
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Encode nop, copy, and interrupt descriptors

Introduce low-level support for serializing nop, copy, and interrupt
operations to the descriptor ring of a client context. Each operation
has a dedicated encoder function overlaying the generic sdxi_desc struct.

> diff --git a/drivers/dma/sdxi/descriptor.c b/drivers/dma/sdxi/descriptor.c
> index be2a9244ce194..41019e747528c 100644
> --- a/drivers/dma/sdxi/descriptor.c
> +++ b/drivers/dma/sdxi/descriptor.c

[ ... ]

> +VISIBLE_IF_KUNIT int __must_check sdxi_encode_size32(u64 size, __le32 *dest)
> +{
> +	/*
> +	 * sizes are encoded as value - 1:
> +	 * value    encoding
> +	 *     1           0
> +	 *     2           1
> +	 *   ...
> +	 *    4G  0xffffffff
> +	 */
> +	if (WARN_ON_ONCE(size > SZ_4G) ||
> +	    WARN_ON_ONCE(size == 0))
> +		return -EINVAL;

Could this WARN_ON_ONCE() be triggered by invalid input from a client driver
or userspace?

If the system is configured with panic_on_warn, passing an invalid length
like 0 might trigger a kernel panic. Should this gracefully return -EINVAL
without the warning?

> +	size = clamp_val(size, 1, SZ_4G);

This isn't a bug, but is this clamp_val() necessary?

The previous if statement already returns -EINVAL if the size is outside
these bounds, so this clamping operation might be redundant.

> +	*dest = cpu_to_le32((u32)(size - 1));
> +	return 0;
> +}

[ ... ]

> +int sdxi_encode_copy(struct sdxi_desc *desc, const struct sdxi_copy *params)
> +{

[ ... ]

> +	/*
> +	 * Reject overlapping src and dst. "Software ... shall not
> +	 * overlap the source buffer, destination buffer, Atomic
> +	 * Return Data, or completion status block." - SDXI 1.0 5.6
> +	 * Memory Consistency Model
> +	 */
> +	if (range_overlaps(&(const struct range) {
> +				   .start = params->src,
> +				   .end   = params->src + params->len - 1,
> +			   },
> +			   &(const struct range) {
> +				   .start = params->dst,
> +				   .end   = params->dst + params->len - 1,
> +			   }))

Does this calculation overflow if an IOMMU assigns an address near U64_MAX?

If params->src + params->len wraps around, the end address could become
smaller than the start address. This might bypass the range_overlaps()
check, allowing overlapping or invalid wrapping buffers to be submitted.

> +		return -EINVAL;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=20