* [PATCH] misc: fastrpc: Fix double free of 'buf' in error path @ 2023-05-18 10:08 Sukrut Bellary 2023-05-18 10:55 ` Dan Carpenter 0 siblings, 1 reply; 14+ messages in thread From: Sukrut Bellary @ 2023-05-18 10:08 UTC (permalink / raw) To: Srinivas Kandagatla, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman Cc: Sukrut Bellary, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-18 10:08 [PATCH] misc: fastrpc: Fix double free of 'buf' in error path Sukrut Bellary @ 2023-05-18 10:55 ` Dan Carpenter 2023-05-19 2:45 ` Sukrut Bellary 2023-05-19 9:52 ` Srinivas Kandagatla 0 siblings, 2 replies; 14+ messages in thread From: Dan Carpenter @ 2023-05-18 10:55 UTC (permalink / raw) To: Sukrut Bellary, Abel Vesa Cc: Srinivas Kandagatla, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-18 10:55 ` Dan Carpenter @ 2023-05-19 2:45 ` Sukrut Bellary 2023-05-19 4:16 ` Dan Carpenter 2023-05-19 9:52 ` Srinivas Kandagatla 1 sibling, 1 reply; 14+ messages in thread From: Sukrut Bellary @ 2023-05-19 2:45 UTC (permalink / raw) To: Dan Carpenter Cc: Abel Vesa, Srinivas Kandagatla, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 2:45 ` Sukrut Bellary @ 2023-05-19 4:16 ` Dan Carpenter 2023-05-19 6:12 ` Sukrut Bellary 0 siblings, 1 reply; 14+ messages in thread From: Dan Carpenter @ 2023-05-19 4:16 UTC (permalink / raw) To: Sukrut Bellary Cc: Abel Vesa, Srinivas Kandagatla, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 4:16 ` Dan Carpenter @ 2023-05-19 6:12 ` Sukrut Bellary 0 siblings, 0 replies; 14+ messages in thread From: Sukrut Bellary @ 2023-05-19 6:12 UTC (permalink / raw) To: Dan Carpenter Cc: Abel Vesa, Srinivas Kandagatla, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-18 10:55 ` Dan Carpenter 2023-05-19 2:45 ` Sukrut Bellary @ 2023-05-19 9:52 ` Srinivas Kandagatla 2023-05-19 10:22 ` Dan Carpenter 1 sibling, 1 reply; 14+ messages in thread From: Srinivas Kandagatla @ 2023-05-19 9:52 UTC (permalink / raw) To: Dan Carpenter, Sukrut Bellary, Abel Vesa Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 9:52 ` Srinivas Kandagatla @ 2023-05-19 10:22 ` Dan Carpenter 2023-05-19 10:39 ` Srinivas Kandagatla 2023-05-19 10:58 ` Dan Carpenter 0 siblings, 2 replies; 14+ messages in thread From: Dan Carpenter @ 2023-05-19 10:22 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Sukrut Bellary, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan > ----------------------->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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 10:22 ` Dan Carpenter @ 2023-05-19 10:39 ` Srinivas Kandagatla 2023-05-19 22:57 ` Sukrut Bellary 2023-05-19 10:58 ` Dan Carpenter 1 sibling, 1 reply; 14+ messages in thread From: Srinivas Kandagatla @ 2023-05-19 10:39 UTC (permalink / raw) To: Dan Carpenter Cc: Sukrut Bellary, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 10:39 ` Srinivas Kandagatla @ 2023-05-19 22:57 ` Sukrut Bellary 2023-06-01 4:45 ` Sukrut Bellary 0 siblings, 1 reply; 14+ messages in thread From: Sukrut Bellary @ 2023-05-19 22:57 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Dan Carpenter, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 22:57 ` Sukrut Bellary @ 2023-06-01 4:45 ` Sukrut Bellary 2023-06-01 7:00 ` Dan Carpenter 0 siblings, 1 reply; 14+ messages in thread From: Sukrut Bellary @ 2023-06-01 4:45 UTC (permalink / raw) To: Srinivas Kandagatla, Dan Carpenter Cc: Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > > > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-06-01 4:45 ` Sukrut Bellary @ 2023-06-01 7:00 ` Dan Carpenter 2023-06-01 19:09 ` Sukrut Bellary 0 siblings, 1 reply; 14+ messages in thread From: Dan Carpenter @ 2023-06-01 7:00 UTC (permalink / raw) To: Sukrut Bellary Cc: Srinivas Kandagatla, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-06-01 7:00 ` Dan Carpenter @ 2023-06-01 19:09 ` Sukrut Bellary 0 siblings, 0 replies; 14+ messages in thread From: Sukrut Bellary @ 2023-06-01 19:09 UTC (permalink / raw) To: Dan Carpenter Cc: Srinivas Kandagatla, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 10:22 ` Dan Carpenter 2023-05-19 10:39 ` Srinivas Kandagatla @ 2023-05-19 10:58 ` Dan Carpenter 2023-05-19 23:39 ` Sukrut Bellary 1 sibling, 1 reply; 14+ messages in thread From: Dan Carpenter @ 2023-05-19 10:58 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Sukrut Bellary, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix double free of 'buf' in error path 2023-05-19 10:58 ` Dan Carpenter @ 2023-05-19 23:39 ` Sukrut Bellary 0 siblings, 0 replies; 14+ messages in thread From: Sukrut Bellary @ 2023-05-19 23:39 UTC (permalink / raw) To: Dan Carpenter Cc: Srinivas Kandagatla, Abel Vesa, Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel, kernel-janitors, Shuah Khan 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-01 19:10 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-18 10:08 [PATCH] misc: fastrpc: Fix double free of 'buf' in error path Sukrut Bellary 2023-05-18 10:55 ` Dan Carpenter 2023-05-19 2:45 ` Sukrut Bellary 2023-05-19 4:16 ` Dan Carpenter 2023-05-19 6:12 ` Sukrut Bellary 2023-05-19 9:52 ` Srinivas Kandagatla 2023-05-19 10:22 ` Dan Carpenter 2023-05-19 10:39 ` Srinivas Kandagatla 2023-05-19 22:57 ` Sukrut Bellary 2023-06-01 4:45 ` Sukrut Bellary 2023-06-01 7:00 ` Dan Carpenter 2023-06-01 19:09 ` Sukrut Bellary 2023-05-19 10:58 ` Dan Carpenter 2023-05-19 23:39 ` Sukrut Bellary
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).