Messages in this thread Patch in this message | | | From | Varad Gautam <> | Subject | [PATCH] ipc/mqueue: Avoid relying on a stack reference past its expiry | Date | Tue, 4 May 2021 17:55:33 +0200 |
| |
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:
[ 240.739977] RIP: 0010:wake_q_add_safe+0x13/0x60 [ 240.739991] Call Trace: [ 240.739999] __x64_sys_mq_timedsend+0x2a9/0x490 [ 240.740003] ? auditd_test_task+0x38/0x40 [ 240.740007] ? auditd_test_task+0x38/0x40 [ 240.740011] do_syscall_64+0x80/0x680 [ 240.740017] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 240.740019] 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. `ewq_addr` gets removed from info->e_wait_q[RECV].list.
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.
Fixes: c5b2cbdbdac563 ("ipc/mqueue.c: update/document memory barriers") 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); } /* pipelined_send() - send a message directly to the task waiting in -- 2.30.2
| |