Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] misc: fastrpc: double of_node_put() in fastrpc_cb_devices_create() - [High] misc: fastrpc: missing DMA and IOMMU setup in fastrpc_cb_devices_create() - [High] misc: fastrpc: premature misc_register() before fastrpc_cb_devices_create() --
commit 17aff65cbac844472a8706f83a0e300c3444f667 Author: Vinayak Katoch <[email protected]> misc: fastrpc: iterate CB nodes manually instead of of_platform_populate This commit replaces of_platform_populate() and the separate fastrpc_cb_driver platform driver with manual iteration over child DT nodes in fastrpc_cb_devices_create() and cleanup in fastrpc_cb_devices_destroy() so context banks are initialized synchronously during fastrpc_rpmsg_probe(). > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index af18ff1992eed..caa479f304c1c 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device > *pdev) > return 0; > } > > -static void fastrpc_cb_remove(struct platform_device *pdev) > +static void fastrpc_cb_devices_destroy(struct rpmsg_device *rpdev) > { > - struct fastrpc_channel_ctx *cctx = dev_get_drvdata(pdev->dev.parent); > - struct fastrpc_session_ctx *sess = dev_get_drvdata(&pdev->dev); > + struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); > + struct device *rdev = &rpdev->dev; > unsigned long flags; > int i; > > spin_lock_irqsave(&cctx->lock, flags); > - for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) { > - if (cctx->session[i].sid == sess->sid) { > - cctx->session[i].valid = false; > - cctx->sesscount--; > - } > - } > + for (i = 0; i < cctx->sesscount; i++) > + cctx->session[i].valid = false; > spin_unlock_irqrestore(&cctx->lock, flags); > + > + device_for_each_child_reverse(rdev, NULL, of_platform_device_destroy); > } > > -static const struct of_device_id fastrpc_match_table[] = { > - { .compatible = "qcom,fastrpc-compute-cb" }, > - { } > -}; > +static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev) > +{ > + struct device *rdev = &rpdev->dev; > + struct platform_device *pdev; > + struct device_node *np; > + int err; > > -static struct platform_driver fastrpc_cb_driver = { > - .probe = fastrpc_cb_probe, > - .remove = fastrpc_cb_remove, > - .driver = { > - .name = "qcom,fastrpc-cb", > - .of_match_table = fastrpc_match_table, > - .suppress_bind_attrs = true, > - }, > -}; > + for_each_available_child_of_node(rdev->of_node, np) { > + if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) { > + of_node_put(np); > + continue; > + } [Severity: High] Won't calling of_node_put(np) before continue in fastrpc_cb_devices_create() cause a double-put on np when an available child node is not compatible with "qcom,fastrpc-compute-cb"? for_each_available_child_of_node() advances by calling of_get_next_available_child() -> of_get_next_status_child() in drivers/of/base.c: of_node_put(prev); raw_spin_unlock_irqrestore(&devtree_lock, flags); return next; Because the loop increment expression dereferences prev->sibling and automatically calls of_node_put(prev) on each step, dropping the reference on np explicitly before continue underflows the struct device_node reference count and can lead to a use-after-free. > + > + pdev = of_platform_device_create(np, NULL, rdev); > + if (!pdev) { > + of_node_put(np); > + fastrpc_cb_devices_destroy(rpdev); > + return -ENODEV; > + } > + > + err = fastrpc_cb_init(pdev); > + if (err) { > + of_node_put(np); > + fastrpc_cb_devices_destroy(rpdev); > + return err; > + } > + } > + > + return 0; > +} [Severity: High] With fastrpc_cb_driver removed, how are DMA and IOMMU configuration and probe deferral handled for the context bank platform devices created in fastrpc_cb_devices_create()? Previously, binding each qcom,fastrpc-compute-cb device to fastrpc_cb_driver went through really_probe() in drivers/base/dd.c, which checked supplier links via device_links_check_suppliers(dev), set dev->driver, and called platform_dma_configure() in drivers/base/platform.c: if (is_of_node(fwnode)) { ret = of_dma_configure(dev, to_of_node(fwnode), true); } else if (is_acpi_device_node(fwnode)) { attr = acpi_get_dma_attr(to_acpi_device_node(fwnode)); ret = acpi_dma_configure(dev, attr); } /* @dev->driver may not be valid when we're called from the IOMMU layer */ if (ret || !drv || to_platform_driver(drv)->driver_managed_dma) return ret; ret = iommu_device_use_default_domain(dev); Without a bound platform_driver or an explicit of_dma_configure() call in fastrpc_cb_devices_create() or fastrpc_cb_init(), pdev->dev.driver is always NULL: 1. Because the iommus property resides on the child compute-cb DT nodes rather than the parent rpmsg node, fw_devlink only creates proxy sync_state links on rdev. During of_platform_device_create() -> device_add(), iommu_init_device() in drivers/iommu/iommu.c calls dev->bus->dma_configure(dev) with dev->driver == NULL and ignores its return value. If the SMMU has not probed yet and of_iommu_configure() returns -EPROBE_DEFER in of_dma_configure_id() (drivers/of/device.c), it returns early before arch_setup_dma_ops(dev, coherent), yet fastrpc_cb_init() still succeeds and marks sess->valid = true with unconfigured DMA ops and no IOMMU translation. A subsequent bus_iommu_probe() replay of of_dma_configure_id() also overwrites *dev->dma_mask with the 32-bit coherent_dma_mask, clobbering the 34-bit mask set via dma_set_mask() in fastrpc_cb_init(). 2. When CONFIG_IOMMU_API is disabled, iommu_bus_notifier is not registered and of_dma_configure() is never called on pdev->dev at all. 3. Even when the SMMU has already probed, platform_dma_configure() returns before calling iommu_device_use_default_domain(dev) because dev->driver is NULL (leaving group->owner_cnt at 0 while fastrpc performs DMA allocations and dma_buf mappings on sess->dev), and managed supplier device links to pdev->dev remain unactivated because device_links_driver_bound() never runs. > @@ -2639,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > data->rpdev = rpdev; > dev_set_drvdata(&rpdev->dev, data); > > - err = of_platform_populate(rdev->of_node, NULL, NULL, rdev); > + err = fastrpc_cb_devices_create(rpdev); > if (err) > goto err_deregister_fdev; > [Severity: High] Could calling fastrpc_cb_devices_create() after fastrpc_device_register() in fastrpc_rpmsg_probe() still expose partially initialized sessions to userspace and trigger a use-after-free on failure? Earlier in fastrpc_rpmsg_probe(), fastrpc_device_register() calls misc_register() before data->refcount, data->users, data->lock, data->ctx_idr, and data->rpdev are initialized, and before fastrpc_cb_devices_create(rpdev) is called (as well as before ADSP duplicate session creation in "misc: fastrpc: move ADSP duplicate session creation to the driver"). Inside fastrpc_cb_init(), sess->valid = true and cctx->sesscount++ are also published under cctx->lock before dma_set_mask() is executed: sess = &cctx->session[cctx->sesscount++]; sess->used = false; sess->valid = true; sess->dev = dev; sess->sid = sid; ... spin_unlock_irqrestore(&cctx->lock, flags); rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits)); if (rc) { dev_err(dev, "%u-bit DMA enable failed\n", dma_bits); return rc; } Because the miscdevices are already registered while fastrpc_cb_devices_create() iterates over the DT child nodes, a concurrent fastrpc_device_open() -> fastrpc_session_alloc() can still observe zero or a partial set of sessions, or acquire an earlier session and take a reference on data via fastrpc_channel_ctx_get(cctx). Unlike of_platform_populate() which previously returned 0 even if child device creation failed, if dma_set_mask() fails or a later CB node fails in of_platform_device_create() or fastrpc_cb_init(), fastrpc_cb_devices_create() destroys the CB platform devices and fastrpc_rpmsg_probe() jumps to err_deregister_fdev -> err_free_data: kfree(data), freeing data while an open file descriptor still holds references to fl->cctx and fl->sctx->dev. Should fastrpc_device_register() be called only after cctx and all CB sessions have been fully initialized? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
