On Thu, May 18, 2023 at 03:08:29AM -0700, Sukrut Bellary wrote:
> smatch warning:
> drivers/misc/fastrpc.c:1926 fastrpc_req_mmap() error: double free of 'buf'
>
> In fastrpc_req_mmap() error path, the fastrpc buffer is freed in
> fastrpc_req_munmap_impl() if unmap is successful.
>
> But in the end, there is an unconditional call to fastrpc_buf_free().
> So the above case triggers the double free of fastrpc buf.
>
> Fix this by avoiding the call to the second fastrpc_buf_free() if
> fastrpc_req_munmap_impl() is successful.
> 'err' is not updated as it needs to retain the err returned by
> qcom_scm_assign_mem(), which is the starting point of this error path.
>
> This is based on static analysis only. Compilation tested.
Please don't put this in the commit message. We want everyone reading
the git log to believe everything is 100% rock solid. :P
We need a Fixes tag.
Fixes: 72fa6f7820c4 ("misc: fastrpc: Rework fastrpc_req_munmap")
Let's add Abel to the CC list.
>
> Reviewed-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Sukrut Bellary <sukrut.bellary@xxxxxxxxx>
> ---
^^^
Put testing caveats here instead, where it will be removed from the
git log.
> drivers/misc/fastrpc.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f48466960f1b..1c3ab78f274f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1921,7 +1921,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> return 0;
>
> err_assign:
> - fastrpc_req_munmap_impl(fl, buf);
> + if (!fastrpc_req_munmap_impl(fl, buf)) {
> + /* buf is freed */
> + return err;
> + }
> err_invoke:
> fastrpc_buf_free(buf);
This bug is real but the fix is not complete.
drivers/misc/fastrpc.c
1911 if (err) {
1912 dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
1913 buf->phys, buf->size, err);
1914 goto err_assign;
1915 }
1916 }
1917
1918 spin_lock(&fl->lock);
1919 list_add_tail(&buf->node, &fl->mmaps);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
buf needs to be removed from the list before we free it, otherwise it
leads to a use after free. The fastrpc_req_munmap_impl() function does
that but here this function just calls fastrpc_buf_free().
1920 spin_unlock(&fl->lock);
1921
1922 if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
1923 err = -EFAULT;
1924 goto err_assign;
1925 }
1926
1927 dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
1928 buf->raddr, buf->size);
1929
1930 return 0;
1931
1932 err_assign:
1933 fastrpc_req_munmap_impl(fl, buf);
1934 err_invoke:
1935 fastrpc_buf_free(buf);
1936
1937 return err;
1938 }
regards,
dan carpenter
