* [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition @ 2023-02-16 1:41 Sangsup Lee 2023-02-16 1:53 ` sangsup lee 2023-03-21 9:27 ` Srinivas Kandagatla 0 siblings, 2 replies; 5+ messages in thread From: Sangsup Lee @ 2023-02-16 1:41 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel This patch adds mutex_lock for fixing an Use-after-free bug. fastrpc_req_munmap_impl can be called concurrently in multi-threded environments. The buf which is allocated by list_for_each_safe can be used after another thread frees it. Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> --- drivers/misc/fastrpc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 5310606113fe..c4b5fa4a50a6 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1806,10 +1806,12 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) struct fastrpc_buf *buf = NULL, *iter, *b; struct fastrpc_req_munmap req; struct device *dev = fl->sctx->dev; + int err; if (copy_from_user(&req, argp, sizeof(req))) return -EFAULT; + mutex_lock(&fl->mutex); spin_lock(&fl->lock); list_for_each_entry_safe(iter, b, &fl->mmaps, node) { if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { @@ -1822,10 +1824,13 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) if (!buf) { dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", req.vaddrout, req.size); + mutex_unlock(&fl->mutex); return -EINVAL; } - return fastrpc_req_munmap_impl(fl, buf); + err = fastrpc_req_munmap_impl(fl, buf); + mutex_unlock(&fl->mutex); + return err; } static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition 2023-02-16 1:41 [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition Sangsup Lee @ 2023-02-16 1:53 ` sangsup lee 2023-03-21 9:27 ` Srinivas Kandagatla 1 sibling, 0 replies; 5+ messages in thread From: sangsup lee @ 2023-02-16 1:53 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel As I'm not an expert for the fastrpc driver, I'm not sure about the patch code. But I think the bug should be patched. Please suggest a proper patch. Best regards, 2023년 2월 16일 (목) 오전 10:41, Sangsup Lee <k1rh4.lee@gmail.com>님이 작성: > > This patch adds mutex_lock for fixing an Use-after-free bug. > fastrpc_req_munmap_impl can be called concurrently in multi-threded environments. > The buf which is allocated by list_for_each_safe can be used after another thread frees it. > > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > --- > drivers/misc/fastrpc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 5310606113fe..c4b5fa4a50a6 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1806,10 +1806,12 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > struct fastrpc_buf *buf = NULL, *iter, *b; > struct fastrpc_req_munmap req; > struct device *dev = fl->sctx->dev; > + int err; > > if (copy_from_user(&req, argp, sizeof(req))) > return -EFAULT; > > + mutex_lock(&fl->mutex); > spin_lock(&fl->lock); > list_for_each_entry_safe(iter, b, &fl->mmaps, node) { > if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { > @@ -1822,10 +1824,13 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > if (!buf) { > dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", > req.vaddrout, req.size); > + mutex_unlock(&fl->mutex); > return -EINVAL; > } > > - return fastrpc_req_munmap_impl(fl, buf); > + err = fastrpc_req_munmap_impl(fl, buf); > + mutex_unlock(&fl->mutex); > + return err; > } > > static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition 2023-02-16 1:41 [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition Sangsup Lee 2023-02-16 1:53 ` sangsup lee @ 2023-03-21 9:27 ` Srinivas Kandagatla 2023-03-22 1:59 ` sangsup lee 1 sibling, 1 reply; 5+ messages in thread From: Srinivas Kandagatla @ 2023-03-21 9:27 UTC (permalink / raw) To: Sangsup Lee Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel Thanks Sangsup for reporting the issue and sharing the patch, Sorry, for some reason I missed this patch. On 16/02/2023 01:41, Sangsup Lee wrote: > This patch adds mutex_lock for fixing an Use-after-free bug. > fastrpc_req_munmap_impl can be called concurrently in multi-threded environments. > The buf which is allocated by list_for_each_safe can be used after another thread frees it. > Commit log can be improved here to something like: fastrcp_munmap takes two steps to unmap the memory, first to find a matching fastrpc buf in the list and second is to send request to DSP to unmap it. There is a potentially window of race between these two operations, which can lead to user-after-free. Fix this by adding locking around this two operations. > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > --- > drivers/misc/fastrpc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 5310606113fe..c4b5fa4a50a6 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1806,10 +1806,12 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > struct fastrpc_buf *buf = NULL, *iter, *b; > struct fastrpc_req_munmap req; > struct device *dev = fl->sctx->dev; > + int err; > > if (copy_from_user(&req, argp, sizeof(req))) > return -EFAULT; > > + mutex_lock(&fl->mutex); > spin_lock(&fl->lock); > list_for_each_entry_safe(iter, b, &fl->mmaps, node) { > if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { > @@ -1822,10 +1824,13 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > if (!buf) { > dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", > req.vaddrout, req.size); > + mutex_unlock(&fl->mutex); > return -EINVAL; > } > > - return fastrpc_req_munmap_impl(fl, buf); > + err = fastrpc_req_munmap_impl(fl, buf); > + mutex_unlock(&fl->mutex); > + return err; How about moving the locking to ioctl: diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index a701132638cf..2f217071a6c3 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -2087,7 +2087,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, err = fastrpc_req_mmap(fl, argp); break; case FASTRPC_IOCTL_MUNMAP: + mutex_lock(&fl->mutex); err = fastrpc_req_munmap(fl, argp); + mutex_unlock(&fl->mutex); break; case FASTRPC_IOCTL_MEM_MAP: err = fastrpc_req_mem_map(fl, argp); thanks, srini > } > > static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition 2023-03-21 9:27 ` Srinivas Kandagatla @ 2023-03-22 1:59 ` sangsup lee 2023-03-22 8:40 ` Srinivas Kandagatla 0 siblings, 1 reply; 5+ messages in thread From: sangsup lee @ 2023-03-22 1:59 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel Sounds great. Thank you for your recommendation. The patch code that you recommend is clear and simple. Please patch this. Signed-off-by: Sangsup lee <k1rh4.lee@gmail.com> --- drivers/misc/fastrpc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 93ebd174d848..aa1cf0e9f4ed 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1901,7 +1901,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, err = fastrpc_req_mmap(fl, argp); break; case FASTRPC_IOCTL_MUNMAP: + mutex_lock(&fl->mutex); err = fastrpc_req_munmap(fl, argp); + mutex_unlock(&fl->mutex); break; case FASTRPC_IOCTL_MEM_MAP: err = fastrpc_req_mem_map(fl, argp); -- 2.25.1 2023년 3월 21일 (화) 오후 6:27, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>님이 작성: > > Thanks Sangsup for reporting the issue and sharing the patch, > > Sorry, for some reason I missed this patch. > > On 16/02/2023 01:41, Sangsup Lee wrote: > > This patch adds mutex_lock for fixing an Use-after-free bug. > > fastrpc_req_munmap_impl can be called concurrently in multi-threded environments. > > The buf which is allocated by list_for_each_safe can be used after another thread frees it. > > > Commit log can be improved here to something like: > > fastrcp_munmap takes two steps to unmap the memory, first to find a > matching fastrpc buf in the list and second is to send request to DSP to > unmap it. > There is a potentially window of race between these two operations, > which can lead to user-after-free. > > Fix this by adding locking around this two operations. > > > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > > --- > > drivers/misc/fastrpc.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > > index 5310606113fe..c4b5fa4a50a6 100644 > > --- a/drivers/misc/fastrpc.c > > +++ b/drivers/misc/fastrpc.c > > @@ -1806,10 +1806,12 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > > struct fastrpc_buf *buf = NULL, *iter, *b; > > struct fastrpc_req_munmap req; > > struct device *dev = fl->sctx->dev; > > + int err; > > > > if (copy_from_user(&req, argp, sizeof(req))) > > return -EFAULT; > > > > + mutex_lock(&fl->mutex); > > spin_lock(&fl->lock); > > list_for_each_entry_safe(iter, b, &fl->mmaps, node) { > > if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { > > @@ -1822,10 +1824,13 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) > > if (!buf) { > > dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", > > req.vaddrout, req.size); > > + mutex_unlock(&fl->mutex); > > return -EINVAL; > > } > > > > - return fastrpc_req_munmap_impl(fl, buf); > > + err = fastrpc_req_munmap_impl(fl, buf); > > + mutex_unlock(&fl->mutex); > > + return err; > > How about moving the locking to ioctl: > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index a701132638cf..2f217071a6c3 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2087,7 +2087,9 @@ static long fastrpc_device_ioctl(struct file > *file, unsigned int cmd, > err = fastrpc_req_mmap(fl, argp); > break; > case FASTRPC_IOCTL_MUNMAP: > + mutex_lock(&fl->mutex); > err = fastrpc_req_munmap(fl, argp); > + mutex_unlock(&fl->mutex); > break; > case FASTRPC_IOCTL_MEM_MAP: > err = fastrpc_req_mem_map(fl, argp); > > > thanks, > srini > > } > > > > static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition 2023-03-22 1:59 ` sangsup lee @ 2023-03-22 8:40 ` Srinivas Kandagatla 0 siblings, 0 replies; 5+ messages in thread From: Srinivas Kandagatla @ 2023-03-22 8:40 UTC (permalink / raw) To: sangsup lee Cc: Amol Maheshwari, Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm, linux-kernel On 22/03/2023 01:59, sangsup lee wrote: > Sounds great. > > Thank you for your recommendation. > The patch code that you recommend is clear and simple. > Please patch this. > > Signed-off-by: Sangsup lee <k1rh4.lee@gmail.com> > --- Please follow kernel patch submission guidelines, any changes to code should be send as new version of patch. Have a look at Documentation/process/submitting-patches.rst for more information. thanks, Srini > drivers/misc/fastrpc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 93ebd174d848..aa1cf0e9f4ed 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1901,7 +1901,9 @@ static long fastrpc_device_ioctl(struct file > *file, unsigned int cmd, > err = fastrpc_req_mmap(fl, argp); > break; > case FASTRPC_IOCTL_MUNMAP: > + mutex_lock(&fl->mutex); > err = fastrpc_req_munmap(fl, argp); > + mutex_unlock(&fl->mutex); > break; > case FASTRPC_IOCTL_MEM_MAP: > err = fastrpc_req_mem_map(fl, argp); > -- > 2.25.1 > > > 2023년 3월 21일 (화) 오후 6:27, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org>님이 작성: >> >> Thanks Sangsup for reporting the issue and sharing the patch, >> >> Sorry, for some reason I missed this patch. >> >> On 16/02/2023 01:41, Sangsup Lee wrote: >>> This patch adds mutex_lock for fixing an Use-after-free bug. >>> fastrpc_req_munmap_impl can be called concurrently in multi-threded environments. >>> The buf which is allocated by list_for_each_safe can be used after another thread frees it. >>> >> Commit log can be improved here to something like: >> >> fastrcp_munmap takes two steps to unmap the memory, first to find a >> matching fastrpc buf in the list and second is to send request to DSP to >> unmap it. >> There is a potentially window of race between these two operations, >> which can lead to user-after-free. >> >> Fix this by adding locking around this two operations. >> >>> Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> >>> --- >>> drivers/misc/fastrpc.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >>> index 5310606113fe..c4b5fa4a50a6 100644 >>> --- a/drivers/misc/fastrpc.c >>> +++ b/drivers/misc/fastrpc.c >>> @@ -1806,10 +1806,12 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) >>> struct fastrpc_buf *buf = NULL, *iter, *b; >>> struct fastrpc_req_munmap req; >>> struct device *dev = fl->sctx->dev; >>> + int err; >>> >>> if (copy_from_user(&req, argp, sizeof(req))) >>> return -EFAULT; >>> >>> + mutex_lock(&fl->mutex); >>> spin_lock(&fl->lock); >>> list_for_each_entry_safe(iter, b, &fl->mmaps, node) { >>> if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) { >>> @@ -1822,10 +1824,13 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp) >>> if (!buf) { >>> dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n", >>> req.vaddrout, req.size); >>> + mutex_unlock(&fl->mutex); >>> return -EINVAL; >>> } >>> >>> - return fastrpc_req_munmap_impl(fl, buf); >>> + err = fastrpc_req_munmap_impl(fl, buf); >>> + mutex_unlock(&fl->mutex); >>> + return err; >> >> How about moving the locking to ioctl: >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index a701132638cf..2f217071a6c3 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -2087,7 +2087,9 @@ static long fastrpc_device_ioctl(struct file >> *file, unsigned int cmd, >> err = fastrpc_req_mmap(fl, argp); >> break; >> case FASTRPC_IOCTL_MUNMAP: >> + mutex_lock(&fl->mutex); >> err = fastrpc_req_munmap(fl, argp); >> + mutex_unlock(&fl->mutex); >> break; >> case FASTRPC_IOCTL_MEM_MAP: >> err = fastrpc_req_mem_map(fl, argp); >> >> >> thanks, >> srini >>> } >>> >>> static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-22 8:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-16 1:41 [PATCH] misc: fastrpc: Fix a Use after-free-bug by race condition Sangsup Lee 2023-02-16 1:53 ` sangsup lee 2023-03-21 9:27 ` Srinivas Kandagatla 2023-03-22 1:59 ` sangsup lee 2023-03-22 8:40 ` Srinivas Kandagatla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox