Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path
From: Srinivas Kandagatla
Date: Fri May 19 2023 - 05:52:39 EST
On 18/05/2023 11:55, Dan Carpenter wrote:
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);
how about doing something like this:
----------------------->cut<---------------------------
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f60bbf99485c..3fdd326e1ae8 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1891,7 +1891,8 @@ static int fastrpc_req_mmap(struct fastrpc_user
*fl, char __user *argp)
&args[0]);
if (err) {
dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
- goto err_invoke;
+ fastrpc_buf_free(buf);
+ return err;
}
/* update the buffer to be able to deallocate the memory on the
DSP */
@@ -1930,11 +1931,7 @@ static int fastrpc_req_mmap(struct fastrpc_user
*fl, char __user *argp)
return 0;
err_assign:
- fastrpc_req_munmap_impl(fl, buf);
-err_invoke:
- fastrpc_buf_free(buf);
-
- return err;
+ return fastrpc_req_munmap_impl(fl, buf);
}
----------------------->cut<---------------------------
This bug is real but the fix is not complete.
Yes, there is a danger of freeing the buf while its added to the list.
Above change should address that, in err cases fd close should take care
of deleting list and freeing buf.
--srini
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