Re: [PATCH v2 3/4] misc: fastrpc: add extended context bank support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] misc: fastrpc: Use-After-Free of ext_cb array due to concurrent krealloc during probe
- [High] misc: fastrpc: memory leak of dynamically allocated ext_cb array elements on probe error path
- [High] misc: fastrpc: premature device registration allows UAF on probe failure
- [High] of: double-free of device node in fastrpc_cb_devices_destroy
- [Medium] misc: fastrpc: memory leak of ext_cb on probe error path
- [Medium] misc: fastrpc: memory leak of ext_cb array on probe error path
--

commit 670e94cdc2cb15ca5f0de40ab9985878a7a72c7b
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

    misc: fastrpc: add extended context bank support
    
This commit detects extended context banks by checking for the iommu-ranges
property on the context bank node. It registers them in a separate ext_cb
array, allowing the mapping layer to direct large allocations into the wider
IOVA window.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 5c7bb56cf46d0..d40cdcd5dec30 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -2352,6 +2359,43 @@ static int fastrpc_cb_init(struct platform_device *pdev)
>  				       of_n_addr_cells(dev->of_node) - 1, &sid))
>  		dev_info(dev, "FastRPC Session ID not specified in DT\n");
>  
> +	if (of_property_present(dev->of_node, "iommu-ranges"))
> +		is_extended_cb = true;
> +
> +	if (is_extended_cb) {
> +		struct fastrpc_session_ctx **new_ext;
> +
> +		sess = kzalloc_obj(*sess);
> +		if (!sess)
> +			return -ENOMEM;

[Severity: High]
Can this new failure path trigger a use-after-free regression due to premature
device registration?

If this allocation fails, fastrpc_cb_init() returns an error, which propagates
up to fastrpc_rpmsg_probe(). In fastrpc_rpmsg_probe(), the misc device is
registered and exposed to userspace before fastrpc_cb_devices_create() is
called. 

If userspace opens the device concurrently during probe, it obtains an active
file descriptor. When the error path in fastrpc_rpmsg_probe() unconditionally
frees the channel context (data), any subsequent ioctl() or close() by userspace
will dereference the freed pointer.

> +
> +		rc = dma_set_mask(dev, DMA_BIT_MASK(40));
> +		if (rc) {
> +			dev_err(dev, "40-bit DMA enable failed\n");
> +			kfree(sess);
> +			return rc;
> +		}
> +
> +		new_ext = krealloc(cctx->ext_cb,
> +				   (cctx->ext_cb_count + 1) * sizeof(*cctx->ext_cb),
> +				   GFP_KERNEL);

[Severity: High]
Does this code cause a use-after-free regression of the ext_cb array pointer?

krealloc() is called without holding cctx->lock. If krealloc() allocates a new
block, it frees the old cctx->ext_cb pointer. The cctx->ext_cb pointer is only
updated to new_ext later inside the spinlock. 

During this window, cctx->ext_cb points to freed memory, and concurrent
readers may fetch the freed pointer if they race with the probe thread.

[Severity: High]
Does this code also introduce a regression that leaks the dynamically allocated
ext_cb array elements on the probe error path?

If a failure occurs during the context bank initialization loop
(e.g., if krealloc() fails for a second child node after the first succeeded),
the error propagates to fastrpc_rpmsg_probe(). 

The error path in fastrpc_rpmsg_probe() directly executes kfree(data), bypassing
the proper teardown sequence in fastrpc_channel_ctx_free(). Because
fastrpc_channel_ctx_free() is circumvented, the sess objects allocated by this
patch are permanently leaked.

[Severity: Medium]
Does this code also leak the ext_cb array itself on the probe error path?

Similar to the array elements, because the probe error path executes kfree(data)
directly and skips calling fastrpc_channel_ctx_free(), the new_ext block
allocated here is never freed and permanently leaked.

> +		if (!new_ext) {
> +			kfree(sess);
> +			return -ENOMEM;
> +		}
> +
> +		spin_lock_irqsave(&cctx->lock, flags);
> +		cctx->ext_cb = new_ext;
> +		sess->valid = true;
> +		sess->dev = dev;
> +		sess->sid = sid;
> +		dev_set_drvdata(dev, sess);
> +		cctx->ext_cb[cctx->ext_cb_count++] = sess;
> +		spin_unlock_irqrestore(&cctx->lock, flags);
> +
> +		return 0;
> +	}
> +
[ ... ]
> @@ -2391,6 +2435,8 @@ static void fastrpc_cb_devices_destroy(struct rpmsg_device *rpdev)
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	for (i = 0; i < cctx->sesscount; i++)
>  		cctx->session[i].valid = false;
> +	for (i = 0; i < cctx->ext_cb_count; i++)
> +		cctx->ext_cb[i]->valid = false;
>  	spin_unlock_irqrestore(&cctx->lock, flags);
>  
>  	for_each_available_child_of_node(rdev->of_node, np) {
>  		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
>  			of_node_put(np);

[Severity: High]
Does this loop cause a double-free regression of the device node?

The for_each_available_child_of_node() macro automatically drops the reference
to the previous node upon advancing in the loop. Explicitly calling
of_node_put(np) inside the loop body (both before continue and at the end of
the iteration) violates OF iterator semantics and results in a double-free.

Since this patch introduces new allocations in fastrpc_cb_init() that can
fail, it exposes this buggy cleanup path when fastrpc_cb_devices_destroy()
is triggered on error.

>  			continue;
>  		}
>  
>  		pdev = of_find_device_by_node(np);
>  		of_node_put(np);
>  		if (pdev)
>  			of_platform_device_destroy(&pdev->dev, NULL);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-extended-mapping-v2-0-5dca3aa2c2d3@oss.qualcomm.com?part=3