io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Pavel Begunkov <asml.silence@gmail.com>
To: Jann Horn <jannh@google.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 20:56:12 +0300	[thread overview]
Message-ID: <9b4f56c1-dce9-1acd-2775-e64a3955d8ee@gmail.com> (raw)
In-Reply-To: <CAG48ez0N_b+kjbddhHe+BUvSnOSvpm1vdfQ9cv+cgTLuCMXqug@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 3062 bytes --]


On 14/12/2019 20:12, Jann Horn wrote:
> 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.

Sounds like hell of a problem. Thanks for sorting this out!

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

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2019-12-14 17:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-14 15:29 [RFC PATCH] io_uring: add support for IORING_OP_IOCTL Pavel Begunkov
2019-12-14 17:12 ` Jann Horn
2019-12-14 17:56   ` Pavel Begunkov [this message]
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 publicly 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=9b4f56c1-dce9-1acd-2775-e64a3955d8ee@gmail.com \
    --to=asml.silence@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=jannh@google.com \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).