Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Critical Use-After-Free and uninitialized state exposure when `fastrpc_rpmsg_probe` fails after prematurely registering the misc device. - [High] Preexisting Use-After-Free in `fastrpc_rpmsg_probe()` where `rdev->dma_mask` is left pointing to freed memory on error paths. - [High] Use-After-Free of context bank devices (`sess->dev`) upon fastrpc channel removal due to missing device references. -- commit e928c95aa5ca7ef57a0d4943351ae2891fcf2b1c Author: Vinayak Katoch <[email protected]> misc: fastrpc: iterate CB nodes manually instead of of_platform_populate of_platform_populate() only guarantees that child devices are registered, not that their probes have completed before it returns. This creates a window where fastrpc_cb_init() may not have run for all context bank nodes, leaving the channel context partially initialised. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d4fac2caca867..a153107d00853 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2363,7 +2363,7 @@ static const struct file_operations fastrpc_fops = { > .compat_ioctl = fastrpc_device_ioctl, > }; > > -static int fastrpc_cb_probe(struct platform_device *pdev) > +static int fastrpc_cb_init(struct platform_device *pdev) > { > struct fastrpc_channel_ctx *cctx; > struct fastrpc_session_ctx *sess; > @@ -2385,7 +2385,7 @@ static int fastrpc_cb_probe(struct platform_device > *pdev) > spin_lock_irqsave(&cctx->lock, flags); > if (cctx->sesscount >= FASTRPC_MAX_SESSIONS) { > spin_unlock_irqrestore(&cctx->lock, flags); > - dev_err(&pdev->dev, "too many sessions\n"); > + dev_err(dev, "too many sessions\n"); > return -ENOSPC; > } > dma_bits = cctx->soc_data->dma_addr_bits_default; [Severity: High] This is a pre-existing issue, but does fastrpc_cb_init() save the child device pointer without holding a reference to it? If sess->dev is assigned without get_device(dev), and fastrpc_rpmsg_remove() calls of_platform_depopulate(), the child platform devices are destroyed. Since active file descriptors aren't revoked, could an active ioctl later call fastrpc_dmabuf_alloc() and invoke dma_alloc_coherent() on fl->sctx->dev, using the dangling pointer and leading to a use-after-free? [ ... ] > @@ -2642,12 +2610,29 @@ 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); > - if (err) > - goto err_deregister_fdev; > + of_node_set_flag(rdev->of_node, OF_POPULATED_BUS); > + > + for_each_available_child_of_node_scoped(rdev->of_node, np) { > + struct platform_device *pdev; > + > + if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) > + continue; > + > + pdev = of_platform_device_create(np, NULL, rdev); > + if (!pdev) { > + err = -EINVAL; > + goto err_depopulate; > + } > + > + err = fastrpc_cb_init(pdev); > + if (err) > + goto err_depopulate; [Severity: Critical] This is a pre-existing issue, but is it possible that this error path leads to a use-after-free and uninitialized state exposure? Earlier in fastrpc_rpmsg_probe(), fastrpc_device_register() is called, which makes the misc device available to userspace. A userspace program can then open the device, acquiring a reference to the data structure. If fastrpc_cb_init() fails here, the code jumps to err_depopulate, and eventually calls kfree(data). Since userspace could already have an open file descriptor pointing to data, would this kfree() result in a use-after-free when subsequent ioctls or close() are called? [Severity: High] This isn't a bug introduced by this patch, but does the same error path leave rdev->dma_mask pointing to freed memory? Earlier in fastrpc_rpmsg_probe(), rdev->dma_mask is redirected to point to the dynamically allocated &data->dma_mask. If this probe function fails and frees data via kfree(data), the rpmsg device remains in the system unbound. Will a userspace read of /sys/devices/.../dma_mask_bits trigger a read from freed memory through dma_mask_bits_show() since rdev->dma_mask wasn't restored to its original value? > + } > > return 0; > > +err_depopulate: > + of_platform_depopulate(rdev); > err_deregister_fdev: > if (data->fdevice) > misc_deregister(&data->fdevice->miscdev); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
