[PATCH] cgroup: handle cset multiidentity issue when migration

[PATCH] cgroup: handle cset multiidentity issue when migration

[shisiyuan]

Bug code flow:
cset X's initial refcount is 1.
1. cgroup_attach_task()
2. [For thread1]
   cgroup_migrate_add_src()
   [For thread2]
   cgroup_migrate_add_src()
     cset X is thread2's src_cset , ref->2,
     and its mg_preload_node is added to
     mgctx->preloaded_src_csets.
3. cgroup_migrate_prepare_dst()
   [For thread1]
   find_css_set()
     cset X is thread1's dst_cset, ref->3
   put_css_set()
     ref->2 because cset X's mg_preload_node is not
     empty(already in mgctx->preloaded_src_csets).
   [For thread2]
   find_css_cset()
     cset X is also thread2's dst_cset, ref->3
     then drop src_cset, ref->1
[cgroup_free] ref->0
4. cgroup_migrate_execute
   [For thread1]
   ref -> 0xc0000000(UAF)
The issue occurs because cset X's refcount should
not be substraced by one in this case. So mark a flag
'multiidentity' to this cset and call put_css_set()
one more time if 'multiidentity' marked in cleanup
progress.

Signed-off-by: shisiyuan <shisiyuan@xiaomi.com>
---
 include/linux/cgroup-defs.h |  5 +++++
 kernel/cgroup/cgroup.c      | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 8c19303..c9f2237 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -280,6 +280,11 @@ struct css_set {
 
 	/* For RCU-protected deletion */
 	struct rcu_head rcu_head;
+
+	/* Indicate if the css_set is the src_cset of one thread and
+	 * the dst_cset of anonther thread during migration process.
+	 */
+	bool multiidentity;
 };
 
 struct cgroup_base_stat {
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index e914646..cfdd12e 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2593,6 +2593,10 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx)
 		cset->mg_dst_cgrp = NULL;
 		cset->mg_dst_cset = NULL;
 		list_del_init(&cset->mg_preload_node);
+		if (cset->multiidentity == true) {
+			cset->multiidentity = false;
+			put_css_set_locked(cset);
+		}
 		put_css_set_locked(cset);
 	}
 
@@ -2648,6 +2652,18 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
 	list_add_tail(&src_cset->mg_preload_node, &mgctx->preloaded_src_csets);
 }
 
+static bool list_contains(struct list_head *node, struct list_head *head)
+{
+	struct list_head *tmp;
+
+	list_for_each(tmp, head) {
+		if (tmp == node)
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * cgroup_migrate_prepare_dst - prepare destination css_sets for migration
  * @mgctx: migration context
@@ -2700,6 +2716,13 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
 		if (list_empty(&dst_cset->mg_preload_node))
 			list_add_tail(&dst_cset->mg_preload_node,
 				      &mgctx->preloaded_dst_csets);
+		/* If dst_cset is already in mgctx->preloaded_src_csets,
+		 * it means the dst_cset has multi identities. Keep it
+		 * in mgctx->preloaded_src_csets, and mark it.
+		 */
+		else if (list_contains(&dst_cset->mg_preload_node,
+					&mgctx->preloaded_src_csets))
+			dst_cset->multiidentity = true;
 		else
 			put_css_set(dst_cset);
 
-- 
2.7.4

[PATCH] cgroup: handle cset multiidentity issue when migration

[shisiyuan]

Bug code flow:
cset X's initial refcount is 1.
1. cgroup_attach_task()
2. [For thread1]
   cgroup_migrate_add_src()
   [For thread2]
   cgroup_migrate_add_src()
     cset X is thread2's src_cset , ref->2,
     and its mg_preload_node is added to
     mgctx->preloaded_src_csets.
3. cgroup_migrate_prepare_dst()
   [For thread1]
   find_css_set()
     cset X is thread1's dst_cset, ref->3
   put_css_set()
     ref->2 because cset X's mg_preload_node is not
     empty(already in mgctx->preloaded_src_csets).
   [For thread2]
   find_css_cset()
     cset X is also thread2's dst_cset, ref->3
     then drop src_cset, ref->1
[cgroup_free] ref->0
4. cgroup_migrate_execute
   [For thread1]
   ref -> 0xc0000000(UAF)
The issue occurs because cset X's refcount should
not be substraced by one in this case. So mark a flag
'multiidentity' to this cset and call put_css_set()
one more time if 'multiidentity' marked in cleanup
progress.

Signed-off-by: shisiyuan <shisiyuan-95E7rUnPDLLQT0dZR+AlfA@public.gmane.org>
---
 include/linux/cgroup-defs.h |  5 +++++
 kernel/cgroup/cgroup.c      | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 8c19303..c9f2237 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -280,6 +280,11 @@ struct css_set {
 
 	/* For RCU-protected deletion */
 	struct rcu_head rcu_head;
+
+	/* Indicate if the css_set is the src_cset of one thread and
+	 * the dst_cset of anonther thread during migration process.
+	 */
+	bool multiidentity;
 };
 
 struct cgroup_base_stat {
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index e914646..cfdd12e 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2593,6 +2593,10 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx)
 		cset->mg_dst_cgrp = NULL;
 		cset->mg_dst_cset = NULL;
 		list_del_init(&cset->mg_preload_node);
+		if (cset->multiidentity == true) {
+			cset->multiidentity = false;
+			put_css_set_locked(cset);
+		}
 		put_css_set_locked(cset);
 	}
 
@@ -2648,6 +2652,18 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
 	list_add_tail(&src_cset->mg_preload_node, &mgctx->preloaded_src_csets);
 }
 
