Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Session duplication non-atomicity introduces a race window allowing a permanent leak of duplicated sessions. Pre-existing issues: - [Critical] Premature character device registration allows unprivileged userspace to free the channel context during probe, leading to a Use-After-Free. - [High] Dangling pointer in rdev->dma_mask after probe failure leads to use-after-free. - [High] Dangling pointer to child platform devices after fastrpc_rpmsg_remove leads to UAF during DMA allocations. -- commit a8d47b7ddf0527e896eee27dcc31b42059209d4c Author: Vinayak Katoch <[email protected]> misc: fastrpc: move ADSP duplicate session creation to the driver For ADSP, only a limited number of FastRPC context banks (CBs) are available. Each CB supports a single session, which means only a few processes can run on ADSP simultaneously. If all sessions are consumed by fastrpc daemons, no session remains available when a user application starts, causing the application to fail. To work around this, qcom,nsessions = <5> was set in DT to duplicate sessions inline during fastrpc_cb_init(). This policy does not belong in DT and should be handled at the driver level instead. Remove the qcom,nsessions DT property read and the per-CB duplication logic from fastrpc_cb_init(). After all context banks have been initialised in fastrpc_rpmsg_probe(), append FASTRPC_DUP_SESSIONS (4) copies of the last session for the ADSP domain. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index a153107d00853..98a61b8b90131 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2399,16 +2398,6 @@ static int fastrpc_cb_init(struct platform_device > *pdev) > if (cctx->domain_id == CDSP_DOMAIN_ID) > dma_bits = cctx->soc_data->dma_addr_bits_cdsp; > > - if (sessions > 0) { > - struct fastrpc_session_ctx *dup_sess; > - > - for (i = 1; i < sessions; i++) { > - if (cctx->sesscount >= FASTRPC_MAX_SESSIONS) > - break; > - dup_sess = &cctx->session[cctx->sesscount++]; > - memcpy(dup_sess, sess, sizeof(*dup_sess)); > - } > - } > spin_unlock_irqrestore(&cctx->lock, flags); [Severity: High] Does moving this duplication logic out of fastrpc_cb_init() introduce a race window? The lock is released here, but duplication now happens later in fastrpc_rpmsg_probe(). In the meantime, userspace can open the device and allocate this session, setting its used flag to true. [Severity: High] This isn't a bug introduced by this patch, but earlier in fastrpc_cb_init(), a weak pointer to the child platform device is stored in the session context without incrementing its reference count: sess->dev = dev; During module unbind, fastrpc_rpmsg_remove() destroys these child devices via of_platform_depopulate(). However, active userspace file descriptors are not closed. Could subsequent ioctls on an open fastrpc file descriptor pass this dangling sess->dev pointer to dma_alloc_coherent(), causing a use-after-free in the DMA API? > rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits)); > if (rc) { [ ... ] > @@ -2629,6 +2618,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > goto err_depopulate; > } > > + if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) { > + struct fastrpc_session_ctx *last_sess; > + struct fastrpc_session_ctx *dup_sess; > + unsigned long flags; > + > + 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] If a racing userspace process has already allocated last_sess and set its used flag to true, does this memcpy() blindly copy the used = true state to the newly duplicated sessions? Since these duplicated sessions are not attached to any open file descriptor, it seems they will never be freed on close(), permanently leaking ADSP sessions. > + } > + spin_unlock_irqrestore(&data->lock, flags); > + } > + > return 0; > > err_depopulate: [Severity: Critical] This isn't a bug introduced by this patch, but earlier in fastrpc_rpmsg_probe(), the character device is registered and exposed to userspace before critical data structures are initialized: err = fastrpc_device_register(rdev, data, secure_dsp, domain); ... kref_init(&data->refcount); If unprivileged userspace immediately opens the device before sessions are added, fastrpc_device_open() fails to find a session and calls fastrpc_channel_ctx_put(). Does this underflow the 0-initialized refcount and free the data struct while fastrpc_rpmsg_probe() is still executing, leading to a use-after-free? [Severity: High] This isn't a bug introduced by this patch, but earlier in fastrpc_rpmsg_probe(), the device's DMA mask pointer is assigned to a field inside the dynamically allocated data structure: rdev->dma_mask = &data->dma_mask; If the probe subsequently fails, the err_free_data path frees data but fails to clear rdev->dma_mask. Since the rpmsg_device outlives the probe failure, could subsequent DMA operations or teardown checks by the driver core dereference this dangling pointer and cause a use-after-free? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
