IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
From: Jann Horn <jannh@google.com>
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>
Subject: Re: [RFC PATCH] io_uring: add support for IORING_OP_IOCTL
Date: Sat, 14 Dec 2019 18:12:03 +0100
Message-ID: <CAG48ez0N_b+kjbddhHe+BUvSnOSvpm1vdfQ9cv+cgTLuCMXqug@mail.gmail.com> (raw)
In-Reply-To: <f77ac379ddb6a67c3ac6a9dc54430142ead07c6f.1576336565.git.asml.silence@gmail.com>

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.

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-14 15:29 Pavel Begunkov
2019-12-14 17:12 ` Jann Horn [this message]
2019-12-14 17:56   ` Pavel Begunkov
2019-12-14 18:52     ` Jens Axboe
2019-12-15 15:40       ` Pavel Begunkov
2020-01-08 13:26       ` Stefan Metzmacher

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAG48ez0N_b+kjbddhHe+BUvSnOSvpm1vdfQ9cv+cgTLuCMXqug@mail.gmail.com \
    --to=jannh@google.com \
    --cc=asml.silence@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git