Re: [PATCH 3/3] dmaengine: loongson: Add Loongson-2K0300 APB DMA mux driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: loongson: fix UAF of parent DMA node on route allocation
- [High] dmaengine: loongson: fix NULL pointer dereference on routing error paths
- [Low] dmaengine: loongson: remove unused 'shift' field from mapping structure
--

commit c46c8efe97ecad5eafdc1a9b6a37374efa172f8a
Author: Binbin Zhou <zhoubinbin@loongson.cn>

dmaengine: loongson: Add Loongson-2K0300 APB DMA mux driver

This commit adds a DMA mux driver for the Loongson-2K0300 SoC. It exposes the
routing fields in the chip global configuration registers as an OF DMA router,
which validates the specifier, programs the route, and forwards the translation
to the parent CMC DMA controller.

> diff --git a/drivers/dma/loongson/loongson2-apb-dmamux.c b/drivers/dma/loongson/loongson2-apb-dmamux.c
> new file mode 100644
> index 0000000000000..b6f1f4112d612
> --- /dev/null
> +++ b/drivers/dma/loongson/loongson2-apb-dmamux.c

[ ... ]

> +struct loongson2_dmamux_map {
> +	u32 reg;
> +	u32 mask;
> +	u8 shift;

[Severity: Low]
Is this shift field needed? Code inspection shows it is initialized in the
ls2k0300_dmamux_maps array for all entries but is never read. 

The routing logic in loongson2_dmamux_set_route() correctly manages the
shift operation implicitly using the field_prep() macro, which extracts
the shift directly from the compile-time constant mask.

> +	enum loongson2_dmamux_type type;
> +	u32 high_reg;
> +	u8 high_shift;
> +};

[ ... ]

> +static void *loongson2_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> +					     struct of_dma *ofdma)
> +{

[ ... ]

> +	/* Program the SoC-specific route register */
> +	ret = loongson2_dmamux_set_route(mux, request, channel);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	/*
> +	 * Rewrite dma_spec so the kernel router framework uses it to look
> +	 * up the parent DMA controller and invoke its xlate.
> +	 */
> +	dma_spec->np = mux->dma_np;

[Severity: High]
Does this need to increment the reference count on the cached device node?

Looking at the OF DMA framework, of_dma_router_xlate() explicitly calls
of_node_put(dma_spec_target.np) after translating the channel request. Since
this assignment directly copies the pointer without taking a reference via
of_node_get(), will each DMA request unconditionally decrement the reference
count of the cached parent device node, leading to a premature release?

> +	dma_spec->args[0] = channel;
> +	dma_spec->args[1] = flags;
> +	dma_spec->args_count = mux->config->parent_args_count;
> +
> +	return NULL;
> +}

[ ... ]

> +static int loongson2_dmamux_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	mux->dma_router.dev = dev;
> +	mux->dma_router.route_free = NULL;

[Severity: High]
Will setting this callback to NULL cause a panic on error paths?

In the framework's of_dma_router_xlate() function, if the parent DMA
controller is not yet available (triggering -EPROBE_DEFER), the framework
unconditionally executes ofdma->dma_router->route_free(...) without
checking if the callback exists.

Could this crash the kernel if the parent CMC DMA controller probes after
the peripheral?

> +
> +	platform_set_drvdata(pdev, mux);
> +
> +	ret = of_dma_router_register(dev->of_node, loongson2_dmamux_route_allocate,
> +				     &mux->dma_router);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790068409.git.zhoubinbin@loongson.cn?part=3