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

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

[Sukrut Bellary]

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.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Sukrut Bellary <sukrut.bellary@linux.com>
---
 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);
 
-- 
2.34.1

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

[Dan Carpenter]

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);

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  }

regards,
dan carpenter

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

[Sukrut Bellary]

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

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

[Dan Carpenter]

On Thu, May 18, 2023 at 07:45:22PM -0700, Sukrut Bellary wrote:
> > > 
> > > 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?

You can't go wrong with always adding it.  The untested part is useful
to know.  Also it's good to know how a bug is found so we can say if
it's affecting user and so we can improve our processes going forward.

regards,
dan carpenter

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

[Sukrut Bellary]

On Fri, May 19, 2023 at 07:16:14AM +0300, Dan Carpenter wrote:
> On Thu, May 18, 2023 at 07:45:22PM -0700, Sukrut Bellary wrote:
> > > > 
> > > > 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?
> 
> You can't go wrong with always adding it.  The untested part is useful
> to know.  Also it's good to know how a bug is found so we can say if
> it's affecting user and so we can improve our processes going forward.
>
Ok, that's clear. Thanks for the input.

Regards,
Sukrut Bellary

> regards,
> dan carpenter
> 

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

[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

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

[Dan Carpenter]

> ----------------------->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);

This will return success if copy_to_user() fails.

regards,
dan carpenter

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

[Srinivas Kandagatla]

On 19/05/2023 11:22, Dan Carpenter wrote:
>> ----------------------->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);
> 
> This will return success if copy_to_user() fails.
> 
that is true, using return value of fastrpc_req_munmap_impl does not 
really make sense here we should just return err in either case to the user.

--srini

> regards,
> dan carpenter
> 

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

[Dan Carpenter]

This is unrelated but I was looking through the driver and I notice
a bunch of code doing:

grep 'return ret ?' drivers/firmware/ -R

	return ret ? : res.result[0];

"ret" here is a kernel error code, and res.result[0] is a firmware
error code.  Mixing error codes is a dangerous thing.  I was reviewing
some of the callers and the firmware error code gets passed quite far
back into the kernel to where we would only expect kernel error codes.

Presumably the firmware is returning positive error codes?  To be honest,
I am just guessing.  It's better to convert custom error codes to kernel
error codes as soon as possible.  I am just guessing.  Sukrut, do you
think you could take a look?  If the callers do not differentiate
between negative kernel error codes and positive custom error codes then
probably just do:

	if (res.result[0])
		ret = -EIO; // -EINVAL?
	return ret;

Also there are a couple places which do:

	return ret ? false : !!res.result[0];

Here true means success and false means failure.  So the !! converts
a firmware error code to true when it should be false so that's a bug.
Quadruple negatives are confusing...  It should be:

	if (ret || res.result[0])
		return false;
	return true;

regards,
dan carpenter

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

[Sukrut Bellary]

On Fri, May 19, 2023 at 11:39:59AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 19/05/2023 11:22, Dan Carpenter wrote:
> > > ----------------------->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);
> > 
> > This will return success if copy_to_user() fails.
> > 
> that is true, using return value of fastrpc_req_munmap_impl does not really
> make sense here we should just return err in either case to the user.
>

Thanks, Srini and Dan, for reviewing the patch and suggestions.
I will add this in v2.

Regards,
Sukrut Bellary

> --srini
> 
> > regards,
> > dan carpenter
> > 

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

[Sukrut Bellary]

On Fri, May 19, 2023 at 01:58:10PM +0300, Dan Carpenter wrote:
> This is unrelated but I was looking through the driver and I notice
> a bunch of code doing:
> 
> grep 'return ret ?' drivers/firmware/ -R
> 
> 	return ret ? : res.result[0];
> 
> "ret" here is a kernel error code, and res.result[0] is a firmware
> error code.  Mixing error codes is a dangerous thing.  I was reviewing
> some of the callers and the firmware error code gets passed quite far
> back into the kernel to where we would only expect kernel error codes.
> 
> Presumably the firmware is returning positive error codes?  To be honest,
> I am just guessing.  It's better to convert custom error codes to kernel
> error codes as soon as possible.  I am just guessing.  Sukrut, do you
> think you could take a look?  If the callers do not differentiate
> between negative kernel error codes and positive custom error codes then
> probably just do:
> 
> 	if (res.result[0])
> 		ret = -EIO; // -EINVAL?
> 	return ret;
> 

Thanks, Dan, for sharing your findings.
Yes, sure, I will take a look.

Regards,
Sukrut Bellary

> Also there are a couple places which do:
> 
> 	return ret ? false : !!res.result[0];
> 
> Here true means success and false means failure.  So the !! converts
> a firmware error code to true when it should be false so that's a bug.
> Quadruple negatives are confusing...  It should be:
> 
> 	if (ret || res.result[0])
> 		return false;
> 	return true;
> 
> regards,
> dan carpenter
> 

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

[Sukrut Bellary]

