Re: [PATCH RFC 11/12] dt-bindings: remoteproc: imx-rproc: support virtio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] remoteproc: out-of-bounds access in rproc_virtio_bounce_slot() fallback
- [High] remoteproc: map_records state corruption for sub-page buffers
- [Medium] remoteproc: fwnode reference leak in rproc_handle_vdev()
- [Low] dt-bindings: spi-virtio.yaml has incorrect $id path
- [Low] dt-bindings: remoteproc-virtio uses maxItems on object
--

commit f36ec8f81fd31468ec48cdde83e92aca3916fcb9
Author: Francesco Valla <francesco@valla.it>

dt-bindings: remoteproc: imx-rproc: support virtio

Add support for virtio devices description.

> diff --git a/Documentation/devicetree/bindings/remoteproc/remoteproc-virtio.yaml b/Documentation/devicetree/bindings/remoteproc/remoteproc-virtio.yaml
> new file mode 100644
> index 0000000000000..c4a0d84b14600
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-virtio.yaml

[ ... ]

> +    patternProperties:
> +      "^vdev@[0-9a-f]+$":
> +        type: object
> +
> +        properties:
> +          reg:
> +            description: Virtio device index inside the resource table.
> +            maxItems: 1
> +
> +        additionalProperties:
> +          type: object
> +          $ref: /schemas/virtio/virtio-device.yaml
> +          maxItems: 1

[Severity: Low]
Does this maxItems: 1 have any effect?

maxItems is a JSON schema keyword that strictly applies only to arrays. Since
the child node is declared as type: object, maxItems: 1 is completely ignored
by the schema validator. Should maxProperties be used on the parent node instead
if the goal was to limit the node to a single child?

> diff --git a/Documentation/devicetree/bindings/spi/spi-virtio.yaml b/Documentation/devicetree/bindings/spi/spi-virtio.yaml
> new file mode 100644
> index 0000000000000..1a0a0763bd2fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-virtio.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-virtio.yaml#

[Severity: Low]
Is the $id path correct?

DT schema subsystem guidelines strictly require the $id property path to exactly
match the file's location relative to Documentation/devicetree/bindings/. This
newly introduced file has its $id incorrectly declared as
http://devicetree.org/schemas/i2c/i2c-virtio.yaml# due to a copy-paste error.
This mismatch breaks the schema cross-referencing system during
dt_binding_check.

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 1ed4067148491..e3ecc61cc1def 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c

[ ... ]

> @@ -511,8 +528,16 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>  	 * as device id, then we get duplication in sysfs, so need to use
>  	 * PLATFORM_DEVID_AUTO to auto select device id.
>  	 */
> -	pdev = platform_device_register_data(dev, "rproc-virtio", PLATFORM_DEVID_AUTO, &rvdev_data,
> -					     sizeof(rvdev_data));
> +	memset(&pdev_info, 0, sizeof(pdev_info));
> +	pdev_info.parent = dev;
> +	pdev_info.fwnode = rproc_get_vdev_fwnode(dev->parent, rvdev_data.index);
> +	pdev_info.of_node_reused = true;
> +	pdev_info.name = "rproc-virtio";
> +	pdev_info.id = PLATFORM_DEVID_AUTO;
> +	pdev_info.data = &rvdev_data;
> +	pdev_info.size_data = sizeof(rvdev_data);
> +
> +	pdev = platform_device_register_full(&pdev_info);

[Severity: Medium]
Does this code leak the fwnode reference in rproc_handle_vdev()?

rproc_get_vdev_fwnode() iterates over child nodes and returns a fwnode handle
with an incremented reference count via fwnode_get_next_child_node(). This
returned fwnode is assigned to pdev_info.fwnode.

platform_device_register_full() internally passes this to device_set_node(),
which explicitly takes its own reference to the fwnode by calling
fwnode_handle_get(). After platform_device_register_full() returns,
rproc_handle_vdev() exits without ever dropping its original reference acquired
by rproc_get_vdev_fwnode(), causing a permanent fwnode resource reference leak.

> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index d5e9ff045a28a..d21b3b8044df9 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c

[ ... ]

> +static inline unsigned int rproc_virtio_bounce_slot(struct device *dma_dev,
> +						    dma_addr_t dma_handle)
> +{
> +	const dma_addr_t dma_base = dma_dev_coherent_base(dma_dev);
> +
> +	return (dma_handle - dma_base) >> PAGE_SHIFT;
> +}
> +
> +static dma_addr_t rproc_virtio_map_page(union virtio_map map, struct page *page,
> +					unsigned long offset, size_t size,
> +					enum dma_data_direction dir,
> +					unsigned long attrs)
> +{

[ ... ]

> +	if (paddr >= dma_base && paddr < (dma_base + dma_size)) {
> +		// The allocation details will be recorded also in this case,
> +		// indicating that no bounce buffer was allocated.
> +		map_handle = (dma_addr_t)paddr;
> +		bounce = NULL;
> +	} else {
> +		// Allocate bounce buffer from device coherent memory
> +		bounce = dma_alloc_coherent(dev, size, &map_handle, GFP_KERNEL | __GFP_ZERO);
> +		if (!bounce)
> +			return DMA_MAPPING_ERROR;
> +
> +		// Copy data to bounce buffer
> +		memcpy(bounce, vaddr, size);
> +	}
> +
> +	// Save bounce details
> +	record = &rvdev->map_records[rproc_virtio_bounce_slot(dev, map_handle)];
> +
> +	record->original = vaddr;
> +	record->size = size;
> +	record->bounce = bounce;

[Severity: High]
Can this write out-of-bounds in rproc_virtio_map_page() if the device lacks a
dedicated coherent memory pool, or if the pool is exhausted?

When a bounce buffer is needed, it calls dma_alloc_coherent(). If the device
pool is exhausted or non-existent, it transparently falls back to generic system
memory. The returned map_handle will not be based at dma_base.

rproc_virtio_bounce_slot() will then compute a completely uncontrolled slot
index using (map_handle - dma_base) >> PAGE_SHIFT. The function then writes to
rvdev->map_records[slot], causing an immediate out-of-bounds array write and
corrupting adjacent kernel memory.

[Severity: High]
Can this corrupt the map_records state for sub-page buffers?

If virtio drivers map multiple sub-page buffers (e.g., headers or small
requests) that happen to fall within the same physical page in the coherent
pool, they will compute the exact same slot index because the shift operation
in rproc_virtio_bounce_slot() discards the offset.

The second mapping will overwrite the map_records entry of the first mapping
(record->original and record->size). During rproc_virtio_unmap_page(), this
leads to a WARN_ON(size != record->size) trigger and incorrectly clears the
shared state, corrupting subsequent unmaps for that page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-remoteproc_virtio_map-v1-0-dac8c5eb4aa9@valla.it?part=11