[PATCH RFC v2 0/3] io_uring: add restrictions to support untrusted applications and guests

[PATCH RFC v2 0/3] io_uring: add restrictions to support untrusted applications and guests

[Stefano Garzarella]

I fixed some issues that Jens pointed out, and also the TODOs that I left
in the previous version.

I still have any doubts about patch 3, any advice?

RFC v1 -> RFC v2:
    - added 'restricted' flag in the ctx [Jens]
    - added IORING_MAX_RESTRICTIONS define
    - returned EBUSY instead of EINVAL when restrictions are already
      registered
    - reset restrictions if an error happened during the registration
    - removed return value of io_sq_offload_start()

RFC v1: https://lore.kernel.org/io-uring/20200710141945.129329-1-sgarzare@redhat.com

Following the proposal that I send about restrictions [1], I wrote this series
to add restrictions in io_uring.

I also wrote helpers in liburing and a test case (test/register-restrictions.c)
available in this repository:
https://github.com/stefano-garzarella/liburing (branch: io_uring_restrictions)

Just to recap the proposal, the idea is to add some restrictions to the
operations (sqe, register, fixed file) to safely allow untrusted applications
or guests to use io_uring queues.

The first patch changes io_uring_register(2) opcodes into an enumeration to
keep track of the last opcode available.

The second patch adds IOURING_REGISTER_RESTRICTIONS opcode and the code to
handle restrictions.

The third patch adds IORING_SETUP_R_DISABLED flag to start the rings disabled,
allowing the user to register restrictions, buffers, files, before to start
processing SQEs.
I'm not sure if this could help seccomp. An alternative pointed out by Jann
Horn could be to register restrictions during io_uring_setup(2), but this
requires some intrusive changes (there is no space in the struct
io_uring_params to pass a pointer to restriction arrays, maybe we can add a
flag and add the pointer at the end of the struct io_uring_params).

Another limitation now is that I need to enable every time
IORING_REGISTER_ENABLE_RINGS in the restrictions to be able to start the rings,
I'm not sure if we should treat it as an exception.

Maybe registering restrictions during io_uring_setup(2) could solve both issues
(seccomp integration and IORING_REGISTER_ENABLE_RINGS registration), but I need
some suggestions to properly extend the io_uring_setup(2).

Comments and suggestions are very welcome.

Thank you in advance,
Stefano

[1] https://lore.kernel.org/io-uring/20200609142406.upuwpfmgqjeji4lc@steredhat/

Stefano Garzarella (3):
  io_uring: use an enumeration for io_uring_register(2) opcodes
  io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  io_uring: allow disabling rings during the creation

 fs/io_uring.c                 | 152 ++++++++++++++++++++++++++++++++--
 include/uapi/linux/io_uring.h |  56 ++++++++++---
 2 files changed, 188 insertions(+), 20 deletions(-)

-- 
2.26.2

[PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Stefano Garzarella]

The enumeration allows us to keep track of the last
io_uring_register(2) opcode available.

Behaviour and opcodes names don't change.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 7843742b8b74..efc50bd0af34 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -253,17 +253,22 @@ struct io_uring_params {
 /*
  * io_uring_register(2) opcodes and arguments
  */
-#define IORING_REGISTER_BUFFERS		0
-#define IORING_UNREGISTER_BUFFERS	1
-#define IORING_REGISTER_FILES		2
-#define IORING_UNREGISTER_FILES		3
-#define IORING_REGISTER_EVENTFD		4
-#define IORING_UNREGISTER_EVENTFD	5
-#define IORING_REGISTER_FILES_UPDATE	6
-#define IORING_REGISTER_EVENTFD_ASYNC	7
-#define IORING_REGISTER_PROBE		8
-#define IORING_REGISTER_PERSONALITY	9
-#define IORING_UNREGISTER_PERSONALITY	10
+enum {
+	IORING_REGISTER_BUFFERS,
+	IORING_UNREGISTER_BUFFERS,
+	IORING_REGISTER_FILES,
+	IORING_UNREGISTER_FILES,
+	IORING_REGISTER_EVENTFD,
+	IORING_UNREGISTER_EVENTFD,
+	IORING_REGISTER_FILES_UPDATE,
+	IORING_REGISTER_EVENTFD_ASYNC,
+	IORING_REGISTER_PROBE,
+	IORING_REGISTER_PERSONALITY,
+	IORING_UNREGISTER_PERSONALITY,
+
+	/* this goes last */
+	IORING_REGISTER_LAST
+};
 
 struct io_uring_files_update {
 	__u32 offset;
-- 
2.26.2

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Pavel Begunkov]

On 16/07/2020 15:48, Stefano Garzarella wrote:
> The enumeration allows us to keep track of the last
> io_uring_register(2) opcode available.
> 
> Behaviour and opcodes names don't change.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 7843742b8b74..efc50bd0af34 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -253,17 +253,22 @@ struct io_uring_params {
>  /*
>   * io_uring_register(2) opcodes and arguments
>   */
> -#define IORING_REGISTER_BUFFERS		0
> -#define IORING_UNREGISTER_BUFFERS	1
> -#define IORING_REGISTER_FILES		2
> -#define IORING_UNREGISTER_FILES		3
> -#define IORING_REGISTER_EVENTFD		4
> -#define IORING_UNREGISTER_EVENTFD	5
> -#define IORING_REGISTER_FILES_UPDATE	6
> -#define IORING_REGISTER_EVENTFD_ASYNC	7
> -#define IORING_REGISTER_PROBE		8
> -#define IORING_REGISTER_PERSONALITY	9
> -#define IORING_UNREGISTER_PERSONALITY	10
> +enum {
> +	IORING_REGISTER_BUFFERS,
> +	IORING_UNREGISTER_BUFFERS,
> +	IORING_REGISTER_FILES,
> +	IORING_UNREGISTER_FILES,
> +	IORING_REGISTER_EVENTFD,
> +	IORING_UNREGISTER_EVENTFD,
> +	IORING_REGISTER_FILES_UPDATE,
> +	IORING_REGISTER_EVENTFD_ASYNC,
> +	IORING_REGISTER_PROBE,
> +	IORING_REGISTER_PERSONALITY,
> +	IORING_UNREGISTER_PERSONALITY,
> +
> +	/* this goes last */
> +	IORING_REGISTER_LAST
> +};

It breaks userspace API. E.g.

#ifdef IORING_REGISTER_BUFFERS

>  
>  struct io_uring_files_update {
>  	__u32 offset;
> 

-- 
Pavel Begunkov

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Jens Axboe]

