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 <[email protected]> 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/[email protected]?part=11
