Re: [PATCH 6/6] remoteproc: amd_mbv_bram: Add RPMsg support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] remoteproc: UAF due to flawed rx_work teardown and early returns
- [Medium] remoteproc: memory leak of carveouts on prepare() failure
--

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

remoteproc: amd_mbv_bram: Add RPMsg support

This commit adds RPMsg support to the AMD MicroBlaze/V BRAM remoteproc
driver. It registers named reserved-memory carveouts and maps them for
virtqueues. It also introduces mailbox support to send and receive
virtqueue notification IDs.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index da621f62f7df5..65d2c3946325e 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> +static int amd_bram_rproc_add_reserved_mem(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct rproc_mem_entry *mem;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	if (!of_property_present(np, "memory-region"))
> +		return 0;
> +
> +	count = of_reserved_mem_region_count(np);
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		struct resource res;
> +		const char *name;
> +
> +		ret = of_property_read_string_index(np, "memory-region-names",
> +						    i, &name);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to read memory-region name %d\n",
> +					     i);

[Severity: Medium]
In amd_bram_rproc_prepare(), resources are added to rproc->carveouts and
rproc->dump_segments before returning a call to this function.

If an error occurs in this loop, the function returns the error early.

The remoteproc core's rproc_fw_boot() handles a failure from ops->prepare
by explicitly bypassing the clean_up_resources label, which means
rproc_resource_cleanup() is never called.

Does this permanently leak any rproc_mem_entry and rproc_dump_segment
objects added before the failure occurs? Retrying the boot might also append
duplicate entries to the lists. Shouldn't the driver clean up its own
carveouts on error, or rely on rproc_resource_cleanup() before returning?

[ ... ]
> @@ -80,7 +279,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
>  	rproc_add_carveout(rproc, mem);
>  	rproc_coredump_add_segment(rproc, da, resource_size(&res));
>  
> -	return 0;
> +	return amd_bram_rproc_add_reserved_mem(rproc);
>  }
>  
>  static int amd_bram_rproc_start(struct rproc *rproc)
> @@ -111,10 +310,17 @@ static int amd_bram_rproc_start(struct rproc *rproc)
>  	/* Hold reset asserted long enough to provide a clean reset pulse. */
>  	usleep_range(10, 20);
>  
> +	ret = amd_bram_rproc_request_mboxes(priv);
> +	if (ret) {
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}
> +
>  	/* Deassert reset and let the processor run. */
>  	ret = gpiod_direction_output(priv->reset, 0);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
> +		amd_bram_rproc_free_mboxes(priv);
>  		clk_disable_unprepare(priv->clk);
>  		return ret;
>  	}

[Severity: High]
If gpiod_direction_output() fails here, amd_bram_rproc_free_mboxes() is
called but cancel_work_sync(&priv->rx_work) is missing.

Could any work scheduled by a mailbox interrupt between request and free
remain queued? If it executes later, could it access the freed priv pointer
and cause a use-after-free?

[ ... ]
> @@ -133,6 +339,10 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}

[Severity: High]
If gpiod_direction_output() fails, this returns early without cancelling the
workqueue or freeing the mailbox channels.

Since the core rproc_del() ignores the stop error and frees the rproc and
priv structs, wouldn't this leave active mailboxes and workqueues pointing
to freed memory?

> +	cancel_work_sync(&priv->rx_work);
> +	bitmap_zero(priv->pending_notify_ids,
> +		    ARRAY_SIZE(priv->notify_msgs));
> +	amd_bram_rproc_free_mboxes(priv);
>  
>  	/* Disable clock after asserting reset */
>  	clk_disable_unprepare(priv->clk);

[Severity: High]
Since cancel_work_sync() is called before freeing the mailboxes, the
mailbox interrupt is still active.

Could an incoming notification trigger amd_bram_rproc_rx_callback() and
reschedule the work right after it was cancelled, leading to a
use-after-free on driver unbind?

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