[PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[qiang.zhang]

From: Zqiang <qiang.zhang@windriver.com>

The syzbot report a UAF when iou-wrk accessing wqe of the hash
waitqueue. in the case of sharing a hash waitqueue between two
io-wq, when one of the io-wq is destroyed, all iou-wrk in this
io-wq are awakened, all wqe belonging to this io-wq are removed
from hash waitqueue, after that, all iou-wrk belonging to this
io-wq begin running, suppose following scenarios, wqe[0] and wqe[1]
belong to this io-wq, and these work has same hash value.

    CPU0	                                             CPU1
iou-wrk0(wqe[0])                                         iou-wrk1(wqe[1])

while test_bit IO_WQ_BIT_EXIT			while test_bit IO_WQ_BIT_EXIT
                                                   io_worker_handle_work
 schedule_timeout (sleep be break by wakeup         io_get_next_work
  and the IO_WQ_BIT_EXIT be set)                      set_bit hash

test_bit IO_WQ_BIT_EXIT (return true)
 wqe->work_list (is not empty)
  io_get_next_work
   io_wq_is_hashed
    test_and_set_bit hash (is true)		    (hash!=-1U&&!next_hashed) true
   (there is no work other than hash work)
    io_wait_on_hash                                 clear_bit hash
     spin_lock					     wq_has_sleeper (is false)
     list_empty(&wqe->wait.entry) (is true)
     __add_wait_queue				    (hash->wait is empty,not wakeup
						    and IO_WQ_BIT_EXIT has been set,
      ........					    the wqe->work_list is empty exit
   (there is no work other than hash work	    while loop)
	io_get_next_work will return NULL)
   return NULL					    (the wqe->work_list is empty
 						    the io_worker_handle_work is not
                                                    called)
io_worker_exit                                         io_worker_exit

In the above scenario, wqe may be mistakenly removing
opportunities from the queue, this leads to when the wqe is
released, it still in hash waitqueue. when a iou-wrk belonging
to another io-wq access hash waitqueue will trigger UAF,
To avoid this phenomenon, after all iou-wrk thread belonging to the
io-wq exit, remove wqe from the hash waiqueue, at this time,
there will be no operation to queue the wqe.

Reported-by: syzbot+6cb11ade52aa17095297@syzkaller.appspotmail.com
Signed-off-by: Zqiang <qiang.zhang@windriver.com>
---
 fs/io-wq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 5361a9b4b47b..911a1274aabd 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -1003,13 +1003,16 @@ static void io_wq_exit_workers(struct io_wq *wq)
 		struct io_wqe *wqe = wq->wqes[node];
 
 		io_wq_for_each_worker(wqe, io_wq_worker_wake, NULL);
-		spin_lock_irq(&wq->hash->wait.lock);
-		list_del_init(&wq->wqes[node]->wait.entry);
-		spin_unlock_irq(&wq->hash->wait.lock);
 	}
 	rcu_read_unlock();
 	io_worker_ref_put(wq);
 	wait_for_completion(&wq->worker_done);
+
+	for_each_node(node) {
+		spin_lock_irq(&wq->hash->wait.lock);
+		list_del_init(&wq->wqes[node]->wait.entry);
+		spin_unlock_irq(&wq->hash->wait.lock);
+	}
 	put_task_struct(wq->task);
 	wq->task = NULL;
 }
-- 
2.17.1

回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Zhang, Qiang]

________________________________________
发件人: Hillf Danton <hdanton@sina.com>
发送时间: 2021年5月24日 16:25
收件人: Zhang, Qiang
抄送: axboe@kernel.dk; asml.silence@gmail.com; syzbot+6cb11ade52aa17095297@syzkaller.appspotmail.com; io-uring@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Please note: This e-mail is from an EXTERNAL e-mail address]

