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

[Sukrut Bellary <sukrut.bellary@linux.com>]

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@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.
>

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