Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <fadbb6b5-a288-40e2-9bb8-7299ea14f0a7@kernel.dk>
Date:   Tue, 28 Nov 2023 08:58:00 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: io_uring: risky use of task work, especially wrt fdget()
Content-Language: en-US
To:     Jann Horn <jannh@google.com>,
        Pavel Begunkov <asml.silence@gmail.com>
Cc:     io-uring <io-uring@vger.kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>
References: <CAG48ez1htVSO3TqmrF8QcX2WFuYTRM-VZ_N10i-VZgbtg=NNqw@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <CAG48ez1htVSO3TqmrF8QcX2WFuYTRM-VZ_N10i-VZgbtg=NNqw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/27/23 2:53 PM, Jann Horn wrote:
> 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?

Let me run some testing on that. It's a mistake to think that it's
usually multithreaded, generally if you end up using io-wq then it's not
a fast path. A fast networked setup, for example, would never touch the
threads and hence no threading would be implied by using io_uring. Ditto
on the storage front, if you're just reading/writing or eg doing polled
IO. That said, those workloads are generally threaded _anyway_ - not
because of io_uring, but because that's how these kinds of workloads are
written to begin with.

So probably won't be much of a concern to do the swap. The only
"interesting" part of the above mix of cancel/register/enter is
obviously the enter part. The rest are not really fast path.

-- 
Jens Axboe