| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 18 May 2022 06:54:11 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Lee Jones <lee.jones@...aro.org>
Cc: Pavel Begunkov <asml.silence@...il.com>, io-uring@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [REPORT] Use-after-free Read in __fdget_raw in v5.10.y
On 5/18/22 6:52 AM, Jens Axboe wrote:
> On 5/18/22 6:50 AM, Lee Jones wrote:
>> On Tue, 17 May 2022, Jens Axboe wrote:
>>
>>> On 5/17/22 7:00 AM, Lee Jones wrote:
>>>> On Tue, 17 May 2022, Jens Axboe wrote:
>>>>
>>>>> On 5/17/22 6:36 AM, Lee Jones wrote:
>>>>>> On Tue, 17 May 2022, Jens Axboe wrote:
>>>>>>
>>>>>>> On 5/17/22 6:24 AM, Lee Jones wrote:
>>>>>>>> On Tue, 17 May 2022, Jens Axboe wrote:
>>>>>>>>
>>>>>>>>> On 5/17/22 5:41 AM, Lee Jones wrote:
>>>>>>>>>> Good afternoon Jens, Pavel, et al.,
>>>>>>>>>>
>>>>>>>>>> Not sure if you are presently aware, but there appears to be a
>>>>>>>>>> use-after-free issue affecting the io_uring worker driver (fs/io-wq.c)
>>>>>>>>>> in Stable v5.10.y.
>>>>>>>>>>
>>>>>>>>>> The full sysbot report can be seen below [0].
>>>>>>>>>>
>>>>>>>>>> The C-reproducer has been placed below that [1].
>>>>>>>>>>
>>>>>>>>>> I had great success running this reproducer in an infinite loop.
>>>>>>>>>>
>>>>>>>>>> My colleague reverse-bisected the fixing commit to:
>>>>>>>>>>
>>>>>>>>>> commit fb3a1f6c745ccd896afadf6e2d6f073e871d38ba
>>>>>>>>>> Author: Jens Axboe <axboe@...nel.dk>
>>>>>>>>>> Date: Fri Feb 26 09:47:20 2021 -0700
>>>>>>>>>>
>>>>>>>>>> io-wq: have manager wait for all workers to exit
>>>>>>>>>>
>>>>>>>>>> Instead of having to wait separately on workers and manager, just have
>>>>>>>>>> the manager wait on the workers. We use an atomic_t for the reference
>>>>>>>>>> here, as we need to start at 0 and allow increment from that. Since the
>>>>>>>>>> number of workers is naturally capped by the allowed nr of processes,
>>>>>>>>>> and that uses an int, there is no risk of overflow.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jens Axboe <axboe@...nel.dk>
>>>>>>>>>>
>>>>>>>>>> fs/io-wq.c | 30 ++++++++++++++++++++++--------
>>>>>>>>>> 1 file changed, 22 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> Does this fix it:
>>>>>>>>>
>>>>>>>>> commit 886d0137f104a440d9dfa1d16efc1db06c9a2c02
>>>>>>>>> Author: Jens Axboe <axboe@...nel.dk>
>>>>>>>>> Date: Fri Mar 5 12:59:30 2021 -0700
>>>>>>>>>
>>>>>>>>> io-wq: fix race in freeing 'wq' and worker access
>>>>>>>>>
>>>>>>>>> Looks like it didn't make it into 5.10-stable, but we can certainly
>>>>>>>>> rectify that.
>>>>>>>>
>>>>>>>> Thanks for your quick response Jens.
>>>>>>>>
>>>>>>>> This patch doesn't apply cleanly to v5.10.y.
>>>>>>>
>>>>>>> This is probably why it never made it into 5.10-stable :-/
>>>>>>
>>>>>> Right. It doesn't apply at all unfortunately.
>>>>>>
>>>>>>>> I'll have a go at back-porting it. Please bear with me.
>>>>>>>
>>>>>>> Let me know if you into issues with that and I can help out.
>>>>>>
>>>>>> I think the dependency list is too big.
>>>>>>
>>>>>> Too much has changed that was never back-ported.
>>>>>>
>>>>>> Actually the list of patches pertaining to fs/io-wq.c alone isn't so
>>>>>> bad, I did start to back-port them all but some of the big ones have
>>>>>> fs/io_uring.c changes incorporated and that list is huge (256 patches
>>>>>> from v5.10 to the fixing patch mentioned above).
>>>>>
>>>>> The problem is that 5.12 went to the new worker setup, and this patch
>>>>> landed after that even though it also applies to the pre-native workers.
>>>>> Hence the dependency chain isn't really as long as it seems, probably
>>>>> just a few patches backporting the change references and completions.
>>>>>
>>>>> I'll take a look this afternoon.
>>>>
>>>> Thanks Jens. I really appreciate it.
>>>
>>> Can you see if this helps? Untested...
>>
>> What base does this apply against please?
>>
>> I tried Mainline and v5.10.116 and both failed.
>
> It's against 5.10.116, so that's puzzling. Let me double check I sent
> the right one...
Looks like I sent the one from the wrong directory, sorry about that.
This one should be better:
diff --git a/fs/io-wq.c b/fs/io-wq.c
index 3d5fc76b92d0..35af489bcaf6 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -125,6 +125,9 @@ struct io_wq {
refcount_t refs;
struct completion done;
+ atomic_t worker_refs;
+ struct completion worker_done;
+
struct hlist_node cpuhp_node;
refcount_t use_refs;
@@ -250,8 +253,8 @@ static void io_worker_exit(struct io_worker *worker)
raw_spin_unlock_irq(&wqe->lock);
kfree_rcu(worker, rcu);
- if (refcount_dec_and_test(&wqe->wq->refs))
- complete(&wqe->wq->done);
+ if (atomic_dec_and_test(&wqe->wq->worker_refs))
+ complete(&wqe->wq->worker_done);
}
static inline bool io_wqe_run_queue(struct io_wqe *wqe)
@@ -695,9 +698,13 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
worker->wqe = wqe;
spin_lock_init(&worker->lock);
+ atomic_inc(&wq->worker_refs);
+
worker->task = kthread_create_on_node(io_wqe_worker, worker, wqe->node,
"io_wqe_worker-%d/%d", index, wqe->node);
if (IS_ERR(worker->task)) {
+ if (atomic_dec_and_test(&wq->worker_refs))
+ complete(&wq->worker_done);
kfree(worker);
return false;
}
@@ -717,7 +724,6 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
if (index == IO_WQ_ACCT_UNBOUND)
atomic_inc(&wq->user->processes);
- refcount_inc(&wq->refs);
wake_up_process(worker->task);
return true;
}
@@ -822,17 +828,18 @@ static int io_wq_manager(void *data)
task_work_run();
out:
- if (refcount_dec_and_test(&wq->refs)) {
- complete(&wq->done);
- return 0;
- }
/* if ERROR is set and we get here, we have workers to wake */
- if (test_bit(IO_WQ_BIT_ERROR, &wq->state)) {
- rcu_read_lock();
- for_each_node(node)
- io_wq_for_each_worker(wq->wqes[node], io_wq_worker_wake, NULL);
- rcu_read_unlock();
- }
+ rcu_read_lock();
+ for_each_node(node)
+ io_wq_for_each_worker(wq->wqes[node], io_wq_worker_wake, NULL);
+ rcu_read_unlock();
+
+ if (atomic_read(&wq->worker_refs))
+ wait_for_completion(&wq->worker_done);
+
+ if (refcount_dec_and_test(&wq->refs))
+ complete(&wq->done);
+
return 0;
}
@@ -1135,6 +1142,9 @@ struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data)
init_completion(&wq->done);
+ init_completion(&wq->worker_done);
+ atomic_set(&wq->worker_refs, 0);
+
wq->manager = kthread_create(io_wq_manager, wq, "io_wq_manager");
if (!IS_ERR(wq->manager)) {
wake_up_process(wq->manager);
@@ -1179,11 +1189,6 @@ static void __io_wq_destroy(struct io_wq *wq)
if (wq->manager)
kthread_stop(wq->manager);
- rcu_read_lock();
- for_each_node(node)
- io_wq_for_each_worker(wq->wqes[node], io_wq_worker_wake, NULL);
- rcu_read_unlock();
-
wait_for_completion(&wq->done);
for_each_node(node)
--
Jens Axboe
Powered by blists - more mailing lists