Re: [PATCH v6 3/3] dt-bindings: misc: qcom,fastrpc: deprecate qcom,nsessions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] misc: fastrpc: double of_node_put() on continue in fastrpc_cb_devices_create()
- [Medium] misc: fastrpc: session leak in fastrpc_rpmsg_probe() when duplicating active session
--

commit 9b30a7a26899dd48c168049c0e649bd641947cdc
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

This patch series (through commit c3dc726c9e7a, "dt-bindings: misc:
qcom,fastrpc: deprecate qcom,nsessions") replaces of_platform_populate()
with manual iteration of context bank child DT nodes in
fastrpc_cb_devices_create() so that context banks are initialized
synchronously during fastrpc_rpmsg_probe().

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index af18ff1992eed..caa479f304c1c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>  	return 0;
>  }

[ ... ]

> +static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev)
> +{
> +	struct device *rdev = &rpdev->dev;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int err;
> +
> +	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;
> +		}

[Severity: High]
Doesn't calling of_node_put(np) before continue in
fastrpc_cb_devices_create() drop the reference on np twice when an
available child node of rdev->of_node does not match
"qcom,fastrpc-compute-cb"?

In include/linux/of.h, for_each_available_child_of_node() advances to the
next node on each loop iteration by passing the current child node as prev
to of_get_next_available_child():

    #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))

And in drivers/of/base.c, of_get_next_available_child() delegates to
of_get_next_status_child(), which already releases the reference on prev:

    for (; next; next = next->sibling) {
        if (!checker(next))
            continue;
        if (of_node_get(next))
            break;
    }
    of_node_put(prev);

When fastrpc_rpmsg_probe() -> fastrpc_cb_devices_create() encounters a
non-matching available child node, the explicit of_node_put(np) followed by
continue causes a reference count underflow on struct device_node and a
potential use-after-free. Should the explicit of_node_put(np) before
continue be removed (while keeping the of_node_put(np) calls on the early
return error paths)?

[ ... ]

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 property handling from
fastrpc_cb_init() and instead duplicates the last initialized session
FASTRPC_DUP_SESSIONS times 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;
>  
> +	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);
> +	}
> +
>  	return 0;

[Severity: Medium]
Can moving the session duplication out of fastrpc_cb_init() and into a
separate lock critical section in fastrpc_rpmsg_probe() leak the duplicated
dup_sess entries if userspace opens the device concurrently during probe?

In fastrpc_rpmsg_probe(), fastrpc_device_register() registers the miscdevice
before fastrpc_cb_devices_create() and the ADSP session duplication block
run. Previously, session duplication happened inside fastrpc_cb_probe()
under the same cctx->lock acquisition that initialized sess->used = false.

Now, fastrpc_cb_init() initializes the session and drops cctx->lock:

    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);

Because the /dev/fastrpc-adsp miscdevice is already live, a concurrent open
from userspace (such as udev or a fastrpc daemon) calling
fastrpc_device_open() -> fastrpc_session_alloc() can allocate the last
session and set last_sess->used = true before fastrpc_rpmsg_probe()
acquires data->lock:

    spin_lock_irqsave(&cctx->lock, flags);
    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];
            ...
            break;
        }
    }
    spin_unlock_irqrestore(&cctx->lock, flags);

When fastrpc_rpmsg_probe() then copies last_sess via memcpy(dup_sess,
last_sess, sizeof(*dup_sess)) without resetting dup_sess->used = false, all
FASTRPC_DUP_SESSIONS duplicated entries are created with used = true and no
owning fastrpc_user to release them, permanently exhausting those sessions
for the lifetime of the channel context and causing later allocations in
fastrpc_session_alloc() to fail with -EBUSY.

Should dup_sess->used = false be set explicitly after the memcpy(), or
should fastrpc_device_register() be called after all sessions and context
banks are initialized?

[ ... ]

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