On Thu, May 18, 2023 at 01:55:07PM +0300, 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.
>
Thank you for reviewing the patch.
I will add a Fixes tag and fix the commit message.
> >
> > 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.
>
Shall I add "This is based on static analysis only. Compilation tested"
here
or
is it not required to mention this for all the fixes?
Can you please recommend what's is the preferred method I need to follow?
> > 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 }
>
Nice catch!
I will address this in the next version.
Regards,
Sukrut Bellary
> regards,
> dan carpenter