On 7/16/20 2:16 PM, Pavel Begunkov wrote:
> On 16/07/2020 15:48, Stefano Garzarella wrote:
>> The enumeration allows us to keep track of the last
>> io_uring_register(2) opcode available.
>>
>> Behaviour and opcodes names don't change.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 7843742b8b74..efc50bd0af34 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>  /*
>>   * io_uring_register(2) opcodes and arguments
>>   */
>> -#define IORING_REGISTER_BUFFERS		0
>> -#define IORING_UNREGISTER_BUFFERS	1
>> -#define IORING_REGISTER_FILES		2
>> -#define IORING_UNREGISTER_FILES		3
>> -#define IORING_REGISTER_EVENTFD		4
>> -#define IORING_UNREGISTER_EVENTFD	5
>> -#define IORING_REGISTER_FILES_UPDATE	6
>> -#define IORING_REGISTER_EVENTFD_ASYNC	7
>> -#define IORING_REGISTER_PROBE		8
>> -#define IORING_REGISTER_PERSONALITY	9
>> -#define IORING_UNREGISTER_PERSONALITY	10
>> +enum {
>> +	IORING_REGISTER_BUFFERS,
>> +	IORING_UNREGISTER_BUFFERS,
>> +	IORING_REGISTER_FILES,
>> +	IORING_UNREGISTER_FILES,
>> +	IORING_REGISTER_EVENTFD,
>> +	IORING_UNREGISTER_EVENTFD,
>> +	IORING_REGISTER_FILES_UPDATE,
>> +	IORING_REGISTER_EVENTFD_ASYNC,
>> +	IORING_REGISTER_PROBE,
>> +	IORING_REGISTER_PERSONALITY,
>> +	IORING_UNREGISTER_PERSONALITY,
>> +
>> +	/* this goes last */
>> +	IORING_REGISTER_LAST
>> +};
> 
> It breaks userspace API. E.g.
> 
> #ifdef IORING_REGISTER_BUFFERS

It can, yes, but we have done that in the past. In this one, for
example:

commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
Author: Jens Axboe <axboe@kernel.dk>
Date:   Wed Dec 11 15:55:43 2019 -0700

    io_uring: ensure we return -EINVAL on unknown opcod

But it would be safer/saner to do this like we have the done the IOSQE_
flags.

-- 
Jens Axboe

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Pavel Begunkov]

On 16/07/2020 23:42, Jens Axboe wrote:
> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>> The enumeration allows us to keep track of the last
>>> io_uring_register(2) opcode available.
>>>
>>> Behaviour and opcodes names don't change.
>>>
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 7843742b8b74..efc50bd0af34 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>>  /*
>>>   * io_uring_register(2) opcodes and arguments
>>>   */
>>> -#define IORING_REGISTER_BUFFERS		0
>>> -#define IORING_UNREGISTER_BUFFERS	1
>>> -#define IORING_REGISTER_FILES		2
>>> -#define IORING_UNREGISTER_FILES		3
>>> -#define IORING_REGISTER_EVENTFD		4
>>> -#define IORING_UNREGISTER_EVENTFD	5
>>> -#define IORING_REGISTER_FILES_UPDATE	6
>>> -#define IORING_REGISTER_EVENTFD_ASYNC	7
>>> -#define IORING_REGISTER_PROBE		8
>>> -#define IORING_REGISTER_PERSONALITY	9
>>> -#define IORING_UNREGISTER_PERSONALITY	10
>>> +enum {
>>> +	IORING_REGISTER_BUFFERS,
>>> +	IORING_UNREGISTER_BUFFERS,
>>> +	IORING_REGISTER_FILES,
>>> +	IORING_UNREGISTER_FILES,
>>> +	IORING_REGISTER_EVENTFD,
>>> +	IORING_UNREGISTER_EVENTFD,
>>> +	IORING_REGISTER_FILES_UPDATE,
>>> +	IORING_REGISTER_EVENTFD_ASYNC,
>>> +	IORING_REGISTER_PROBE,
>>> +	IORING_REGISTER_PERSONALITY,
>>> +	IORING_UNREGISTER_PERSONALITY,
>>> +
>>> +	/* this goes last */
>>> +	IORING_REGISTER_LAST
>>> +};
>>
>> It breaks userspace API. E.g.
>>
>> #ifdef IORING_REGISTER_BUFFERS
> 
> It can, yes, but we have done that in the past. In this one, for

Ok, if nobody on the userspace side cares, then better to do that
sooner than later.


> example:
> 
> commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
> Author: Jens Axboe <axboe@kernel.dk>
> Date:   Wed Dec 11 15:55:43 2019 -0700
> 
>     io_uring: ensure we return -EINVAL on unknown opcod
> 
> But it would be safer/saner to do this like we have the done the IOSQE_
> flags.

IOSQE_ are a bitmask, but this would look peculiar

enum {
	__IORING_REGISTER_BUFFERS,
	...
};
define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS

-- 
Pavel Begunkov

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Jens Axboe]

On 7/16/20 2:47 PM, Pavel Begunkov wrote:
> On 16/07/2020 23:42, Jens Axboe wrote:
>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>>> The enumeration allows us to keep track of the last
>>>> io_uring_register(2) opcode available.
>>>>
>>>> Behaviour and opcodes names don't change.
>>>>
>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> ---
>>>>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>>> index 7843742b8b74..efc50bd0af34 100644
>>>> --- a/include/uapi/linux/io_uring.h
>>>> +++ b/include/uapi/linux/io_uring.h
>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>>>  /*
>>>>   * io_uring_register(2) opcodes and arguments
>>>>   */
>>>> -#define IORING_REGISTER_BUFFERS		0
>>>> -#define IORING_UNREGISTER_BUFFERS	1
>>>> -#define IORING_REGISTER_FILES		2
>>>> -#define IORING_UNREGISTER_FILES		3
>>>> -#define IORING_REGISTER_EVENTFD		4
>>>> -#define IORING_UNREGISTER_EVENTFD	5
>>>> -#define IORING_REGISTER_FILES_UPDATE	6
>>>> -#define IORING_REGISTER_EVENTFD_ASYNC	7
>>>> -#define IORING_REGISTER_PROBE		8
>>>> -#define IORING_REGISTER_PERSONALITY	9
>>>> -#define IORING_UNREGISTER_PERSONALITY	10
>>>> +enum {
>>>> +	IORING_REGISTER_BUFFERS,
>>>> +	IORING_UNREGISTER_BUFFERS,
>>>> +	IORING_REGISTER_FILES,
>>>> +	IORING_UNREGISTER_FILES,
>>>> +	IORING_REGISTER_EVENTFD,
>>>> +	IORING_UNREGISTER_EVENTFD,
>>>> +	IORING_REGISTER_FILES_UPDATE,
>>>> +	IORING_REGISTER_EVENTFD_ASYNC,
>>>> +	IORING_REGISTER_PROBE,
>>>> +	IORING_REGISTER_PERSONALITY,
>>>> +	IORING_UNREGISTER_PERSONALITY,
>>>> +
>>>> +	/* this goes last */
>>>> +	IORING_REGISTER_LAST
>>>> +};
>>>
>>> It breaks userspace API. E.g.
>>>
>>> #ifdef IORING_REGISTER_BUFFERS
>>
>> It can, yes, but we have done that in the past. In this one, for
> 
> Ok, if nobody on the userspace side cares, then better to do that
> sooner than later.
> 
> 
>> example:
>>
>> commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
>> Author: Jens Axboe <axboe@kernel.dk>
>> Date:   Wed Dec 11 15:55:43 2019 -0700
>>
>>     io_uring: ensure we return -EINVAL on unknown opcod
>>
>> But it would be safer/saner to do this like we have the done the IOSQE_
>> flags.
> 
> IOSQE_ are a bitmask, but this would look peculiar
> 
> enum {
> 	__IORING_REGISTER_BUFFERS,
> 	...
> };
> define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS

