FAILED: patch "[PATCH] shm: extend forced shm destroy to support objects from" failed to apply to 4.14-stable tree

PATCH In review [email protected] 3 participants 4 messages Nov 22, 2021 – Nov 30, 2021 Stable branch

Share & tools

[email protected]

author maintainer

2021-11-22 12:21 UTC #0 lore raw

The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 85b6d24646e4125c591639841169baa98a2da503 Mon Sep 17 00:00:00 2001
From: Alexander Mikhalitsyn <[email protected]>
Date: Fri, 19 Nov 2021 16:43:21 -0800
Subject: [PATCH] shm: extend forced shm destroy to support objects from
several IPC nses

Currently, the exit_shm() function not designed to work properly when task->sysvshm.shm_clist holds shm objects from different IPC namespaces.

This is a real pain when sysctl kernel.shm_rmid_forced = 1, because it leads to use-after-free (reproducer exists).

This is an attempt to fix the problem by extending exit_shm mechanism to handle shm's destroy from several IPC ns'es.

To achieve that we do several things:

1. add a namespace (non-refcounted) pointer to the struct shmid_kernel

2. during new shm object creation (newseg()/shmget syscall) we initialize this pointer by current task IPC ns

3. exit_shm() fully reworked such that it traverses over all shp's in task->sysvshm.shm_clist and gets IPC namespace not from current task as it was before but from shp's object itself, then call shm_destroy(shp, ns).

Note: We need to be really careful here, because as it was said before (1), our pointer to IPC ns non-refcnt'ed. To be on the safe side we using special helper get_ipc_ns_not_zero() which allows to get IPC ns refcounter only if IPC ns not in the "state of destruction".

Q/A

Q: Why can we access shp->ns memory using non-refcounted pointer?
A: Because shp object lifetime is always shorther than IPC namespace
lifetime, so, if we get shp object from the task->sysvshm.shm_clist while holding task_lock(task) nobody can steal our namespace.

Q: Does this patch change semantics of unshare/setns/clone syscalls?
A: No. It's just fixes non-covered case when process may leave IPC
namespace without getting task->sysvshm.shm_clist list cleaned up.

Link https://lkml.kernel.org/r/[email protected] open here

Fixes ab602f79915 ("shm: make exit_shm work proportional to task activity")

Co-developed-by Manfred Spraul <[email protected]>

Signed-off-by Manfred Spraul <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Greg KH <[email protected]>

Cc Andrei Vagin <[email protected]>

Cc Pavel Tikhomirov <[email protected]>

Cc Vasily Averin <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

include/linux/ipc_namespace.h +15 -0

index 05e22770af51..b75395ec8d52 100644
@@ -131,6 +131,16 @@ static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
 	return ns;
 }
 
+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+	if (ns) {
+		if (refcount_inc_not_zero(&ns->ns.count))
+			return ns;
+	}
+
+	return NULL;
+}
+
 extern void put_ipc_ns(struct ipc_namespace *ns);
 #else
 static inline struct ipc_namespace *copy_ipcs(unsigned long flags,

@@ -147,6 +157,11 @@ static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
 	return ns;
 }
 
+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+	return ns;
+}
+
 static inline void put_ipc_ns(struct ipc_namespace *ns)
 {
 }

include/linux/sched/task.h +1 -1

index ba88a6987400..058d7f371e25 100644
@@ -158,7 +158,7 @@ static inline struct vm_struct *task_stack_vm_area(const struct task_struct *t)
  * Protects ->fs, ->files, ->mm, ->group_info, ->comm, keyring
  * subscriptions and synchronises with wait4().  Also used in procfs.  Also
  * pins the final release of task.io_context.  Also protects ->cpuset and
- * ->cgroup.subsys[]. And ->vfork_done.
+ * ->cgroup.subsys[]. And ->vfork_done. And ->sysvshm.shm_clist.
  *
  * Nests both inside and outside of read_lock(&tasklist_lock).
  * It must not be nested with write_lock_irq(&tasklist_lock),

ipc/shm.c +143 ~~-46~~

264 lines not on this page · raw message

Alexander Mikhalitsyn

2021-11-29 19:49 UTC #1 lore raw

PATCH 1/1 shm: extend forced shm destroy to support objects from several IPC nses

For 4.14.y:

Upstream commit 85b6d24646e4 ("shm: extend forced shm destroy to support objects from several IPC nses")

Currently, the exit_shm() function not designed to work properly when task->sysvshm.shm_clist holds shm objects from different IPC namespaces.

This is a real pain when sysctl kernel.shm_rmid_forced = 1, because it leads to use-after-free (reproducer exists).

This is an attempt to fix the problem by extending exit_shm mechanism to handle shm's destroy from several IPC ns'es.

