Re: [PATCH v4 1/3] misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Critical Use-After-Free and uninitialized state exposure when `fastrpc_rpmsg_probe` fails after prematurely registering the misc device.
- [High] Preexisting Use-After-Free in `fastrpc_rpmsg_probe()` where `rdev->dma_mask` is left pointing to freed memory on error paths.
- [High] Use-After-Free of context bank devices (`sess->dev`) upon fastrpc channel removal due to missing device references.
--

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

    misc: fastrpc: iterate CB nodes manually instead of of_platform_populate
    
    of_platform_populate() only guarantees that child devices are registered,
    not that their probes have completed before it returns. This creates a
    window where fastrpc_cb_init() may not have run for all context bank
    nodes, leaving the channel context partially initialised.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d4fac2caca867..a153107d00853 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2363,7 +2363,7 @@ static const struct file_operations fastrpc_fops = {
>  	.compat_ioctl = fastrpc_device_ioctl,
>  };
>  
> -static int fastrpc_cb_probe(struct platform_device *pdev)
> +static int fastrpc_cb_init(struct platform_device *pdev)
>  {
>  	struct fastrpc_channel_ctx *cctx;
>  	struct fastrpc_session_ctx *sess;
> @@ -2385,7 +2385,7 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	if (cctx->sesscount >= FASTRPC_MAX_SESSIONS) {
>  		spin_unlock_irqrestore(&cctx->lock, flags);
> -		dev_err(&pdev->dev, "too many sessions\n");
> +		dev_err(dev, "too many sessions\n");
>  		return -ENOSPC;
>  	}
>  	dma_bits = cctx->soc_data->dma_addr_bits_default;

[Severity: High]
This is a pre-existing issue, but does fastrpc_cb_init() save the child
device pointer without holding a reference to it?

If sess->dev is assigned without get_device(dev), and fastrpc_rpmsg_remove()
calls of_platform_depopulate(), the child platform devices are destroyed.

Since active file descriptors aren't revoked, could an active ioctl later
call fastrpc_dmabuf_alloc() and invoke dma_alloc_coherent() on fl->sctx->dev,
using the dangling pointer and leading to a use-after-free?

[ ... ]

> @@ -2642,12 +2610,29 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	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;
> +	of_node_set_flag(rdev->of_node, OF_POPULATED_BUS);
> +
> +	for_each_available_child_of_node_scoped(rdev->of_node, np) {
> +		struct platform_device *pdev;
> +
> +		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb"))
> +			continue;
> +
> +		pdev = of_platform_device_create(np, NULL, rdev);
> +		if (!pdev) {
> +			err = -EINVAL;
> +			goto err_depopulate;
> +		}
> +
> +		err = fastrpc_cb_init(pdev);
> +		if (err)
> +			goto err_depopulate;

[Severity: Critical]
This is a pre-existing issue, but is it possible that this error path leads
to a use-after-free and uninitialized state exposure?

Earlier in fastrpc_rpmsg_probe(), fastrpc_device_register() is called, which
makes the misc device available to userspace. A userspace program can then
open the device, acquiring a reference to the data structure.

If fastrpc_cb_init() fails here, the code jumps to err_depopulate, and
eventually calls kfree(data).

Since userspace could already have an open file descriptor pointing to data,
would this kfree() result in a use-after-free when subsequent ioctls or
close() are called?

[Severity: High]
This isn't a bug introduced by this patch, but does the same error path leave
rdev->dma_mask pointing to freed memory?

Earlier in fastrpc_rpmsg_probe(), rdev->dma_mask is redirected to point
to the dynamically allocated &data->dma_mask.

If this probe function fails and frees data via kfree(data), the rpmsg
device remains in the system unbound.

Will a userspace read of /sys/devices/.../dma_mask_bits trigger a read from
freed memory through dma_mask_bits_show() since rdev->dma_mask wasn't
restored to its original value?

> +	}
>  
>  	return 0;
>  
> +err_depopulate:
> +	of_platform_depopulate(rdev);
>  err_deregister_fdev:
>  	if (data->fdevice)
>  		misc_deregister(&data->fdevice->miscdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-dup-sessions-v4-0-35555d2bfed4@oss.qualcomm.com?part=1