lkml.org 
[lkml]   [2023]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path
From


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

\
 
 \ /
  Last update: 2023-05-19 11:55    [W:0.301 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site