Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Critical] The patch introduces a critical lifetime mismatch by tying the reserved memory region's release to the device unbind phase via `devm_of_reserved_mem_device_init()`. The fastrpc driver allows user-specific DMA buffers (held by `miscdev` open file descriptors) to outlive the device's unbind phase. When userspace closes the file descriptor after `remove()` has completed, `fastrpc_buf_free()` calls `dma_free_coherent()` on `rdev`. Because the devres cleanup has already executed and cleared the device's coherent memory pool (`dev->dma_mem = NULL`), `dma_free_coherent()` falls back to freeing the reserved memory pages into the general kernel buddy allocator, causing severe memory corruption. Pre-existing issues: - [High] In `fastrpc_rpmsg_probe()`, `rdev->dma_mask` is pointed to the dynamically allocated `&data->dma_mask`. If the probe function fails later (e.g., in `of_platform_populate()`), `data` is freed via `kfree(data)`, but `rdev->dma_mask` is not cleared. This leaves a dangling pointer in the persistent `rpmsg_device` structure that can lead to a Use-After-Free if the DMA mask is later queried by the driver core or during reprobe. - [High] For `SDSP_DOMAIN_ID`, the entire reserved memory region is assigned to the DSP via `qcom_scm_assign_mem()`. However, the driver fails to revert this assignment back to `QCOM_SCM_VMID_HLOS` on subsequent probe error paths and in `fastrpc_rpmsg_remove()`. As a result, the physical memory remains permanently owned by TrustZone, preventing driver reload and potentially causing hypervisor faults on subsequent Linux accesses. - [High] Missing NULL check for `cctx->rpdev` in `fastrpc_remote_heap_alloc` leading to a potential crash if ioctls are called after device removal. - [High] The fastrpc misc devices are registered and exposed to userspace before their critical context data structures (locks, IDR, refcount, rpdev) are initialized. - [High] Use-after-free of child platform_device pointer during buffer freeing. -- commit 690dc3f2f1a71f7a802b45a29611ac1e33c88795 Author: Mukesh Ojha <[email protected]> misc: fastrpc: Use devm_of_reserved_mem_device_init() The commit updates the fastrpc driver to use the devres-managed devm_of_reserved_mem_device_init() during probe. This ensures the reserved memory region is automatically released on device removal. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 90fd669636ec1..21f084c76f81d 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2562,7 +2562,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > return -EINVAL; > } > > - if (of_reserved_mem_device_init_by_idx(rdev, rdev->of_node, 0)) > + if (devm_of_reserved_mem_device_init(rdev)) [Severity: Critical] Does this introduce a lifetime mismatch for user-specific DMA buffers? By tying the reserved memory region's release to the device unbind phase via devres, the region is cleared before all userspace file descriptors might be closed. The fastrpc driver allows user-specific DMA buffers (held by miscdev open file descriptors) to outlive the device's unbind phase. It allocates user-managed memory from the rdev reserved memory pool, which outlives device unbind: drivers/misc/fastrpc.c:fastrpc_remote_heap_alloc() { ... struct device *rdev = &fl->cctx->rpdev->dev; return __fastrpc_buf_alloc(fl, rdev, size, obuf); ... } When userspace closes the file descriptor after remove() has completed, fastrpc_buf_free() calls dma_free_coherent() on rdev: drivers/misc/fastrpc.c:fastrpc_buf_free() { ... dma_free_coherent(buf->dev, buf->size, buf->virt, fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr)); ... } Because the devres cleanup has already executed and cleared the device's coherent memory pool, will dma_free_coherent() fall back to freeing the reserved memory pages into the general kernel buddy allocator, causing severe memory corruption? > dev_info(rdev, "no reserved DMA memory for FASTRPC\n"); > > vmcount = of_property_read_variable_u32_array(rdev->of_node, [Severity: High] This is a pre-existing issue, but does fastrpc_rpmsg_probe() leave a dangling pointer if probe fails? During probe, rdev->dma_mask is pointed to dynamically allocated memory: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... rdev->dma_mask = &data->dma_mask; ... } If a subsequent step like of_platform_populate() fails, the error path frees data: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err_free_data: kfree(data); return err; } Since rdev->dma_mask is not cleared, could this result in a Use-After-Free if the rpmsg bus or driver core later queries the dangling DMA mask pointer? [Severity: High] This is a pre-existing issue, but does the driver permanently lose physical memory to TrustZone on probe error or device unbind? For SDSP_DOMAIN_ID, the entire reserved memory region is assigned to the DSP: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms, data->vmperms, data->vmcount); ... } However, neither the probe error paths: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err_deregister_fdev: if (data->fdevice) ... } nor fastrpc_rpmsg_remove() revert this assignment back to QCOM_SCM_VMID_HLOS. Can this cause hypervisor faults (XPU/SMMU violation) and kernel panics if the kernel attempts to reuse the physical memory after device unbind or driver reload? [Severity: High] This is a pre-existing issue, but is there a missing NULL check for cctx->rpdev in fastrpc_remote_heap_alloc()? During device unbind, cctx->rpdev is cleared: drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() { ... cctx->rpdev = NULL; ... } If ioctls like FASTRPC_IOCTL_MMAP are called after device removal, fastrpc_remote_heap_alloc() dereferences the pointer without checking: drivers/misc/fastrpc.c:fastrpc_remote_heap_alloc() { ... struct device *rdev = &fl->cctx->rpdev->dev; ... } Could this result in a NULL pointer dereference and a kernel crash? [Severity: High] This is a pre-existing issue, but are the fastrpc misc devices exposed to userspace too early during probe? The misc device is registered before its downstream context fields are initialized: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err = fastrpc_device_register(rdev, data, secure_dsp, domain); ... } If userspace calls open() on the device node exactly between misc_register() and the initialization of context structures (like locks, IDR, refcount), could functions like fastrpc_session_alloc() acquire an uninitialized lock: drivers/misc/fastrpc.c:fastrpc_session_alloc() { ... spin_lock_irqsave(&cctx->lock, flags); ... } Would this lead to a kernel panic under lock debugging? [Severity: High] This is a pre-existing issue, but is there a use-after-free of the child platform_device pointer during buffer freeing? When a buffer is allocated, an un-referenced pointer to a child platform_device is stored: drivers/misc/fastrpc.c:__fastrpc_buf_alloc() { ... buf->dev = dev; ... } If the parent rpmsg device is unbound, of_platform_depopulate() destroys the child platform devices. If a user subsequently closes their file descriptor, fastrpc_buf_free() dereferences the freed pointer: drivers/misc/fastrpc.c:fastrpc_buf_free() { ... dma_free_coherent(buf->dev, buf->size, buf->virt, fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr)); ... } Could this result in memory corruption or a kernel crash when the dangling pointer is passed to dma_free_coherent()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