On Mon, 24 May 2021 15:18:44 +0800
> From: Zqiang <qiang.zhang@windriver.com>
>
> The syzbot report a UAF when iou-wrk accessing wqe of the hash
> waitqueue. in the case of sharing a hash waitqueue between two
> io-wq, when one of the io-wq is destroyed, all iou-wrk in this
> io-wq are awakened, all wqe belonging to this io-wq are removed
> from hash waitqueue, after that, all iou-wrk belonging to this
> io-wq begin running, suppose following scenarios, wqe[0] and wqe[1]
> belong to this io-wq, and these work has same hash value.
>
>     CPU0                                                   CPU1
> iou-wrk0(wqe[0])                                         iou-wrk1(wqe[1])
>
> while test_bit IO_WQ_BIT_EXIT                 while test_bit IO_WQ_BIT_EXIT
>                                                    io_worker_handle_work
>  schedule_timeout (sleep be break by wakeup         io_get_next_work
>   and the IO_WQ_BIT_EXIT be set)                      set_bit hash
>
> test_bit IO_WQ_BIT_EXIT (return true)
>  wqe->work_list (is not empty)
>   io_get_next_work
>    io_wq_is_hashed
>     test_and_set_bit hash (is true)               (hash!=-1U&&!next_hashed) true
>    (there is no work other than hash work)
>     io_wait_on_hash                                 clear_bit hash
>      spin_lock                                             wq_has_sleeper (is false)
>      list_empty(&wqe->wait.entry) (is true)
>      __add_wait_queue                             (hash->wait is empty,not wakeup
>                                                   and IO_WQ_BIT_EXIT has been set,
>       ........                                            the wqe->work_list is empty exit
>    (there is no work other than hash work         while loop)
>       io_get_next_work will return NULL)
>    return NULL                                            (the wqe->work_list is empty
>                                                   the io_worker_handle_work is not
>                                                     called)
> io_worker_exit                                         io_worker_exit
>
> In the above scenario, wqe may be mistakenly removing
> opportunities from the queue, this leads to when the wqe is
> released, it still in hash waitqueue. when a iou-wrk belonging
> to another io-wq access hash waitqueue will trigger UAF,
> To avoid this phenomenon, after all iou-wrk thread belonging to the
> io-wq exit, remove wqe from the hash waiqueue, at this time,
> there will be no operation to queue the wqe.
>
> Reported-by: syzbot+6cb11ade52aa17095297@syzkaller.appspotmail.com
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> ---
>  fs/io-wq.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 5361a9b4b47b..911a1274aabd 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -1003,13 +1003,16 @@ static void io_wq_exit_workers(struct io_wq *wq)
>               struct io_wqe *wqe = wq->wqes[node];
>
>               io_wq_for_each_worker(wqe, io_wq_worker_wake, NULL);
> -             spin_lock_irq(&wq->hash->wait.lock);
> -             list_del_init(&wq->wqes[node]->wait.entry);
> -             spin_unlock_irq(&wq->hash->wait.lock);
>       }
>       rcu_read_unlock();
>       io_worker_ref_put(wq);
>       wait_for_completion(&wq->worker_done);
> +
> +     for_each_node(node) {
> +             spin_lock_irq(&wq->hash->wait.lock);
> +             list_del_init(&wq->wqes[node]->wait.entry);
> +             spin_unlock_irq(&wq->hash->wait.lock);
> +     }
>       put_task_struct(wq->task);
>       wq->task = NULL;
>  }
> --
> 2.17.1

>Scratch scalp one inch off to work out how this is a cure given a) uaf makes
>no sense without free and b) how io workers could survive
>wait_for_completion(&wq->worker_done).
>
>If they could OTOH then this is not the pill for the leak in worker_refs.

Hello Pavel Begunkov, Hillf Danton

Sorry there is a problem with the calltrace described in my message. Please ignore this modification 

Thanks
Qiang

Re: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Pavel Begunkov]

On 5/24/21 10:19 AM, Zhang, Qiang wrote:
[...]
>> Scratch scalp one inch off to work out how this is a cure given a) uaf makes
>> no sense without free and b) how io workers could survive
>> wait_for_completion(&wq->worker_done).
>>
>> If they could OTOH then this is not the pill for the leak in worker_refs.
> 
> Hello Pavel Begunkov, Hillf Danton
> 
> Sorry there is a problem with the calltrace described in my message. Please ignore this modification 

Haven't looked at the trace and description, but I do think
there is a problem it solves.

1) io_wait_on_hash() -> __add_wait_queue(&hash->wait, &wqe->wait);
2) (note: wqe is a worker) wqe's workers exit dropping refs
3) refs are zero, free io-wq
4) @hash is shared, so other task/wq does wake_up(&wq->hash->wait);
5) it wakes freed wqe

step 4) is a bit more trickier than that, tl;dr;
wq3:worker1 	| locks bit1
wq1:worker2 	| waits bit1
wq2:worker1 	| waits bit1
wq1:worker3 	| waits bit1

wq3:worker1	| drop  bit1
wq1:worker2	| locks bit1
wq1:worker2	| completes all wq1 bit1 work items
wq1:worker2	| drop  bit1, exit and free io-wq

