| 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
| ||
|
Message-ID: <4a920143-d899-8811-a767-d114dba1e4e3@kernel.dk>
Date: Thu, 24 Oct 2019 18:52:34 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Jann Horn <jannh@...gle.com>
Cc: linux-block@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/3] io_uring: add support for async work inheriting files
table
On 10/24/19 6:35 PM, Jens Axboe wrote:
> On 10/24/19 5:13 PM, Jann Horn wrote:
>> On Fri, Oct 25, 2019 at 12:04 AM Jens Axboe <axboe@...nel.dk> wrote:
>>> On 10/24/19 2:31 PM, Jann Horn wrote:
>>>> On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@...nel.dk> wrote:
>>>>> On 10/18/19 12:50 PM, Jann Horn wrote:
>>>>>> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@...nel.dk> wrote:
>>>>>>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>>>>>>> But actually, by the way: Is this whole files_struct thing creating a
>>>>>>>> reference loop? The files_struct has a reference to the uring file,
>>>>>>>> and the uring file has ACCEPT work that has a reference to the
>>>>>>>> files_struct. If the task gets killed and the accept work blocks, the
>>>>>>>> entire files_struct will stay alive, right?
>>>>>>>
>>>>>>> Yes, for the lifetime of the request, it does create a loop. So if the
>>>>>>> application goes away, I think you're right, the files_struct will stay.
>>>>>>> And so will the io_uring, for that matter, as we depend on the closing
>>>>>>> of the files to do the final reap.
>>>>>>>
>>>>>>> Hmm, not sure how best to handle that, to be honest. We need some way to
>>>>>>> break the loop, if the request never finishes.
>>>>>>
>>>>>> A wacky and dubious approach would be to, instead of taking a
>>>>>> reference to the files_struct, abuse f_op->flush() to synchronously
>>>>>> flush out pending requests with references to the files_struct... But
>>>>>> it's probably a bad idea, given that in f_op->flush(), you can't
>>>>>> easily tell which files_struct the close is coming from. I suppose you
>>>>>> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
>>>>>> have come in and then let f_op->flush() probe whether the file
>>>>>> pointers are gone from them...
>>>>>
>>>>> Got back to this after finishing the io-wq stuff, which we need for the
>>>>> cancel.
>>>>>
>>>>> Here's an updated patch:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
>>>>>
>>>>> that seems to work for me (lightly tested), we correctly find and cancel
>>>>> work that is holding on to the file table.
>>>>>
>>>>> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
>>>>> viewed here:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
>>>>>
>>>>> Let me know what you think!
>>>>
>>>> Ah, I didn't realize that the second argument to f_op->flush is a
>>>> pointer to the files_struct. That's neat.
>>>>
>>>>
>>>> Security: There is no guarantee that ->flush() will run after the last
>>>> io_uring_enter() finishes. You can race like this, with threads A and
>>>> B in one process and C in another one:
>>>>
>>>> A: sends uring fd to C via unix domain socket
>>>> A: starts syscall io_uring_enter(fd, ...)
>>>> A: calls fdget(fd), takes reference to file
>>>> B: starts syscall close(fd)
>>>> B: fd table entry is removed
>>>> B: f_op->flush is invoked and finds no pending transactions
>>>> B: syscall close() returns
>>>> A: continues io_uring_enter(), grabbing current->files
>>>> A: io_uring_enter() returns
>>>> A and B: exit
>>>> worker: use-after-free access to files_struct
>>>>
>>>> I think the solution to this would be (unless you're fine with adding
>>>> some broad global read-write mutex) something like this in
>>>> __io_queue_sqe(), where "fd" and "f" are the variables from
>>>> io_uring_enter(), plumbed through the stack somehow:
>>>>
>>>> if (req->flags & REQ_F_NEED_FILES) {
>>>> rcu_read_lock();
>>>> spin_lock_irq(&ctx->inflight_lock);
>>>> if (fcheck(fd) == f) {
>>>> list_add(&req->inflight_list,
>>>> &ctx->inflight_list);
>>>> req->work.files = current->files;
>>>> ret = 0;
>>>> } else {
>>>> ret = -EBADF;
>>>> }
>>>> spin_unlock_irq(&ctx->inflight_lock);
>>>> rcu_read_unlock();
>>>> if (ret)
>>>> goto put_req;
>>>> }
>>>
>>> First of all, thanks for the thorough look at this! We already have f
>>> available here, it's req->file. And we just made a copy of the sqe, so
>>> we have sqe->fd available as well. I fixed this up.
>>
>> sqe->fd is the file descriptor we're doing I/O on, not the file
>> descriptor of the uring file, right? Same thing for req->file. This
>> check only detects whether the fd we're doing I/O on was closed, which
>> is irrelevant.
>
> Duh yes, I'm an idiot. Easily fixable, I'll update this for the ring fd.
Incremental:
diff --git a/fs/io_uring.c b/fs/io_uring.c
index ec9dadfa90d2..4d94886a3d13 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -262,11 +262,13 @@ struct io_ring_ctx {
struct sqe_submit {
const struct io_uring_sqe *sqe;
+ struct file *ring_file;
unsigned short index;
bool has_user : 1;
bool in_async : 1;
bool needs_fixed_file : 1;
u32 sequence;
+ int ring_fd;
};
/*
@@ -2329,14 +2331,13 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s,
return 0;
}
-static int io_grab_files(struct io_ring_ctx *ctx, struct io_kiocb *req,
- struct io_uring_sqe *sqe)
+static int io_grab_files(struct io_ring_ctx *ctx, struct io_kiocb *req)
{
int ret = -EBADF;
rcu_read_lock();
spin_lock_irq(&ctx->inflight_lock);
- if (fcheck(sqe->fd) == req->file) {
+ if (fcheck(req->submit.ring_fd) == req->submit.ring_file) {
list_add(&req->inflight_entry, &ctx->inflight_list);
req->work.files = current->files;
ret = 0;
@@ -2367,7 +2368,7 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
s->sqe = sqe_copy;
memcpy(&req->submit, s, sizeof(*s));
if (req->flags & REQ_F_NEED_FILES) {
- ret = io_grab_files(ctx, req, sqe_copy);
+ ret = io_grab_files(ctx, req);
if (ret) {
kfree(sqe_copy);
goto err;
@@ -2585,6 +2586,7 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
head = READ_ONCE(sq_array[head & ctx->sq_mask]);
if (head < ctx->sq_entries) {
+ s->ring_file = NULL;
s->index = head;
s->sqe = &ctx->sq_sqes[head];
s->sequence = ctx->cached_sq_head;
@@ -2782,7 +2784,8 @@ static int io_sq_thread(void *data)
return 0;
}
-static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
+static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
+ struct file *ring_file, int ring_fd)
{
struct io_submit_state state, *statep = NULL;
struct io_kiocb *link = NULL;
@@ -2824,9 +2827,11 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
}
out:
+ s.ring_file = ring_file;
s.has_user = true;
s.in_async = false;
s.needs_fixed_file = false;
+ s.ring_fd = ring_fd;
submit++;
trace_io_uring_submit_sqe(ctx, true, false);
io_submit_sqe(ctx, &s, statep, &link);
@@ -3828,10 +3833,9 @@ static int io_uring_flush(struct file *file, void *data)
{
struct io_ring_ctx *ctx = file->private_data;
+ io_uring_cancel_files(ctx, data);
if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
io_wq_cancel_all(ctx->io_wq);
- else
- io_uring_cancel_files(ctx, data);
return 0;
}
@@ -3903,7 +3907,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
to_submit = min(to_submit, ctx->sq_entries);
mutex_lock(&ctx->uring_lock);
- submitted = io_ring_submit(ctx, to_submit);
+ submitted = io_ring_submit(ctx, to_submit, f.file, fd);
mutex_unlock(&ctx->uring_lock);
}
if (flags & IORING_ENTER_GETEVENTS) {
--
Jens Axboe
Powered by blists - more mailing lists