Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path

[Dan Carpenter]

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