wq2:worker1	| locks bit1
wq1 		| free complete
wq2:worker1	| drops bit1
wq1:worker3 	| waked up, even though freed

Can be simplified, don't want to waste time on that

-- 
Pavel Begunkov

Re: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Pavel Begunkov]

On 5/24/21 10:19 AM, Zhang, Qiang wrote:
> On Mon, 24 May 2021 15:18:44 +0800
>> From: Zqiang <qiang.zhang@windriver.com>
>>
>> The syzbot report a UAF when iou-wrk accessing wqe of the hash
>> waitqueue. in the case of sharing a hash waitqueue between two
>> io-wq, when one of the io-wq is destroyed, all iou-wrk in this
>> io-wq are awakened, all wqe belonging to this io-wq are removed
>> from hash waitqueue, after that, all iou-wrk belonging to this
>> io-wq begin running, suppose following scenarios, wqe[0] and wqe[1]
>> belong to this io-wq, and these work has same hash value.

Zhang, btw check your mail encoding, should some plain unicode


-- 
Pavel Begunkov

回复: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Zhang, Qiang]

________________________________________
发件人: Pavel Begunkov <asml.silence@gmail.com>
发送时间: 2021年5月24日 18:16
收件人: Zhang, Qiang; Hillf Danton; axboe@kernel.dk
抄送: syzbot+6cb11ade52aa17095297@syzkaller.appspotmail.com; io-uring@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Please note: This e-mail is from an EXTERNAL e-mail address]

On 5/24/21 10:19 AM, Zhang, Qiang wrote:
[...]
>> Scratch scalp one inch off to work out how this is a cure given a) uaf makes
>> no sense without free and b) how io workers could survive
>> wait_for_completion(&wq->worker_done).
>>
>> If they could OTOH then this is not the pill for the leak in worker_refs.
>
> Hello Pavel Begunkov, Hillf Danton
>
> Sorry there is a problem with the calltrace described in my message. Please ignore this modification
>
>Haven't looked at the trace and description, but I do think
>there is a problem it solves.
>
>1) io_wait_on_hash() -> __add_wait_queue(&hash->wait, &wqe->wait);
>2) (note: wqe is a worker) wqe's workers exit dropping refs
>3) refs are zero, free io-wq
>4) @hash is shared, so other task/wq does wake_up(&wq->hash->wait);
>5) it wakes freed wqe
>
>step 4) is a bit more trickier than that, tl;dr;
>wq3:worker1     | locks bit1
>wq1:worker2     | waits bit1
>wq2:worker1     | waits bit1
>wq1:worker3     | waits bit1
>
>wq3:worker1     | drop  bit1
>wq1:worker2     | locks bit1
>wq1:worker2     | completes all wq1 bit1 work items
>wq1:worker2     | drop  bit1, exit and free io-wq
>
>wq2:worker1     | locks bit1
>wq1             | free complete
>wq2:worker1     | drops bit1
>wq1:worker3     | waked up, even though freed
>
>Can be simplified, don't want to waste time on that

Thanks Pavel

Your description is better.  I have another question: under what circumstances will three io-wq(wq1, wq2, wq3) be created to share this @hash?

This kind of problem also occurs between two io-wq(wq1, wq2). Is the following description OK？

wq1:worker2     | locks bit1
wq2:worker1     | waits bit1
wq1:worker3     | waits bit1

wq1:worker2     | completes all wq1 bit1 work items
wq1:worker2     | drop  bit1, exit and free io-wq

wq2:worker1     | locks bit1
wq1                       | free complete
wq2:worker1     | drops bit1
wq1:worker3     | waked up, even though freed

Zhang
>
>--
>Pavel Begunkov

Re: 回复: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Pavel Begunkov]

On 5/25/21 3:01 AM, Zhang, Qiang wrote:
[...]
>> Haven't looked at the trace and description, but I do think
>> there is a problem it solves.
>>
>> 1) io_wait_on_hash() -> __add_wait_queue(&hash->wait, &wqe->wait);
>> 2) (note: wqe is a worker) wqe's workers exit dropping refs
>> 3) refs are zero, free io-wq
>> 4) @hash is shared, so other task/wq does wake_up(&wq->hash->wait);
>> 5) it wakes freed wqe
>>
>> step 4) is a bit more trickier than that, tl;dr;
>> wq3:worker1     | locks bit1
>> wq1:worker2     | waits bit1
>> wq2:worker1     | waits bit1
>> wq1:worker3     | waits bit1
>>
>> wq3:worker1     | drop  bit1
>> wq1:worker2     | locks bit1
>> wq1:worker2     | completes all wq1 bit1 work items
>> wq1:worker2     | drop  bit1, exit and free io-wq
>>
>> wq2:worker1     | locks bit1
>> wq1             | free complete
>> wq2:worker1     | drops bit1
>> wq1:worker3     | waked up, even though freed
>>
>> Can be simplified, don't want to waste time on that
> 
> Thanks Pavel
> 
> Your description is better.  I have another question: under what circumstances will three io-wq(wq1, wq2, wq3) be created to share this @hash?