+static bool list_contains(struct list_head *node, struct list_head *head)
+{
+	struct list_head *tmp;
+
+	list_for_each(tmp, head) {
+		if (tmp == node)
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * cgroup_migrate_prepare_dst - prepare destination css_sets for migration
  * @mgctx: migration context
@@ -2700,6 +2716,13 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
 		if (list_empty(&dst_cset->mg_preload_node))
 			list_add_tail(&dst_cset->mg_preload_node,
 				      &mgctx->preloaded_dst_csets);
+		/* If dst_cset is already in mgctx->preloaded_src_csets,
+		 * it means the dst_cset has multi identities. Keep it
+		 * in mgctx->preloaded_src_csets, and mark it.
+		 */
+		else if (list_contains(&dst_cset->mg_preload_node,
+					&mgctx->preloaded_src_csets))
+			dst_cset->multiidentity = true;
 		else
 			put_css_set(dst_cset);
 
-- 
2.7.4

Re: [PATCH] cgroup: handle cset multiidentity issue when migration

[Michal Koutný]

Hello.

On Fri, Jun 03, 2022 at 12:34:48AM +0800, shisiyuan <shisiyuan19870131@gmail.com> wrote:
> Bug code flow:
> cset X's initial refcount is 1.
> 1. cgroup_attach_task()
> 2. [For thread1]
>    cgroup_migrate_add_src()
>    [For thread2]
>    cgroup_migrate_add_src()
>      cset X is thread2's src_cset , ref->2,
>      and its mg_preload_node is added to
>      mgctx->preloaded_src_csets.
> 3. cgroup_migrate_prepare_dst()
>    [For thread1]
>    find_css_set()
>      cset X is thread1's dst_cset, ref->3
>    put_css_set()
>      ref->2 because cset X's mg_preload_node is not
>      empty(already in mgctx->preloaded_src_csets).
>    [For thread2]
>    find_css_cset()
>      cset X is also thread2's dst_cset, ref->3
>      then drop src_cset, ref->1
> [cgroup_free] ref->0
> 4. cgroup_migrate_execute
>    [For thread1]
>    ref -> 0xc0000000(UAF)

I'm trying to understand when this happens.
You migrate a process with two threads while one of them exits?

This should be properly synchronized with cgroup_threadgroup_rwsem, so I
don't understand where does the [cgroup_free] between 3. and 4. come
from.

Do you have a reproducer for this?

Thanks,
Michal

Re: [PATCH] cgroup: handle cset multiidentity issue when migration

[Michal Koutný]

Hello.

On Fri, Jun 03, 2022 at 12:34:48AM +0800, shisiyuan <shisiyuan19870131-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Bug code flow:
> cset X's initial refcount is 1.
> 1. cgroup_attach_task()
> 2. [For thread1]
>    cgroup_migrate_add_src()
>    [For thread2]
>    cgroup_migrate_add_src()
>      cset X is thread2's src_cset , ref->2,
>      and its mg_preload_node is added to
>      mgctx->preloaded_src_csets.
> 3. cgroup_migrate_prepare_dst()
>    [For thread1]
>    find_css_set()
>      cset X is thread1's dst_cset, ref->3
>    put_css_set()
>      ref->2 because cset X's mg_preload_node is not
>      empty(already in mgctx->preloaded_src_csets).
>    [For thread2]
>    find_css_cset()
>      cset X is also thread2's dst_cset, ref->3
>      then drop src_cset, ref->1
> [cgroup_free] ref->0
> 4. cgroup_migrate_execute
>    [For thread1]
>    ref -> 0xc0000000(UAF)

I'm trying to understand when this happens.
You migrate a process with two threads while one of them exits?

This should be properly synchronized with cgroup_threadgroup_rwsem, so I
don't understand where does the [cgroup_free] between 3. and 4. come
from.

Do you have a reproducer for this?

Thanks,
Michal

Re: [PATCH] cgroup: handle cset multiidentity issue when migration

[Michal Koutný]

Hello.

On Thu, Jun 09, 2022 at 11:49:38AM +0800, 史思远 <shisiyuan19870131@gmail.com> wrote:
> The process is like above photo, thread 2 exits
> between cgroup_migrate_prepare_dst() and cgroup_migrate_execute().
> Then the refcount of csetX turns to be 0 here, and UAF appears when thread1
> migrating.
> Thread2 exits asynchronously, can rwsem prevent it?

See the bailout in cgroup_migrate_add_task():

	if (task->flags & PF_EXITING)
	        return;

And cgroup_threadgroup_change_begin(tsk) in exit_signals().

> The purpose of my patch is to keep csetX's refcount still 1 after thread2
> exits, and make sure thread1 migrating successfully.

Why is not src_cset==dst_cset in cgroup_migrate_prepare_dst() not
sufficient?

Still, can this be reproduced in real world or is your reasoning based
on theory only?

Thanks,
Michal

Re: [PATCH] cgroup: handle cset multiidentity issue when migration

[Michal Koutný]

Hello.

On Thu, Jun 09, 2022 at 11:49:38AM +0800, 史思远 <shisiyuan19870131-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> The process is like above photo, thread 2 exits
> between cgroup_migrate_prepare_dst() and cgroup_migrate_execute().
> Then the refcount of csetX turns to be 0 here, and UAF appears when thread1
> migrating.
> Thread2 exits asynchronously, can rwsem prevent it?

See the bailout in cgroup_migrate_add_task():

	if (task->flags & PF_EXITING)
	        return;

And cgroup_threadgroup_change_begin(tsk) in exit_signals().

> The purpose of my patch is to keep csetX's refcount still 1 after thread2
> exits, and make sure thread1 migrating successfully.

Why is not src_cset==dst_cset in cgroup_migrate_prepare_dst() not
sufficient?

Still, can this be reproduced in real world or is your reasoning based
on theory only?

Thanks,
Michal