* [PATCH 0/3] misc: fastrpc: fixes for 6.1
@ 2022-11-24 17:49 Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Srinivas Kandagatla
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2022-11-24 17:49 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, Srinivas Kandagatla
Hi Greg,
Here are 3 fixes that were discovered during product stress testing, fixes are
around a race condtion resulting in use after free which are fixed with
using correct locking.
Am really not sure if you are taking fixes late in this cycle.
In case you are not could you please apply them for 6.2
Thanks,
Srini
Abel Vesa (2):
misc: fastrpc: Fix use-after-free and race in fastrpc_map_find
misc: fastrpc: Don't remove map on creater_process and device_release
Ola Jeppsson (1):
misc: fastrpc: Fix use-after-free race condition for maps
drivers/misc/fastrpc.c | 67 ++++++++++++++++++++++--------------------
1 file changed, 35 insertions(+), 32 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find
2022-11-24 17:49 [PATCH 0/3] misc: fastrpc: fixes for 6.1 Srinivas Kandagatla
@ 2022-11-24 17:49 ` Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 3/3] misc: fastrpc: Fix use-after-free race condition for maps Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2022-11-24 17:49 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, Abel Vesa, Ola Jeppsson, Srinivas Kandagatla
From: Abel Vesa <abel.vesa@linaro.org>
Currently, there is a race window between the point when the mutex is
unlocked in fastrpc_map_lookup and the reference count increasing
(fastrpc_map_get) in fastrpc_map_find, which can also lead to
use-after-free.
So lets merge fastrpc_map_find into fastrpc_map_lookup which allows us
to both protect the maps list by also taking the &fl->lock spinlock and
the reference count, since the spinlock will be released only after.
Add take_ref argument to make this suitable for all callers.
Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
Co-developed-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/misc/fastrpc.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7ff0b63c25e3..fc2173c59f8b 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -333,30 +333,31 @@ static void fastrpc_map_get(struct fastrpc_map *map)
static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
- struct fastrpc_map **ppmap)
+ struct fastrpc_map **ppmap, bool take_ref)
{
+ struct fastrpc_session_ctx *sess = fl->sctx;
struct fastrpc_map *map = NULL;
+ int ret = -ENOENT;
- mutex_lock(&fl->mutex);
+ spin_lock(&fl->lock);
list_for_each_entry(map, &fl->maps, node) {
- if (map->fd == fd) {
- *ppmap = map;
- mutex_unlock(&fl->mutex);
- return 0;
- }
- }
- mutex_unlock(&fl->mutex);
-
- return -ENOENT;
-}
+ if (map->fd != fd)
+ continue;
-static int fastrpc_map_find(struct fastrpc_user *fl, int fd,
- struct fastrpc_map **ppmap)
-{
- int ret = fastrpc_map_lookup(fl, fd, ppmap);
+ if (take_ref) {
+ ret = fastrpc_map_get(map);
+ if (ret) {
+ dev_dbg(sess->dev, "%s: Failed to get map fd=%d ret=%d\n",
+ __func__, fd, ret);
+ break;
+ }
+ }
- if (!ret)
- fastrpc_map_get(*ppmap);
+ *ppmap = map;
+ ret = 0;
+ break;
+ }
+ spin_unlock(&fl->lock);
return ret;
}
@@ -703,7 +704,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
struct fastrpc_map *map = NULL;
int err = 0;
- if (!fastrpc_map_find(fl, fd, ppmap))
+ if (!fastrpc_map_lookup(fl, fd, ppmap, true))
return 0;
map = kzalloc(sizeof(*map), GFP_KERNEL);
@@ -1026,7 +1027,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
if (!fdlist[i])
break;
- if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
+ if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
fastrpc_map_put(mmap);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release
2022-11-24 17:49 [PATCH 0/3] misc: fastrpc: fixes for 6.1 Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Srinivas Kandagatla
@ 2022-11-24 17:49 ` Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 3/3] misc: fastrpc: Fix use-after-free race condition for maps Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2022-11-24 17:49 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, Abel Vesa, Ola Jeppsson, Srinivas Kandagatla
From: Abel Vesa <abel.vesa@linaro.org>
Do not remove the map from the list on error path in
fastrpc_init_create_process, instead call fastrpc_map_put, to avoid
use-after-free. Do not remove it on fastrpc_device_release either,
call fastrpc_map_put instead.
The fastrpc_free_map is the only proper place to remove the map.
This is called only after the reference count is 0.
Fixes: b49f6d83e290 ("misc: fastrpc: Fix a possible double free")
Co-developed-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/misc/fastrpc.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index fc2173c59f8b..9ecbf673d321 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -316,6 +316,13 @@ static void fastrpc_free_map(struct kref *ref)
dma_buf_put(map->buf);
}
+ if (map->fl) {
+ spin_lock(&map->fl->lock);
+ list_del(&map->node);
+ spin_unlock(&map->fl->lock);
+ map->fl = NULL;
+ }
+
kfree(map);
}
@@ -1266,12 +1273,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
fl->init_mem = NULL;
fastrpc_buf_free(imem);
err_alloc:
- if (map) {
- spin_lock(&fl->lock);
- list_del(&map->node);
- spin_unlock(&fl->lock);
- fastrpc_map_put(map);
- }
+ fastrpc_map_put(map);
err:
kfree(args);
@@ -1347,10 +1349,8 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
fastrpc_context_put(ctx);
}
- list_for_each_entry_safe(map, m, &fl->maps, node) {
- list_del(&map->node);
+ list_for_each_entry_safe(map, m, &fl->maps, node)
fastrpc_map_put(map);
- }
list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
list_del(&buf->node);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] misc: fastrpc: Fix use-after-free race condition for maps
2022-11-24 17:49 [PATCH 0/3] misc: fastrpc: fixes for 6.1 Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Srinivas Kandagatla
@ 2022-11-24 17:49 ` Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2022-11-24 17:49 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, Ola Jeppsson, Abel Vesa, Srinivas Kandagatla
From: Ola Jeppsson <ola@snap.com>
It is possible that in between calling fastrpc_map_get() until
map->fl->lock is taken in fastrpc_free_map(), another thread can call
fastrpc_map_lookup() and get a reference to a map that is about to be
deleted.
Rewrite fastrpc_map_get() to only increase the reference count of a map
if it's non-zero. Propagate this to callers so they can know if a map is
about to be deleted.
Fixes this warning:
refcount_t: addition on 0; use-after-free.
WARNING: CPU: 5 PID: 10100 at lib/refcount.c:25 refcount_warn_saturate
...
Call trace:
refcount_warn_saturate
[fastrpc_map_get inlined]
[fastrpc_map_lookup inlined]
fastrpc_map_create
fastrpc_internal_invoke
fastrpc_device_ioctl
__arm64_sys_ioctl
invoke_syscall
Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Signed-off-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/misc/fastrpc.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 9ecbf673d321..80811e852d8f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -332,10 +332,12 @@ static void fastrpc_map_put(struct fastrpc_map *map)
kref_put(&map->refcount, fastrpc_free_map);
}
-static void fastrpc_map_get(struct fastrpc_map *map)
+static int fastrpc_map_get(struct fastrpc_map *map)
{
- if (map)
- kref_get(&map->refcount);
+ if (!map)
+ return -ENOENT;
+
+ return kref_get_unless_zero(&map->refcount) ? 0 : -ENOENT;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find
@ 2022-09-02 15:14 Abel Vesa
2022-09-02 15:14 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Abel Vesa
0 siblings, 1 reply; 6+ messages in thread
From: Abel Vesa @ 2022-09-02 15:14 UTC (permalink / raw)
To: Srinivas Kandagatla, Amol Maheshwari, Ekansh Gupta,
Vamsi Krishna Gattupalli
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm,
Linux Kernel Mailing List, Ola Jeppsson
Currently, there is a race window between the point when the mutex is
unlocked in fastrpc_map_lookup and the reference count increasing
(fastrpc_map_get) in fastrpc_map_find, which can also lead to
use-after-free.
So lets merge fastrpc_map_find into fastrpc_map_lookup which allows us
to both protect the maps list by also taking the &fl->lock spinlock and
the reference count, since the spinlock will be released only after.
Add take_ref argument to make this suitable for all callers.
Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
Co-developed-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
drivers/misc/fastrpc.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 93ebd174d848..0c816a11eeec 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -333,30 +333,31 @@ static void fastrpc_map_get(struct fastrpc_map *map)
static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
- struct fastrpc_map **ppmap)
+ struct fastrpc_map **ppmap, bool take_ref)
{
+ struct fastrpc_session_ctx *sess = fl->sctx;
struct fastrpc_map *map = NULL;
+ int ret = -ENOENT;
- mutex_lock(&fl->mutex);
+ spin_lock(&fl->lock);
list_for_each_entry(map, &fl->maps, node) {
- if (map->fd == fd) {
- *ppmap = map;
- mutex_unlock(&fl->mutex);
- return 0;
- }
- }
- mutex_unlock(&fl->mutex);
-
- return -ENOENT;
-}
+ if (map->fd != fd)
+ continue;
-static int fastrpc_map_find(struct fastrpc_user *fl, int fd,
- struct fastrpc_map **ppmap)
-{
- int ret = fastrpc_map_lookup(fl, fd, ppmap);
+ if (take_ref) {
+ ret = fastrpc_map_get(map);
+ if (ret) {
+ dev_dbg(sess->dev, "%s: Failed to get map fd=%d ret=%d\n",
+ __func__, fd, ret);
+ break;
+ }
+ }
- if (!ret)
- fastrpc_map_get(*ppmap);
+ *ppmap = map;
+ ret = 0;
+ break;
+ }
+ spin_unlock(&fl->lock);
return ret;
}
@@ -703,7 +704,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
struct fastrpc_map *map = NULL;
int err = 0;
- if (!fastrpc_map_find(fl, fd, ppmap))
+ if (!fastrpc_map_lookup(fl, fd, ppmap, true))
return 0;
map = kzalloc(sizeof(*map), GFP_KERNEL);
@@ -1026,7 +1027,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
if (!fdlist[i])
break;
- if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
+ if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
fastrpc_map_put(mmap);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release
2022-09-02 15:14 [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Abel Vesa
@ 2022-09-02 15:14 ` Abel Vesa
2022-09-22 22:46 ` Srinivas Kandagatla
0 siblings, 1 reply; 6+ messages in thread
From: Abel Vesa @ 2022-09-02 15:14 UTC (permalink / raw)
To: Srinivas Kandagatla, Amol Maheshwari, Ekansh Gupta,
Vamsi Krishna Gattupalli
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm,
Linux Kernel Mailing List, Ola Jeppsson
Do not remove the map from the list on error path in
fastrpc_init_create_process, instead call fastrpc_map_put, to avoid
use-after-free. Do not remove it on fastrpc_device_release either,
call fastrpc_map_put instead.
The fastrpc_free_map is the only proper place to remove the map.
This is called only after the reference count is 0.
Fixes: b49f6d83e290f ("misc: fastrpc: Fix a possible double free")
Co-developed-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Ola Jeppsson <ola@snap.com>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
drivers/misc/fastrpc.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 0c816a11eeec..50c17f5da3a8 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -316,6 +316,13 @@ static void fastrpc_free_map(struct kref *ref)
dma_buf_put(map->buf);
}
+ if (map->fl) {
+ spin_lock(&map->fl->lock);
+ list_del(&map->node);
+ spin_unlock(&map->fl->lock);
+ map->fl = NULL;
+ }
+
kfree(map);
}
@@ -1266,12 +1273,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
fl->init_mem = NULL;
fastrpc_buf_free(imem);
err_alloc:
- if (map) {
- spin_lock(&fl->lock);
- list_del(&map->node);
- spin_unlock(&fl->lock);
- fastrpc_map_put(map);
- }
+ fastrpc_map_put(map);
err:
kfree(args);
@@ -1347,10 +1349,8 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
fastrpc_context_put(ctx);
}
- list_for_each_entry_safe(map, m, &fl->maps, node) {
- list_del(&map->node);
+ list_for_each_entry_safe(map, m, &fl->maps, node)
fastrpc_map_put(map);
- }
list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
list_del(&buf->node);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release
2022-09-02 15:14 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Abel Vesa
@ 2022-09-22 22:46 ` Srinivas Kandagatla
0 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2022-09-22 22:46 UTC (permalink / raw)
To: Abel Vesa, Amol Maheshwari, Ekansh Gupta, Vamsi Krishna Gattupalli
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-arm-msm,
Linux Kernel Mailing List, Ola Jeppsson
On 02/09/2022 16:14, Abel Vesa wrote:
> Do not remove the map from the list on error path in
> fastrpc_init_create_process, instead call fastrpc_map_put, to avoid
> use-after-free. Do not remove it on fastrpc_device_release either,
> call fastrpc_map_put instead.
>
> The fastrpc_free_map is the only proper place to remove the map.
> This is called only after the reference count is 0.
>
> Fixes: b49f6d83e290f ("misc: fastrpc: Fix a possible double free")
> Co-developed-by: Ola Jeppsson <ola@snap.com>
> Signed-off-by: Ola Jeppsson <ola@snap.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> drivers/misc/fastrpc.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 0c816a11eeec..50c17f5da3a8 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -316,6 +316,13 @@ static void fastrpc_free_map(struct kref *ref)
> dma_buf_put(map->buf);
> }
>
> + if (map->fl) {
> + spin_lock(&map->fl->lock);
> + list_del(&map->node);
> + spin_unlock(&map->fl->lock);
> + map->fl = NULL;
> + }
> +
> kfree(map);
> }
>
> @@ -1266,12 +1273,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> fl->init_mem = NULL;
> fastrpc_buf_free(imem);
> err_alloc:
> - if (map) {
> - spin_lock(&fl->lock);
> - list_del(&map->node);
> - spin_unlock(&fl->lock);
> - fastrpc_map_put(map);
> - }
> + fastrpc_map_put(map);
> err:
> kfree(args);
>
> @@ -1347,10 +1349,8 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
> fastrpc_context_put(ctx);
> }
>
> - list_for_each_entry_safe(map, m, &fl->maps, node) {
> - list_del(&map->node);
> + list_for_each_entry_safe(map, m, &fl->maps, node)
> fastrpc_map_put(map);
> - }
>
> list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
> list_del(&buf->node);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-24 17:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24 17:49 [PATCH 0/3] misc: fastrpc: fixes for 6.1 Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Srinivas Kandagatla
2022-11-24 17:49 ` [PATCH 3/3] misc: fastrpc: Fix use-after-free race condition for maps Srinivas Kandagatla
-- strict thread matches above, loose matches on Subject: below --
2022-09-02 15:14 [PATCH 1/3] misc: fastrpc: Fix use-after-free and race in fastrpc_map_find Abel Vesa
2022-09-02 15:14 ` [PATCH 2/3] misc: fastrpc: Don't remove map on creater_process and device_release Abel Vesa
2022-09-22 22:46 ` Srinivas Kandagatla
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.