Yeah true of course, that won't really work for this case at all.

That said, I don't think it's a huge deal to turn it into an enum.


-- 
Jens Axboe

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Jens Axboe]

On 7/16/20 2:51 PM, Jens Axboe wrote:
> On 7/16/20 2:47 PM, Pavel Begunkov wrote:
>> On 16/07/2020 23:42, Jens Axboe wrote:
>>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>>>> The enumeration allows us to keep track of the last
>>>>> io_uring_register(2) opcode available.
>>>>>
>>>>> Behaviour and opcodes names don't change.
>>>>>
>>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>> ---
>>>>>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>>>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>>>> index 7843742b8b74..efc50bd0af34 100644
>>>>> --- a/include/uapi/linux/io_uring.h
>>>>> +++ b/include/uapi/linux/io_uring.h
>>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>>>>  /*
>>>>>   * io_uring_register(2) opcodes and arguments
>>>>>   */
>>>>> -#define IORING_REGISTER_BUFFERS		0
>>>>> -#define IORING_UNREGISTER_BUFFERS	1
>>>>> -#define IORING_REGISTER_FILES		2
>>>>> -#define IORING_UNREGISTER_FILES		3
>>>>> -#define IORING_REGISTER_EVENTFD		4
>>>>> -#define IORING_UNREGISTER_EVENTFD	5
>>>>> -#define IORING_REGISTER_FILES_UPDATE	6
>>>>> -#define IORING_REGISTER_EVENTFD_ASYNC	7
>>>>> -#define IORING_REGISTER_PROBE		8
>>>>> -#define IORING_REGISTER_PERSONALITY	9
>>>>> -#define IORING_UNREGISTER_PERSONALITY	10
>>>>> +enum {
>>>>> +	IORING_REGISTER_BUFFERS,
>>>>> +	IORING_UNREGISTER_BUFFERS,
>>>>> +	IORING_REGISTER_FILES,
>>>>> +	IORING_UNREGISTER_FILES,
>>>>> +	IORING_REGISTER_EVENTFD,
>>>>> +	IORING_UNREGISTER_EVENTFD,
>>>>> +	IORING_REGISTER_FILES_UPDATE,
>>>>> +	IORING_REGISTER_EVENTFD_ASYNC,
>>>>> +	IORING_REGISTER_PROBE,
>>>>> +	IORING_REGISTER_PERSONALITY,
>>>>> +	IORING_UNREGISTER_PERSONALITY,
>>>>> +
>>>>> +	/* this goes last */
>>>>> +	IORING_REGISTER_LAST
>>>>> +};
>>>>
>>>> It breaks userspace API. E.g.
>>>>
>>>> #ifdef IORING_REGISTER_BUFFERS
>>>
>>> It can, yes, but we have done that in the past. In this one, for
>>
>> Ok, if nobody on the userspace side cares, then better to do that
>> sooner than later.

I actually don't think it's a huge issue. Normally if applications
do this, it's because they are using it and need it. Ala:

#ifndef IORING_REGISTER_SOMETHING
#define IORING_REGISTER_SOMETHING	fooval
#endif

and that'll still work just fine, even if an identical enum is there.

-- 
Jens Axboe

Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

[Stefano Garzarella]

On Thu, Jul 16, 2020 at 03:20:53PM -0600, Jens Axboe wrote:
> On 7/16/20 2:51 PM, Jens Axboe wrote:
> > On 7/16/20 2:47 PM, Pavel Begunkov wrote:
> >> On 16/07/2020 23:42, Jens Axboe wrote:
> >>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
> >>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
> >>>>> The enumeration allows us to keep track of the last
> >>>>> io_uring_register(2) opcode available.
> >>>>>
> >>>>> Behaviour and opcodes names don't change.
> >>>>>
> >>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >>>>> ---
> >>>>>  include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
> >>>>>  1 file changed, 16 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> >>>>> index 7843742b8b74..efc50bd0af34 100644
> >>>>> --- a/include/uapi/linux/io_uring.h
> >>>>> +++ b/include/uapi/linux/io_uring.h
> >>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
> >>>>>  /*
> >>>>>   * io_uring_register(2) opcodes and arguments
> >>>>>   */
> >>>>> -#define IORING_REGISTER_BUFFERS		0
> >>>>> -#define IORING_UNREGISTER_BUFFERS	1
> >>>>> -#define IORING_REGISTER_FILES		2
> >>>>> -#define IORING_UNREGISTER_FILES		3
> >>>>> -#define IORING_REGISTER_EVENTFD		4
> >>>>> -#define IORING_UNREGISTER_EVENTFD	5
> >>>>> -#define IORING_REGISTER_FILES_UPDATE	6
> >>>>> -#define IORING_REGISTER_EVENTFD_ASYNC	7
> >>>>> -#define IORING_REGISTER_PROBE		8
> >>>>> -#define IORING_REGISTER_PERSONALITY	9
> >>>>> -#define IORING_UNREGISTER_PERSONALITY	10
> >>>>> +enum {
> >>>>> +	IORING_REGISTER_BUFFERS,
> >>>>> +	IORING_UNREGISTER_BUFFERS,
> >>>>> +	IORING_REGISTER_FILES,
> >>>>> +	IORING_UNREGISTER_FILES,
> >>>>> +	IORING_REGISTER_EVENTFD,
> >>>>> +	IORING_UNREGISTER_EVENTFD,
> >>>>> +	IORING_REGISTER_FILES_UPDATE,
> >>>>> +	IORING_REGISTER_EVENTFD_ASYNC,
> >>>>> +	IORING_REGISTER_PROBE,
> >>>>> +	IORING_REGISTER_PERSONALITY,
> >>>>> +	IORING_UNREGISTER_PERSONALITY,
> >>>>> +
> >>>>> +	/* this goes last */
> >>>>> +	IORING_REGISTER_LAST
> >>>>> +};
> >>>>
> >>>> It breaks userspace API. E.g.
> >>>>
> >>>> #ifdef IORING_REGISTER_BUFFERS
> >>>
> >>> It can, yes, but we have done that in the past. In this one, for
> >>
> >> Ok, if nobody on the userspace side cares, then better to do that
> >> sooner than later.
> 
> I actually don't think it's a huge issue. Normally if applications
> do this, it's because they are using it and need it. Ala:
> 
> #ifndef IORING_REGISTER_SOMETHING
> #define IORING_REGISTER_SOMETHING	fooval
> #endif
> 
> and that'll still work just fine, even if an identical enum is there.
> 

Thank you both for the review!

Then if you agree, I'll leave this patch as it is by introducing the enum.

Stefano

[PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Stefano Garzarella]

The new io_uring_register(2) IOURING_REGISTER_RESTRICTIONS opcode
permanently installs a feature allowlist on an io_ring_ctx.
The io_ring_ctx can then be passed to untrusted code with the
knowledge that only operations present in the allowlist can be
executed.

The allowlist approach ensures that new features added to io_uring
do not accidentally become available when an existing application
is launched on a newer kernel version.

Currently is it possible to restrict sqe opcodes and register
opcodes. It is also possible to allow only fixed files.