Oops, missed the email. It's created by io_uring, and passed to
io-wq, which is per-task and created on demand by io_uring.

Can be achieved by a snippet just below, where threads
haven't had io_uring instances before.

thread1: ring = create_io_uring();
thread2: submit_sqes(ring);
thread3: submit_sqes(ring);

> 
> This kind of problem also occurs between two io-wq(wq1, wq2). Is the following description OK？

Yep, and I feel like there are cases simpler (and
more likely) than the one I described.

> 
> wq1:worker2     | locks bit1
> wq2:worker1     | waits bit1
> wq1:worker3     | waits bit1
> 
> wq1:worker2     | completes all wq1 bit1 work items
> wq1:worker2     | drop  bit1, exit and free io-wq
> 
> wq2:worker1     | locks bit1
> wq1                       | free complete
> wq2:worker1     | drops bit1
> wq1:worker3     | waked up, even though freed


-- 
Pavel Begunkov

Re: 回复: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Zhang, Qiang]

________________________________________
From: Pavel Begunkov <asml.silence@gmail.com>
Sent: Tuesday, 8 June 2021 01:38
To: Zhang, Qiang; Hillf Danton; axboe@kernel.dk
Cc: syzbot+6cb11ade52aa17095297@syzkaller.appspotmail.com; io-uring@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: 回复: 回复: [PATCH] io-wq: Fix UAF when wakeup wqe in hash waitqueue

[Please note: This e-mail is from an EXTERNAL e-mail address]

On 5/25/21 3:01 AM, Zhang, Qiang wrote:
[...]
>> Haven't looked at the trace and description, but I do think
>> there is a problem it solves.
>>
>> 1) io_wait_on_hash() -> __add_wait_queue(&hash->wait, &wqe->wait);
>> 2) (note: wqe is a worker) wqe's workers exit dropping refs
>> 3) refs are zero, free io-wq
>> 4) @hash is shared, so other task/wq does wake_up(&wq->hash->wait);
>> 5) it wakes freed wqe
>>
>> step 4) is a bit more trickier than that, tl;dr;
>> wq3:worker1     | locks bit1
>> wq1:worker2     | waits bit1
>> wq2:worker1     | waits bit1
>> wq1:worker3     | waits bit1
>>
>> wq3:worker1     | drop  bit1
>> wq1:worker2     | locks bit1
>> wq1:worker2     | completes all wq1 bit1 work items
>> wq1:worker2     | drop  bit1, exit and free io-wq
>>
>> wq2:worker1     | locks bit1
>> wq1             | free complete
>> wq2:worker1     | drops bit1
>> wq1:worker3     | waked up, even though freed
>>
>> Can be simplified, don't want to waste time on that
>
> Thanks Pavel
>
> Your description is better.  I have another question: under what circumstances will three io-wq(wq1, wq2, wq3) be created to share this @hash?

>Oops, missed the email. It's created by io_uring, and passed to
>io-wq, which is per-task and created on demand by io_uring.
>
>Can be achieved by a snippet just below, where threads
>haven't had io_uring instances before.
>
>thread1: ring = create_io_uring();
>thread2: submit_sqes(ring);
>thread3: submit_sqes(ring);

 Thank you for your explanation, Pavel

>
> This kind of problem also occurs between two io-wq(wq1, wq2). Is the following description OK？

>Yep, and I feel like there are cases simpler (and
>more likely) than the one I described.

>
> wq1:worker2     | locks bit1
> wq2:worker1     | waits bit1
> wq1:worker3     | waits bit1
>
> wq1:worker2     | completes all wq1 bit1 work items
> wq1:worker2     | drop  bit1, exit and free io-wq
>
> wq2:worker1     | locks bit1
> wq1                       | free complete
> wq2:worker1     | drops bit1
> wq1:worker3     | waked up, even though freed


>--
>Pavel Begunkov