| Message ID | 20200716124833.93667-3-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: add restrictions to support untrusted applications and guests | expand |
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.
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
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
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"?
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?
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/
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
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
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(-)