Re: [PATCH v6 2/2] remoteproc: add AMD MicroBlaze/V BRAM-based remote processor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Format string vulnerability due to passing `dev_name(priv->dev)` directly as a format string to `rproc_mem_entry_init`.
- [Medium] Clock enable count and remoteproc resource leak on the error path in `amd_bram_rproc_stop()`.
- [Low] The driver references undefined functions (`rproc_mem_entry_ioremap_wc`, `rproc_mem_entry_iounmap`, and `rproc_elf_load_rsc_table_optional`), leading to immediate compilation failures.
--

--- Patch [2]: [PATCH v6 2/2] remoteproc: add AMD MicroBlaze/V BRAM-based remote processor driver ---
commit 86b0a2cc7621606c7e9542f2f1f85e8f3f1d960a
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: add AMD MicroBlaze/V BRAM-based remote processor driver

Add a remoteproc driver for AMD MicroBlaze/V soft-core processor
subsystems instantiated in programmable logic and using dual-port BRAM
for firmware storage and execution.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> new file mode 100644
> index 0000000000000..e4a103cf84558
> --- /dev/null
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c

[ ... ]

> +static int amd_bram_rproc_prepare(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;

[ ... ]

> +	mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start,
> +				   resource_size(&res), da,
> +				   rproc_mem_entry_ioremap_wc,
> +				   rproc_mem_entry_iounmap,

[Severity: Low]
Are rproc_mem_entry_ioremap_wc and rproc_mem_entry_iounmap declared?

It looks like referencing these undefined functions causes a compilation
failure when building with CONFIG_AMD_MBV_BRAM_REMOTEPROC.

> +				   dev_name(priv->dev));

[Severity: Medium]
Could this lead to a format string vulnerability?

rproc_mem_entry_init() is a variadic function that uses its name argument
as a format string in a call to vsnprintf(). By passing dev_name(priv->dev)
directly as the format string, an uninitialized stack read regression could
occur if a maliciously crafted device tree overlay contains a '%' character
in the node name.

Would it be safer to pass "%s", dev_name(priv->dev) instead?

> +	if (!mem)
> +		return -ENOMEM;
> +
> +	rproc_add_carveout(rproc, mem);
> +	rproc_coredump_add_segment(rproc, da, resource_size(&res));
> +
> +	return 0;
> +}

[ ... ]

> +static int amd_bram_rproc_stop(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	/* Assert reset before disabling the processor clock. */
> +	ret = gpiod_set_value_cansleep(priv->reset, 1);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
> +		return ret;

[Severity: Medium]
Does this error path leak the clock enable count by returning early
before calling clk_disable_unprepare(priv->clk)?

Also, if this early return occurs, the remoteproc core's rproc_shutdown()
aborts before calling rproc_resource_cleanup(). Does this cause the
rproc_mem_entry objects on the carveouts list to leak when the device
is later removed via rproc_del()?

> +	}
> +
> +	/* Disable clock after asserting reset */
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int amd_bram_rproc_parse_fw(struct rproc *rproc,
> +				   const struct firmware *fw)
> +{
> +	rproc_elf_load_rsc_table_optional(rproc, fw, dev_dbg,
> +					  "no resource table found\n");

[Severity: Low]
Is rproc_elf_load_rsc_table_optional() declared?

Similar to the callbacks in amd_bram_rproc_prepare(), this appears to be
an undefined function that triggers a build failure.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714202441.554065-1-ben.levinsky@amd.com?part=2