IOURING_REGISTER_RESTRICTIONS can only be made once. Afterwards
it is not possible to change restrictions anymore.
This prevents untrusted code from removing restrictions.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
RFC v2:
 - added 'restricted' flag in the ctx [Jens]
 - added IORING_MAX_RESTRICTIONS define
 - returned EBUSY instead of EINVAL when restrictions are already
   registered
 - reset restrictions if an error happened during the registration
---
 fs/io_uring.c                 | 102 +++++++++++++++++++++++++++++++++-
 include/uapi/linux/io_uring.h |  27 +++++++++
 2 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9fd7e69696c3..23a2b03d9528 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -97,6 +97,8 @@
 #define IORING_MAX_FILES_TABLE	(1U << IORING_FILE_TABLE_SHIFT)
 #define IORING_FILE_TABLE_MASK	(IORING_MAX_FILES_TABLE - 1)
 #define IORING_MAX_FIXED_FILES	(64 * IORING_MAX_FILES_TABLE)
+#define IORING_MAX_RESTRICTIONS	(IORING_RESTRICTION_LAST + \
+				 IORING_REGISTER_LAST + IORING_OP_LAST)
 
 struct io_uring {
 	u32 head ____cacheline_aligned_in_smp;
@@ -218,6 +220,12 @@ struct io_buffer {
 	__u16 bid;
 };
 
+struct io_restriction {
+	DECLARE_BITMAP(register_op, IORING_REGISTER_LAST);
+	DECLARE_BITMAP(sqe_op, IORING_OP_LAST);
+	DECLARE_BITMAP(restriction_op, IORING_RESTRICTION_LAST);
+};
+
 struct io_ring_ctx {
 	struct {
 		struct percpu_ref	refs;
@@ -230,6 +238,7 @@ struct io_ring_ctx {
 		unsigned int		cq_overflow_flushed: 1;
 		unsigned int		drain_next: 1;
 		unsigned int		eventfd_async: 1;
+		unsigned int		restricted: 1;
 
 		/*
 		 * Ring buffer of indices into array of io_uring_sqe, which is
@@ -337,6 +346,7 @@ struct io_ring_ctx {
 	struct llist_head		file_put_llist;
 
 	struct work_struct		exit_work;
+	struct io_restriction		restrictions;
 };
 
 /*
@@ -5496,6 +5506,11 @@ static int io_req_set_file(struct io_submit_state *state, struct io_kiocb *req,
 	if (unlikely(!fixed && io_async_submit(req->ctx)))
 		return -EBADF;
 
+	if (unlikely(!fixed && req->ctx->restricted &&
+		     test_bit(IORING_RESTRICTION_FIXED_FILES_ONLY,
+			      req->ctx->restrictions.restriction_op)))
+		return -EACCES;
+
 	return io_file_get(state, req, fd, &req->file, fixed);
 }
 
@@ -5900,6 +5915,10 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	if (unlikely(req->opcode >= IORING_OP_LAST))
 		return -EINVAL;
 
+	if (unlikely(ctx->restricted &&
+		     !test_bit(req->opcode, ctx->restrictions.sqe_op)))
+		return -EACCES;
+
 	if (unlikely(io_sq_thread_acquire_mm(ctx, req)))
 		return -EFAULT;
 
@@ -8099,6 +8118,71 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
 	return -EINVAL;
 }
 
+static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
+				    unsigned int nr_args)
+{
+	struct io_uring_restriction *res;
+	size_t size;
+	int i, ret;
+
+	/* We allow only a single restrictions registration */
+	if (ctx->restricted)
+		return -EBUSY;
+
+	if (!arg || nr_args > IORING_MAX_RESTRICTIONS)
+		return -EINVAL;
+
+	size = array_size(nr_args, sizeof(*res));
+	if (size == SIZE_MAX)
+		return -EOVERFLOW;
+
+	res = kmalloc(size, GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	if (copy_from_user(res, arg, size)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	for (i = 0; i < nr_args; i++) {
+		if (res[i].opcode >= IORING_RESTRICTION_LAST) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		__set_bit(res[i].opcode, ctx->restrictions.restriction_op);
+
+		if (res[i].opcode == IORING_RESTRICTION_REGISTER_OP) {
+			if (res[i].register_op >= IORING_REGISTER_LAST) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			__set_bit(res[i].register_op,
+				  ctx->restrictions.register_op);
+		} else if (res[i].opcode == IORING_RESTRICTION_SQE_OP) {
+			if (res[i].sqe_op >= IORING_OP_LAST) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			__set_bit(res[i].sqe_op, ctx->restrictions.sqe_op);
+		}
+	}
+
+	ctx->restricted = 1;
+
+	ret = 0;
+out:
+	/* Reset all restrictions if an error happened */
+	if (ret != 0)
+		memset(&ctx->restrictions, 0, sizeof(ctx->restrictions));
+
+	kfree(res);
+	return ret;
+}
+
 static bool io_register_op_must_quiesce(int op)
 {
 	switch (op) {
@@ -8145,6 +8229,18 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
 		if (ret) {
 			percpu_ref_resurrect(&ctx->refs);
 			ret = -EINTR;
+			goto out_quiesce;
+		}
+	}
+
+	if (ctx->restricted) {
+		if (opcode >= IORING_REGISTER_LAST) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (!test_bit(opcode, ctx->restrictions.register_op)) {
+			ret = -EACCES;
 			goto out;
 		}
 	}
@@ -8208,15 +8304,19 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
 			break;
 		ret = io_unregister_personality(ctx, nr_args);
 		break;
+	case IORING_REGISTER_RESTRICTIONS:
+		ret = io_register_restrictions(ctx, arg, nr_args);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
+out:
 	if (io_register_op_must_quiesce(opcode)) {
 		/* bring the ctx back to life */
 		percpu_ref_reinit(&ctx->refs);
-out:
+out_quiesce:
 		reinit_completion(&ctx->ref_comp);
 	}
 	return ret;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index efc50bd0af34..0774d5382c65 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -265,6 +265,7 @@ enum {
 	IORING_REGISTER_PROBE,
 	IORING_REGISTER_PERSONALITY,
 	IORING_UNREGISTER_PERSONALITY,
+	IORING_REGISTER_RESTRICTIONS,
 
 	/* this goes last */
 	IORING_REGISTER_LAST
@@ -293,4 +294,30 @@ struct io_uring_probe {
 	struct io_uring_probe_op ops[0];
 };
 
+struct io_uring_restriction {
+	__u16 opcode;
+	union {
+		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
+		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
+	};
+	__u8 resv;
+	__u32 resv2[3];
+};
+
+/*
+ * io_uring_restriction->opcode values
+ */
+enum {
+	/* Allow an io_uring_register(2) opcode */
+	IORING_RESTRICTION_REGISTER_OP,
+
+	/* Allow an sqe opcode */
+	IORING_RESTRICTION_SQE_OP,
+
+	/* Only allow fixed files */
+	IORING_RESTRICTION_FIXED_FILES_ONLY,
+
+	IORING_RESTRICTION_LAST
+};
+
 #endif
-- 
2.26.2

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Jens Axboe]

On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index efc50bd0af34..0774d5382c65 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -265,6 +265,7 @@ enum {
>  	IORING_REGISTER_PROBE,
>  	IORING_REGISTER_PERSONALITY,
>  	IORING_UNREGISTER_PERSONALITY,
> +	IORING_REGISTER_RESTRICTIONS,
>  
>  	/* this goes last */
>  	IORING_REGISTER_LAST
> @@ -293,4 +294,30 @@ struct io_uring_probe {
>  	struct io_uring_probe_op ops[0];
>  };
>  
> +struct io_uring_restriction {
> +	__u16 opcode;
> +	union {
> +		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> +		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> +	};
> +	__u8 resv;
> +	__u32 resv2[3];
> +};
> +
> +/*
> + * io_uring_restriction->opcode values
> + */
> +enum {
> +	/* Allow an io_uring_register(2) opcode */
> +	IORING_RESTRICTION_REGISTER_OP,
> +
> +	/* Allow an sqe opcode */
> +	IORING_RESTRICTION_SQE_OP,
> +
> +	/* Only allow fixed files */
> +	IORING_RESTRICTION_FIXED_FILES_ONLY,
> +
> +	IORING_RESTRICTION_LAST
> +};
> +

Not sure I totally love this API. Maybe it'd be cleaner to have separate
ops for this, instead of muxing it like this. One for registering op
code restrictions, and one for disallowing other parts (like fixed
files, etc).

I think that would look a lot cleaner than the above.

-- 
Jens Axboe

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Stefano Garzarella]

On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > index efc50bd0af34..0774d5382c65 100644
> > --- a/include/uapi/linux/io_uring.h
> > +++ b/include/uapi/linux/io_uring.h
> > @@ -265,6 +265,7 @@ enum {
> >  	IORING_REGISTER_PROBE,
> >  	IORING_REGISTER_PERSONALITY,
> >  	IORING_UNREGISTER_PERSONALITY,
> > +	IORING_REGISTER_RESTRICTIONS,
> >  
> >  	/* this goes last */
> >  	IORING_REGISTER_LAST
> > @@ -293,4 +294,30 @@ struct io_uring_probe {
> >  	struct io_uring_probe_op ops[0];
> >  };
> >  
> > +struct io_uring_restriction {
> > +	__u16 opcode;
> > +	union {
> > +		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> > +		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> > +	};
> > +	__u8 resv;
> > +	__u32 resv2[3];
> > +};
> > +
> > +/*
> > + * io_uring_restriction->opcode values
> > + */
> > +enum {
> > +	/* Allow an io_uring_register(2) opcode */
> > +	IORING_RESTRICTION_REGISTER_OP,
> > +
> > +	/* Allow an sqe opcode */
> > +	IORING_RESTRICTION_SQE_OP,
> > +
> > +	/* Only allow fixed files */
> > +	IORING_RESTRICTION_FIXED_FILES_ONLY,
> > +
> > +	IORING_RESTRICTION_LAST
> > +};
> > +
> 
> Not sure I totally love this API. Maybe it'd be cleaner to have separate
> ops for this, instead of muxing it like this. One for registering op
> code restrictions, and one for disallowing other parts (like fixed
> files, etc).
> 
> I think that would look a lot cleaner than the above.
> 

I'm not sure I got your point.

Do you mean two different register ops?
For example, maybe with better names... ;-):

	IORING_REGISTER_RESTRICTIONS_OPS
	IORING_REGISTER_RESTRICTIONS_OTHERS


Or a single register op like now, and a new restriction opcode adding
also a new field in the struct io_uring_restriction?
For example:

	struct io_uring_restriction {
		__u16 opcode;
		union {
			__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
			__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
			__u8 restriction; /* IORING_RESTRICTION_OP */
		};
		__u8 resv;
		__u32 resv2[3];
	};

	/*
	 * io_uring_restriction->opcode values
	 */
	enum {
		/* Allow an io_uring_register(2) opcode */
		IORING_RESTRICTION_REGISTER_OP,

		/* Allow an sqe opcode */
		IORING_RESTRICTION_SQE_OP,

		IORING_RESTRICTION_OP,

		IORING_RESTRICTION_LAST
	};

	enum {
		/* Only allow fixed files */
		IORING_RESTRICTION_OP_FIXED_FILES_ONLY,
	};


Thanks,
Stefano

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Stefano Garzarella]

On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > index efc50bd0af34..0774d5382c65 100644
> > --- a/include/uapi/linux/io_uring.h
> > +++ b/include/uapi/linux/io_uring.h
> > @@ -265,6 +265,7 @@ enum {
> >  	IORING_REGISTER_PROBE,
> >  	IORING_REGISTER_PERSONALITY,
> >  	IORING_UNREGISTER_PERSONALITY,
> > +	IORING_REGISTER_RESTRICTIONS,
> >  
> >  	/* this goes last */
> >  	IORING_REGISTER_LAST
> > @@ -293,4 +294,30 @@ struct io_uring_probe {
> >  	struct io_uring_probe_op ops[0];
> >  };
> >  
> > +struct io_uring_restriction {
> > +	__u16 opcode;
> > +	union {
> > +		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> > +		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> > +	};
> > +	__u8 resv;
> > +	__u32 resv2[3];
> > +};
> > +
> > +/*
> > + * io_uring_restriction->opcode values
> > + */
> > +enum {
> > +	/* Allow an io_uring_register(2) opcode */
> > +	IORING_RESTRICTION_REGISTER_OP,
> > +
> > +	/* Allow an sqe opcode */
> > +	IORING_RESTRICTION_SQE_OP,
> > +
> > +	/* Only allow fixed files */
> > +	IORING_RESTRICTION_FIXED_FILES_ONLY,
> > +
> > +	IORING_RESTRICTION_LAST
> > +};
> > +
> 
> Not sure I totally love this API. Maybe it'd be cleaner to have separate
> ops for this, instead of muxing it like this. One for registering op
> code restrictions, and one for disallowing other parts (like fixed
> files, etc).
> 
> I think that would look a lot cleaner than the above.
> 

Talking with Stefan, an alternative, maybe more near to your suggestion,
would be to remove the 'struct io_uring_restriction' and add the
following register ops:

    /* Allow an sqe opcode */
    IORING_REGISTER_RESTRICTION_SQE_OP

    /* Allow an io_uring_register(2) opcode */
    IORING_REGISTER_RESTRICTION_REG_OP

    /* Register IORING_RESTRICTION_*  */
    IORING_REGISTER_RESTRICTION_OP


    enum {
        /* Only allow fixed files */
        IORING_RESTRICTION_FIXED_FILES_ONLY,

        IORING_RESTRICTION_LAST
    }


We can also enable restriction only when the rings started, to avoid to
register IORING_REGISTER_ENABLE_RINGS opcode. Once rings are started,
the restrictions cannot be changed or disabled.

If you agree, I'll send a v3 following this.

Thanks,
Stefano

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Jens Axboe]

On 7/21/20 4:40 AM, Stefano Garzarella wrote:
> On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
>> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index efc50bd0af34..0774d5382c65 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -265,6 +265,7 @@ enum {
>>>  	IORING_REGISTER_PROBE,
>>>  	IORING_REGISTER_PERSONALITY,
>>>  	IORING_UNREGISTER_PERSONALITY,
>>> +	IORING_REGISTER_RESTRICTIONS,
>>>  
>>>  	/* this goes last */
>>>  	IORING_REGISTER_LAST
>>> @@ -293,4 +294,30 @@ struct io_uring_probe {
>>>  	struct io_uring_probe_op ops[0];
>>>  };
>>>  
>>> +struct io_uring_restriction {
>>> +	__u16 opcode;
>>> +	union {
>>> +		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
>>> +		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
>>> +	};
>>> +	__u8 resv;
>>> +	__u32 resv2[3];
>>> +};
>>> +
>>> +/*
>>> + * io_uring_restriction->opcode values
>>> + */
>>> +enum {
>>> +	/* Allow an io_uring_register(2) opcode */
>>> +	IORING_RESTRICTION_REGISTER_OP,
>>> +
>>> +	/* Allow an sqe opcode */
>>> +	IORING_RESTRICTION_SQE_OP,
>>> +
>>> +	/* Only allow fixed files */
>>> +	IORING_RESTRICTION_FIXED_FILES_ONLY,
>>> +
>>> +	IORING_RESTRICTION_LAST
>>> +};
>>> +
>>
>> Not sure I totally love this API. Maybe it'd be cleaner to have separate
>> ops for this, instead of muxing it like this. One for registering op
>> code restrictions, and one for disallowing other parts (like fixed
>> files, etc).
>>
>> I think that would look a lot cleaner than the above.
>>
> 
> Talking with Stefan, an alternative, maybe more near to your suggestion,
> would be to remove the 'struct io_uring_restriction' and add the
> following register ops:
> 
>     /* Allow an sqe opcode */
>     IORING_REGISTER_RESTRICTION_SQE_OP
> 
>     /* Allow an io_uring_register(2) opcode */
>     IORING_REGISTER_RESTRICTION_REG_OP
> 
>     /* Register IORING_RESTRICTION_*  */
>     IORING_REGISTER_RESTRICTION_OP
> 
> 
>     enum {
>         /* Only allow fixed files */
>         IORING_RESTRICTION_FIXED_FILES_ONLY,
> 
>         IORING_RESTRICTION_LAST
>     }
> 
> 
> We can also enable restriction only when the rings started, to avoid to
> register IORING_REGISTER_ENABLE_RINGS opcode. Once rings are started,
> the restrictions cannot be changed or disabled.

My concerns are largely:

1) An API that's straight forward to use
2) Something that'll work with future changes

The "allow these opcodes" is straightforward, and ditto for the register
opcodes. The fixed file I guess is the odd one out. So if we need to
disallow things in the future, we'll need to add a new restriction
sub-op. Should this perhaps be "these flags must be set", and that could
easily be augmented with "these flags must not be set"?

-- 
Jens Axboe

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Daurnimator]

On Wed, 22 Jul 2020 at 03:11, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/21/20 4:40 AM, Stefano Garzarella wrote:
> > On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
> >> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> >>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> >>> index efc50bd0af34..0774d5382c65 100644
> >>> --- a/include/uapi/linux/io_uring.h
> >>> +++ b/include/uapi/linux/io_uring.h
> >>> @@ -265,6 +265,7 @@ enum {
> >>>     IORING_REGISTER_PROBE,
> >>>     IORING_REGISTER_PERSONALITY,
> >>>     IORING_UNREGISTER_PERSONALITY,
> >>> +   IORING_REGISTER_RESTRICTIONS,
> >>>
> >>>     /* this goes last */
> >>>     IORING_REGISTER_LAST
> >>> @@ -293,4 +294,30 @@ struct io_uring_probe {
> >>>     struct io_uring_probe_op ops[0];
> >>>  };
> >>>
> >>> +struct io_uring_restriction {
> >>> +   __u16 opcode;
> >>> +   union {
> >>> +           __u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> >>> +           __u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> >>> +   };
> >>> +   __u8 resv;
> >>> +   __u32 resv2[3];
> >>> +};
> >>> +
> >>> +/*
> >>> + * io_uring_restriction->opcode values
> >>> + */
> >>> +enum {
> >>> +   /* Allow an io_uring_register(2) opcode */
> >>> +   IORING_RESTRICTION_REGISTER_OP,
> >>> +
> >>> +   /* Allow an sqe opcode */
> >>> +   IORING_RESTRICTION_SQE_OP,
> >>> +
> >>> +   /* Only allow fixed files */
> >>> +   IORING_RESTRICTION_FIXED_FILES_ONLY,
> >>> +
> >>> +   IORING_RESTRICTION_LAST
> >>> +};
> >>> +
> >>
> >> Not sure I totally love this API. Maybe it'd be cleaner to have separate
> >> ops for this, instead of muxing it like this. One for registering op
> >> code restrictions, and one for disallowing other parts (like fixed
> >> files, etc).
> >>
> >> I think that would look a lot cleaner than the above.
> >>
> >
> > Talking with Stefan, an alternative, maybe more near to your suggestion,
> > would be to remove the 'struct io_uring_restriction' and add the
> > following register ops:
> >
> >     /* Allow an sqe opcode */
> >     IORING_REGISTER_RESTRICTION_SQE_OP
> >
> >     /* Allow an io_uring_register(2) opcode */
> >     IORING_REGISTER_RESTRICTION_REG_OP
> >
> >     /* Register IORING_RESTRICTION_*  */
> >     IORING_REGISTER_RESTRICTION_OP
> >
> >
> >     enum {
> >         /* Only allow fixed files */
> >         IORING_RESTRICTION_FIXED_FILES_ONLY,
> >
> >         IORING_RESTRICTION_LAST
> >     }
> >
> >
> > We can also enable restriction only when the rings started, to avoid to
> > register IORING_REGISTER_ENABLE_RINGS opcode. Once rings are started,
> > the restrictions cannot be changed or disabled.
>
> My concerns are largely:
>
> 1) An API that's straight forward to use
> 2) Something that'll work with future changes
>
> The "allow these opcodes" is straightforward, and ditto for the register
> opcodes. The fixed file I guess is the odd one out. So if we need to
> disallow things in the future, we'll need to add a new restriction
> sub-op. Should this perhaps be "these flags must be set", and that could
> easily be augmented with "these flags must not be set"?
>
> --
> Jens Axboe
>

This is starting to sound a lot like seccomp filtering.
Perhaps we should go straight to adding a BPF hook that fires when
reading off the submission queue?

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Stefano Garzarella]

On Wed, Jul 22, 2020 at 12:35:15PM +1000, Daurnimator wrote:
> On Wed, 22 Jul 2020 at 03:11, Jens Axboe <axboe@kernel.dk> wrote:
> >
> > On 7/21/20 4:40 AM, Stefano Garzarella wrote:
> > > On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
> > >> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> > >>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > >>> index efc50bd0af34..0774d5382c65 100644
> > >>> --- a/include/uapi/linux/io_uring.h
> > >>> +++ b/include/uapi/linux/io_uring.h
> > >>> @@ -265,6 +265,7 @@ enum {
> > >>>     IORING_REGISTER_PROBE,
> > >>>     IORING_REGISTER_PERSONALITY,
> > >>>     IORING_UNREGISTER_PERSONALITY,
> > >>> +   IORING_REGISTER_RESTRICTIONS,
> > >>>
> > >>>     /* this goes last */
> > >>>     IORING_REGISTER_LAST
> > >>> @@ -293,4 +294,30 @@ struct io_uring_probe {
> > >>>     struct io_uring_probe_op ops[0];
> > >>>  };
> > >>>
> > >>> +struct io_uring_restriction {
> > >>> +   __u16 opcode;
> > >>> +   union {
> > >>> +           __u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> > >>> +           __u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> > >>> +   };
> > >>> +   __u8 resv;
> > >>> +   __u32 resv2[3];
> > >>> +};
> > >>> +
> > >>> +/*
> > >>> + * io_uring_restriction->opcode values
> > >>> + */
> > >>> +enum {
> > >>> +   /* Allow an io_uring_register(2) opcode */
> > >>> +   IORING_RESTRICTION_REGISTER_OP,
> > >>> +
> > >>> +   /* Allow an sqe opcode */
> > >>> +   IORING_RESTRICTION_SQE_OP,
> > >>> +
> > >>> +   /* Only allow fixed files */
> > >>> +   IORING_RESTRICTION_FIXED_FILES_ONLY,
> > >>> +
> > >>> +   IORING_RESTRICTION_LAST
> > >>> +};
> > >>> +
> > >>
> > >> Not sure I totally love this API. Maybe it'd be cleaner to have separate
> > >> ops for this, instead of muxing it like this. One for registering op
> > >> code restrictions, and one for disallowing other parts (like fixed
> > >> files, etc).
> > >>
> > >> I think that would look a lot cleaner than the above.
> > >>
> > >
> > > Talking with Stefan, an alternative, maybe more near to your suggestion,
> > > would be to remove the 'struct io_uring_restriction' and add the
> > > following register ops:
> > >
> > >     /* Allow an sqe opcode */
> > >     IORING_REGISTER_RESTRICTION_SQE_OP
> > >
> > >     /* Allow an io_uring_register(2) opcode */
> > >     IORING_REGISTER_RESTRICTION_REG_OP
> > >
> > >     /* Register IORING_RESTRICTION_*  */
> > >     IORING_REGISTER_RESTRICTION_OP
> > >
> > >
> > >     enum {
> > >         /* Only allow fixed files */
> > >         IORING_RESTRICTION_FIXED_FILES_ONLY,
> > >
> > >         IORING_RESTRICTION_LAST
> > >     }
> > >
> > >
> > > We can also enable restriction only when the rings started, to avoid to
> > > register IORING_REGISTER_ENABLE_RINGS opcode. Once rings are started,
> > > the restrictions cannot be changed or disabled.
> >
> > My concerns are largely:
> >
> > 1) An API that's straight forward to use
> > 2) Something that'll work with future changes
> >
> > The "allow these opcodes" is straightforward, and ditto for the register
> > opcodes. The fixed file I guess is the odd one out. So if we need to
> > disallow things in the future, we'll need to add a new restriction
> > sub-op. Should this perhaps be "these flags must be set", and that could
> > easily be augmented with "these flags must not be set"?
> >
> > --
> > Jens Axboe
> >
> 
> This is starting to sound a lot like seccomp filtering.
> Perhaps we should go straight to adding a BPF hook that fires when
> reading off the submission queue?
> 

