Re: [PATCH v8 2/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch introduces a critical use-after-free and list corruption vulnerability in the fastrpc_req_mmap() error path.

Pre-existing issues:
- [High] Concurrent accesses to fl->mmaps without holding fl->lock when handling -ERESTARTSYS.
--

commit a53abf279a47f6742d5cb44fb5a332592c4787b8
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Remove buffer from list prior to unmap operation

This commit removes the buffer from the fastrpc user's mapped list before
calling the unmap operation to prevent concurrent unmap races. If the
unmap operation fails, the buffer is re-added to the list.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 96961217b8569..517884000331b 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1889,9 +1889,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>  				      &args[0]);
>  	if (!err) {
>  		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
> -		spin_lock(&fl->lock);
> -		list_del(&buf->node);
> -		spin_unlock(&fl->lock);

[Severity: Critical]
Does removing this list_del() introduce a use-after-free in the error
path of fastrpc_req_mmap()?

When fastrpc_req_mmap() encounters a copy_to_user() failure after successfully
adding the buffer to the fl->mmaps list, it jumps to the err_assign label
and calls fastrpc_req_munmap_impl() without first unlinking the buffer:

drivers/misc/fastrpc.c:fastrpc_req_mmap() {
        ...
        spin_lock(&fl->lock);
        list_add_tail(&buf->node, &fl->mmaps);
        spin_unlock(&fl->lock);

        if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
                err = -EFAULT;
                goto err_assign;
        }
        ...
err_assign:
        fastrpc_req_munmap_impl(fl, buf);

        return err;
}

Since fastrpc_req_munmap_impl() no longer removes the buffer from fl->mmaps
before calling fastrpc_buf_free(), the freed buffer will remain in the
list. Subsequent traverses of fl->mmaps will trigger a use-after-free.

>  		fastrpc_buf_free(buf);
>  	} else {
>  		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);

[ ... ]

> @@ -1924,7 +1923,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  		return -EINVAL;
>  	}
>  
> -	return fastrpc_req_munmap_impl(fl, buf);
> +	err = fastrpc_req_munmap_impl(fl, buf);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the mmap
list handling here, I noticed concurrent modifications to fl->mmaps happen
locklessly in fastrpc_internal_invoke() when handling -ERESTARTSYS:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
        ...
        if (err == -ERESTARTSYS) {
                list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
                        list_del(&buf->node);
                        list_add_tail(&buf->node, 
                                      &fl->cctx->invoke_interrupted_mmaps);
                }
        }
        ...
}

Can this lead to list corruption if another thread concurrently executes
fastrpc_req_mmap() or fastrpc_req_munmap() and modifies fl->mmaps with
fl->lock held?

> +	if (err) {
> +		spin_lock(&fl->lock);
> +		list_add_tail(&buf->node, &fl->mmaps);
> +		spin_unlock(&fl->lock);
> +	}
> +
> +	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=2