Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
From:   Jann Horn <jannh@google.com>
Date:   Mon, 27 Nov 2023 22:53:48 +0100
Message-ID: <CAG48ez1htVSO3TqmrF8QcX2WFuYTRM-VZ_N10i-VZgbtg=NNqw@mail.gmail.com>
Subject: io_uring: risky use of task work, especially wrt fdget()
To:     Jens Axboe <axboe@kernel.dk>,
        Pavel Begunkov <asml.silence@gmail.com>
Cc:     io-uring <io-uring@vger.kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi!

I noticed something that I think does not currently cause any
significant security issues, but could be problematic in the future:

io_uring sometimes processes task work in the middle of syscalls,
including between fdget() and fdput(). My understanding of task work
is that it is expected to run in a context similar to directly at
syscall entry/exit: task context, no locks held, sleeping is okay, and
it doesn't execute in the middle of some syscall that expects private
state of the task_struct to stay the same.

An example of another user of task work is the keyring subsystem,
which does task_work_add() in keyctl_session_to_parent() to change the
cred pointers of another task.

Several places in io_uring process task work while holding an fdget()
reference to some file descriptor. For example, the io_uring_enter
syscall handler calls io_iopoll_check() while the io_ring_ctx is only
referenced via fdget(). This means that if there were another kernel
subsystem that uses task work to close file descriptors, io_uring
would become unsafe. And io_uring does _almost_ that itself, I think:
io_queue_worker_create() can be run on a workqueue, and uses task work
to launch a worker thread from the context of a userspace thread; and
this worker thread can then accept commands to close file descriptors.
Except it doesn't accept commands to close io_uring file descriptors.

A closer miss might be io_sync_cancel(), which holds a reference to
some normal file with fdget()/fdput() while calling into
io_run_task_work_sig(). However, from what I can tell, the only things
that are actually done with this file pointer are pointer comparisons,
so this also shouldn't have significant security impact.

Would it make sense to use fget()/fput() instead of fdget()/fdput() in
io_sync_cancel(), io_uring_enter and io_uring_register? These
functions probably usually run in multithreaded environments anyway
(thanks to the io_uring worker threads), so I would think fdget()
shouldn't bring significant performance savings here?