Re: [PATCH v6 2/3] misc: fastrpc: move ADSP duplicate session creation to the driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] misc: fastrpc: session leak in fastrpc_rpmsg_probe() on race with open
- [High] misc: fastrpc: double of_node_put() in fastrpc_cb_devices_create()
- [High] misc: fastrpc: use-after-free on fastrpc_cb_devices_create() failure in probe
--

commit 810d79d90cb45359bb71f6c3439fb0921ee5af9c
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: move ADSP duplicate session creation to the driver

This commit removes the qcom,nsessions device tree property read and per-bank
session duplication from fastrpc_cb_init(), and instead appends four copies of
the last session in fastrpc_rpmsg_probe() for the adsp domain.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index caa479f304c1c..5dd1990d3e2a9 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -2368,16 +2371,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);
>  	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
>  	if (rc) {
> @@ -2656,6 +2649,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (err)
>  		goto err_deregister_fdev;

[Severity: High]
Could calling of_node_put(np) before continue in fastrpc_cb_devices_create()
cause a double release of the device_node reference on np?

Right before this check in drivers/misc/fastrpc.c, fastrpc_rpmsg_probe() calls
fastrpc_cb_devices_create(rpdev) (introduced earlier in this series by commit
"misc: fastrpc: iterate CB nodes manually instead of of_platform_populate"):

drivers/misc/fastrpc.c:fastrpc_cb_devices_create() {
	for_each_available_child_of_node(rdev->of_node, np) {
		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
			of_node_put(np);
			continue;
		}
		...
}

In include/linux/of.h, for_each_available_child_of_node() advances the loop
with of_get_next_available_child(parent, child), which already drops the
reference on the previous child node:

#define for_each_available_child_of_node(parent, child) \
	for (child = of_get_next_available_child(parent, NULL); child != NULL; \
	     child = of_get_next_available_child(parent, child))

When an available child node under rdev->of_node is not compatible with
"qcom,fastrpc-compute-cb", calling of_node_put(np) before continue drops the
reference on np a first time, and the loop increment drops it a second time,
leading to a reference count underflow and potential use-after-free on np.

>  
> +	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));
> +		}
> +		spin_unlock_irqrestore(&data->lock, flags);
> +	}

[Severity: High]
What prevents a race with fastrpc_device_open() from permanently leaking the
duplicated dup_sess entries in data->session[] here?

In drivers/misc/fastrpc.c, fastrpc_rpmsg_probe() registers the character
device via fastrpc_device_register() -> misc_register() before calling
fastrpc_cb_devices_create(rpdev):

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
	...
	case ADSP_DOMAIN_ID:
	case MDSP_DOMAIN_ID:
	case SDSP_DOMAIN_ID:
		data->unsigned_support = false;
		err = fastrpc_device_register(rdev, data, secure_dsp, domain);
		...
	err = fastrpc_cb_devices_create(rpdev);
}

Previously, fastrpc_cb_init() duplicated the sessions inside the same
cctx->lock critical section that initialized sess->used = false. With this
regression, fastrpc_cb_init() sets sess->used = false and sess->valid = true,
increments cctx->sesscount, and drops cctx->lock before calling
dma_set_mask():

drivers/misc/fastrpc.c:fastrpc_cb_init() {
	...
	sess = &cctx->session[cctx->sesscount++];
	sess->used = false;
	sess->valid = true;
	sess->dev = dev;
	sess->sid = sid;

	if (cctx->domain_id == CDSP_DOMAIN_ID)
		dma_bits = cctx->soc_data->dma_addr_bits_cdsp;

	spin_unlock_irqrestore(&cctx->lock, flags);
	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
}

If userspace (such as fastrpc daemons triggered when misc_register() exposes
the device node) opens the device during this unlocked window,
fastrpc_device_open() -> fastrpc_session_alloc() can allocate up to last_sess
and set last_sess->used = true:

drivers/misc/fastrpc.c:fastrpc_session_alloc() {
	...
	for (i = 0; i < cctx->sesscount; i++) {
		if (!cctx->session[i].used && cctx->session[i].valid) {
			cctx->session[i].used = true;
			session = &cctx->session[i];
			fl->client_id = i + 1;
			break;
		}
	}
}

When fastrpc_rpmsg_probe() then checks data->sesscount > 0 (outside
data->lock), acquires data->lock, and copies last_sess into each dup_sess via
memcpy(dup_sess, last_sess, sizeof(*dup_sess)), used = true is copied into all
four dup_sess slots without resetting dup_sess->used = false.

Because no fastrpc_user instance owns those dup_sess pointers,
fastrpc_session_free() is never called on them, leaving all four duplicated
sessions permanently marked used = true and unallocatable.

> +
>  	return 0;
>  
>  err_deregister_fdev:

[Severity: High]
If fastrpc_cb_devices_create() fails after a session has been marked valid,
can kfree(data) in the err_deregister_fdev path cause a use-after-free on
data and the destroyed child platform devices?

Premature registration of the miscdevice prior to child population and
kfree(data) on the error path existed prior to this commit, and commit
9b30a7a26899 ("misc: fastrpc: iterate CB nodes manually instead of
of_platform_populate") in this series replaced of_platform_populate() with
fastrpc_cb_devices_create() while this patch modifies fastrpc_cb_init() and
fastrpc_rpmsg_probe().

During fastrpc_rpmsg_probe() -> fastrpc_cb_devices_create() ->
fastrpc_cb_init() in drivers/misc/fastrpc.c, once the first context bank sets
sess->valid = true and drops cctx->lock, a concurrent open() on the registered
miscdevice can succeed in fastrpc_device_open():

drivers/misc/fastrpc.c:fastrpc_device_open() {
	...
	/* Released in fastrpc_device_release() */
	fastrpc_channel_ctx_get(cctx);
	...
	fl->sctx = fastrpc_session_alloc(fl);
	...
	spin_lock_irqsave(&cctx->lock, flags);
	list_add_tail(&fl->user, &cctx->users);
	spin_unlock_irqrestore(&cctx->lock, flags);
}

If dma_set_mask() then fails in fastrpc_cb_init(), or a subsequent child node
fails in of_platform_device_create() or fastrpc_cb_init(),
fastrpc_cb_devices_create() destroys the child platform devices via
fastrpc_cb_devices_destroy(rpdev) and returns an error:

drivers/misc/fastrpc.c:fastrpc_cb_devices_create() {
	...
		err = fastrpc_cb_init(pdev);
		if (err) {
			of_node_put(np);
			fastrpc_cb_devices_destroy(rpdev);
			return err;
		}
}

fastrpc_rpmsg_probe() then jumps to err_deregister_fdev, calls
misc_deregister() (which does not close existing open file descriptors), and
unconditionally frees data with kfree(data) instead of releasing the initial
reference via fastrpc_channel_ctx_put(data):

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
	...
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);
	return err;
}

When the open file descriptor later performs an ioctl or closes the file in
fastrpc_device_release() -> fastrpc_user_put(), it dereferences the freed
cctx (data) to lock cctx->lock, remove fl->user from cctx->users, clear
fl->sctx->used, and call fastrpc_channel_ctx_put(fl->cctx).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-dup-sessions-v6-0-578ab52ba66c@oss.qualcomm.com?part=2