________________________________________
??????: Hillf Danton <[email protected]>
????ʱ??: 2021??5??24?? 16:25
?ռ???: Zhang, Qiang
????: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
????: 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 <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Zqiang <[email protected]>
> ---
> 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
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
On 5/24/21 10:19 AM, Zhang, Qiang wrote:
> On Mon, 24 May 2021 15:18:44 +0800
>> From: Zqiang <[email protected]>
>>
>> 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
________________________________________
??????: Pavel Begunkov <[email protected]>
????ʱ??: 2021??5??24?? 18:16
?ռ???: Zhang, Qiang; Hillf Danton; [email protected]
????: [email protected]; [email protected]; [email protected]
????: 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
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
________________________________________
From: Pavel Begunkov <[email protected]>
Sent: Tuesday, 8 June 2021 01:38
To: Zhang, Qiang; Hillf Danton; [email protected]
Cc: [email protected]; [email protected]; [email protected]
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?H
>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