You're right. I e-mailed about that whit Kees Cook [1] and he agreed that the
restrictions in io_uring should allow us to address some issues that with
seccomp it's a bit difficult. For example:
- different restrictions for different io_uring instances in the same
  process
- limit SQEs to use only registered fds and buffers

Maybe seccomp could take advantage of the restrictions to filter SQEs opcodes.

Thanks,
Stefano

[1] https://lore.kernel.org/io-uring/202007160751.ED56C55@keescook/

Re: [PATCH RFC v2 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode

[Stefano Garzarella]

On Tue, Jul 21, 2020 at 11:11:17AM -0600, Jens Axboe wrote:
> On 7/21/20 4:40 AM, Stefano Garzarella wrote:
> > On Thu, Jul 16, 2020 at 03:26:51PM -0600, Jens Axboe wrote:
> >> On 7/16/20 6:48 AM, Stefano Garzarella wrote:
> >>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> >>> index efc50bd0af34..0774d5382c65 100644
> >>> --- a/include/uapi/linux/io_uring.h
> >>> +++ b/include/uapi/linux/io_uring.h
> >>> @@ -265,6 +265,7 @@ enum {
> >>>  	IORING_REGISTER_PROBE,
> >>>  	IORING_REGISTER_PERSONALITY,
> >>>  	IORING_UNREGISTER_PERSONALITY,
> >>> +	IORING_REGISTER_RESTRICTIONS,
> >>>  
> >>>  	/* this goes last */
> >>>  	IORING_REGISTER_LAST
> >>> @@ -293,4 +294,30 @@ struct io_uring_probe {
> >>>  	struct io_uring_probe_op ops[0];
> >>>  };
> >>>  
> >>> +struct io_uring_restriction {
> >>> +	__u16 opcode;
> >>> +	union {
> >>> +		__u8 register_op; /* IORING_RESTRICTION_REGISTER_OP */
> >>> +		__u8 sqe_op;      /* IORING_RESTRICTION_SQE_OP */
> >>> +	};
> >>> +	__u8 resv;
> >>> +	__u32 resv2[3];
> >>> +};
> >>> +
> >>> +/*
> >>> + * io_uring_restriction->opcode values
> >>> + */
> >>> +enum {
> >>> +	/* Allow an io_uring_register(2) opcode */
> >>> +	IORING_RESTRICTION_REGISTER_OP,
> >>> +
> >>> +	/* Allow an sqe opcode */
> >>> +	IORING_RESTRICTION_SQE_OP,
> >>> +
> >>> +	/* Only allow fixed files */
> >>> +	IORING_RESTRICTION_FIXED_FILES_ONLY,
> >>> +
> >>> +	IORING_RESTRICTION_LAST
> >>> +};
> >>> +
> >>
> >> Not sure I totally love this API. Maybe it'd be cleaner to have separate
> >> ops for this, instead of muxing it like this. One for registering op
> >> code restrictions, and one for disallowing other parts (like fixed
> >> files, etc).
> >>
> >> I think that would look a lot cleaner than the above.
> >>
> > 
> > Talking with Stefan, an alternative, maybe more near to your suggestion,
> > would be to remove the 'struct io_uring_restriction' and add the
> > following register ops:
> > 
> >     /* Allow an sqe opcode */
> >     IORING_REGISTER_RESTRICTION_SQE_OP
> > 
> >     /* Allow an io_uring_register(2) opcode */
> >     IORING_REGISTER_RESTRICTION_REG_OP
> > 
> >     /* Register IORING_RESTRICTION_*  */
> >     IORING_REGISTER_RESTRICTION_OP
> > 
> > 
> >     enum {
> >         /* Only allow fixed files */
> >         IORING_RESTRICTION_FIXED_FILES_ONLY,
> > 
> >         IORING_RESTRICTION_LAST
> >     })
> > 
> > 
> > We can also enable restriction only when the rings started, to avoid to
> > register IORING_REGISTER_ENABLE_RINGS opcode. Once rings are started,
> > the restrictions cannot be changed or disabled.
> 
> My concerns are largely:
> 
> 1) An API that's straight forward to use
> 2) Something that'll work with future changes
> 
> The "allow these opcodes" is straightforward, and ditto for the register
> opcodes. The fixed file I guess is the odd one out. So if we need to
> disallow things in the future, we'll need to add a new restriction
> sub-op. Should this perhaps be "these flags must be set", and that could
> easily be augmented with "these flags must not be set"?

