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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `.find_loaded_rsc_table` callback in `rproc_ops` breaks firmware IPC initialization.
- [Low] Undefined functions used in driver cause immediate build failure.

Pre-existing issues:
- [Critical] Integer overflow in `rproc_coredump()` leads to a massive heap buffer overflow.
--

commit 114ec0b0402b4fa78538fad8bece1af5eac1df59
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: add AMD BRAM-based remote processor driver

Add a remoteproc driver for AMD soft-core processor subsystems using BRAM
for firmware storage. The driver parses the memory window from devicetree,
registers the carveout, and controls the processor via reset GPIO and clock.

> diff --git a/drivers/remoteproc/amd_bram_rproc.c b/drivers/remoteproc/amd_bram_rproc.c
> new file mode 100644
> index 0000000000000..a595875f9ce8c
> --- /dev/null
> +++ b/drivers/remoteproc/amd_bram_rproc.c
[ ... ]
> +static int amd_bram_rproc_prepare(struct rproc *rproc)
> +{
[ ... ]
> +	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,
> +				   dev_name(priv->dev));

[Severity: Low]
Are rproc_mem_entry_ioremap_wc and rproc_mem_entry_iounmap defined anywhere
in the kernel tree? It appears these functions are missing, which would cause
the driver to fail to compile and link.

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

[Severity: Critical]
This is a pre-existing issue, but does adding an unbounded coredump segment
size here risk an integer overflow in the remoteproc core?

In drivers/remoteproc/remoteproc_coredump.c:rproc_coredump(), segment sizes
are accumulated into data_size without overflow checks:

        if (dump_conf == RPROC_COREDUMP_ENABLED)
                data_size += segment->size;

If a massive size (approaching U32_MAX on 32-bit systems) is provided from
the devicetree reg property, this could wrap around, leading to a much
smaller vmalloc allocation. Later, rproc_copy_segment() is called with the
original massive size, writing blindly into the undersized heap buffer.

> +
> +	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 defined anywhere? Similar to the
allocator callbacks above, this function appears to be missing from the tree.

> +	return 0;
> +}
> +
> +static const struct rproc_ops amd_bram_rproc_ops = {
> +	.prepare	= amd_bram_rproc_prepare,
> +	.start		= amd_bram_rproc_start,
> +	.stop		= amd_bram_rproc_stop,
> +	.load		= rproc_elf_load_segments,
> +	.sanity_check	= rproc_elf_sanity_check,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.parse_fw	= amd_bram_rproc_parse_fw,
> +};

[Severity: High]
Will firmware IPC initialize correctly without a .find_loaded_rsc_table
callback?

When the core loads a firmware with a resource table, rproc_fw_boot() calls
rproc_find_loaded_rsc_table(). Since .find_loaded_rsc_table is omitted here,
it returns NULL. This causes the core to silently skip copying the updated
rproc->cached_table to the device's loaded table in memory, leaving the
firmware to use uninitialized or FW_RSC_ADDR_ANY addresses for
communication.

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