Re: [PATCH v2 2/2] misc: fastrpc: create duplicate sessions after all CB probing

[sashiko-bot@kernel.org]

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 <vinayak.katoch@oss.qualcomm.com>

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/20260708-dup-sessions-v2-0-da40f9c98a2b@oss.qualcomm.com?part=2