Re: [PATCH v3 2/2] remoteproc: xlnx: enable auto boot feature

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Dereferencing r5_cores[j]->dev and calling rproc_free() after rproc_add() fails causes a Use-After-Free and Double-Free.

Pre-existing issues:
- [Critical] The driver blindly trusts a physical address and size provided by the remote processor in shared memory.
- [High] The driver frees the mailbox information structure without canceling the associated background workqueue.
- [High] Data race on shared IPI transmit buffer ipi->tx_mc_buf during remoteproc kick.
- [High] A TOCTOU race condition exists between counting available DT child nodes and iterating over them, leading to a heap buffer overflow.
--

commit 0aa3216df858c52c70549e4af6feda736a8dcc9b
Author: Tanmay Shah <tanmay.shah@amd.com>

remoteproc: xlnx: enable auto boot feature

The remoteproc framework has capability to start (or attach to) the remote
processor automatically if auto boot flag is set by the driver during
probe. This patch delays adding the remoteproc instance until the end of
cluster initialization to ensure all hardware is initialized before starting
the remote processor.

> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 45a62cb980725..652030f9cea20 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
[ ... ]
> @@ -1280,6 +1284,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>  			if (zynqmp_r5_get_rsc_table_va(r5_core))

[Severity: Critical]
This is a pre-existing issue, but does zynqmp_r5_get_rsc_table_va() blindly
trust the physical address and size provided by the remote processor in shared
memory?

The driver reads the resource table address and size from a shared memory
region and passes this unvalidated physical address directly to ioremap_wc().
Could a compromised remote processor supply the physical address of sensitive
host kernel memory, allowing it to overwrite arbitrary host memory and
escalate privileges when the host kernel writes status updates into it?

>  				dev_dbg(r5_core->dev, "rsc tbl not found\n");
>  			r5_core->rproc->state = RPROC_DETACHED;
> +			r5_core->rproc->auto_boot = true;
>  		}
>  	}
[ ... ]
> @@ -1304,7 +1309,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>  	enum rpu_oper_mode fw_reg_val;
>  	struct device **child_devs;
>  	enum rpu_tcm_comb tcm_mode;
> -	int core_count, ret, i;
> +	int core_count, ret, i, j;
>  	struct mbox_info *ipi;

[Severity: High]
This is a pre-existing issue, but is there a race condition between
counting the available child nodes and iterating over them?

The driver allocates arrays based on the initial count from
of_get_available_child_count(), and then iterates over the child nodes
using for_each_available_child_of_node_scoped().

Since the device tree lock is not held across both operations, could
concurrent modifications (like applying an overlay) cause the loop to yield
more child nodes than initially allocated, overflowing the arrays?

[Severity: High]
This is a pre-existing issue, but is there a data race on the shared IPI
transmit buffer (ipi->tx_mc_buf) during a remoteproc kick?

In zynqmp_r5_rproc_kick(), the virtqueue ID is written to the shared buffer
before calling mbox_send_message(). Since there is no internal locking to
serialize kicks, could concurrent calls from different threads kicking
different virtqueues race on this buffer and overwrite the virtqueue ID before
it is transmitted?

[ ... ]
> @@ -1435,16 +1440,31 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>  		goto release_r5_cores;
>  	}
>  
> +	for (j = 0; j < cluster->core_count; j++) {
> +		/* Add R5 remoteproc core */
> +		ret = rproc_add(r5_cores[j]->rproc);
> +		if (ret) {
> +			dev_err_probe(r5_cores[j]->dev, ret,
> +				      "failed to add remoteproc\n");

[Severity: Critical]
If rproc_add() fails, it drops the initial reference count to 0 and
frees the rproc object, which also frees the appended private data
(r5_cores[j]). Does logging the failure with r5_cores[j]->dev cause a
use-after-free here?

> +			goto delete_r5_cores;
> +		}
> +	}
> +
>  	kfree(child_devs);
>  	return 0;
>  
> +delete_r5_cores:
> +	i = core_count - 1;
> +	/* delete previous added rproc */
> +	while (--j >= 0)
> +		rproc_del(r5_cores[j]->rproc);
> +
>  release_r5_cores:
>  	while (i >= 0) {
>  		put_device(child_devs[i]);
>  		if (r5_cores[i]) {
>  			zynqmp_r5_free_mbox(r5_cores[i]->ipi);

[Severity: High]
This is a pre-existing issue, but if an error occurs and we clean up by
calling zynqmp_r5_free_mbox(), the driver frees the ipi structure without
canceling the associated background workqueue (ipi->mbox_work).

If the remote processor is already running and sends a message, scheduling the
work, will the system workqueue subsequently execute the pending work and
dereference the freed ipi object?

>  			of_reserved_mem_device_release(r5_cores[i]->dev);
> -			rproc_del(r5_cores[i]->rproc);
>  			rproc_free(r5_cores[i]->rproc);

[Severity: Critical]
Additionally, since the rproc object was already freed by rproc_add()
on failure, will jumping to delete_r5_cores and then release_r5_cores
lead to a double-free when it calls rproc_free(r5_cores[i]->rproc)?

>  		}
>  		i--;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527051611.194844-1-tanmay.shah@amd.com?part=2