Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Concurrent `open()` calls during the unprotected gap after child probing permanently leak duplicated sessions. Pre-existing issues: - [Critical] The misc device is exposed to userspace before crucial synchronization primitives and lists are initialized, leading to uninitialized spinlock usage and NULL pointer dereferences. - [High] Missing `of_platform_depopulate()` on the `of_platform_populate()` error path causes dangling child devices with freed drvdata. - [High] On the error path of `fastrpc_rpmsg_probe()`, `kfree(data)` unconditionally frees memory while a userspace file descriptor may hold a reference. -- --- Patch [2]: [PATCH v2 2/2] misc: fastrpc: create duplicate sessions after all CB probing --- commit 56303754fdfc0beae4b68e60fa9dc54ca7e73396 Author: Vinayak Katoch <[email protected]> misc: fastrpc: create duplicate sessions after all CB probing Instead of duplicating sessions inline during fastrpc_cb_probe() using the qcom,nsessions DT property, defer duplication until after of_platform_populate() returns in fastrpc_rpmsg_probe(), at which point all compute-CB child nodes have been probed and the session array is fully populated. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2352,6 +2352,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) [ ... ] > switch (domain_id) { > case ADSP_DOMAIN_ID: > case MDSP_DOMAIN_ID: > case SDSP_DOMAIN_ID: > /* Unsigned PD offloading is only supported on CDSP and GDSP */ > data->unsigned_support = false; > err = fastrpc_device_register(rdev, data, secure_dsp, domain); [Severity: Critical] This isn't a bug introduced by this patch, but does registering the misc device here expose it to userspace before crucial synchronization primitives are initialized? A concurrent userspace process could immediately call open(), which would attempt to use data->refcount and data->lock before they are initialized further down in this function. > if (err) > goto err_free_data; > break; [ ... ] > spin_lock_init(&data->lock); > idr_init(&data->ctx_idr); > data->domain_id = domain_id; > 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; [Severity: High] This is a pre-existing issue, but if of_platform_populate() fails halfway through, does the error path miss a call to of_platform_depopulate()? Since the channel context data is freed on the error path, this could leave dangling child devices that might dereference freed drvdata when they are later unbound. > + 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] Can this loop permanently leak session capacity if userspace calls open() concurrently? Since the lock is dropped in fastrpc_cb_probe() long before we get here, could a concurrent open() claim last_sess and set its used flag to true? If so, this memcpy() would blindly duplicate the used=true state into all new sessions, making them permanently unavailable to subsequent users. > + } > + spin_unlock_irqrestore(&data->lock, flags); > + } > + > return 0; > > err_deregister_fdev: > if (data->fdevice) > misc_deregister(&data->fdevice->miscdev); > if (data->secure_fdevice) > misc_deregister(&data->secure_fdevice->miscdev); > > err_free_data: > kfree(data); [Severity: High] This is a pre-existing issue, but does calling kfree(data) here unconditionally cause a use-after-free if userspace has already opened the misc device? Because the misc device is registered earlier in this function, a userspace process might already hold an open file descriptor. If we free the channel context here upon a probe failure, any subsequent ioctls or close operations by that process will access freed memory. > return err; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