To achieve that we do several things:

1. add a namespace (non-refcounted) pointer to the struct shmid_kernel

2. during new shm object creation (newseg()/shmget syscall) we initialize this pointer by current task IPC ns

Q/A

Link https://lkml.kernel.org/r/[email protected] open here

Fixes ab602f79915 ("shm: make exit_shm work proportional to task activity")

Co-developed-by Manfred Spraul <[email protected]>

Signed-off-by Manfred Spraul <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Greg KH <[email protected]>

Cc Andrei Vagin <[email protected]>

Cc Pavel Tikhomirov <[email protected]>

Cc Vasily Averin <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

---
 include/linux/ipc_namespace.h |  15 +++
 include/linux/sched/task.h    |   2 +-
 include/linux/shm.h           |  13 ++-
 ipc/shm.c                     | 176 +++++++++++++++++++++++++---------
 4 files changed, 159 insertions(+), 47 deletions(-)

include/linux/ipc_namespace.h +15 -0

index 474812abe773..e661765271ea 100644
@@ -127,6 +127,16 @@ static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
 	return ns;
 }
 
+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+	if (ns) {
+		if (refcount_inc_not_zero(&ns->count))
+			return ns;
+	}
+
+	return NULL;
+}
+
 extern void put_ipc_ns(struct ipc_namespace *ns);
 #else
 static inline struct ipc_namespace *copy_ipcs(unsigned long flags,

@@ -143,6 +153,11 @@ static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
 	return ns;
 }
 
+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+	return ns;
+}
+
 static inline void put_ipc_ns(struct ipc_namespace *ns)
 {
 }

include/linux/sched/task.h +1 -1

index 11b4fba82950..c3d157a37073 100644
@@ -122,7 +122,7 @@ static inline struct vm_struct *task_stack_vm_area(const struct task_struct *t)
  * Protects ->fs, ->files, ->mm, ->group_info, ->comm, keyring
  * subscriptions and synchronises with wait4().  Also used in procfs.  Also
  * pins the final release of task.io_context.  Also protects ->cpuset and
- * ->cgroup.subsys[]. And ->vfork_done.
+ * ->cgroup.subsys[]. And ->vfork_done. And ->sysvshm.shm_clist.
  *
  * Nests both inside and outside of read_lock(&tasklist_lock).
  * It must not be nested with write_lock_irq(&tasklist_lock),

include/linux/shm.h +11 -2

index 2bbafacfbfc9..3ef303df784e 100644
@@ -20,9 +20,18 @@ struct shmid_kernel /* private to the kernel */
 	pid_t			shm_lprid;
 	struct user_struct	*mlock_user;
 
-	/* The task created the shm object.  NULL if the task is dead. */
+	/*
+	 * The task created the shm object, for
+	 * task_lock(shp->shm_creator)
+	 */
 	struct task_struct	*shm_creator;
-	struct list_head	shm_clist;	/* list by creator */
+
+	/*
+	 * List by creator. task_lock(->shm_creator) required for read/write.
+	 * If list_empty(), then the creator is dead already.
+	 */
+	struct list_head	shm_clist;
+	struct ipc_namespace	*ns;
 } __randomize_layout;
 
 /* shm_mode upper byte flags */

ipc/shm.c +132 ~~-44~~

244 lines not on this page · raw message

Signature · 1 line

2.31.1

Manfred Spraul

2021-11-30 09:56 UTC #2 lore raw

Re: [PATCH] shm: extend forced shm destroy to support objects from several IPC nses

Hi,

Quoted 56 lines · Alexander Mikhalitsyn

For 4.14.y:
Upstream commit 85b6d24646e4 ("shm: extend forced shm destroy to support objects from several IPC nses")
Currently, the exit_shm() function not designed to work properly when task->sysvshm.shm_clist holds shm objects from different IPC namespaces.
This is a real pain when sysctl kernel.shm_rmid_forced = 1, because it leads to use-after-free (reproducer exists).
This is an attempt to fix the problem by extending exit_shm mechanism to handle shm's destroy from several IPC ns'es.
To achieve that we do several things:
1. add a namespace (non-refcounted) pointer to the struct shmid_kernel
2. during new shm object creation (newseg()/shmget syscall) we initialize this pointer by current task IPC ns
3. exit_shm() fully reworked such that it traverses over all shp's in task->sysvshm.shm_clist and gets IPC namespace not from current task as it was before but from shp's object itself, then call shm_destroy(shp, ns).
Note: We need to be really careful here, because as it was said before (1), our pointer to IPC ns non-refcnt'ed. To be on the safe side we using special helper get_ipc_ns_not_zero() which allows to get IPC ns refcounter only if IPC ns not in the "state of destruction".
Q/A
Q: Why can we access shp->ns memory using non-refcounted pointer?
A: Because shp object lifetime is always shorther than IPC namespace
lifetime, so, if we get shp object from the task->sysvshm.shm_clist while holding task_lock(task) nobody can steal our namespace.
Q: Does this patch change semantics of unshare/setns/clone syscalls?
A: No. It's just fixes non-covered case when process may leave IPC
    namespace without getting task->sysvshm.shm_clist list cleaned up.
