Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests
@ 2020-08-13 15:32 Stefano Garzarella
  2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-13 15:32 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, linux-fsdevel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

v4:
 - rebased on top of io_uring-5.9
 - fixed io_uring_enter() exit path when ring is disabled

v3: https://lore.kernel.org/io-uring/20200728160101.48554-1-sgarzare@redhat.c=
om/
RFC v2: https://lore.kernel.org/io-uring/20200716124833.93667-1-sgarzare@redh=
at.com
RFC v1: https://lore.kernel.org/io-uring/20200710141945.129329-1-sgarzare@red=
hat.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 opcode and flags, register opcode) 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.

Comments and suggestions are very welcome.

Thank you in advance,
Stefano

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

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                 | 160 ++++++++++++++++++++++++++++++++--
 include/uapi/linux/io_uring.h |  60 ++++++++++---
 2 files changed, 203 insertions(+), 17 deletions(-)

--=20
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
  2020-08-13 15:32 [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
@ 2020-08-13 15:32 ` Stefano Garzarella
  2020-08-26 19:40   ` Kees Cook
  2020-08-26 19:43   ` Kees Cook
  2020-08-13 15:32 ` [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode Stefano Garzarella
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-13 15:32 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, linux-fsdevel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

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 d65fde732518..cdc98afbacc3 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -255,17 +255,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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  2020-08-13 15:32 [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
  2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
@ 2020-08-13 15:32 ` Stefano Garzarella
  2020-08-13 17:42   ` kernel test robot
  2020-08-26 19:46   ` Kees Cook
  2020-08-13 15:32 ` [PATCH v4 3/3] io_uring: allow disabling rings during the creation Stefano Garzarella
  2020-08-25 15:20 ` [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
  3 siblings, 2 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-13 15:32 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, linux-fsdevel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

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, sqe flags, and
register opcodes.

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>
---
v3:
 - added IORING_RESTRICTION_SQE_FLAGS_ALLOWED and
   IORING_RESTRICTION_SQE_FLAGS_REQUIRED
 - removed IORING_RESTRICTION_FIXED_FILES_ONLY

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                 | 112 +++++++++++++++++++++++++++++++++-
 include/uapi/linux/io_uring.h |  31 ++++++++++
 2 files changed, 142 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1ec25ee71372..cb365e6e0af7 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -98,6 +98,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;
@@ -219,6 +221,13 @@ struct io_buffer {
 	__u16 bid;
 };
 
+struct io_restriction {
+	DECLARE_BITMAP(register_op, IORING_REGISTER_LAST);
+	DECLARE_BITMAP(sqe_op, IORING_OP_LAST);
+	u8 sqe_flags_allowed;
+	u8 sqe_flags_required;
+};
+
 struct io_ring_ctx {
 	struct {
 		struct percpu_ref	refs;
@@ -231,6 +240,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
@@ -338,6 +348,7 @@ struct io_ring_ctx {
 	struct llist_head		file_put_llist;
 
 	struct work_struct		exit_work;
+	struct io_restriction		restrictions;
 };
 
 /*
@@ -6353,6 +6364,19 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS))
 		return -EINVAL;
 
+	if (unlikely(ctx->restricted)) {
+		if (!test_bit(req->opcode, ctx->restrictions.sqe_op))
+			return -EACCES;
+
+		if ((sqe_flags & ctx->restrictions.sqe_flags_required) !=
+		    ctx->restrictions.sqe_flags_required)
+			return -EACCES;
+
+		if (sqe_flags & ~(ctx->restrictions.sqe_flags_allowed |
+				  ctx->restrictions.sqe_flags_required))
+			return -EACCES;
+	}
+
 	if ((sqe_flags & IOSQE_BUFFER_SELECT) &&
 	    !io_op_defs[req->opcode].buffer_select)
 		return -EOPNOTSUPP;
@@ -8650,6 +8674,76 @@ 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++) {
+		switch (res[i].opcode) {
+		case 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);
+			break;
+		case 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);
+			break;
+		case IORING_RESTRICTION_SQE_FLAGS_ALLOWED:
+			ctx->restrictions.sqe_flags_allowed = res[i].sqe_flags;
+			break;
+		case IORING_RESTRICTION_SQE_FLAGS_REQUIRED:
+			ctx->restrictions.sqe_flags_required = res[i].sqe_flags;
+			break;
+		default:
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	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) {
@@ -8696,6 +8790,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;
 		}
 	}
@@ -8759,15 +8865,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 cdc98afbacc3..be54bc3cf173 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -267,6 +267,7 @@ enum {
 	IORING_REGISTER_PROBE,
 	IORING_REGISTER_PERSONALITY,
 	IORING_UNREGISTER_PERSONALITY,
+	IORING_REGISTER_RESTRICTIONS,
 
 	/* this goes last */
 	IORING_REGISTER_LAST
@@ -295,4 +296,34 @@ 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 sqe_flags;   /* IORING_RESTRICTION_SQE_FLAGS_* */
+	};
+	__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,
+
+	/* Allow sqe flags */
+	IORING_RESTRICTION_SQE_FLAGS_ALLOWED,
+
+	/* Require sqe flags (these flags must be set on each submission) */
+	IORING_RESTRICTION_SQE_FLAGS_REQUIRED,
+
+	IORING_RESTRICTION_LAST
+};
+
 #endif
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 3/3] io_uring: allow disabling rings during the creation
  2020-08-13 15:32 [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
  2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
  2020-08-13 15:32 ` [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode Stefano Garzarella
@ 2020-08-13 15:32 ` Stefano Garzarella
  2020-08-26 19:50   ` Kees Cook
  2020-08-25 15:20 ` [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
  3 siblings, 1 reply; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-13 15:32 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, linux-fsdevel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

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>
---
v4:
 - fixed io_uring_enter() exit path when ring is disabled

v3:
 - enabled restrictions only when the rings start

RFC v2:
 - removed return value of io_sq_offload_start()
---
 fs/io_uring.c                 | 52 ++++++++++++++++++++++++++++++-----
 include/uapi/linux/io_uring.h |  2 ++
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index cb365e6e0af7..09fedc380a41 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -226,6 +226,7 @@ struct io_restriction {
 	DECLARE_BITMAP(sqe_op, IORING_OP_LAST);
 	u8 sqe_flags_allowed;
 	u8 sqe_flags_required;
+	bool registered;
 };
 
 struct io_ring_ctx {
@@ -7420,8 +7421,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;
 
@@ -7458,7 +7459,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;
@@ -7479,6 +7479,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 inline void __io_unaccount_mem(struct user_struct *user,
 				      unsigned long nr_pages)
 {
@@ -8218,6 +8224,9 @@ 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)
+		goto out_fput;
+
 	/*
 	 * For SQ polling, the thread will do all submissions and completions.
 	 * Just return the requested submit count, and wake the thread if
@@ -8532,10 +8541,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);
@@ -8598,7 +8610,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);
@@ -8681,8 +8694,12 @@ 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)
+	if (ctx->restrictions.registered)
 		return -EBUSY;
 
 	if (!arg || nr_args > IORING_MAX_RESTRICTIONS)
@@ -8732,7 +8749,7 @@ static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
 		}
 	}
 
-	ctx->restricted = 1;
+	ctx->restrictions.registered = true;
 
 	ret = 0;
 out:
@@ -8744,6 +8761,21 @@ 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;
+
+	if (ctx->restrictions.registered)
+		ctx->restricted = 1;
+
+	ctx->flags &= ~IORING_SETUP_R_DISABLED;
+
+	io_sq_offload_start(ctx);
+
+	return 0;
+}
+
 static bool io_register_op_must_quiesce(int op)
 {
 	switch (op) {
@@ -8865,6 +8897,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 be54bc3cf173..ddb30513e027 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -95,6 +95,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,
@@ -268,6 +269,7 @@ enum {
 	IORING_REGISTER_PERSONALITY,
 	IORING_UNREGISTER_PERSONALITY,
 	IORING_REGISTER_RESTRICTIONS,
+	IORING_REGISTER_ENABLE_RINGS,
 
 	/* this goes last */
 	IORING_REGISTER_LAST
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  2020-08-13 15:32 ` [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode Stefano Garzarella
@ 2020-08-13 17:42   ` kernel test robot
  2020-08-17 10:41     ` Stefano Garzarella
  2020-08-26 19:46   ` Kees Cook
  1 sibling, 1 reply; 19+ messages in thread
From: kernel test robot @ 2020-08-13 17:42 UTC (permalink / raw)
  To: Stefano Garzarella, Jens Axboe
  Cc: kbuild-all, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Kees Cook, Alexander Viro,
	Kernel Hardening, Stefan Hajnoczi

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Hi Stefano,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.8 next-20200813]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Stefano-Garzarella/io_uring-add-restrictions-to-support-untrusted-applications-and-guests/20200813-233653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git dc06fe51d26efc100ac74121607c01a454867c91
config: s390-randconfig-c003-20200813 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> fs/io_uring.c:8516:7-14: WARNING opportunity for memdup_user

vim +8516 fs/io_uring.c

  8497	
  8498	static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
  8499					    unsigned int nr_args)
  8500	{
  8501		struct io_uring_restriction *res;
  8502		size_t size;
  8503		int i, ret;
  8504	
  8505		/* We allow only a single restrictions registration */
  8506		if (ctx->restricted)
  8507			return -EBUSY;
  8508	
  8509		if (!arg || nr_args > IORING_MAX_RESTRICTIONS)
  8510			return -EINVAL;
  8511	
  8512		size = array_size(nr_args, sizeof(*res));
  8513		if (size == SIZE_MAX)
  8514			return -EOVERFLOW;
  8515	
> 8516		res = kmalloc(size, GFP_KERNEL);
  8517		if (!res)
  8518			return -ENOMEM;
  8519	
  8520		if (copy_from_user(res, arg, size)) {
  8521			ret = -EFAULT;
  8522			goto out;
  8523		}
  8524	
  8525		for (i = 0; i < nr_args; i++) {
  8526			switch (res[i].opcode) {
  8527			case IORING_RESTRICTION_REGISTER_OP:
  8528				if (res[i].register_op >= IORING_REGISTER_LAST) {
  8529					ret = -EINVAL;
  8530					goto out;
  8531				}
  8532	
  8533				__set_bit(res[i].register_op,
  8534					  ctx->restrictions.register_op);
  8535				break;
  8536			case IORING_RESTRICTION_SQE_OP:
  8537				if (res[i].sqe_op >= IORING_OP_LAST) {
  8538					ret = -EINVAL;
  8539					goto out;
  8540				}
  8541	
  8542				__set_bit(res[i].sqe_op, ctx->restrictions.sqe_op);
  8543				break;
  8544			case IORING_RESTRICTION_SQE_FLAGS_ALLOWED:
  8545				ctx->restrictions.sqe_flags_allowed = res[i].sqe_flags;
  8546				break;
  8547			case IORING_RESTRICTION_SQE_FLAGS_REQUIRED:
  8548				ctx->restrictions.sqe_flags_required = res[i].sqe_flags;
  8549				break;
  8550			default:
  8551				ret = -EINVAL;
  8552				goto out;
  8553			}
  8554		}
  8555	
  8556		ctx->restricted = 1;
  8557	
  8558		ret = 0;
  8559	out:
  8560		/* Reset all restrictions if an error happened */
  8561		if (ret != 0)
  8562			memset(&ctx->restrictions, 0, sizeof(ctx->restrictions));
  8563	
  8564		kfree(res);
  8565		return ret;
  8566	}
  8567	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31422 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  2020-08-13 17:42   ` kernel test robot
@ 2020-08-17 10:41     ` Stefano Garzarella
  0 siblings, 0 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-17 10:41 UTC (permalink / raw)
  To: kernel test robot
  Cc: Jens Axboe, kbuild-all, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Kees Cook, Alexander Viro,
	Kernel Hardening, Stefan Hajnoczi

On Fri, Aug 14, 2020 at 01:42:15AM +0800, kernel test robot wrote:
> Hi Stefano,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.8 next-20200813]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Stefano-Garzarella/io_uring-add-restrictions-to-support-untrusted-applications-and-guests/20200813-233653
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git dc06fe51d26efc100ac74121607c01a454867c91
> config: s390-randconfig-c003-20200813 (attached as .config)
> compiler: s390-linux-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> coccinelle warnings: (new ones prefixed by >>)
> 
> >> fs/io_uring.c:8516:7-14: WARNING opportunity for memdup_user

Yeah, I think make sense.

I'll use memdup_user() in the next version.

> 
> vim +8516 fs/io_uring.c
> 
>   8497	
>   8498	static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg,
>   8499					    unsigned int nr_args)
>   8500	{
>   8501		struct io_uring_restriction *res;
>   8502		size_t size;
>   8503		int i, ret;
>   8504	
>   8505		/* We allow only a single restrictions registration */
>   8506		if (ctx->restricted)
>   8507			return -EBUSY;
>   8508	
>   8509		if (!arg || nr_args > IORING_MAX_RESTRICTIONS)
>   8510			return -EINVAL;
>   8511	
>   8512		size = array_size(nr_args, sizeof(*res));
>   8513		if (size == SIZE_MAX)
>   8514			return -EOVERFLOW;
>   8515	
> > 8516		res = kmalloc(size, GFP_KERNEL);
>   8517		if (!res)
>   8518			return -ENOMEM;
>   8519	
>   8520		if (copy_from_user(res, arg, size)) {
>   8521			ret = -EFAULT;
>   8522			goto out;
>   8523		}
>   8524	
>   8525		for (i = 0; i < nr_args; i++) {
>   8526			switch (res[i].opcode) {
>   8527			case IORING_RESTRICTION_REGISTER_OP:
>   8528				if (res[i].register_op >= IORING_REGISTER_LAST) {
>   8529					ret = -EINVAL;
>   8530					goto out;
>   8531				}
>   8532	
>   8533				__set_bit(res[i].register_op,
>   8534					  ctx->restrictions.register_op);
>   8535				break;
>   8536			case IORING_RESTRICTION_SQE_OP:
>   8537				if (res[i].sqe_op >= IORING_OP_LAST) {
>   8538					ret = -EINVAL;
>   8539					goto out;
>   8540				}
>   8541	
>   8542				__set_bit(res[i].sqe_op, ctx->restrictions.sqe_op);
>   8543				break;
>   8544			case IORING_RESTRICTION_SQE_FLAGS_ALLOWED:
>   8545				ctx->restrictions.sqe_flags_allowed = res[i].sqe_flags;
>   8546				break;
>   8547			case IORING_RESTRICTION_SQE_FLAGS_REQUIRED:
>   8548				ctx->restrictions.sqe_flags_required = res[i].sqe_flags;
>   8549				break;
>   8550			default:
>   8551				ret = -EINVAL;
>   8552				goto out;
>   8553			}
>   8554		}
>   8555	
>   8556		ctx->restricted = 1;
>   8557	
>   8558		ret = 0;
>   8559	out:
>   8560		/* Reset all restrictions if an error happened */
>   8561		if (ret != 0)
>   8562			memset(&ctx->restrictions, 0, sizeof(ctx->restrictions));
>   8563	
>   8564		kfree(res);
>   8565		return ret;
>   8566	}
>   8567	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests
  2020-08-13 15:32 [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
                   ` (2 preceding siblings ...)
  2020-08-13 15:32 ` [PATCH v4 3/3] io_uring: allow disabling rings during the creation Stefano Garzarella
@ 2020-08-25 15:20 ` Stefano Garzarella
  2020-08-26 16:47   ` Jens Axboe
  3 siblings, 1 reply; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-25 15:20 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, Linux FS Devel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, kernel list, Aleksa Sarai, io-uring

Hi Jens,
this is a gentle ping.

I'll respin, using memdup_user() for restriction registration.
I'd like to get some feedback to see if I should change anything else.

Do you think it's in good shape?

Thanks,
Stefano

On Thu, Aug 13, 2020 at 5:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> v4:
>  - rebased on top of io_uring-5.9
>  - fixed io_uring_enter() exit path when ring is disabled
>
> v3: https://lore.kernel.org/io-uring/20200728160101.48554-1-sgarzare@redhat.c=
> om/
> RFC v2: https://lore.kernel.org/io-uring/20200716124833.93667-1-sgarzare@redh=
> at.com
> RFC v1: https://lore.kernel.org/io-uring/20200710141945.129329-1-sgarzare@red=
> hat.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 opcode and flags, register opcode) 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.
>
> Comments and suggestions are very welcome.
>
> Thank you in advance,
> Stefano
>
> [1] https://lore.kernel.org/io-uring/20200609142406.upuwpfmgqjeji4lc@steredha=
> t/
>
> 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                 | 160 ++++++++++++++++++++++++++++++++--
>  include/uapi/linux/io_uring.h |  60 ++++++++++---
>  2 files changed, 203 insertions(+), 17 deletions(-)
>
> --=20
> 2.26.2
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests
  2020-08-25 15:20 ` [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
@ 2020-08-26 16:47   ` Jens Axboe
  2020-08-26 19:40     ` Kees Cook
  0 siblings, 1 reply; 19+ messages in thread
From: Jens Axboe @ 2020-08-26 16:47 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, Linux FS Devel,
	Sargun Dhillon, Kees Cook, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, kernel list, Aleksa Sarai, io-uring

On 8/25/20 9:20 AM, Stefano Garzarella wrote:
> Hi Jens,
> this is a gentle ping.
> 
> I'll respin, using memdup_user() for restriction registration.
> I'd like to get some feedback to see if I should change anything else.
> 
> Do you think it's in good shape?

As far as I'm concerned, this is fine. But I want to make sure that Kees
is happy with it, as he's the one that's been making noise on this front.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests
  2020-08-26 16:47   ` Jens Axboe
@ 2020-08-26 19:40     ` Kees Cook
  2020-08-27  7:24       ` Stefano Garzarella
  0 siblings, 1 reply; 19+ messages in thread
From: Kees Cook @ 2020-08-26 19:40 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Stefano Garzarella, Christian Brauner, Jann Horn, Jeff Moyer,
	Linux FS Devel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, kernel list, Aleksa Sarai, io-uring

On Wed, Aug 26, 2020 at 10:47:36AM -0600, Jens Axboe wrote:
> On 8/25/20 9:20 AM, Stefano Garzarella wrote:
> > Hi Jens,
> > this is a gentle ping.
> > 
> > I'll respin, using memdup_user() for restriction registration.
> > I'd like to get some feedback to see if I should change anything else.
> > 
> > Do you think it's in good shape?
> 
> As far as I'm concerned, this is fine. But I want to make sure that Kees
> is happy with it, as he's the one that's been making noise on this front.

Oop! Sorry, I didn't realize this was blocked on me. Once I saw how
orthogonal io_uring was to "regular" process trees, I figured this
series didn't need seccomp input. (I mean, I am still concerned about
attack surface reduction, but that seems like a hard problem given
io_uring's design -- it is, however, totally covered by the LSMs, so I'm
satisfied from that perspective.)

I'll go review... thanks for the poke. :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
  2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
@ 2020-08-26 19:40   ` Kees Cook
  2020-08-26 19:43   ` Kees Cook
  1 sibling, 0 replies; 19+ messages in thread
From: Kees Cook @ 2020-08-26 19:40 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Thu, Aug 13, 2020 at 05:32:52PM +0200, 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>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
  2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
  2020-08-26 19:40   ` Kees Cook
@ 2020-08-26 19:43   ` Kees Cook
  2020-08-26 19:52     ` Andreas Dilger
  1 sibling, 1 reply; 19+ messages in thread
From: Kees Cook @ 2020-08-26 19:43 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Thu, Aug 13, 2020 at 05:32:52PM +0200, 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 d65fde732518..cdc98afbacc3 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -255,17 +255,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,

Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
careful here and explicitly assign values? We can't change the meaning
of a number (UAPI) but we can add new ones, etc? This would help if an
OP were removed (to stop from triggering a cascade of changed values)...

for example:

enum {
	IORING_REGISTER_BUFFERS = 0,
	IORING_UNREGISTER_BUFFERS = 1,
	...


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  2020-08-13 15:32 ` [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode Stefano Garzarella
  2020-08-13 17:42   ` kernel test robot
@ 2020-08-26 19:46   ` Kees Cook
  2020-08-27  7:12     ` Stefano Garzarella
  1 sibling, 1 reply; 19+ messages in thread
From: Kees Cook @ 2020-08-26 19:46 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Thu, Aug 13, 2020 at 05:32:53PM +0200, Stefano Garzarella wrote:
> +/*
> + * 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,
> +
> +	/* Allow sqe flags */
> +	IORING_RESTRICTION_SQE_FLAGS_ALLOWED,
> +
> +	/* Require sqe flags (these flags must be set on each submission) */
> +	IORING_RESTRICTION_SQE_FLAGS_REQUIRED,
> +
> +	IORING_RESTRICTION_LAST
> +};

Same thought on enum literals, but otherwise, looks good:

Reviewed-by: Kees Cook <keescook@chromium.org>


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] io_uring: allow disabling rings during the creation
  2020-08-13 15:32 ` [PATCH v4 3/3] io_uring: allow disabling rings during the creation Stefano Garzarella
@ 2020-08-26 19:50   ` Kees Cook
  2020-08-27  7:18     ` Stefano Garzarella
  0 siblings, 1 reply; 19+ messages in thread
From: Kees Cook @ 2020-08-26 19:50 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Thu, Aug 13, 2020 at 05:32:54PM +0200, Stefano Garzarella wrote:
> 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>

Reviewed-by: Kees Cook <keescook@chromium.org>

Where can I find the io_uring selftests? I'd expect an additional set of
patches to implement the selftests for this new feature.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
  2020-08-26 19:43   ` Kees Cook
@ 2020-08-26 19:52     ` Andreas Dilger
  2020-08-27  7:11       ` Stefano Garzarella
  0 siblings, 1 reply; 19+ messages in thread
From: Andreas Dilger @ 2020-08-26 19:52 UTC (permalink / raw)
  To: Kees Cook
  Cc: Stefano Garzarella, Jens Axboe, Christian Brauner, Jann Horn,
	Jeff Moyer, linux-fsdevel, Sargun Dhillon, Alexander Viro,
	Kernel Hardening, Stefan Hajnoczi, Linux Kernel Mailing List,
	Aleksa Sarai, io-uring

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

On Aug 26, 2020, at 1:43 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Thu, Aug 13, 2020 at 05:32:52PM +0200, 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 d65fde732518..cdc98afbacc3 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -255,17 +255,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,
> 
> Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
> careful here and explicitly assign values? We can't change the meaning
> of a number (UAPI) but we can add new ones, etc? This would help if an
> OP were removed (to stop from triggering a cascade of changed values)...
> 
> for example:
> 
> enum {
> 	IORING_REGISTER_BUFFERS = 0,
> 	IORING_UNREGISTER_BUFFERS = 1,
> 	...

Definitely that is preferred, IMHO, for enums used as part of UAPI,
as it avoids accidental changes to the values, and it also makes it
easier to see what the actual values are.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
  2020-08-26 19:52     ` Andreas Dilger
@ 2020-08-27  7:11       ` Stefano Garzarella
  0 siblings, 0 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-27  7:11 UTC (permalink / raw)
  To: Andreas Dilger, Kees Cook
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, Linux Kernel Mailing List, Aleksa Sarai,
	io-uring

On Wed, Aug 26, 2020 at 01:52:38PM -0600, Andreas Dilger wrote:
> On Aug 26, 2020, at 1:43 PM, Kees Cook <keescook@chromium.org> wrote:
> > 
> > On Thu, Aug 13, 2020 at 05:32:52PM +0200, 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 d65fde732518..cdc98afbacc3 100644
> >> --- a/include/uapi/linux/io_uring.h
> >> +++ b/include/uapi/linux/io_uring.h
> >> @@ -255,17 +255,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,
> > 
> > Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
> > careful here and explicitly assign values? We can't change the meaning
> > of a number (UAPI) but we can add new ones, etc? This would help if an
> > OP were removed (to stop from triggering a cascade of changed values)...
> > 
> > for example:
> > 
> > enum {
> > 	IORING_REGISTER_BUFFERS = 0,
> > 	IORING_UNREGISTER_BUFFERS = 1,
> > 	...
> 
> Definitely that is preferred, IMHO, for enums used as part of UAPI,
> as it avoids accidental changes to the values, and it also makes it
> easier to see what the actual values are.
> 

Sure, I agree.

I'll put the values in the enumerations in the v5.

Thanks,
Stefano


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode
  2020-08-26 19:46   ` Kees Cook
@ 2020-08-27  7:12     ` Stefano Garzarella
  0 siblings, 0 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-27  7:12 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Wed, Aug 26, 2020 at 12:46:24PM -0700, Kees Cook wrote:
> On Thu, Aug 13, 2020 at 05:32:53PM +0200, Stefano Garzarella wrote:
> > +/*
> > + * 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,
> > +
> > +	/* Allow sqe flags */
> > +	IORING_RESTRICTION_SQE_FLAGS_ALLOWED,
> > +
> > +	/* Require sqe flags (these flags must be set on each submission) */
> > +	IORING_RESTRICTION_SQE_FLAGS_REQUIRED,
> > +
> > +	IORING_RESTRICTION_LAST
> > +};
> 
> Same thought on enum literals, but otherwise, looks good:

Sure, I'll fix the enum in the next version.

> 
> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks for the review,
Stefano


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] io_uring: allow disabling rings during the creation
  2020-08-26 19:50   ` Kees Cook
@ 2020-08-27  7:18     ` Stefano Garzarella
  2020-08-27 15:04       ` Kees Cook
  0 siblings, 1 reply; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-27  7:18 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Wed, Aug 26, 2020 at 12:50:31PM -0700, Kees Cook wrote:
> On Thu, Aug 13, 2020 at 05:32:54PM +0200, Stefano Garzarella wrote:
> > 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>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> Where can I find the io_uring selftests? I'd expect an additional set of
> patches to implement the selftests for this new feature.

Since the io_uring selftests are stored in the liburing repository, I created
a new test case (test/register-restrictions.c) in my fork and I'll send it
when this series is accepted. It's available in this repository:

https://github.com/stefano-garzarella/liburing (branch: io_uring_restrictions)

Thanks for the review,
Stefano


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests
  2020-08-26 19:40     ` Kees Cook
@ 2020-08-27  7:24       ` Stefano Garzarella
  0 siblings, 0 replies; 19+ messages in thread
From: Stefano Garzarella @ 2020-08-27  7:24 UTC (permalink / raw)
  To: Kees Cook, Jens Axboe
  Cc: Christian Brauner, Jann Horn, Jeff Moyer, Linux FS Devel,
	Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, kernel list, Aleksa Sarai, io-uring

On Wed, Aug 26, 2020 at 12:40:24PM -0700, Kees Cook wrote:
> On Wed, Aug 26, 2020 at 10:47:36AM -0600, Jens Axboe wrote:
> > On 8/25/20 9:20 AM, Stefano Garzarella wrote:
> > > Hi Jens,
> > > this is a gentle ping.
> > > 
> > > I'll respin, using memdup_user() for restriction registration.
> > > I'd like to get some feedback to see if I should change anything else.
> > > 
> > > Do you think it's in good shape?
> > 
> > As far as I'm concerned, this is fine. But I want to make sure that Kees
> > is happy with it, as he's the one that's been making noise on this front.
> 
> Oop! Sorry, I didn't realize this was blocked on me. Once I saw how
> orthogonal io_uring was to "regular" process trees, I figured this
> series didn't need seccomp input. (I mean, I am still concerned about
> attack surface reduction, but that seems like a hard problem given
> io_uring's design -- it is, however, totally covered by the LSMs, so I'm
> satisfied from that perspective.)
> 
> I'll go review... thanks for the poke. :)
> 

Jens, Kees, thanks for your feedbacks!
I'll send v5 adding the values to the enumerations.

Stefano


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 3/3] io_uring: allow disabling rings during the creation
  2020-08-27  7:18     ` Stefano Garzarella
@ 2020-08-27 15:04       ` Kees Cook
  0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2020-08-27 15:04 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Jens Axboe, Christian Brauner, Jann Horn, Jeff Moyer,
	linux-fsdevel, Sargun Dhillon, Alexander Viro, Kernel Hardening,
	Stefan Hajnoczi, linux-kernel, Aleksa Sarai, io-uring

On Thu, Aug 27, 2020 at 09:18:02AM +0200, Stefano Garzarella wrote:
> On Wed, Aug 26, 2020 at 12:50:31PM -0700, Kees Cook wrote:
> > On Thu, Aug 13, 2020 at 05:32:54PM +0200, Stefano Garzarella wrote:
> > > 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>
> > 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> > Where can I find the io_uring selftests? I'd expect an additional set of
> > patches to implement the selftests for this new feature.
> 
> Since the io_uring selftests are stored in the liburing repository, I created
> a new test case (test/register-restrictions.c) in my fork and I'll send it
> when this series is accepted. It's available in this repository:
> 
> https://github.com/stefano-garzarella/liburing (branch: io_uring_restrictions)

Ah-ha; thank you! Looks good. :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-08-27 15:08 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-13 15:32 [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
2020-08-13 15:32 ` [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes Stefano Garzarella
2020-08-26 19:40   ` Kees Cook
2020-08-26 19:43   ` Kees Cook
2020-08-26 19:52     ` Andreas Dilger
2020-08-27  7:11       ` Stefano Garzarella
2020-08-13 15:32 ` [PATCH v4 2/3] io_uring: add IOURING_REGISTER_RESTRICTIONS opcode Stefano Garzarella
2020-08-13 17:42   ` kernel test robot
2020-08-17 10:41     ` Stefano Garzarella
2020-08-26 19:46   ` Kees Cook
2020-08-27  7:12     ` Stefano Garzarella
2020-08-13 15:32 ` [PATCH v4 3/3] io_uring: allow disabling rings during the creation Stefano Garzarella
2020-08-26 19:50   ` Kees Cook
2020-08-27  7:18     ` Stefano Garzarella
2020-08-27 15:04       ` Kees Cook
2020-08-25 15:20 ` [PATCH v4 0/3] io_uring: add restrictions to support untrusted applications and guests Stefano Garzarella
2020-08-26 16:47   ` Jens Axboe
2020-08-26 19:40     ` Kees Cook
2020-08-27  7:24       ` Stefano Garzarella

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).