Messages in this thread Patch in this message | | | Date | Fri, 19 May 2023 10:52:27 +0100 | Subject | Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path | From | Srinivas Kandagatla <> |
| |
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@linuxfoundation.org> >> Signed-off-by: Sukrut Bellary <sukrut.bellary@linux.com> >> --- > ^^^ > 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
| |