Okay, now I get it, and I think that's a good point. I'm going to change that
to restrict SQE flags.

About the registration of restrictions, what do you think is the best solution
among them?
1. a single register op (e.g. IORING_REGISTER_RESTRICTIONS) which has an array
   of restrictions as a parameter.
2. a register op for each restriction (sqe ops, register ops, sqe flags, etc.),
   that requires multiple io_uring_register() calls to register all the
   restrictions.

I'd go for the first one (basically as it's implemented in this RFC) because
it seems more extensible and manageable to me, but I'd like to have your
opinion.

Thanks for your suggestions,
Stefano

[PATCH RFC v2 3/3] io_uring: allow disabling rings during the creation

[Stefano Garzarella]

This patch adds a new IORING_SETUP_R_DISABLED flag to start the
rings disabled, allowing the user to register restrictions,
buffers, files, before to start processing SQEs.

When IORING_SETUP_R_DISABLED is set, SQE are not processed and
SQPOLL kthread is not started.

The restrictions registration are allowed only when the rings
are disable to prevent concurrency issue while processing SQEs.

The rings can be enabled using IORING_REGISTER_ENABLE_RINGS
opcode with io_uring_register(2).

Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
RFC v2:
 - removed return value of io_sq_offload_start()
---
 fs/io_uring.c                 | 50 +++++++++++++++++++++++++++++------
 include/uapi/linux/io_uring.h |  2 ++
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 23a2b03d9528..2f2ecfa10c94 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6969,8 +6969,8 @@ static int io_init_wq_offload(struct io_ring_ctx *ctx,
 	return ret;
 }
 
