Re: [PATCH] ipc/mqueue: Avoid relying on a stack reference past its expiry

[Manfred Spraul <manfred@colorfullife.com>]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

Hi Varad,

On 5/4/21 5:55 PM, Varad Gautam wrote:
> do_mq_timedsend::__pipelined_op() should not dereference `this` after
> setting STATE_READY, as the receiver counterpart is now free to return.
> Change __pipelined_op to call wake_q_add_safe on the receiver's
> task_struct returned by get_task_struct, instead of dereferencing
> `this` which sits on the receiver's stack.
Correct. I was so concentrated on the risks of reordered memory that I 
have overlooked the simple bug.
> Fixes: c5b2cbdbdac563 ("ipc/mqueue.c: update/document memory barriers")
Actually, sem.c and msg.c contain the same bug. Thus all three must be 
fixed.
> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
> Reported-by: Matthias von Faber <matthias.vonfaber@aox-tech.de>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Manfred Spraul <manfred@colorfullife.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Davidlohr Bueso <dbueso@suse.de>
>
> ---
>   ipc/mqueue.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 8031464ed4ae2..8f78057c6be53 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -1004,12 +1004,14 @@ static inline void __pipelined_op(struct wake_q_head *wake_q,
>   				  struct mqueue_inode_info *info,
>   				  struct ext_wait_queue *this)
>   {
> +	struct task_struct *t;
> +
>   	list_del(&this->list);
> -	get_task_struct(this->task);
> +	t = get_task_struct(this->task);
>   
>   	/* see MQ_BARRIER for purpose/pairing */
>   	smp_store_release(&this->state, STATE_READY);
> -	wake_q_add_safe(wake_q, this->task);
> +	wake_q_add_safe(wake_q, t);
>   }

The change fixes the issue, but I would prefer to use t = this->task 
instead of using the return value of get_task_struct():
Then all wake_q_add_safe() users are identical.

Ok for you?

Slightly tested patch attached.

--

     Manfred


[-- Attachment #2: 0001-ipc-sem.c-mqueue.c-msg.c-Fix-incorrect-wake_q_add_sa.patch --]
[-- Type: text/x-patch, Size: 4005 bytes --]

From 06f91b4bbc440e9509c85acb7be1b15388b7bc0f Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
Date: Sat, 8 May 2021 18:41:30 +0200
Subject: [PATCH] ipc/sem.c, mqueue.c, msg.c: Fix incorrect wake_q_add_safe()

The wakeup code used by ipc contains a potential use-after-free:
When modifying the code to use wake_q_add_safe(), it was
forgotten to transfer the task struct pointer into a local
variable before the smp_store_release().

Solution: Add local variables to the affected functions.

Result: ipc is now using the same approach as kernel/futex.c
and kernel/locking/rwsem.c.

Note: No need to use READ_ONCE(), as smp_store_release() contains
a barrier(), thus the compiler cannot reread ptr->task.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Fixes: c5b2cbdbdac563 ("ipc/mqueue.c: update/document memory barriers")
Fixes: 8116b54e7e23ef ("ipc/sem.c: document and update memory barriers")
Fixes: 0d97a82ba830d8 ("ipc/msg.c: update and document memory barriers")
Reported-by: Matthias von Faber <matthias.vonfaber@aox-tech.de>
Cc: Varad Gautam <varad.gautam@suse.com>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/mqueue.c | 12 +++++++++---
 ipc/msg.c    |  6 ++++--
 ipc/sem.c    |  6 ++++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 8031464ed4ae..838c4f24a337 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -78,7 +78,11 @@ struct posix_msg_tree_node {
  * MQ_BARRIER:
  * To achieve proper release/acquire memory barrier pairing, the state is set to
  * STATE_READY with smp_store_release(), and it is read with READ_ONCE followed
- * by smp_acquire__after_ctrl_dep(). In addition, wake_q_add_safe() is used.
+ * by smp_acquire__after_ctrl_dep(). Immediately after the smp_store_release(),
+ * the struct ext_wait_queue can go out of scope. Thus the task struct pointer
+ * is copied into a local variable. The wakeup is performed using
+ * wake_q_add_safe().
+ *
  *
  * This prevents the following races:
  *
@@ -1004,12 +1008,14 @@ static inline void __pipelined_op(struct wake_q_head *wake_q,
 				  struct mqueue_inode_info *info,
 				  struct ext_wait_queue *this)
 {
+	struct task_struct *task = this->task;
+
 	list_del(&this->list);
-	get_task_struct(this->task);
+	get_task_struct(task);
 
 	/* see MQ_BARRIER for purpose/pairing */
 	smp_store_release(&this->state, STATE_READY);
-	wake_q_add_safe(wake_q, this->task);
+	wake_q_add_safe(wake_q, task);
 }
 
 /* pipelined_send() - send a message directly to the task waiting in
diff --git a/ipc/msg.c b/ipc/msg.c
index acd1bc7af55a..d273482b71ea 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -251,11 +251,13 @@ static void expunge_all(struct msg_queue *msq, int res,
 	struct msg_receiver *msr, *t;
 
 	list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) {
-		get_task_struct(msr->r_tsk);
+		struct task_struct *task = msr->r_tsk;
+
+		get_task_struct(task);
 
 		/* see MSG_BARRIER for purpose/pairing */
 		smp_store_release(&msr->r_msg, ERR_PTR(res));
-		wake_q_add_safe(wake_q, msr->r_tsk);
+		wake_q_add_safe(wake_q, task);
 	}
 }
 
diff --git a/ipc/sem.c b/ipc/sem.c
index e0ec239680cb..04700a823e79 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -784,12 +784,14 @@ static int perform_atomic_semop(struct sem_array *sma, struct sem_queue *q)
 static inline void wake_up_sem_queue_prepare(struct sem_queue *q, int error,
 					     struct wake_q_head *wake_q)
 {
-	get_task_struct(q->sleeper);
+	struct task_struct *task = q->sleeper;
+
+	get_task_struct(task);
 
 	/* see SEM_BARRIER_2 for purpose/pairing */
 	smp_store_release(&q->status, error);
 
-	wake_q_add_safe(wake_q, q->sleeper);
+	wake_q_add_safe(wake_q, task);
 }
 
 static void unlink_queue(struct sem_array *sma, struct sem_queue *q)
-- 
2.30.2