Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <f77ac379ddb6a67c3ac6a9dc54430142ead07c6f.1576336565.git.asml.silence@gmail.com>
In-Reply-To: <f77ac379ddb6a67c3ac6a9dc54430142ead07c6f.1576336565.git.asml.silence@gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Sat, 14 Dec 2019 18:12:03 +0100
Message-ID: <CAG48ez0N_b+kjbddhHe+BUvSnOSvpm1vdfQ9cv+cgTLuCMXqug@mail.gmail.com>
Subject: Re: [RFC PATCH] io_uring: add support for IORING_OP_IOCTL
To:     Pavel Begunkov <asml.silence@gmail.com>
Cc:     Jens Axboe <axboe@kernel.dk>, io-uring <io-uring@vger.kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Dec 14, 2019 at 4:30 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
> This works almost like ioctl(2), except it doesn't support a bunch of
> common opcodes, (e.g. FIOCLEX and FIBMAP, see ioctl.c), and goes
> straight to a device specific implementation.
>
> The case in mind is dma-buf, drm and other ioctl-centric interfaces.
>
> Not-yet Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>
> It clearly needs some testing first, though works fine with dma-buf,
> but I'd like to discuss whether the use cases are convincing enough,
> and is it ok to desert some ioctl opcodes. For the last point it's
> fairly easy to add, maybe except three requiring fd (e.g. FIOCLEX)
>
> P.S. Probably, it won't benefit enough to consider using io_uring
> in drm/mesa, but anyway.
[...]
> +static int io_ioctl(struct io_kiocb *req,
> +                   struct io_kiocb **nxt, bool force_nonblock)
> +{
> +       const struct io_uring_sqe *sqe = req->sqe;
> +       unsigned int cmd = READ_ONCE(sqe->ioctl_cmd);
> +       unsigned long arg = READ_ONCE(sqe->ioctl_arg);
> +       int ret;
> +
> +       if (!req->file)
> +               return -EBADF;
> +       if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
> +               return -EINVAL;
> +       if (unlikely(sqe->ioprio || sqe->addr || sqe->buf_index
> +               || sqe->rw_flags))
> +               return -EINVAL;
> +       if (force_nonblock)
> +               return -EAGAIN;
> +
> +       ret = security_file_ioctl(req->file, cmd, arg);
> +       if (!ret)
> +               ret = (int)vfs_ioctl(req->file, cmd, arg);

This isn't going to work. For several of the syscalls that were added,
special care had to be taken to avoid bugs - like for RECVMSG, for the
upcoming OPEN/CLOSE stuff, and so on.

And in principle, ioctls handlers can do pretty much all of the things
syscalls can do, and more. They can look at the caller's PID, they can
open and close (well, technically that's slightly unsafe, but IIRC
autofs does it anyway) things in the file descriptor table, they can
give another process access to the calling process in some way, and so
on. If you just allow calling arbitrary ioctls through io_uring, you
will certainly get bugs, and probably security bugs, too.

Therefore, I would prefer to see this not happen at all; and if you do
have a usecase where you think the complexity is worth it, then I
think you'll have to add new infrastructure that allows each
file_operations instance to opt in to having specific ioctls called
via this mechanism, or something like that, and ensure that each of
the exposed ioctls only performs operations that are safe from uring
worker context.

Also, I'm not sure, but it might be a good idea to CC linux-api if you
continue working on this.