Link https://lkml.kernel.org/r/[email protected] open here
Link https://lkml.kernel.org/r/[email protected] open here
Fixes ab602f79915 ("shm: make exit_shm work proportional to task activity")
Co-developed-by Manfred Spraul <[email protected]>
Signed-off-by Manfred Spraul <[email protected]>
Signed-off-by Alexander Mikhalitsyn <[email protected]>
Cc "Eric W. Biederman" <[email protected]>
Cc Davidlohr Bueso <[email protected]>
Cc Greg KH <[email protected]>
Cc Andrei Vagin <[email protected]>
Cc Pavel Tikhomirov <[email protected]>
Cc Vasily Averin <[email protected]>
Cc <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>

Quoted 6 lines

---
  include/linux/ipc_namespace.h |  15 +++
  include/linux/sched/task.h    |   2 +-
  include/linux/shm.h           |  13 ++-
  ipc/shm.c                     | 176 +++++++++++++++++++++++++---------
  4 files changed, 159 insertions(+), 47 deletions(-)

Tested ok: The crash is resolved, no observed degradations.

Manfred

Greg KH

author maintainer

2021-11-30 10:32 UTC #3 lore raw

Re: [PATCH] shm: extend forced shm destroy to support objects from several IPC nses

Quoted 66 lines · Manfred Spraul

Hi,
Quoted 56 lines · Alexander Mikhalitsyn
For 4.14.y:
Upstream commit 85b6d24646e4 ("shm: extend forced shm destroy to support objects from several IPC nses")
Currently, the exit_shm() function not designed to work properly when task->sysvshm.shm_clist holds shm objects from different IPC namespaces.
This is a real pain when sysctl kernel.shm_rmid_forced = 1, because it leads to use-after-free (reproducer exists).
This is an attempt to fix the problem by extending exit_shm mechanism to handle shm's destroy from several IPC ns'es.
To achieve that we do several things:
1. add a namespace (non-refcounted) pointer to the struct shmid_kernel
2. during new shm object creation (newseg()/shmget syscall) we initialize this pointer by current task IPC ns
3. exit_shm() fully reworked such that it traverses over all shp's in task->sysvshm.shm_clist and gets IPC namespace not from current task as it was before but from shp's object itself, then call shm_destroy(shp, ns).
Note: We need to be really careful here, because as it was said before (1), our pointer to IPC ns non-refcnt'ed. To be on the safe side we using special helper get_ipc_ns_not_zero() which allows to get IPC ns refcounter only if IPC ns not in the "state of destruction".
Q/A
Q: Why can we access shp->ns memory using non-refcounted pointer?
A: Because shp object lifetime is always shorther than IPC namespace
lifetime, so, if we get shp object from the task->sysvshm.shm_clist while holding task_lock(task) nobody can steal our namespace.
Q: Does this patch change semantics of unshare/setns/clone syscalls?
A: No. It's just fixes non-covered case when process may leave IPC
    namespace without getting task->sysvshm.shm_clist list cleaned up.
Link https://lkml.kernel.org/r/[email protected] open here
Link https://lkml.kernel.org/r/[email protected] open here
Fixes ab602f79915 ("shm: make exit_shm work proportional to task activity")
Co-developed-by Manfred Spraul <[email protected]>
Signed-off-by Manfred Spraul <[email protected]>
Signed-off-by Alexander Mikhalitsyn <[email protected]>
Cc "Eric W. Biederman" <[email protected]>
Cc Davidlohr Bueso <[email protected]>
Cc Greg KH <[email protected]>
Cc Andrei Vagin <[email protected]>
Cc Pavel Tikhomirov <[email protected]>
Cc Vasily Averin <[email protected]>
Cc <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
Quoted 6 lines
---
  include/linux/ipc_namespace.h |  15 +++
  include/linux/sched/task.h    |   2 +-
  include/linux/shm.h           |  13 ++-
  ipc/shm.c                     | 176 +++++++++++++++++++++++++---------
  4 files changed, 159 insertions(+), 47 deletions(-)

Quoted 1 line

Tested ok: The crash is resolved, no observed degradations.

Now queued up, thanks.

greg k-h