Re: [PATCH v3] dmaengine: ti: k3-udma-glue: fix NULL dereference in k3_udma_glue_release_rx_chn()

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

Pre-existing issues:
- [High] The error cleanup path in `k3_udma_glue_release_rx_chn()` can read memory out of bounds (causing a kernel panic) and incorrectly free unowned General Purpose (GP) RX flows (causing resource corruption).
- [High] Integer overflow in the GP flow range boundary check allows an out-of-bounds bitmap clearance, leading to massive memory corruption.
- [High] Double free of `tx_chn->ringtx` occurs on failure of `k3_ringacc_request_rings_pair`.
--

--- Patch [1]: [PATCH v3] dmaengine: ti: k3-udma-glue: fix NULL dereference in k3_udma_glue_release_rx_chn() ---
commit 7e8964856380a3ca6be40518b2b7b3a48d46e4be
Author: Alexander Chesnokov <[email protected]>
dmaengine: ti: k3-udma-glue: fix NULL dereference in k3_udma_glue_release_rx_chn()

This commit fixes a NULL pointer dereference in the error cleanup path of
rx channel request functions. It skips the flow release loop in
k3_udma_glue_release_rx_chn() when the flows array is not allocated.

> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> index 686dc140293eb..70eaf7ee57e68 100644
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c
> @@ -1243,8 +1243,9 @@ void k3_udma_glue_release_rx_chn(struct k3_udma_glue_rx_channel *rx_chn)
>  		rx_chn->psil_paired = false;
>  	}
>  
> -	for (i = 0; i < rx_chn->flow_num; i++)
> -		k3_udma_glue_release_rx_flow(rx_chn, i);
> +	if (rx_chn->flows)
> +		for (i = 0; i < rx_chn->flow_num; i++)
> +			k3_udma_glue_release_rx_flow(rx_chn, i);
>  
>  	if (xudma_rflow_is_gp(rx_chn->common.udmax, rx_chn->flow_id_base))

[Severity: High]
This isn't a bug introduced by this patch, but can this error path lead to an
out-of-bounds read or an incorrect free of unowned General Purpose (GP) RX
flows?

If an early failure occurs (such as devm_kcalloc() returning NULL for
rx_chn->flows) and flow_id_base is -1 (used to request dynamic allocation),
calling xudma_rflow_is_gp() will execute:

drivers/dma/ti/k3-udma-private.c:xudma_rflow_is_gp() {
    ...
	return !test_bit(id, ud->rflow_gp_map);
}

Since test_bit() expects an unsigned long, passing -1 will cast to ULONG_MAX,
causing an out-of-bounds memory read.

Additionally, if flow_id_base is a valid GP flow ID that was not yet allocated
by this channel, is it possible for xudma_free_gp_rflow_range() to
unconditionally free a GP flow range that might be actively used by another
driver?

[Severity: High]
This is also a pre-existing issue, but does __udma_free_gp_rflow_range()
contain a signed integer overflow vulnerability in its bounds check?

If the cleanup path calls xudma_free_gp_rflow_range() with an abnormally large
channel flow count (cnt), the bounds check in __udma_free_gp_rflow_range():

drivers/dma/ti/k3-udma.c:__udma_free_gp_rflow_range() {
    ...
	if (from + cnt > ud->rflow_cnt)
		return -EINVAL;

	bitmap_clear(ud->rflow_gp_map_allocated, from, cnt);
    ...
}

could overflow the signed integer addition (from + cnt) to a negative value.
Since a negative value is not greater than ud->rflow_cnt, this would bypass the
bounds check and pass the huge cnt to bitmap_clear(), potentially clearing bits
across out-of-bounds kernel memory.

>  		xudma_free_gp_rflow_range(rx_chn->common.udmax,
>  					  rx_chn->flow_id_base,
>  					  rx_chn->flow_num);

[Severity: High]
This is another pre-existing issue unrelated to the rx_chn changes, but does
the TX channel error handling contain a double free if completion ring
allocation fails?

If k3_ringacc_request_rings_pair() fails to allocate the completion ring, it
frees the forward ring but leaves the pointer dangling:

drivers/soc/ti/k3-ringacc.c:k3_ringacc_request_rings_pair() {
    ...
	*compl_ring = k3_ringacc_request_ring(ringacc, compl_id, 0);
	if (!(*compl_ring)) {
		k3_ringacc_ring_free(*fwd_ring);
		ret = -ENODEV;
	}
    ...
}

Because *fwd_ring is not set to NULL, the caller's tx_chn->ringtx still holds
the dangling pointer. When k3_udma_glue_release_tx_chn() runs during cleanup:

drivers/dma/ti/k3-udma-glue.c:k3_udma_glue_release_tx_chn() {
    ...
	if (tx_chn->ringtx)
		k3_ringacc_ring_free(tx_chn->ringtx);
    ...
}

will it mistakenly free the same ring a second time, potentially causing
use-after-free and ring allocator state corruption if another process reuses
the ring ID?

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