Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] misc: fastrpc: Use-After-Free of ext_cb array due to concurrent krealloc during probe - [High] misc: fastrpc: memory leak of dynamically allocated ext_cb array elements on probe error path - [High] misc: fastrpc: premature device registration allows UAF on probe failure - [High] of: double-free of device node in fastrpc_cb_devices_destroy - [Medium] misc: fastrpc: memory leak of ext_cb on probe error path - [Medium] misc: fastrpc: memory leak of ext_cb array on probe error path --
commit 670e94cdc2cb15ca5f0de40ab9985878a7a72c7b Author: Vinayak Katoch <[email protected]> misc: fastrpc: add extended context bank support This commit detects extended context banks by checking for the iommu-ranges property on the context bank node. It registers them in a separate ext_cb array, allowing the mapping layer to direct large allocations into the wider IOVA window. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 5c7bb56cf46d0..d40cdcd5dec30 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2352,6 +2359,43 @@ static int fastrpc_cb_init(struct platform_device > *pdev) > of_n_addr_cells(dev->of_node) - 1, &sid)) > dev_info(dev, "FastRPC Session ID not specified in DT\n"); > > + if (of_property_present(dev->of_node, "iommu-ranges")) > + is_extended_cb = true; > + > + if (is_extended_cb) { > + struct fastrpc_session_ctx **new_ext; > + > + sess = kzalloc_obj(*sess); > + if (!sess) > + return -ENOMEM; [Severity: High] Can this new failure path trigger a use-after-free regression due to premature device registration? If this allocation fails, fastrpc_cb_init() returns an error, which propagates up to fastrpc_rpmsg_probe(). In fastrpc_rpmsg_probe(), the misc device is registered and exposed to userspace before fastrpc_cb_devices_create() is called. If userspace opens the device concurrently during probe, it obtains an active file descriptor. When the error path in fastrpc_rpmsg_probe() unconditionally frees the channel context (data), any subsequent ioctl() or close() by userspace will dereference the freed pointer. > + > + rc = dma_set_mask(dev, DMA_BIT_MASK(40)); > + if (rc) { > + dev_err(dev, "40-bit DMA enable failed\n"); > + kfree(sess); > + return rc; > + } > + > + new_ext = krealloc(cctx->ext_cb, > + (cctx->ext_cb_count + 1) * > sizeof(*cctx->ext_cb), > + GFP_KERNEL); [Severity: High] Does this code cause a use-after-free regression of the ext_cb array pointer? krealloc() is called without holding cctx->lock. If krealloc() allocates a new block, it frees the old cctx->ext_cb pointer. The cctx->ext_cb pointer is only updated to new_ext later inside the spinlock. During this window, cctx->ext_cb points to freed memory, and concurrent readers may fetch the freed pointer if they race with the probe thread. [Severity: High] Does this code also introduce a regression that leaks the dynamically allocated ext_cb array elements on the probe error path? If a failure occurs during the context bank initialization loop (e.g., if krealloc() fails for a second child node after the first succeeded), the error propagates to fastrpc_rpmsg_probe(). The error path in fastrpc_rpmsg_probe() directly executes kfree(data), bypassing the proper teardown sequence in fastrpc_channel_ctx_free(). Because fastrpc_channel_ctx_free() is circumvented, the sess objects allocated by this patch are permanently leaked. [Severity: Medium] Does this code also leak the ext_cb array itself on the probe error path? Similar to the array elements, because the probe error path executes kfree(data) directly and skips calling fastrpc_channel_ctx_free(), the new_ext block allocated here is never freed and permanently leaked. > + if (!new_ext) { > + kfree(sess); > + return -ENOMEM; > + } > + > + spin_lock_irqsave(&cctx->lock, flags); > + cctx->ext_cb = new_ext; > + sess->valid = true; > + sess->dev = dev; > + sess->sid = sid; > + dev_set_drvdata(dev, sess); > + cctx->ext_cb[cctx->ext_cb_count++] = sess; > + spin_unlock_irqrestore(&cctx->lock, flags); > + > + return 0; > + } > + [ ... ] > @@ -2391,6 +2435,8 @@ static void fastrpc_cb_devices_destroy(struct > rpmsg_device *rpdev) > spin_lock_irqsave(&cctx->lock, flags); > for (i = 0; i < cctx->sesscount; i++) > cctx->session[i].valid = false; > + for (i = 0; i < cctx->ext_cb_count; i++) > + cctx->ext_cb[i]->valid = false; > spin_unlock_irqrestore(&cctx->lock, flags); > > for_each_available_child_of_node(rdev->of_node, np) { > if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) { > of_node_put(np); [Severity: High] Does this loop cause a double-free regression of the device node? The for_each_available_child_of_node() macro automatically drops the reference to the previous node upon advancing in the loop. Explicitly calling of_node_put(np) inside the loop body (both before continue and at the end of the iteration) violates OF iterator semantics and results in a double-free. Since this patch introduces new allocations in fastrpc_cb_init() that can fail, it exposes this buggy cleanup path when fastrpc_cb_devices_destroy() is triggered on error. > continue; > } > > pdev = of_find_device_by_node(np); > of_node_put(np); > if (pdev) > of_platform_device_destroy(&pdev->dev, NULL); > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
