| From a11ddb37bf367e6b5239b95ca759e5389bb46048 Mon Sep 17 00:00:00 2001 |
| From: Varad Gautam <varad.gautam@suse.com> |
| Date: Sat, 22 May 2021 17:41:49 -0700 |
| Subject: ipc/mqueue, msg, sem: avoid relying on a stack reference past its expiry |
| |
| From: Varad Gautam <varad.gautam@suse.com> |
| |
| commit a11ddb37bf367e6b5239b95ca759e5389bb46048 upstream. |
| |
| do_mq_timedreceive calls wq_sleep with a stack local address. The |
| sender (do_mq_timedsend) uses this address to later call pipelined_send. |
| |
| This leads to a very hard to trigger race where a do_mq_timedreceive |
| call might return and leave do_mq_timedsend to rely on an invalid |
| address, causing the following crash: |
| |
| RIP: 0010:wake_q_add_safe+0x13/0x60 |
| Call Trace: |
| __x64_sys_mq_timedsend+0x2a9/0x490 |
| do_syscall_64+0x80/0x680 |
| entry_SYSCALL_64_after_hwframe+0x44/0xa9 |
| RIP: 0033:0x7f5928e40343 |
| |
| The race occurs as: |
| |
| 1. do_mq_timedreceive calls wq_sleep with the address of `struct |
| ext_wait_queue` on function stack (aliased as `ewq_addr` here) - it |
| holds a valid `struct ext_wait_queue *` as long as the stack has not |
| been overwritten. |
| |
| 2. `ewq_addr` gets added to info->e_wait_q[RECV].list in wq_add, and |
| do_mq_timedsend receives it via wq_get_first_waiter(info, RECV) to call |
| __pipelined_op. |
| |
| 3. Sender calls __pipelined_op::smp_store_release(&this->state, |
| STATE_READY). Here is where the race window begins. (`this` is |
| `ewq_addr`.) |
| |
| 4. If the receiver wakes up now in do_mq_timedreceive::wq_sleep, it |
| will see `state == STATE_READY` and break. |
| |
| 5. do_mq_timedreceive returns, and `ewq_addr` is no longer guaranteed |
| to be a `struct ext_wait_queue *` since it was on do_mq_timedreceive's |
| stack. (Although the address may not get overwritten until another |
| function happens to touch it, which means it can persist around for an |
| indefinite time.) |
| |
| 6. do_mq_timedsend::__pipelined_op() still believes `ewq_addr` is a |
| `struct ext_wait_queue *`, and uses it to find a task_struct to pass to |
| the wake_q_add_safe call. In the lucky case where nothing has |
| overwritten `ewq_addr` yet, `ewq_addr->task` is the right task_struct. |
| In the unlucky case, __pipelined_op::wake_q_add_safe gets handed a |
| bogus address as the receiver's task_struct causing the crash. |
| |
| 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. |
| |
| As Manfred pointed out, the race potentially also exists in |
| ipc/msg.c::expunge_all and ipc/sem.c::wake_up_sem_queue_prepare. Fix |
| those in the same way. |
| |
| Link: https://lkml.kernel.org/r/20210510102950.12551-1-varad.gautam@suse.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") |
| Signed-off-by: Varad Gautam <varad.gautam@suse.com> |
| Reported-by: Matthias von Faber <matthias.vonfaber@aox-tech.de> |
| Acked-by: Davidlohr Bueso <dbueso@suse.de> |
| Acked-by: Manfred Spraul <manfred@colorfullife.com> |
| Cc: Christian Brauner <christian.brauner@ubuntu.com> |
| Cc: Oleg Nesterov <oleg@redhat.com> |
| Cc: "Eric W. Biederman" <ebiederm@xmission.com> |
| Cc: <stable@vger.kernel.org> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| ipc/mqueue.c | 6 ++++-- |
| ipc/msg.c | 6 ++++-- |
| ipc/sem.c | 6 ++++-- |
| 3 files changed, 12 insertions(+), 6 deletions(-) |
| |
| --- a/ipc/mqueue.c |
| +++ b/ipc/mqueue.c |
| @@ -1003,12 +1003,14 @@ static inline void __pipelined_op(struct |
| struct mqueue_inode_info *info, |
| struct ext_wait_queue *this) |
| { |
| + struct task_struct *task; |
| + |
| list_del(&this->list); |
| - get_task_struct(this->task); |
| + task = 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, task); |
| } |
| |
| /* pipelined_send() - send a message directly to the task waiting in |
| --- a/ipc/msg.c |
| +++ b/ipc/msg.c |
| @@ -251,11 +251,13 @@ static void expunge_all(struct msg_queue |
| 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 *r_tsk; |
| + |
| + r_tsk = get_task_struct(msr->r_tsk); |
| |
| /* 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, r_tsk); |
| } |
| } |
| |
| --- a/ipc/sem.c |
| +++ b/ipc/sem.c |
| @@ -784,12 +784,14 @@ would_block: |
| 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 *sleeper; |
| + |
| + sleeper = get_task_struct(q->sleeper); |
| |
| /* see SEM_BARRIER_2 for purpuse/pairing */ |
| smp_store_release(&q->status, error); |
| |
| - wake_q_add_safe(wake_q, q->sleeper); |
| + wake_q_add_safe(wake_q, sleeper); |
| } |
| |
| static void unlink_queue(struct sem_array *sma, struct sem_queue *q) |