-static int io_sq_offload_start(struct io_ring_ctx *ctx,
-			       struct io_uring_params *p)
+static int io_sq_offload_create(struct io_ring_ctx *ctx,
+				struct io_uring_params *p)
 {
 	int ret;
 
@@ -7007,7 +7007,6 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
 			ctx->sqo_thread = NULL;
 			goto err;
 		}
-		wake_up_process(ctx->sqo_thread);
 	} else if (p->flags & IORING_SETUP_SQ_AFF) {
 		/* Can't have SQ_AFF without SQPOLL */
 		ret = -EINVAL;
@@ -7026,6 +7025,12 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
 	return ret;
 }
 
+static void io_sq_offload_start(struct io_ring_ctx *ctx)
+{
+	if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sqo_thread)
+		wake_up_process(ctx->sqo_thread);
+}
+
 static void io_unaccount_mem(struct user_struct *user, unsigned long nr_pages)
 {
 	atomic_long_sub(nr_pages, &user->locked_vm);
@@ -7654,9 +7659,6 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 	int submitted = 0;
 	struct fd f;
 
-	if (current->task_works)
-		task_work_run();
-
 	if (flags & ~(IORING_ENTER_GETEVENTS | IORING_ENTER_SQ_WAKEUP))
 		return -EINVAL;
 
@@ -7673,6 +7675,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 	if (!percpu_ref_tryget(&ctx->refs))
 		goto out_fput;
 
+	if (ctx->flags & IORING_SETUP_R_DISABLED)
+		return -EBADF;
+
+	if (current->task_works)
+		task_work_run();
+
 	/*
 	 * For SQ polling, the thread will do all submissions and completions.
 	 * Just return the requested submit count, and wake the thread if
@@ -7978,10 +7986,13 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p,
 	if (ret)
 		goto err;
 
-	ret = io_sq_offload_start(ctx, p);
+	ret = io_sq_offload_create(ctx, p);
 	if (ret)
 		goto err;
 
+	if (!(p->flags & IORING_SETUP_R_DISABLED))
+		io_sq_offload_start(ctx);
+
 	memset(&p->sq_off, 0, sizeof(p->sq_off));
 	p->sq_off.head = offsetof(struct io_rings, sq.head);
 	p->sq_off.tail = offsetof(struct io_rings, sq.tail);
@@ -8042,7 +8053,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
 
 	if (p.flags & ~(IORING_SETUP_IOPOLL | IORING_SETUP_SQPOLL |
 			IORING_SETUP_SQ_AFF | IORING_SETUP_CQSIZE |
-			IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ))
+			IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ |
+			IORING_SETUP_R_DISABLED))
 		return -EINVAL;
 
 	return  io_uring_create(entries, &p, params);
@@ -8125,6 +8137,10 @@ static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
 	size_t size;
 	int i, ret;
 
+	/* Restrictions allowed only if rings started disabled */
+	if (!(ctx->flags & IORING_SETUP_R_DISABLED))
+		return -EINVAL;
+
 	/* We allow only a single restrictions registration */
 	if (ctx->restricted)
 		return -EBUSY;
@@ -8183,6 +8199,18 @@ static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
 	return ret;
 }
 
