Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues: - [High] Deferring session duplication to fastrpc_rpmsg_probe() creates a race condition with concurrent device open, permanently leaking duplicated sessions. - [High] An unlocked check on data->sesscount > 0 introduces a TOCTOU race with concurrent device unbinding, leading to an out-of-bounds read. - [Medium] Relying on of_platform_populate() for synchronous child device probing fails under asynchronous driver probing, leaving ADSP with missing sessions. Pre-existing issues: - [Critical] The fastrpc misc device is registered and exposed to userspace before its underlying data structures and locks are initialized. - [High] Removing a fastrpc context bank decrements sesscount without shifting the active sessions, corrupting the session array upon subsequent binds. - [High] If of_platform_populate() fails, the parent's data context is freed without depopulating successfully probed child devices, leading to a Use-After-Free. - [High] A failure in dma_set_mask() leaves a corrupted, non-DMA-capable session in the array, which fastrpc_rpmsg_probe() will wrongfully duplicate. - [High] SCM memory assignment is not reverted on error paths in fastrpc_rpmsg_probe. - [High] Unbinding a fastrpc-cb device while a session is actively in use leaves a dangling device pointer in the active session context. -- commit f82362da546a7805f6c3c7071b9238f77f55a851 Author: Vinayak Katoch <[email protected]> misc: fastrpc: create duplicate sessions after all CB probing This commit defers the duplication of ADSP sessions from the per-CB probe path to the main fastrpc_rpmsg_probe function after all child nodes have been populated. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 1080f9acf70a3..46afbae9c2341 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2195,7 +2196,6 @@ static int fastrpc_cb_probe(struct platform_device > *pdev) [ ... ] > if (!cctx) > return -EINVAL; > > - of_property_read_u32(dev->of_node, "qcom,nsessions", &sessions); > - > spin_lock_irqsave(&cctx->lock, flags); [Severity: High] This is a pre-existing issue, but when unbinding a fastrpc-cb device, fastrpc_cb_remove() decrements cctx->sesscount without shifting the active sessions to fill the gap. Does this mean cctx->sesscount becomes an incorrect insertion index, causing subsequent probes to blindly overwrite an active session upon a new bind? [Severity: High] This is also a pre-existing issue, but fastrpc_cb_remove() marks the session invalid and decrements sesscount without waiting for active users to close their file descriptors. Does this leave a dangling device pointer in the active session context, triggering a use-after-free when subsequent ioctls attempt to use it? > @@ -2225,16 +2223,6 @@ static int fastrpc_cb_probe(struct platform_device > *pdev) > if (of_property_read_u32(dev->of_node, "reg", &sess->sid)) > dev_info(dev, "FastRPC Session ID not specified in DT\n"); > > - if (sessions > 0) { [ ... ] > - } > spin_unlock_irqrestore(&cctx->lock, flags); > rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits)); > if (rc) { [Severity: High] This isn't a bug introduced by this patch, but what happens if dma_set_mask() fails here? The function logs an error and returns, but cctx->sesscount was already incremented. Does this leave a dangling, non-DMA-capable session in the array that the new loop in fastrpc_rpmsg_probe() will now wrongfully duplicate? > @@ -2445,6 +2433,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) [Severity: Critical] This is a pre-existing issue, but fastrpc_device_register() is called earlier in this function to expose the device to userspace. Since data->lock and data->users are initialized later, couldn't a concurrent userspace open trigger a kernel panic by attempting to acquire an uninitialized spinlock? [Severity: High] This is another pre-existing issue, but earlier in this function qcom_scm_assign_mem() is called to assign memory ownership to the DSP. If a subsequent step in the probe fails, the error paths do not restore the memory permissions back to the host. Does this permanently blackhole the memory? > if (err) > goto err_deregister_fdev; [Severity: High] This isn't a bug introduced by this patch, but if of_platform_populate() fails right before this check, the error path frees the data context. Does it skip tearing down the successfully probed child devices, leaving them with a dangling pointer that could cause a use-after-free upon removal? > > + if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) { [Severity: High] Does checking data->sesscount here without holding data->lock introduce a race with concurrent device unbinding? If a child context bank device is unbound before the lock is acquired, sesscount could be decremented to 0. When the lock is acquired below, would data->sesscount - 1 resolve to index -1 and cause an out-of-bounds read? [Severity: Medium] Will this check fail if asynchronous driver probing is enabled? If child probes are deferred to a workqueue, of_platform_populate() could return immediately while data->sesscount is still 0, silently skipping the session duplication. > + struct fastrpc_session_ctx *last_sess; > + struct fastrpc_session_ctx *dup_sess; > + unsigned long flags; > + int i; > + > + spin_lock_irqsave(&data->lock, flags); > + last_sess = &data->session[data->sesscount - 1]; > + for (i = 0; i < FASTRPC_DUP_SESSIONS; i++) { > + if (data->sesscount >= FASTRPC_MAX_SESSIONS) > + break; > + dup_sess = &data->session[data->sesscount++]; > + memcpy(dup_sess, last_sess, sizeof(*dup_sess)); [Severity: High] Since the /dev/fastrpc-* device is exposed to userspace before this loop runs, could a concurrent application open the device and set the last_sess->used flag to true via fastrpc_session_alloc()? If so, would this memcpy() blindly copy the dirtied state, permanently leaking the duplicated sessions since they wouldn't be attached to any active client? > + } > + spin_unlock_irqrestore(&data->lock, flags); > + } > + > return 0; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
