Re: [PATCH v4 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 4 potential issue(s) to consider:

New issues:
- [High] Session duplication non-atomicity introduces a race window allowing a permanent leak of duplicated sessions.

Pre-existing issues:
- [Critical] Premature character device registration allows unprivileged userspace to free the channel context during probe, leading to a Use-After-Free.
- [High] Dangling pointer in rdev->dma_mask after probe failure leads to use-after-free.
- [High] Dangling pointer to child platform devices after fastrpc_rpmsg_remove leads to UAF during DMA allocations.
--

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

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

For ADSP, only a limited number of FastRPC context banks (CBs) are
available. Each CB supports a single session, which means only a few
processes can run on ADSP simultaneously. If all sessions are consumed
by fastrpc daemons, no session remains available when a user application
starts, causing the application to fail.

To work around this, qcom,nsessions = <5> was set in DT to duplicate
sessions inline during fastrpc_cb_init(). This policy does not belong
in DT and should be handled at the driver level instead.

Remove the qcom,nsessions DT property read and the per-CB duplication
logic from fastrpc_cb_init(). After all context banks have been
initialised in fastrpc_rpmsg_probe(), append FASTRPC_DUP_SESSIONS (4)
copies of the last session for the ADSP domain.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a153107d00853..98a61b8b90131 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -2399,16 +2398,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);

[Severity: High]
Does moving this duplication logic out of fastrpc_cb_init() introduce a
race window?

The lock is released here, but duplication now happens later in
fastrpc_rpmsg_probe(). In the meantime, userspace can open the device
and allocate this session, setting its used flag to true.

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
fastrpc_cb_init(), a weak pointer to the child platform device is stored
in the session context without incrementing its reference count:

    sess->dev = dev;

During module unbind, fastrpc_rpmsg_remove() destroys these child
devices via of_platform_depopulate(). However, active userspace file
descriptors are not closed.

Could subsequent ioctls on an open fastrpc file descriptor pass this
dangling sess->dev pointer to dma_alloc_coherent(), causing a
use-after-free in the DMA API?

>  	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
>  	if (rc) {
[ ... ]
> @@ -2629,6 +2618,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  			goto err_depopulate;
>  	}
>  
> +	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]
If a racing userspace process has already allocated last_sess and set
its used flag to true, does this memcpy() blindly copy the used = true
state to the newly duplicated sessions?

Since these duplicated sessions are not attached to any open file
descriptor, it seems they will never be freed on close(), permanently
leaking ADSP sessions.

> +		}
> +		spin_unlock_irqrestore(&data->lock, flags);
> +	}
> +
>  	return 0;
>  
>  err_depopulate:

[Severity: Critical]
This isn't a bug introduced by this patch, but earlier in
fastrpc_rpmsg_probe(), the character device is registered and exposed
to userspace before critical data structures are initialized:

    err = fastrpc_device_register(rdev, data, secure_dsp, domain);
    ...
    kref_init(&data->refcount);

If unprivileged userspace immediately opens the device before sessions
are added, fastrpc_device_open() fails to find a session and calls
fastrpc_channel_ctx_put().

Does this underflow the 0-initialized refcount and free the data struct
while fastrpc_rpmsg_probe() is still executing, leading to a
use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
fastrpc_rpmsg_probe(), the device's DMA mask pointer is assigned to a
field inside the dynamically allocated data structure:

    rdev->dma_mask = &data->dma_mask;

If the probe subsequently fails, the err_free_data path frees data but
fails to clear rdev->dma_mask. Since the rpmsg_device outlives the
probe failure, could subsequent DMA operations or teardown checks by
the driver core dereference this dangling pointer and cause a
use-after-free?

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