Re: [PATCH v2] misc: fastrpc: fix double-free in fastrpc_map_attach() error path

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

New issues:
- [High] Routing the qcom_scm_assign_mem() failure path to get_err introduces a massive DMA-buf and memory leak because fastrpc_free_map() aborts early on cleanup.

Pre-existing issues:
- [Critical] Lockless and un-referenced use of fastrpc_map pointers leads to Use-After-Free vulnerabilities.
--

--- Patch [1]: [PATCH v2] misc: fastrpc: fix double-free in fastrpc_map_attach() error path ---
commit e44cb66ae041e0f79b6c9be086dbcbf912aa8a9e
Author: Yifei Gao <[email protected]>

misc: fastrpc: fix double-free in fastrpc_map_attach() error path

This patch attempts to fix a double-free in the error path of
fastrpc_map_attach() by routing map->table-is-set failure branches to
get_err instead of map_err. This makes fastrpc_free_map() the sole owner
of the unmap/detach/put sequence for these branches.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index eb6c2a78d3c7b..d480a87752a7f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -881,7 +881,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>  		dev_dbg(sess->dev, "Bad size passed len 0x%llx map size 0x%llx\n",
>  				len, map->size);
>  		err = -EINVAL;
> -		goto map_err;
> +		goto get_err;
>  	}
>  	map->va = sg_virt(map->table->sgl);
>  	map->len = len;

[ ... ]

> @@ -904,7 +904,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>  			dev_err(sess->dev,
>  				"Failed to assign memory with dma_addr %pad size 0x%llx err %d\n",
>  				&map->dma_addr, map->len, err);
> -			goto map_err;
> +			goto get_err;

[Severity: High]
Does routing this qcom_scm_assign_mem() failure to get_err introduce a memory
and DMA-buf leak?

When the forward assignment to TrustZone fails, map->attr is already set to
FASTRPC_ATTR_SECUREMAP. If we jump to get_err, fastrpc_map_put() is called
which in turn calls fastrpc_free_map(). Inside fastrpc_free_map(), it will
see this flag and attempt a reverse qcom_scm_assign_mem().

Since the forward assignment just failed, the reverse assignment will also
fail. Looking at fastrpc_free_map(), if the reverse assignment fails, it
returns early:

fastrpc_free_map() {
    ...
    if (map->attr & FASTRPC_ATTR_SECUREMAP) {
        ...
        err = qcom_scm_assign_mem(...);
        if (err)
            return;
    }
    ...
}

Will this early return completely skip dma_buf_unmap_attachment_unlocked(),
dma_buf_detach(), dma_buf_put(), and kfree(map), leaving untrusted user
space able to exhaust kernel memory?

>  		}
>  	}
>  	spin_lock(&fl->lock);

[Severity: Critical]
This is a pre-existing issue, but can the lockless use of fastrpc_map
pointers lead to a use-after-free elsewhere in this file?

In fastrpc_req_mem_unmap_impl(), the fl->maps list is searched under fl->lock
to find a map pointer. If the lock is then dropped without incrementing the
object's reference count, concurrent ioctls could free the map while it is
still being accessed.

A similar pattern appears in fastrpc_put_args() where fastrpc_map_lookup()
is called without taking a reference, returning an unreferenced pointer that
is later passed to fastrpc_map_put(), causing a refcount imbalance.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1