On Fri, May 19, 2023 at 03:57:59PM -0700, Sukrut Bellary wrote:
> On Fri, May 19, 2023 at 11:39:59AM +0100, Srinivas Kandagatla wrote:
> > 
> > 
> > On 19/05/2023 11:22, Dan Carpenter wrote:
> > > > ----------------------->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);
> > > 
> > > This will return success if copy_to_user() fails.
> > > 
> > that is true, using return value of fastrpc_req_munmap_impl does not really
> > make sense here we should just return err in either case to the user.
> >
>

I have one follow-up question before I send the v2 patch.
With the following approach, I do see one issue.

----------------------->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;
+       fastrpc_req_munmap_impl(fl, buf);
+       return err;
 }
----------------------->cut<---------------------------

In this, if qcom_scm_assign_mem() fails, the buf is not added to the list.
But the call to fastrpc_req_munmap_impl() tries to delete the buf
from the list.

To avoid this, we can use the following approach.
Here we first add the buf to the list and fastrpc_req_munmap_impl() would
take care of deleting it in the failure path
Please let me know your review comments.

----------------------->cut<---------------------------
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f48466960f1b..56751609f412 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1882,7 +1882,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 */
@@ -1890,6 +1891,9 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl,
char __user *argp)
 
        /* let the client know the address to use */
        req.vaddrout = rsp_msg.vaddr;
+       spin_lock(&fl->lock);
+       list_add_tail(&buf->node, &fl->mmaps);
+       spin_unlock(&fl->lock);
 
        /* Add memory to static PD pool, protection thru hypervisor */
        if (req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
@@ -1906,9 +1910,6 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl,
char __user *argp)
                }
        }
 
-       spin_lock(&fl->lock);
-       list_add_tail(&buf->node, &fl->mmaps);
-       spin_unlock(&fl->lock);
 
        if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
                err = -EFAULT;
@@ -1922,10 +1923,8 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl,
char __user *argp)
 
 err_assign:
        fastrpc_req_munmap_impl(fl, buf);
-err_invoke:
-       fastrpc_buf_free(buf);
-
        return err;
+
 }
----------------------->cut<---------------------------

Regards,
Sukrut Bellary

> Thanks, Srini and Dan, for reviewing the patch and suggestions.
> I will add this in v2.
> 
> Regards,
> Sukrut Bellary
> 
> > --srini
> > 
> > > regards,
> > > dan carpenter
> > > 

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

[Dan Carpenter]

On Wed, May 31, 2023 at 09:45:38PM -0700, Sukrut Bellary wrote:
> On Fri, May 19, 2023 at 03:57:59PM -0700, Sukrut Bellary wrote:
> > On Fri, May 19, 2023 at 11:39:59AM +0100, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 19/05/2023 11:22, Dan Carpenter wrote:
> > > > > ----------------------->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);
> > > > 
> > > > This will return success if copy_to_user() fails.
> > > > 
> > > that is true, using return value of fastrpc_req_munmap_impl does not really
> > > make sense here we should just return err in either case to the user.
> > >
> >
> 
> I have one follow-up question before I send the v2 patch.
> With the following approach, I do see one issue.
> 
> ----------------------->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;
> +       fastrpc_req_munmap_impl(fl, buf);
> +       return err;
>  }
> ----------------------->cut<---------------------------
> 
> In this, if qcom_scm_assign_mem() fails, the buf is not added to the list.
> But the call to fastrpc_req_munmap_impl() tries to delete the buf
> from the list.
> 
> To avoid this, we can use the following approach.

The list is initialized in __fastrpc_buf_alloc().

	INIT_LIST_HEAD(&buf->node);

So calling list_del(&buf->node); is fine.  It's not necessary but it's
not harmful.

regards,
dan carpenter

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

[Sukrut Bellary]

On Thu, Jun 01, 2023 at 10:00:43AM +0300, Dan Carpenter wrote:
> On Wed, May 31, 2023 at 09:45:38PM -0700, Sukrut Bellary wrote:
> > On Fri, May 19, 2023 at 03:57:59PM -0700, Sukrut Bellary wrote:
> > > On Fri, May 19, 2023 at 11:39:59AM +0100, Srinivas Kandagatla wrote:
> > > > 
> > > > 
> > > > On 19/05/2023 11:22, Dan Carpenter wrote:
> > > > > > ----------------------->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);
> > > > > 
> > > > > This will return success if copy_to_user() fails.
> > > > > 
> > > > that is true, using return value of fastrpc_req_munmap_impl does not really
> > > > make sense here we should just return err in either case to the user.
> > > >
> > >
> > 
> > I have one follow-up question before I send the v2 patch.
> > With the following approach, I do see one issue.
> > 
> > ----------------------->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;
> > +       fastrpc_req_munmap_impl(fl, buf);
> > +       return err;
> >  }
> > ----------------------->cut<---------------------------
> > 
> > In this, if qcom_scm_assign_mem() fails, the buf is not added to the list.
> > But the call to fastrpc_req_munmap_impl() tries to delete the buf
> > from the list.
> > 
> > To avoid this, we can use the following approach.
> 
> The list is initialized in __fastrpc_buf_alloc().
> 
> 	INIT_LIST_HEAD(&buf->node);
> 
> So calling list_del(&buf->node); is fine.  It's not necessary but it's
> not harmful.
>

OK, Thanks Dan. I will submit the v2 with 1st approach.

Regards,
Sukrut Bellary

> regards,
> dan carpenter
>