Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f47b17c1-1c02-2aa3-ba10-fcef70cb25a8@linaro.org>
Date:   Fri, 19 May 2023 10:52:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path
Content-Language: en-US
To:     Dan Carpenter <dan.carpenter@linaro.org>,
        Sukrut Bellary <sukrut.bellary@linux.com>,
        Abel Vesa <abel.vesa@linaro.org>
Cc:     Amol Maheshwari <amahesh@qti.qualcomm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-janitors@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20230518100829.515143-1-sukrut.bellary@linux.com>
 <9194ebdf-f335-4cd6-bf89-bb4f86a57784@kili.mountain>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <9194ebdf-f335-4cd6-bf89-bb4f86a57784@kili.mountain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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