+static int io_register_enable_rings(struct io_ring_ctx *ctx)
+{
+	if (!(ctx->flags & IORING_SETUP_R_DISABLED))
+		return -EINVAL;
+
+	ctx->flags &= ~IORING_SETUP_R_DISABLED;
+
+	io_sq_offload_start(ctx);
+
+	return 0;
+}
+
 static bool io_register_op_must_quiesce(int op)
 {
 	switch (op) {
@@ -8304,6 +8332,12 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
 			break;
 		ret = io_unregister_personality(ctx, nr_args);
 		break;
+	case IORING_REGISTER_ENABLE_RINGS:
+		ret = -EINVAL;
+		if (arg || nr_args)
+			break;
+		ret = io_register_enable_rings(ctx);
+		break;
 	case IORING_REGISTER_RESTRICTIONS:
 		ret = io_register_restrictions(ctx, arg, nr_args);
 		break;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 0774d5382c65..ce52333bdc2b 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -94,6 +94,7 @@ enum {
 #define IORING_SETUP_CQSIZE	(1U << 3)	/* app defines CQ size */
 #define IORING_SETUP_CLAMP	(1U << 4)	/* clamp SQ/CQ ring sizes */
 #define IORING_SETUP_ATTACH_WQ	(1U << 5)	/* attach to existing wq */
+#define IORING_SETUP_R_DISABLED	(1U << 6)	/* start with ring disabled */
 
 enum {
 	IORING_OP_NOP,
@@ -266,6 +267,7 @@ enum {
 	IORING_REGISTER_PERSONALITY,
 	IORING_UNREGISTER_PERSONALITY,
 	IORING_REGISTER_RESTRICTIONS,
+	IORING_REGISTER_ENABLE_RINGS,
 
 	/* this goes last */
 	IORING_REGISTER_LAST
-- 
2.26.2