Re: [PATCH v3 3/5] remoteproc: mark wc-ioremap carveouts as iomem

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] An integer overflow in `rproc_da_to_va()` allows a bounds-check bypass, leading to potential out-of-bounds reads and writes.
- [High] The `rproc->crash_handler` work item is not properly cancelled during remoteproc teardown, leading to a Use-After-Free if a crash is reported concurrently with device removal.
- [High] The debugfs interfaces for carveout memories and resource tables lack proper locking, allowing them to race with device shutdown and access freed memory.
--

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

remoteproc: mark wc-ioremap carveouts as iomem

Carveouts registered through the shared wc-ioremap helper are backed by
I/O memory, but rproc_da_to_va() only reports that to its callers when
mem->is_iomem is set on the carveout. This patch marks shared wc-ioremap
carveouts as iomem so the framework uses the proper I/O memory accessors.

> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 46080c1c030e8..9afda697271de 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -136,6 +136,7 @@ static inline int rproc_mem_entry_ioremap_wc(struct rproc *rproc,
>  	}
>  
>  	mem->va = (__force void *)va;
> +	mem->is_iomem = true;
>  
>  	return 0;
>  }

While reviewing this, I noticed a few pre-existing issues in the remoteproc
core.

drivers/remoteproc/remoteproc_core.c:rproc_alloc() {
...
	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
...
}

[Severity: High]
This is a pre-existing issue, but is there a missing cancellation for the
crash_handler work item during teardown?

During device removal, rproc_del() and rproc_free() are called, ultimately
triggering rproc_type_release() to free the rproc object via kfree(rproc).

The framework does not appear to call cancel_work_sync(&rproc->crash_handler)
during this teardown sequence.

If a hardware interrupt reports a crash right before or during device
unbinding, could the pending work item execute rproc_crash_handler_work()
after the rproc struct has been freed, resulting in a use-after-free?


drivers/remoteproc/remoteproc_debugfs.c:rproc_carveouts_show() {
	struct rproc *rproc = seq->private;
	struct rproc_mem_entry *carveout;

	list_for_each_entry(carveout, &rproc->carveouts, node) {
		seq_puts(seq, "Carveout memory entry:\n");
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this list traversal lack
proper locking?

The rproc_carveouts_show() function traverses the rproc->carveouts list
without holding rproc->lock or using RCU primitives.

Concurrently, rproc_shutdown() can be triggered (e.g., via a sysfs write
to the state file), which executes rproc_resource_cleanup(). This removes
entries from the rproc->carveouts list and calls kfree() on them.

If a user reads the debugfs file while a shutdown is in progress, could the
concurrent list mutation cause the debugfs read to dereference a freed
pointer, leading to a use-after-free?


drivers/remoteproc/remoteproc_core.c:rproc_da_to_va() {
...
	list_for_each_entry(carveout, &rproc->carveouts, node) {
		int offset = da - carveout->da;
...
		/* try next carveout if da is too large */
		if (offset + len > carveout->len)
			continue;
...
}

[Severity: Critical]
This is a pre-existing issue, but does this bounds check protect against
integer overflow?

Since offset is an int and len is a size_t, could an extremely large len
(e.g., SIZE_MAX) provided by malformed firmware cause offset + len to
overflow the size_t type and wrap around to a small value?

This could cause the bounds check to evaluate to false, bypassing the
protection and returning a valid pointer inside the carveout.

When rproc_elf_load_segments() uses this returned pointer, could it execute
memcpy() or memset() with massively overflowed lengths, resulting in an
out-of-bounds write?

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