[PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Ankit Kumar]

This patchset adds test/io_uring_passthrough.c to submit uring passthrough
commands to nvme-ns character device. The uring passthrough was introduced
with 5.19 io_uring.

To send nvme uring passthrough commands we require helpers to fetch NVMe
char device (/dev/ngXnY) specific fields such as namespace id, lba size.

How to run:
./test/io_uring_passthrough.t /dev/ng0n1

This covers basic write/read with verify for sqthread poll,
vectored / nonvectored and fixed IO buffers, which can be easily extended
in future.

Ankit Kumar (5):
  configure: check for nvme uring command support
  io_uring.h: sync sqe entry with 5.20 io_uring
  nvme: add nvme opcodes, structures and helper functions
  test: add io_uring passthrough test
  test/io_uring_passthrough: add test case for poll IO

 configure                       |  20 ++
 src/include/liburing/io_uring.h |  17 +-
 test/Makefile                   |   1 +
 test/io_uring_passthrough.c     | 395 ++++++++++++++++++++++++++++++++
 test/nvme.h                     | 168 ++++++++++++++
 5 files changed, 599 insertions(+), 2 deletions(-)
 create mode 100644 test/io_uring_passthrough.c
 create mode 100644 test/nvme.h

-- 
2.17.1

[PATCH liburing 1/5] configure: check for nvme uring command support

[Ankit Kumar]

Modify configure to check availability of nvme_uring_cmd.
The follow up patch will have uring passthrough tests.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 configure | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/configure b/configure
index 43071dd..1b0cc50 100755
--- a/configure
+++ b/configure
@@ -367,6 +367,23 @@ if compile_prog "" "" "has_ucontext"; then
 fi
 print_config "has_ucontext" "$has_ucontext"
 
+##########################################
+# Check NVME_URING_CMD support
+nvme_uring_cmd="no"
+cat > $TMPC << EOF
+#include <linux/nvme_ioctl.h>
+int main(void)
+{
+  struct nvme_uring_cmd *cmd;
+
+  return sizeof(struct nvme_uring_cmd);
+}
+EOF
+if compile_prog "" "" "nvme uring cmd"; then
+  nvme_uring_cmd="yes"
+fi
+print_config "NVMe uring command support" "$nvme_uring_cmd"
+
 #############################################################################
 if test "$liburing_nolibc" = "yes"; then
   output_sym "CONFIG_NOLIBC"
@@ -402,6 +419,9 @@ fi
 if test "$array_bounds" = "yes"; then
   output_sym "CONFIG_HAVE_ARRAY_BOUNDS"
 fi
+if test "$nvme_uring_cmd" = "yes"; then
+  output_sym "CONFIG_HAVE_NVME_URING"
+fi
 
 echo "CC=$cc" >> $config_host_mak
 print_config "CC" "$cc"
-- 
2.17.1

[PATCH liburing 2/5] io_uring.h: sync sqe entry with 5.20 io_uring

[Ankit Kumar]

Add a few missing fields which was added for uring command

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 src/include/liburing/io_uring.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h
index 51126c4..581381d 100644
--- a/src/include/liburing/io_uring.h
+++ b/src/include/liburing/io_uring.h
@@ -26,6 +26,10 @@ struct io_uring_sqe {
 	union {
 		__u64	off;	/* offset into file */
 		__u64	addr2;
+		struct {
+			__u32	cmd_op;
+			__u32	__pad1;
+		};
 	};
 	union {
 		__u64	addr;	/* pointer to buffer or iovecs */
@@ -65,8 +69,17 @@ struct io_uring_sqe {
 		__s32	splice_fd_in;
 		__u32	file_index;
 	};
-	__u64	addr3;
-	__u64	__pad2[1];
+	union {
+		struct {
+			__u64	addr3;
+			__u64	__pad2[1];
+		};
+		/*
+		 * If the ring is initialized with IORING_SETUP_SQE128, then
+		 * this field is used for 80 bytes of arbitrary command data
+		 */
+		__u8	cmd[0];
+	};
 };
 
 /*
-- 
2.17.1

[PATCH liburing 3/5] nvme: add nvme opcodes, structures and helper functions

[Ankit Kumar]

Add bare minimum structures and helper functions required for
io_uring passthrough commands. This will enable the follow up
patch to add tests for nvme-ns generic character device.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 test/nvme.h | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)
 create mode 100644 test/nvme.h

diff --git a/test/nvme.h b/test/nvme.h
new file mode 100644
index 0000000..866a7e6
--- /dev/null
+++ b/test/nvme.h
@@ -0,0 +1,168 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Description: Helpers for NVMe uring passthrough commands
+ */
+#ifndef LIBURING_NVME_H
+#define LIBURING_NVME_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <sys/ioctl.h>
+#include <linux/nvme_ioctl.h>
+
+/*
+ * If the uapi headers installed on the system lacks nvme uring command
+ * support, use the local version to prevent compilation issues.
+ */
+#ifndef CONFIG_HAVE_NVME_URING
+struct nvme_uring_cmd {
+	__u8	opcode;
+	__u8	flags;
+	__u16	rsvd1;
+	__u32	nsid;
+	__u32	cdw2;
+	__u32	cdw3;
+	__u64	metadata;
+	__u64	addr;
+	__u32	metadata_len;
+	__u32	data_len;
+	__u32	cdw10;
+	__u32	cdw11;
+	__u32	cdw12;
+	__u32	cdw13;
+	__u32	cdw14;
+	__u32	cdw15;
+	__u32	timeout_ms;
+	__u32   rsvd2;
+};
+
+#define NVME_URING_CMD_IO	_IOWR('N', 0x80, struct nvme_uring_cmd)
+#define NVME_URING_CMD_IO_VEC	_IOWR('N', 0x81, struct nvme_uring_cmd)
+#endif /* CONFIG_HAVE_NVME_URING */
+
+#define NVME_DEFAULT_IOCTL_TIMEOUT 0
+#define NVME_IDENTIFY_DATA_SIZE 4096
+#define NVME_IDENTIFY_CSI_SHIFT 24
+#define NVME_IDENTIFY_CNS_NS 0
+#define NVME_CSI_NVM 0
+
+enum nvme_admin_opcode {
+	nvme_admin_identify		= 0x06,
+};
+
+enum nvme_io_opcode {
+	nvme_cmd_write			= 0x01,
+	nvme_cmd_read			= 0x02,
+};
+
+int nsid;
+__u32 lba_shift;
+
+struct nvme_lbaf {
+	__le16			ms;
+	__u8			ds;
+	__u8			rp;
+};
+
+struct nvme_id_ns {
+	__le64			nsze;
+	__le64			ncap;
+	__le64			nuse;
+	__u8			nsfeat;
+	__u8			nlbaf;
+	__u8			flbas;
+	__u8			mc;
+	__u8			dpc;
+	__u8			dps;
+	__u8			nmic;
+	__u8			rescap;
+	__u8			fpi;
+	__u8			dlfeat;
+	__le16			nawun;
+	__le16			nawupf;
+	__le16			nacwu;
+	__le16			nabsn;
+	__le16			nabo;
+	__le16			nabspf;
+	__le16			noiob;
+	__u8			nvmcap[16];
+	__le16			npwg;
+	__le16			npwa;
+	__le16			npdg;
+	__le16			npda;
+	__le16			nows;
+	__le16			mssrl;
+	__le32			mcl;
+	__u8			msrc;
+	__u8			rsvd81[11];
+	__le32			anagrpid;
+	__u8			rsvd96[3];
+	__u8			nsattr;
+	__le16			nvmsetid;
+	__le16			endgid;
+	__u8			nguid[16];
+	__u8			eui64[8];
+	struct nvme_lbaf	lbaf[16];
+	__u8			rsvd192[192];
+	__u8			vs[3712];
+};
+
+static inline int ilog2(uint32_t i)
+{
+	int log = -1;
+
+	while (i) {
+		i >>= 1;
+		log++;
+	}
+	return log;
+}
+
+int fio_nvme_get_info(const char *file)
+{
+	struct nvme_id_ns ns;
+	int fd, err;
+	__u32 lba_size;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return -errno;
+
+	nsid = ioctl(fd, NVME_IOCTL_ID);
+	if (nsid < 0) {
+		fprintf(stderr, "failed to fetch namespace-id\n");
+		close(fd);
+		return -errno;
+	}
+
+	struct nvme_passthru_cmd cmd = {
+		.opcode         = nvme_admin_identify,
+		.nsid           = nsid,
+		.addr           = (__u64)(uintptr_t)&ns,
+		.data_len       = NVME_IDENTIFY_DATA_SIZE,
+		.cdw10          = NVME_IDENTIFY_CNS_NS,
+		.cdw11          = NVME_CSI_NVM << NVME_IDENTIFY_CSI_SHIFT,
+		.timeout_ms     = NVME_DEFAULT_IOCTL_TIMEOUT,
+	};
+
+	err = ioctl(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
+	if (err) {
+		fprintf(stderr, "failed to fetch identify namespace\n");
+		close(fd);
+		return err;
+	}
+
+	lba_size = 1 << ns.lbaf[(ns.flbas & 0x0f)].ds;
+	lba_shift = ilog2(lba_size);
+
+	close(fd);
+	return 0;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
-- 
2.17.1

[PATCH liburing 4/5] test: add io_uring passthrough test

[Ankit Kumar]

Add a way to test uring passthrough commands, which was added
with 5.19 kernel. This requires nvme-ns character device (/dev/ngXnY)
as filename argument. It runs a combination of read/write tests with
sqthread poll, vectored and non-vectored commands, fixed I/O buffers.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 test/Makefile               |   1 +
 test/io_uring_passthrough.c | 319 ++++++++++++++++++++++++++++++++++++
 2 files changed, 320 insertions(+)
 create mode 100644 test/io_uring_passthrough.c

diff --git a/test/Makefile b/test/Makefile
index 45674c3..8cafcfd 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -90,6 +90,7 @@ test_srcs := \
 	io-cancel.c \
 	iopoll.c \
 	io_uring_enter.c \
+	io_uring_passthrough.c \
 	io_uring_register.c \
 	io_uring_setup.c \
 	lfs-openat.c \
diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c
new file mode 100644
index 0000000..2e2b806
--- /dev/null
+++ b/test/io_uring_passthrough.c
@@ -0,0 +1,319 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Description: basic read/write tests for io_uring passthrough commands
+ */
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "helpers.h"
+#include "liburing.h"
+#include "nvme.h"
+
+#define FILE_SIZE	(256 * 1024)
+#define BS		8192
+#define BUFFERS		(FILE_SIZE / BS)
+
+static struct iovec *vecs;
+
+/*
+ * Each offset in the file has the ((test_case / 2) * FILE_SIZE)
+ * + (offset / sizeof(int)) stored for every
+ * sizeof(int) address.
+ */
+static int verify_buf(int tc, void *buf, off_t off)
+{
+	int i, u_in_buf = BS / sizeof(unsigned int);
+	unsigned int *ptr;
+
+	off /= sizeof(unsigned int);
+	off += (tc / 2) * FILE_SIZE;
+	ptr = buf;
+	for (i = 0; i < u_in_buf; i++) {
+		if (off != *ptr) {
+			fprintf(stderr, "Found %u, wanted %lu\n", *ptr, off);
+			return 1;
+		}
+		ptr++;
+		off++;
+	}
+
+	return 0;
+}
+
+static int fill_pattern(int tc)
+{
+	unsigned int val, *ptr;
+	int i, j;
+	int u_in_buf = BS / sizeof(val);
+
+	val = (tc / 2) * FILE_SIZE;
+	for (i = 0; i < BUFFERS; i++) {
+		ptr = vecs[i].iov_base;
+		for (j = 0; j < u_in_buf; j++) {
+			*ptr = val;
+			val++;
+			ptr++;
+		}
+	}
+
+	return 0;
+}
+
+static int __test_io(const char *file, struct io_uring *ring, int tc, int read,
+		     int sqthread, int fixed, int nonvec)
+{
+	struct io_uring_sqe *sqe;
+	struct io_uring_cqe *cqe;
+	struct nvme_uring_cmd *cmd;
+	int open_flags;
+	int do_fixed;
+	int i, ret, fd = -1;
+	off_t offset;
+	__u64 slba;
+	__u32 nlb;
+
+#ifdef VERBOSE
+	fprintf(stdout, "%s: start %d/%d/%d/%d: ", __FUNCTION__, read,
+							sqthread, fixed,
+							nonvec);
+#endif
+	if (read)
+		open_flags = O_RDONLY;
+	else
+		open_flags = O_WRONLY;
+
+	if (fixed) {
+		ret = t_register_buffers(ring, vecs, BUFFERS);
+		if (ret == T_SETUP_SKIP)
+			return 0;
+		if (ret != T_SETUP_OK) {
+			fprintf(stderr, "buffer reg failed: %d\n", ret);
+			goto err;
+		}
+	}
+
+	fd = open(file, open_flags);
+	if (fd < 0) {
+		perror("file open");
+		goto err;
+	}
+
+	if (sqthread) {
+		ret = io_uring_register_files(ring, &fd, 1);
+		if (ret) {
+			fprintf(stderr, "file reg failed: %d\n", ret);
+			goto err;
+		}
+	}
+
+	if (!read)
+		fill_pattern(tc);
+
+	offset = 0;
+	for (i = 0; i < BUFFERS; i++) {
+		sqe = io_uring_get_sqe(ring);
+		if (!sqe) {
+			fprintf(stderr, "sqe get failed\n");
+			goto err;
+		}
+		if (read) {
+			int use_fd = fd;
+
+			do_fixed = fixed;
+
+			if (sqthread)
+				use_fd = 0;
+			if (fixed && (i & 1))
+				do_fixed = 0;
+			if (do_fixed) {
+				io_uring_prep_read_fixed(sqe, use_fd, vecs[i].iov_base,
+								vecs[i].iov_len,
+								offset, i);
+				sqe->cmd_op = NVME_URING_CMD_IO;
+			} else if (nonvec) {
+				io_uring_prep_read(sqe, use_fd, vecs[i].iov_base,
+							vecs[i].iov_len, offset);
+				sqe->cmd_op = NVME_URING_CMD_IO;
+			} else {
+				io_uring_prep_readv(sqe, use_fd, &vecs[i], 1,
+								offset);
+				sqe->cmd_op = NVME_URING_CMD_IO_VEC;
+			}
+		} else {
+			int use_fd = fd;
+
+			do_fixed = fixed;
+
+			if (sqthread)
+				use_fd = 0;
+			if (fixed && (i & 1))
+				do_fixed = 0;
+			if (do_fixed) {
+				io_uring_prep_write_fixed(sqe, use_fd, vecs[i].iov_base,
+								vecs[i].iov_len,
+								offset, i);
+				sqe->cmd_op = NVME_URING_CMD_IO;
+			} else if (nonvec) {
+				io_uring_prep_write(sqe, use_fd, vecs[i].iov_base,
+							vecs[i].iov_len, offset);
+				sqe->cmd_op = NVME_URING_CMD_IO;
+			} else {
+				io_uring_prep_writev(sqe, use_fd, &vecs[i], 1,
+								offset);
+				sqe->cmd_op = NVME_URING_CMD_IO_VEC;
+			}
+		}
+		sqe->opcode = IORING_OP_URING_CMD;
+		sqe->user_data = ((uint64_t)offset << 32) | i;
+		if (sqthread)
+			sqe->flags |= IOSQE_FIXED_FILE;
+
+		/* 80 bytes for NVMe uring passthrough command */
+		cmd = (struct nvme_uring_cmd *)sqe->cmd;
+		memset(cmd, 0, sizeof(struct nvme_uring_cmd));
+
+		cmd->opcode = read ? nvme_cmd_read : nvme_cmd_write;
+
+		slba = offset >> lba_shift;
+		nlb = (BS >> lba_shift) - 1;
+
+		/* cdw10 and cdw11 represent starting lba */
+		cmd->cdw10 = slba & 0xffffffff;
+		cmd->cdw11 = slba >> 32;
+		/* cdw12 represent number of lba's for read/write */
+		cmd->cdw12 = nlb;
+		if (do_fixed || nonvec) {
+			cmd->addr = (__u64)(uintptr_t)vecs[i].iov_base;
+			cmd->data_len = vecs[i].iov_len;
+		} else {
+			cmd->addr = (__u64)(uintptr_t)&vecs[i];
+			cmd->data_len = 1;
+		}
+		cmd->nsid = nsid;
+
+		offset += BS;
+	}
+
+	ret = io_uring_submit(ring);
+	if (ret != BUFFERS) {
+		fprintf(stderr, "submit got %d, wanted %d\n", ret, BUFFERS);
+		goto err;
+	}
+
+	for (i = 0; i < BUFFERS; i++) {
+		ret = io_uring_wait_cqe(ring, &cqe);
+		if (ret) {
+			fprintf(stderr, "wait_cqe=%d\n", ret);
+			goto err;
+		}
+		if (cqe->res != 0) {
+			fprintf(stderr, "cqe res %d, wanted 0\n", cqe->res);
+			goto err;
+		}
+		io_uring_cqe_seen(ring, cqe);
+		if (read) {
+			int index = cqe->user_data & 0xffffffff;
+			void *buf = vecs[index].iov_base;
+			off_t voff = cqe->user_data >> 32;
+
+			if (verify_buf(tc, buf, voff))
+				goto err;
+		}
+	}
+
+	if (fixed) {
+		ret = io_uring_unregister_buffers(ring);
+		if (ret) {
+			fprintf(stderr, "buffer unreg failed: %d\n", ret);
+			goto err;
+		}
+	}
+	if (sqthread) {
+		ret = io_uring_unregister_files(ring);
+		if (ret) {
+			fprintf(stderr, "file unreg failed: %d\n", ret);
+			goto err;
+		}
+	}
+
+	close(fd);
+#ifdef VERBOSE
+	fprintf(stdout, "PASS\n");
+#endif
+	return 0;
+err:
+#ifdef VERBOSE
+	fprintf(stderr, "FAILED\n");
+#endif
+	if (fd != -1)
+		close(fd);
+	return 1;
+}
+
+static int test_io(const char *file, int tc, int read, int sqthread,
+		   int fixed, int nonvec)
+{
+	struct io_uring ring;
+	int ret, ring_flags = 0;
+
+	ring_flags |= IORING_SETUP_SQE128;
+	ring_flags |= IORING_SETUP_CQE32;
+
+	if (sqthread)
+		ring_flags |= IORING_SETUP_SQPOLL;
+
+	ret = t_create_ring(64, &ring, ring_flags);
+	if (ret == T_SETUP_SKIP)
+		return 0;
+	if (ret != T_SETUP_OK) {
+		fprintf(stderr, "ring create failed: %d\n", ret);
+		return 1;
+	}
+
+	ret = __test_io(file, &ring, tc, read, sqthread, fixed, nonvec);
+	io_uring_queue_exit(&ring);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	int i, ret;
+	char *fname;
+
+	if (argc < 2) {
+		printf("%s: requires NVMe character device\n", argv[0]);
+		return T_EXIT_SKIP;
+	}
+
+	fname = argv[1];
+	ret = fio_nvme_get_info(fname);
+
+	if (ret) {
+		fprintf(stderr, "failed to fetch device info: %d\n", ret);
+		goto err;
+	}
+
+	vecs = t_create_buffers(BUFFERS, BS);
+
+	for (i = 0; i < 16; i++) {
+		int read = (i & 1) != 0;
+		int sqthread = (i & 2) != 0;
+		int fixed = (i & 4) != 0;
+		int nonvec = (i & 8) != 0;
+
+		ret = test_io(fname, i, read, sqthread, fixed, nonvec);
+		if (ret) {
+			fprintf(stderr, "test_io failed %d/%d/%d/%d\n",
+				read, sqthread, fixed, nonvec);
+			goto err;
+		}
+	}
+
+	return T_EXIT_PASS;
+err:
+	return T_EXIT_FAIL;
+}
-- 
2.17.1

[PATCH liburing 5/5] test/io_uring_passthrough: add test case for poll IO

[Ankit Kumar]

For uring passthrough add test case for poll IO completion.
If poll IO is not supported return success.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 test/io_uring_passthrough.c | 76 +++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c
index 2e2b806..acb57f8 100644
--- a/test/io_uring_passthrough.c
+++ b/test/io_uring_passthrough.c
@@ -10,6 +10,7 @@
 
 #include "helpers.h"
 #include "liburing.h"
+#include "../src/syscall.h"
 #include "nvme.h"
 
 #define FILE_SIZE	(256 * 1024)
@@ -279,6 +280,75 @@ static int test_io(const char *file, int tc, int read, int sqthread,
 	return ret;
 }
 
+extern int __io_uring_flush_sq(struct io_uring *ring);
+
+/*
+ * if we are polling io_uring_submit needs to always enter the
+ * kernel to fetch events
+ */
+static int test_io_uring_submit_enters(const char *file)
+{
+	struct io_uring ring;
+	int fd, i, ret, ring_flags, open_flags;
+	unsigned head;
+	struct io_uring_cqe *cqe;
+
+	ring_flags = IORING_SETUP_IOPOLL;
+	ring_flags |= IORING_SETUP_SQE128;
+	ring_flags |= IORING_SETUP_CQE32;
+
+	ret = io_uring_queue_init(64, &ring, ring_flags);
+	if (ret) {
+		fprintf(stderr, "ring create failed: %d\n", ret);
+		return 1;
+	}
+
+	open_flags = O_WRONLY;
+	fd = open(file, open_flags);
+	if (fd < 0) {
+		perror("file open");
+		goto err;
+	}
+
+	for (i = 0; i < BUFFERS; i++) {
+		struct io_uring_sqe *sqe;
+		off_t offset = BS * (rand() % BUFFERS);
+
+		sqe = io_uring_get_sqe(&ring);
+		io_uring_prep_writev(sqe, fd, &vecs[i], 1, offset);
+		sqe->user_data = 1;
+	}
+
+	/* submit manually to avoid adding IORING_ENTER_GETEVENTS */
+	ret = __sys_io_uring_enter(ring.ring_fd, __io_uring_flush_sq(&ring), 0,
+						0, NULL);
+	if (ret < 0)
+		goto err;
+
+	for (i = 0; i < 500; i++) {
+		ret = io_uring_submit(&ring);
+		if (ret != 0) {
+			fprintf(stderr, "still had %d sqes to submit, this is unexpected", ret);
+			goto err;
+		}
+
+		io_uring_for_each_cqe(&ring, head, cqe) {
+			if (cqe->res == -EOPNOTSUPP)
+				fprintf(stdout, "doesn't support polled IO\n");
+			goto ok;
+		}
+		usleep(10000);
+	}
+err:
+	ret = 1;
+	if (fd != -1)
+		close(fd);
+
+ok:
+	io_uring_queue_exit(&ring);
+	return ret;
+}
+
 int main(int argc, char *argv[])
 {
 	int i, ret;
@@ -313,6 +383,12 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	ret = test_io_uring_submit_enters(fname);
+	if (ret) {
+		fprintf(stderr, "test_io_uring_submit_enters failed\n");
+		goto err;
+	}
+
 	return T_EXIT_PASS;
 err:
 	return T_EXIT_FAIL;
-- 
2.17.1

Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Casey Schaufler]

On 7/19/2022 6:52 AM, Ankit Kumar wrote:
> This patchset adds test/io_uring_passthrough.c to submit uring passthrough
> commands to nvme-ns character device. The uring passthrough was introduced
> with 5.19 io_uring.
>
> To send nvme uring passthrough commands we require helpers to fetch NVMe
> char device (/dev/ngXnY) specific fields such as namespace id, lba size.

There wouldn't be a way to run these tests using a more general
configuration, would there? I spent way too much time trying to
coax my systems into pretending it has this device.

>
> How to run:
> ./test/io_uring_passthrough.t /dev/ng0n1
>
> This covers basic write/read with verify for sqthread poll,
> vectored / nonvectored and fixed IO buffers, which can be easily extended
> in future.
>
> Ankit Kumar (5):
>   configure: check for nvme uring command support
>   io_uring.h: sync sqe entry with 5.20 io_uring
>   nvme: add nvme opcodes, structures and helper functions
>   test: add io_uring passthrough test
>   test/io_uring_passthrough: add test case for poll IO
>
>  configure                       |  20 ++
>  src/include/liburing/io_uring.h |  17 +-
>  test/Makefile                   |   1 +
>  test/io_uring_passthrough.c     | 395 ++++++++++++++++++++++++++++++++
>  test/nvme.h                     | 168 ++++++++++++++
>  5 files changed, 599 insertions(+), 2 deletions(-)
>  create mode 100644 test/io_uring_passthrough.c
>  create mode 100644 test/nvme.h
>

Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Jens Axboe]

On 8/11/22 6:43 PM, Casey Schaufler wrote:
> On 7/19/2022 6:52 AM, Ankit Kumar wrote:
>> This patchset adds test/io_uring_passthrough.c to submit uring passthrough
>> commands to nvme-ns character device. The uring passthrough was introduced
>> with 5.19 io_uring.
>>
>> To send nvme uring passthrough commands we require helpers to fetch NVMe
>> char device (/dev/ngXnY) specific fields such as namespace id, lba size.
> 
> There wouldn't be a way to run these tests using a more general
> configuration, would there? I spent way too much time trying to
> coax my systems into pretending it has this device.

It's only plumbed up for nvme. Just use qemu with an nvme device?

-drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \
-device nvme,drive=drv1,serial=blah2

Paul was pondering wiring up a no-op kind of thing for null, though.

-- 
Jens Axboe

Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Paul Moore]

On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 8/11/22 6:43 PM, Casey Schaufler wrote:
> > On 7/19/2022 6:52 AM, Ankit Kumar wrote:
> >> This patchset adds test/io_uring_passthrough.c to submit uring passthrough
> >> commands to nvme-ns character device. The uring passthrough was introduced
> >> with 5.19 io_uring.
> >>
> >> To send nvme uring passthrough commands we require helpers to fetch NVMe
> >> char device (/dev/ngXnY) specific fields such as namespace id, lba size.
> >
> > There wouldn't be a way to run these tests using a more general
> > configuration, would there? I spent way too much time trying to
> > coax my systems into pretending it has this device.
>
> It's only plumbed up for nvme. Just use qemu with an nvme device?
>
> -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \
> -device nvme,drive=drv1,serial=blah2
>
> Paul was pondering wiring up a no-op kind of thing for null, though.

Yep, I started working on that earlier this week, but I've gotten
pulled back into the SCTP stuff to try and sort out something odd.

Casey, what I have isn't tested, but I'll toss it into my next kernel
build to make sure it at least doesn't crash on boot and if it looks
good I'll send it to you off-list.

-- 
paul-moore.com

Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Casey Schaufler]

On 8/12/2022 8:33 AM, Paul Moore wrote:
> On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 8/11/22 6:43 PM, Casey Schaufler wrote:
>>> On 7/19/2022 6:52 AM, Ankit Kumar wrote:
>>>> This patchset adds test/io_uring_passthrough.c to submit uring passthrough
>>>> commands to nvme-ns character device. The uring passthrough was introduced
>>>> with 5.19 io_uring.
>>>>
>>>> To send nvme uring passthrough commands we require helpers to fetch NVMe
>>>> char device (/dev/ngXnY) specific fields such as namespace id, lba size.
>>> There wouldn't be a way to run these tests using a more general
>>> configuration, would there? I spent way too much time trying to
>>> coax my systems into pretending it has this device.
>> It's only plumbed up for nvme. Just use qemu with an nvme device?
>>
>> -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \
>> -device nvme,drive=drv1,serial=blah2
>>
>> Paul was pondering wiring up a no-op kind of thing for null, though.
> Yep, I started working on that earlier this week, but I've gotten
> pulled back into the SCTP stuff to try and sort out something odd.
>
> Casey, what I have isn't tested, but I'll toss it into my next kernel
> build to make sure it at least doesn't crash on boot and if it looks
> good I'll send it to you off-list.

Super. Playing with qemu configuration always seems to suck time
and rarely gets me where I want to be.

Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands

[Kanchan Joshi]

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

On Fri, Aug 12, 2022 at 09:03:12AM -0700, Casey Schaufler wrote:
>On 8/12/2022 8:33 AM, Paul Moore wrote:
>> On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <axboe@kernel.dk> wrote:
>>> On 8/11/22 6:43 PM, Casey Schaufler wrote:
>>>> On 7/19/2022 6:52 AM, Ankit Kumar wrote:
>>>>> This patchset adds test/io_uring_passthrough.c to submit uring passthrough
>>>>> commands to nvme-ns character device. The uring passthrough was introduced
>>>>> with 5.19 io_uring.
>>>>>
>>>>> To send nvme uring passthrough commands we require helpers to fetch NVMe
>>>>> char device (/dev/ngXnY) specific fields such as namespace id, lba size.
>>>> There wouldn't be a way to run these tests using a more general
>>>> configuration, would there? I spent way too much time trying to
>>>> coax my systems into pretending it has this device.
>>> It's only plumbed up for nvme. Just use qemu with an nvme device?
>>>
>>> -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \
>>> -device nvme,drive=drv1,serial=blah2
>>>
>>> Paul was pondering wiring up a no-op kind of thing for null, though.
>> Yep, I started working on that earlier this week, but I've gotten
>> pulled back into the SCTP stuff to try and sort out something odd.
>>
>> Casey, what I have isn't tested, but I'll toss it into my next kernel
>> build to make sure it at least doesn't crash on boot and if it looks
>> good I'll send it to you off-list.
>
>Super. Playing with qemu configuration always seems to suck time
>and rarely gets me where I want to be.

FWIW, one more option (not easier than no-op/null though) is to emulate
nvme over a regular-file using loopback-fabrics target.
A coarse script is here -
https://github.com/joshkan/loopback-nvme-uring/commit/96853963a196f2d307d4d8e60d1276a08b520307


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

Limit io_uring "cmd" options to files for which the caller has
Smack read access. There may be cases where the cmd option may
be closer to a write access than a read, but there is no way
to make that determination.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
--
 security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 001831458fa2..bffccdc494cb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -42,6 +42,7 @@
 #include <linux/fs_context.h>
 #include <linux/fs_parser.h>
 #include <linux/watch_queue.h>
+#include <linux/io_uring.h>
 #include "smack.h"
 
 #define TRANS_TRUE	"TRUE"
@@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
 	return -EPERM;
 }
 
+/**
+ * smack_uring_cmd - check on file operations for io_uring
+ * @ioucmd: the command in question
+ *
+ * Make a best guess about whether a io_uring "command" should
+ * be allowed. Use the same logic used for determining if the
+ * file could be opened for read in the absence of better criteria.
+ */
+static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
+{
+	struct file *file = ioucmd->file;
+	struct smk_audit_info ad;
+	struct task_smack *tsp;
+	struct inode *inode;
+	int rc;
+
+	if (!file)
+		return -EINVAL;
+
+	tsp = smack_cred(file->f_cred);
+	inode = file_inode(file);
+
+	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
+	smk_ad_setfield_u_fs_path(&ad, file->f_path);
+	rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
+	rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
+
+	return rc;
+}
+
 #endif /* CONFIG_IO_URING */
 
 struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
@@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 #ifdef CONFIG_IO_URING
 	LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
 	LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
+	LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
 #endif
 };
 

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Paul Moore]

On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Limit io_uring "cmd" options to files for which the caller has
> Smack read access. There may be cases where the cmd option may
> be closer to a write access than a read, but there is no way
> to make that determination.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> --
>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 001831458fa2..bffccdc494cb 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c

...

> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>         return -EPERM;
>  }
>
> +/**
> + * smack_uring_cmd - check on file operations for io_uring
> + * @ioucmd: the command in question
> + *
> + * Make a best guess about whether a io_uring "command" should
> + * be allowed. Use the same logic used for determining if the
> + * file could be opened for read in the absence of better criteria.
> + */
> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> +{
> +       struct file *file = ioucmd->file;
> +       struct smk_audit_info ad;
> +       struct task_smack *tsp;
> +       struct inode *inode;
> +       int rc;
> +
> +       if (!file)
> +               return -EINVAL;

Perhaps this is a better question for Jens, but ioucmd->file is always
going to be valid when the LSM hook is called, yes?

> +       tsp = smack_cred(file->f_cred);
> +       inode = file_inode(file);
> +
> +       smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
> +       smk_ad_setfield_u_fs_path(&ad, file->f_path);
> +       rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
> +       rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
> +
> +       return rc;
> +}
> +
>  #endif /* CONFIG_IO_URING */

-- 
paul-moore.com

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Jens Axboe]

On 8/23/22 6:05 PM, Paul Moore wrote:
> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> Limit io_uring "cmd" options to files for which the caller has
>> Smack read access. There may be cases where the cmd option may
>> be closer to a write access than a read, but there is no way
>> to make that determination.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> --
>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 001831458fa2..bffccdc494cb 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
> 
> ...
> 
>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>         return -EPERM;
>>  }
>>
>> +/**
>> + * smack_uring_cmd - check on file operations for io_uring
>> + * @ioucmd: the command in question
>> + *
>> + * Make a best guess about whether a io_uring "command" should
>> + * be allowed. Use the same logic used for determining if the
>> + * file could be opened for read in the absence of better criteria.
>> + */
>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>> +{
>> +       struct file *file = ioucmd->file;
>> +       struct smk_audit_info ad;
>> +       struct task_smack *tsp;
>> +       struct inode *inode;
>> +       int rc;
>> +
>> +       if (!file)
>> +               return -EINVAL;
> 
> Perhaps this is a better question for Jens, but ioucmd->file is always
> going to be valid when the LSM hook is called, yes?

file will always be valid for uring commands, as they are marked as
requiring a file. If no valid fd is given for it, it would've been
errored early on, before reaching f_op->uring_cmd().

-- 
Jens Axboe

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Paul Moore]

On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 8/23/22 6:05 PM, Paul Moore wrote:
> > On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>
> >> Limit io_uring "cmd" options to files for which the caller has
> >> Smack read access. There may be cases where the cmd option may
> >> be closer to a write access than a read, but there is no way
> >> to make that determination.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> --
> >>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> >>  1 file changed, 32 insertions(+)
> >>
> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >> index 001831458fa2..bffccdc494cb 100644
> >> --- a/security/smack/smack_lsm.c
> >> +++ b/security/smack/smack_lsm.c
> >
> > ...
> >
> >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> >>         return -EPERM;
> >>  }
> >>
> >> +/**
> >> + * smack_uring_cmd - check on file operations for io_uring
> >> + * @ioucmd: the command in question
> >> + *
> >> + * Make a best guess about whether a io_uring "command" should
> >> + * be allowed. Use the same logic used for determining if the
> >> + * file could be opened for read in the absence of better criteria.
> >> + */
> >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> >> +{
> >> +       struct file *file = ioucmd->file;
> >> +       struct smk_audit_info ad;
> >> +       struct task_smack *tsp;
> >> +       struct inode *inode;
> >> +       int rc;
> >> +
> >> +       if (!file)
> >> +               return -EINVAL;
> >
> > Perhaps this is a better question for Jens, but ioucmd->file is always
> > going to be valid when the LSM hook is called, yes?
>
> file will always be valid for uring commands, as they are marked as
> requiring a file. If no valid fd is given for it, it would've been
> errored early on, before reaching f_op->uring_cmd().

Hey Casey, where do things stand with this patch?  To be specific, did
you want me to include this in the lsm/stable-6.0 PR for Linus or are
you planning to send it separately?  If you want me to send it up, are
you planning another revision?

There is no right or wrong answer here as far as I'm concerned, I'm
just trying to make sure we are all on the same page.

-- 
paul-moore.com

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

On 8/26/2022 8:15 AM, Paul Moore wrote:
> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 8/23/22 6:05 PM, Paul Moore wrote:
>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Limit io_uring "cmd" options to files for which the caller has
>>>> Smack read access. There may be cases where the cmd option may
>>>> be closer to a write access than a read, but there is no way
>>>> to make that determination.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> --
>>>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>> index 001831458fa2..bffccdc494cb 100644
>>>> --- a/security/smack/smack_lsm.c
>>>> +++ b/security/smack/smack_lsm.c
>>> ...
>>>
>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>>>         return -EPERM;
>>>>  }
>>>>
>>>> +/**
>>>> + * smack_uring_cmd - check on file operations for io_uring
>>>> + * @ioucmd: the command in question
>>>> + *
>>>> + * Make a best guess about whether a io_uring "command" should
>>>> + * be allowed. Use the same logic used for determining if the
>>>> + * file could be opened for read in the absence of better criteria.
>>>> + */
>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>>>> +{
>>>> +       struct file *file = ioucmd->file;
>>>> +       struct smk_audit_info ad;
>>>> +       struct task_smack *tsp;
>>>> +       struct inode *inode;
>>>> +       int rc;
>>>> +
>>>> +       if (!file)
>>>> +               return -EINVAL;
>>> Perhaps this is a better question for Jens, but ioucmd->file is always
>>> going to be valid when the LSM hook is called, yes?
>> file will always be valid for uring commands, as they are marked as
>> requiring a file. If no valid fd is given for it, it would've been
>> errored early on, before reaching f_op->uring_cmd().
> Hey Casey, where do things stand with this patch?  To be specific, did
> you want me to include this in the lsm/stable-6.0 PR for Linus or are
> you planning to send it separately?  If you want me to send it up, are
> you planning another revision?
>
> There is no right or wrong answer here as far as I'm concerned, I'm
> just trying to make sure we are all on the same page.

I think the whole LSM fix for io_uring looks better the more complete
it is. I don't see the Smack check changing until such time as there's
better information available to make decisions upon. If you send it along
with the rest of the patch set I think we'll have done our best.

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Paul Moore]

On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/26/2022 8:15 AM, Paul Moore wrote:
> > On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 8/23/22 6:05 PM, Paul Moore wrote:
> >>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> Limit io_uring "cmd" options to files for which the caller has
> >>>> Smack read access. There may be cases where the cmd option may
> >>>> be closer to a write access than a read, but there is no way
> >>>> to make that determination.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>> --
> >>>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >>>> index 001831458fa2..bffccdc494cb 100644
> >>>> --- a/security/smack/smack_lsm.c
> >>>> +++ b/security/smack/smack_lsm.c
> >>> ...
> >>>
> >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> >>>>         return -EPERM;
> >>>>  }
> >>>>
> >>>> +/**
> >>>> + * smack_uring_cmd - check on file operations for io_uring
> >>>> + * @ioucmd: the command in question
> >>>> + *
> >>>> + * Make a best guess about whether a io_uring "command" should
> >>>> + * be allowed. Use the same logic used for determining if the
> >>>> + * file could be opened for read in the absence of better criteria.
> >>>> + */
> >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> >>>> +{
> >>>> +       struct file *file = ioucmd->file;
> >>>> +       struct smk_audit_info ad;
> >>>> +       struct task_smack *tsp;
> >>>> +       struct inode *inode;
> >>>> +       int rc;
> >>>> +
> >>>> +       if (!file)
> >>>> +               return -EINVAL;
> >>> Perhaps this is a better question for Jens, but ioucmd->file is always
> >>> going to be valid when the LSM hook is called, yes?
> >> file will always be valid for uring commands, as they are marked as
> >> requiring a file. If no valid fd is given for it, it would've been
> >> errored early on, before reaching f_op->uring_cmd().
> > Hey Casey, where do things stand with this patch?  To be specific, did
> > you want me to include this in the lsm/stable-6.0 PR for Linus or are
> > you planning to send it separately?  If you want me to send it up, are
> > you planning another revision?
> >
> > There is no right or wrong answer here as far as I'm concerned, I'm
> > just trying to make sure we are all on the same page.
>
> I think the whole LSM fix for io_uring looks better the more complete
> it is. I don't see the Smack check changing until such time as there's
> better information available to make decisions upon. If you send it along
> with the rest of the patch set I think we'll have done our best.

Okay, will do.  Would you like me to tag the patch with the 'Fixes:'
and stable tags, similar to the LSM and SELinux patches?

-- 
paul-moore.com

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

On 8/26/2022 11:59 AM, Paul Moore wrote:
> On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/26/2022 8:15 AM, Paul Moore wrote:
>>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 8/23/22 6:05 PM, Paul Moore wrote:
>>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> Limit io_uring "cmd" options to files for which the caller has
>>>>>> Smack read access. There may be cases where the cmd option may
>>>>>> be closer to a write access than a read, but there is no way
>>>>>> to make that determination.
>>>>>>
>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>>> --
>>>>>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>>> index 001831458fa2..bffccdc494cb 100644
>>>>>> --- a/security/smack/smack_lsm.c
>>>>>> +++ b/security/smack/smack_lsm.c
>>>>> ...
>>>>>
>>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>>>>>         return -EPERM;
>>>>>>  }
>>>>>>
>>>>>> +/**
>>>>>> + * smack_uring_cmd - check on file operations for io_uring
>>>>>> + * @ioucmd: the command in question
>>>>>> + *
>>>>>> + * Make a best guess about whether a io_uring "command" should
>>>>>> + * be allowed. Use the same logic used for determining if the
>>>>>> + * file could be opened for read in the absence of better criteria.
>>>>>> + */
>>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>>>>>> +{
>>>>>> +       struct file *file = ioucmd->file;
>>>>>> +       struct smk_audit_info ad;
>>>>>> +       struct task_smack *tsp;
>>>>>> +       struct inode *inode;
>>>>>> +       int rc;
>>>>>> +
>>>>>> +       if (!file)
>>>>>> +               return -EINVAL;
>>>>> Perhaps this is a better question for Jens, but ioucmd->file is always
>>>>> going to be valid when the LSM hook is called, yes?
>>>> file will always be valid for uring commands, as they are marked as
>>>> requiring a file. If no valid fd is given for it, it would've been
>>>> errored early on, before reaching f_op->uring_cmd().
>>> Hey Casey, where do things stand with this patch?  To be specific, did
>>> you want me to include this in the lsm/stable-6.0 PR for Linus or are
>>> you planning to send it separately?  If you want me to send it up, are
>>> you planning another revision?
>>>
>>> There is no right or wrong answer here as far as I'm concerned, I'm
>>> just trying to make sure we are all on the same page.
>> I think the whole LSM fix for io_uring looks better the more complete
>> it is. I don't see the Smack check changing until such time as there's
>> better information available to make decisions upon. If you send it along
>> with the rest of the patch set I think we'll have done our best.
> Okay, will do.  Would you like me to tag the patch with the 'Fixes:'
> and stable tags, similar to the LSM and SELinux patches?

Yes, I think that's best.

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Paul Moore]

On Fri, Aug 26, 2022 at 3:04 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/26/2022 11:59 AM, Paul Moore wrote:
> > On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/26/2022 8:15 AM, Paul Moore wrote:
> >>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 8/23/22 6:05 PM, Paul Moore wrote:
> >>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>> Limit io_uring "cmd" options to files for which the caller has
> >>>>>> Smack read access. There may be cases where the cmd option may
> >>>>>> be closer to a write access than a read, but there is no way
> >>>>>> to make that determination.
> >>>>>>
> >>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>>>> --
> >>>>>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> >>>>>>  1 file changed, 32 insertions(+)
> >>>>>>
> >>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >>>>>> index 001831458fa2..bffccdc494cb 100644
> >>>>>> --- a/security/smack/smack_lsm.c
> >>>>>> +++ b/security/smack/smack_lsm.c
> >>>>> ...
> >>>>>
> >>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> >>>>>>         return -EPERM;
> >>>>>>  }
> >>>>>>
> >>>>>> +/**
> >>>>>> + * smack_uring_cmd - check on file operations for io_uring
> >>>>>> + * @ioucmd: the command in question
> >>>>>> + *
> >>>>>> + * Make a best guess about whether a io_uring "command" should
> >>>>>> + * be allowed. Use the same logic used for determining if the
> >>>>>> + * file could be opened for read in the absence of better criteria.
> >>>>>> + */
> >>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> >>>>>> +{
> >>>>>> +       struct file *file = ioucmd->file;
> >>>>>> +       struct smk_audit_info ad;
> >>>>>> +       struct task_smack *tsp;
> >>>>>> +       struct inode *inode;
> >>>>>> +       int rc;
> >>>>>> +
> >>>>>> +       if (!file)
> >>>>>> +               return -EINVAL;
> >>>>> Perhaps this is a better question for Jens, but ioucmd->file is always
> >>>>> going to be valid when the LSM hook is called, yes?
> >>>> file will always be valid for uring commands, as they are marked as
> >>>> requiring a file. If no valid fd is given for it, it would've been
> >>>> errored early on, before reaching f_op->uring_cmd().
> >>> Hey Casey, where do things stand with this patch?  To be specific, did
> >>> you want me to include this in the lsm/stable-6.0 PR for Linus or are
> >>> you planning to send it separately?  If you want me to send it up, are
> >>> you planning another revision?
> >>>
> >>> There is no right or wrong answer here as far as I'm concerned, I'm
> >>> just trying to make sure we are all on the same page.
> >> I think the whole LSM fix for io_uring looks better the more complete
> >> it is. I don't see the Smack check changing until such time as there's
> >> better information available to make decisions upon. If you send it along
> >> with the rest of the patch set I think we'll have done our best.
> > Okay, will do.  Would you like me to tag the patch with the 'Fixes:'
> > and stable tags, similar to the LSM and SELinux patches?
>
> Yes, I think that's best.

Done and merged to lsm/stable-6.0.  I'm going to let the automated
stuff do it's thing and assuming no problems I'll plan to send it to
Linus on Monday ... sending stuff like this last thing on a Friday is
a little too risky for my tastes.

-- 
paul-moore.com

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

On 8/26/2022 12:10 PM, Paul Moore wrote:
> On Fri, Aug 26, 2022 at 3:04 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/26/2022 11:59 AM, Paul Moore wrote:
>>> On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 8/26/2022 8:15 AM, Paul Moore wrote:
>>>>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 8/23/22 6:05 PM, Paul Moore wrote:
>>>>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>> Limit io_uring "cmd" options to files for which the caller has
>>>>>>>> Smack read access. There may be cases where the cmd option may
>>>>>>>> be closer to a write access than a read, but there is no way
>>>>>>>> to make that determination.
>>>>>>>>
>>>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>>>>> --
>>>>>>>>  security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>>>>> index 001831458fa2..bffccdc494cb 100644
>>>>>>>> --- a/security/smack/smack_lsm.c
>>>>>>>> +++ b/security/smack/smack_lsm.c
>>>>>>> ...
>>>>>>>
>>>>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>>>>>>>         return -EPERM;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * smack_uring_cmd - check on file operations for io_uring
>>>>>>>> + * @ioucmd: the command in question
>>>>>>>> + *
>>>>>>>> + * Make a best guess about whether a io_uring "command" should
>>>>>>>> + * be allowed. Use the same logic used for determining if the
>>>>>>>> + * file could be opened for read in the absence of better criteria.
>>>>>>>> + */
>>>>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>>>>>>>> +{
>>>>>>>> +       struct file *file = ioucmd->file;
>>>>>>>> +       struct smk_audit_info ad;
>>>>>>>> +       struct task_smack *tsp;
>>>>>>>> +       struct inode *inode;
>>>>>>>> +       int rc;
>>>>>>>> +
>>>>>>>> +       if (!file)
>>>>>>>> +               return -EINVAL;
>>>>>>> Perhaps this is a better question for Jens, but ioucmd->file is always
>>>>>>> going to be valid when the LSM hook is called, yes?
>>>>>> file will always be valid for uring commands, as they are marked as
>>>>>> requiring a file. If no valid fd is given for it, it would've been
>>>>>> errored early on, before reaching f_op->uring_cmd().
>>>>> Hey Casey, where do things stand with this patch?  To be specific, did
>>>>> you want me to include this in the lsm/stable-6.0 PR for Linus or are
>>>>> you planning to send it separately?  If you want me to send it up, are
>>>>> you planning another revision?
>>>>>
>>>>> There is no right or wrong answer here as far as I'm concerned, I'm
>>>>> just trying to make sure we are all on the same page.
>>>> I think the whole LSM fix for io_uring looks better the more complete
>>>> it is. I don't see the Smack check changing until such time as there's
>>>> better information available to make decisions upon. If you send it along
>>>> with the rest of the patch set I think we'll have done our best.
>>> Okay, will do.  Would you like me to tag the patch with the 'Fixes:'
>>> and stable tags, similar to the LSM and SELinux patches?
>> Yes, I think that's best.
> Done and merged to lsm/stable-6.0.  I'm going to let the automated
> stuff do it's thing and assuming no problems I'll plan to send it to
> Linus on Monday ... sending stuff like this last thing on a Friday is
> a little too risky for my tastes.

Agreed. Thank you.

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Kanchan Joshi]

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

On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote:
>Limit io_uring "cmd" options to files for which the caller has
>Smack read access. There may be cases where the cmd option may
>be closer to a write access than a read, but there is no way
>to make that determination.
>
>Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>--
> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
>diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>index 001831458fa2..bffccdc494cb 100644
>--- a/security/smack/smack_lsm.c
>+++ b/security/smack/smack_lsm.c
>@@ -42,6 +42,7 @@
> #include <linux/fs_context.h>
> #include <linux/fs_parser.h>
> #include <linux/watch_queue.h>
>+#include <linux/io_uring.h>
> #include "smack.h"
>
> #define TRANS_TRUE	"TRUE"
>@@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> 	return -EPERM;
> }
>
>+/**
>+ * smack_uring_cmd - check on file operations for io_uring
>+ * @ioucmd: the command in question
>+ *
>+ * Make a best guess about whether a io_uring "command" should
>+ * be allowed. Use the same logic used for determining if the
>+ * file could be opened for read in the absence of better criteria.
>+ */
>+static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>+{
>+	struct file *file = ioucmd->file;
>+	struct smk_audit_info ad;
>+	struct task_smack *tsp;
>+	struct inode *inode;
>+	int rc;
>+
>+	if (!file)
>+		return -EINVAL;
>+
>+	tsp = smack_cred(file->f_cred);
>+	inode = file_inode(file);
>+
>+	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
>+	smk_ad_setfield_u_fs_path(&ad, file->f_path);
>+	rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
>+	rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
>+
>+	return rc;
>+}
>+
> #endif /* CONFIG_IO_URING */
>
> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>@@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> #ifdef CONFIG_IO_URING
> 	LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
> 	LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
>+	LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
> #endif

Tried this on nvme device (/dev/ng0n1).
Took a while to come out of noob setup-related issues but I see that
smack is listed (in /sys/kernel/security/lsm), smackfs is present, and
the hook (smack_uring_cmd) gets triggered fine on doing I/O on
/dev/ng0n1.

I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which
is set to floor).

$ chsmack -L /dev/ng0n1
/dev/ng0n1 access="_"

I ran fio (/usr/bin/fio), which also has the same label.
Hope you expect the same outcome.

Do you run something else to see that things are fine e.g. for
/dev/null, which also has the same label "_".
If yes, I can try the same on nvme side.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

On 8/27/2022 8:59 AM, Kanchan Joshi wrote:
> On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote:
>> Limit io_uring "cmd" options to files for which the caller has
>> Smack read access. There may be cases where the cmd option may
>> be closer to a write access than a read, but there is no way
>> to make that determination.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> -- 
>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 001831458fa2..bffccdc494cb 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -42,6 +42,7 @@
>> #include <linux/fs_context.h>
>> #include <linux/fs_parser.h>
>> #include <linux/watch_queue.h>
>> +#include <linux/io_uring.h>
>> #include "smack.h"
>>
>> #define TRANS_TRUE    "TRUE"
>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>     return -EPERM;
>> }
>>
>> +/**
>> + * smack_uring_cmd - check on file operations for io_uring
>> + * @ioucmd: the command in question
>> + *
>> + * Make a best guess about whether a io_uring "command" should
>> + * be allowed. Use the same logic used for determining if the
>> + * file could be opened for read in the absence of better criteria.
>> + */
>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>> +{
>> +    struct file *file = ioucmd->file;
>> +    struct smk_audit_info ad;
>> +    struct task_smack *tsp;
>> +    struct inode *inode;
>> +    int rc;
>> +
>> +    if (!file)
>> +        return -EINVAL;
>> +
>> +    tsp = smack_cred(file->f_cred);
>> +    inode = file_inode(file);
>> +
>> +    smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
>> +    smk_ad_setfield_u_fs_path(&ad, file->f_path);
>> +    rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
>> +    rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
>> +
>> +    return rc;
>> +}
>> +
>> #endif /* CONFIG_IO_URING */
>>
>> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[]
>> __lsm_ro_after_init = {
>> #ifdef CONFIG_IO_URING
>>     LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
>>     LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
>> +    LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
>> #endif
>
> Tried this on nvme device (/dev/ng0n1).
> Took a while to come out of noob setup-related issues but I see that
> smack is listed (in /sys/kernel/security/lsm), smackfs is present, and
> the hook (smack_uring_cmd) gets triggered fine on doing I/O on
> /dev/ng0n1.
>
> I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which
> is set to floor).
>
> $ chsmack -L /dev/ng0n1
> /dev/ng0n1 access="_"

Setting the Smack on the object that the cmd operates on to
something other than "_" would be the correct test. If that
is /dev/ng0n1 you could use

	# chsmack -a Snap /dev/ng0n1

The unprivileged user won't be able to read /dev/ng0n1 so you
won't get as far as testing the cmd interface. I don't know
io_uring and nvme well enough to know what other objects may
be involved. Noob here, too.

>
> I ran fio (/usr/bin/fio), which also has the same label.
> Hope you expect the same outcome.
>
> Do you run something else to see that things are fine e.g. for
> /dev/null, which also has the same label "_".
> If yes, I can try the same on nvme side.
>

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Joel Granados]

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

Hey Casey

I have tested this patch and I see the smack_uring_cmd prevents access
to file when smack labels are different. They way I got there was a bit
convoluted and I'll describe it here so ppl can judge how valid the test
is.

Tested-by: Joel Granados <j.granados@samsung.com>

I started by reproducing what Kanchan had done by changing the smack
label from "_" to "Snap". Then I ran the io_uring passthrough test
included in liburing with an unprivileged user and saw that the
smack_uring_cmd function was NOT executed because smack prevented an open on
the device file ("/dev/ng0n1" on my box).

So here I got a bit sneaky and changed the label after the open to the
device was done. This is how I did it:
1. In the io_uring_passthrough.c tests I stopped execution after the
   device was open and before the actual IO.
2. While on hold I changed the label of the device from "_" to "Snap".
   At this point the device had a "Snap" label and the executable had a
   "_" label.
3. Previous to execution I had placed a printk inside the
   smack_uring_cmd function. And I also made sure to printk whenever
   security_uring_command returned because of a security violation.
4. I continued execution and saw that both smack_uiring_cmd and
   io_uring_cmd returned error. Which is what I expected.

I'm still a bit unsure of my setup so if anyone has comments or a way to
make it better, I would really appreciate feedback.

Best

Joel

On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote:
> On 8/27/2022 8:59 AM, Kanchan Joshi wrote:
> > On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote:
> >> Limit io_uring "cmd" options to files for which the caller has
> >> Smack read access. There may be cases where the cmd option may
> >> be closer to a write access than a read, but there is no way
> >> to make that determination.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> -- 
> >> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> >> 1 file changed, 32 insertions(+)
> >>
> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >> index 001831458fa2..bffccdc494cb 100644
> >> --- a/security/smack/smack_lsm.c
> >> +++ b/security/smack/smack_lsm.c
> >> @@ -42,6 +42,7 @@
> >> #include <linux/fs_context.h>
> >> #include <linux/fs_parser.h>
> >> #include <linux/watch_queue.h>
> >> +#include <linux/io_uring.h>
> >> #include "smack.h"
> >>
> >> #define TRANS_TRUE    "TRUE"
> >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> >>     return -EPERM;
> >> }
> >>
> >> +/**
> >> + * smack_uring_cmd - check on file operations for io_uring
> >> + * @ioucmd: the command in question
> >> + *
> >> + * Make a best guess about whether a io_uring "command" should
> >> + * be allowed. Use the same logic used for determining if the
> >> + * file could be opened for read in the absence of better criteria.
> >> + */
> >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> >> +{
> >> +    struct file *file = ioucmd->file;
> >> +    struct smk_audit_info ad;
> >> +    struct task_smack *tsp;
> >> +    struct inode *inode;
> >> +    int rc;
> >> +
> >> +    if (!file)
> >> +        return -EINVAL;
> >> +
> >> +    tsp = smack_cred(file->f_cred);
> >> +    inode = file_inode(file);
> >> +
> >> +    smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
> >> +    smk_ad_setfield_u_fs_path(&ad, file->f_path);
> >> +    rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
> >> +    rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
> >> +
> >> +    return rc;
> >> +}
> >> +
> >> #endif /* CONFIG_IO_URING */
> >>
> >> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> >> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[]
> >> __lsm_ro_after_init = {
> >> #ifdef CONFIG_IO_URING
> >>     LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
> >>     LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
> >> +    LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
> >> #endif
> >
> > Tried this on nvme device (/dev/ng0n1).
> > Took a while to come out of noob setup-related issues but I see that
> > smack is listed (in /sys/kernel/security/lsm), smackfs is present, and
> > the hook (smack_uring_cmd) gets triggered fine on doing I/O on
> > /dev/ng0n1.
> >
> > I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which
> > is set to floor).
> >
> > $ chsmack -L /dev/ng0n1
> > /dev/ng0n1 access="_"
> 
> Setting the Smack on the object that the cmd operates on to
> something other than "_" would be the correct test. If that
> is /dev/ng0n1 you could use
> 
> 	# chsmack -a Snap /dev/ng0n1
> 
> The unprivileged user won't be able to read /dev/ng0n1 so you
> won't get as far as testing the cmd interface. I don't know
> io_uring and nvme well enough to know what other objects may
> be involved. Noob here, too.
> 
> >
> > I ran fio (/usr/bin/fio), which also has the same label.
> > Hope you expect the same outcome.
> >
> > Do you run something else to see that things are fine e.g. for
> > /dev/null, which also has the same label "_".
> > If yes, I can try the same on nvme side.
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Casey Schaufler]

On 8/30/2022 6:08 AM, Joel Granados wrote:
> Hey Casey
>
> I have tested this patch and I see the smack_uring_cmd prevents access
> to file when smack labels are different. They way I got there was a bit
> convoluted and I'll describe it here so ppl can judge how valid the test
> is.
>
> Tested-by: Joel Granados <j.granados@samsung.com>

Thank you.

>
> I started by reproducing what Kanchan had done by changing the smack
> label from "_" to "Snap". Then I ran the io_uring passthrough test
> included in liburing with an unprivileged user and saw that the
> smack_uring_cmd function was NOT executed because smack prevented an open on
> the device file ("/dev/ng0n1" on my box).
>
> So here I got a bit sneaky and changed the label after the open to the
> device was done. This is how I did it:
> 1. In the io_uring_passthrough.c tests I stopped execution after the
>    device was open and before the actual IO.
> 2. While on hold I changed the label of the device from "_" to "Snap".
>    At this point the device had a "Snap" label and the executable had a
>    "_" label.
> 3. Previous to execution I had placed a printk inside the
>    smack_uring_cmd function. And I also made sure to printk whenever
>    security_uring_command returned because of a security violation.
> 4. I continued execution and saw that both smack_uiring_cmd and
>    io_uring_cmd returned error. Which is what I expected.
>
> I'm still a bit unsure of my setup so if anyone has comments or a way to
> make it better, I would really appreciate feedback.

This is a perfectly rational approach to the test. Another approach
would be to add a Smack access rule:

	echo "_ Snap r" > /sys/fs/smackfs/load2

and label the device before the test begins. Step 2 changes from labeling
the device to removing the access rule:

	echo "_ Snap -" > /sys/fs/smackfs/load2

and you will get the same result. I wouldn't change your test, but I
would probably add another that does it using the rule change.

> Best
>
> Joel
>
> On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote:
>> On 8/27/2022 8:59 AM, Kanchan Joshi wrote:
>>> On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote:
>>>> Limit io_uring "cmd" options to files for which the caller has
>>>> Smack read access. There may be cases where the cmd option may
>>>> be closer to a write access than a read, but there is no way
>>>> to make that determination.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> -- 
>>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>> index 001831458fa2..bffccdc494cb 100644
>>>> --- a/security/smack/smack_lsm.c
>>>> +++ b/security/smack/smack_lsm.c
>>>> @@ -42,6 +42,7 @@
>>>> #include <linux/fs_context.h>
>>>> #include <linux/fs_parser.h>
>>>> #include <linux/watch_queue.h>
>>>> +#include <linux/io_uring.h>
>>>> #include "smack.h"
>>>>
>>>> #define TRANS_TRUE    "TRUE"
>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
>>>>     return -EPERM;
>>>> }
>>>>
>>>> +/**
>>>> + * smack_uring_cmd - check on file operations for io_uring
>>>> + * @ioucmd: the command in question
>>>> + *
>>>> + * Make a best guess about whether a io_uring "command" should
>>>> + * be allowed. Use the same logic used for determining if the
>>>> + * file could be opened for read in the absence of better criteria.
>>>> + */
>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>>>> +{
>>>> +    struct file *file = ioucmd->file;
>>>> +    struct smk_audit_info ad;
>>>> +    struct task_smack *tsp;
>>>> +    struct inode *inode;
>>>> +    int rc;
>>>> +
>>>> +    if (!file)
>>>> +        return -EINVAL;
>>>> +
>>>> +    tsp = smack_cred(file->f_cred);
>>>> +    inode = file_inode(file);
>>>> +
>>>> +    smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
>>>> +    smk_ad_setfield_u_fs_path(&ad, file->f_path);
>>>> +    rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
>>>> +    rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
>>>> +
>>>> +    return rc;
>>>> +}
>>>> +
>>>> #endif /* CONFIG_IO_URING */
>>>>
>>>> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>>> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[]
>>>> __lsm_ro_after_init = {
>>>> #ifdef CONFIG_IO_URING
>>>>     LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
>>>>     LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
>>>> +    LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
>>>> #endif
>>> Tried this on nvme device (/dev/ng0n1).
>>> Took a while to come out of noob setup-related issues but I see that
>>> smack is listed (in /sys/kernel/security/lsm), smackfs is present, and
>>> the hook (smack_uring_cmd) gets triggered fine on doing I/O on
>>> /dev/ng0n1.
>>>
>>> I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which
>>> is set to floor).
>>>
>>> $ chsmack -L /dev/ng0n1
>>> /dev/ng0n1 access="_"
>> Setting the Smack on the object that the cmd operates on to
>> something other than "_" would be the correct test. If that
>> is /dev/ng0n1 you could use
>>
>> 	# chsmack -a Snap /dev/ng0n1
>>
>> The unprivileged user won't be able to read /dev/ng0n1 so you
>> won't get as far as testing the cmd interface. I don't know
>> io_uring and nvme well enough to know what other objects may
>> be involved. Noob here, too.
>>
>>> I ran fio (/usr/bin/fio), which also has the same label.
>>> Hope you expect the same outcome.
>>>
>>> Do you run something else to see that things are fine e.g. for
>>> /dev/null, which also has the same label "_".
>>> If yes, I can try the same on nvme side.
>>>

Re: [PATCH] Smack: Provide read control for io_uring_cmd

[Joel Granados]

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

On Tue, Aug 30, 2022 at 07:16:55AM -0700, Casey Schaufler wrote:
> On 8/30/2022 6:08 AM, Joel Granados wrote:
> > Hey Casey
> >
> > I have tested this patch and I see the smack_uring_cmd prevents access
> > to file when smack labels are different. They way I got there was a bit
> > convoluted and I'll describe it here so ppl can judge how valid the test
> > is.
> >
> > Tested-by: Joel Granados <j.granados@samsung.com>
> 
> Thank you.
np

> 
> >
> > I started by reproducing what Kanchan had done by changing the smack
> > label from "_" to "Snap". Then I ran the io_uring passthrough test
> > included in liburing with an unprivileged user and saw that the
> > smack_uring_cmd function was NOT executed because smack prevented an open on
> > the device file ("/dev/ng0n1" on my box).
> >
> > So here I got a bit sneaky and changed the label after the open to the
> > device was done. This is how I did it:
> > 1. In the io_uring_passthrough.c tests I stopped execution after the
> >    device was open and before the actual IO.
> > 2. While on hold I changed the label of the device from "_" to "Snap".
> >    At this point the device had a "Snap" label and the executable had a
> >    "_" label.
> > 3. Previous to execution I had placed a printk inside the
> >    smack_uring_cmd function. And I also made sure to printk whenever
> >    security_uring_command returned because of a security violation.
> > 4. I continued execution and saw that both smack_uiring_cmd and
> >    io_uring_cmd returned error. Which is what I expected.
> >
> > I'm still a bit unsure of my setup so if anyone has comments or a way to
> > make it better, I would really appreciate feedback.
> 
> This is a perfectly rational approach to the test. Another approach
> would be to add a Smack access rule:
> 
> 	echo "_ Snap r" > /sys/fs/smackfs/load2
> 
> and label the device before the test begins. Step 2 changes from labeling
> the device to removing the access rule:
> 
> 	echo "_ Snap -" > /sys/fs/smackfs/load2
> 
> and you will get the same result. I wouldn't change your test, but I
> would probably add another that does it using the rule change.

Followed your proposal and I could see that it went passed the "file
open: permission denied" error. However it did not execute
smack_uring_cmd as smack prevented execution of an ioctl call [1]. This
is probably because the test that I'm using from liburing does a lot of
things to set things up besides just opening the device.
I tried several strings on /sys/fs/smackfs/load2 but had no
luck at actually arriving to the smack_uring_cmd function.

Here is what I tried:
1. echo "_ Snap r-x---" > /sys/fs/smackfs/load2
   which prevented access but not in smack_uring_cmd

2. echo "_ Snap -wx---" > /sys/fs/smackfs/load2
   This of course prevented me from opening the /dev/ng0n1

3. echo "_ Snap rw----" > /sys/fs/smackfs/load2
   This went through the smack_uring_cmd and allowed the interaction.

[1] : Here is the traceback of where smack prevents execution of the
      ioctl call:

#0  smk_tskacc (tsp=0xffff888107a27300, obj_known=0xffff888105dda540, mode=mode@entry=2, a=a@entry=0xffffc90000c3be80)
    at ../security/smack/smack_access.c:258
#1  0xffffffff8143fbb0 in smk_curacc (obj_known=<optimized out>, mode=mode@entry=2, a=a@entry=0xffffc90000c3be80) at ../security/smack/smack_access.c:278
#2  0xffffffff8143b4e4 in smack_file_ioctl (file=<optimized out>, cmd=3225964097, arg=<optimized out>) at ../security/smack/smack_lsm.c:1539
#3  0xffffffff81411c3f in security_file_ioctl (file=file@entry=0xffff8881038c8b00, cmd=cmd@entry=3225964097, arg=arg@entry=140728408424048)
    at ../security/security.c:1552
#4  0xffffffff8126ca3e in __do_sys_ioctl (arg=140728408424048, cmd=3225964097, fd=3) at ../fs/ioctl.c:864
#5  __se_sys_ioctl (arg=140728408424048, cmd=3225964097, fd=3) at ../fs/ioctl.c:856
#6  __x64_sys_ioctl (regs=<optimized out>) at ../fs/ioctl.c:856
#7  0xffffffff81da0978 in do_syscall_x64 (nr=<optimized out>, regs=0xffffc90000c3bf58) at ../arch/x86/entry/common.c:50
#8  do_syscall_64 (regs=0xffffc90000c3bf58, nr=<optimized out>) at ../arch/x86/entry/common.c:80
#9  0xffffffff81e0009b in entry_SYSCALL_64 () at ../arch/x86/entry/entry_64.S:120
#10 0x00007f9c22ae9000 in ?? ()
#11 0x0000000000000000 in ?? ()


> 
> > Best
> >
> > Joel
> >
> > On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote:
> >> On 8/27/2022 8:59 AM, Kanchan Joshi wrote:
> >>> On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote:
> >>>> Limit io_uring "cmd" options to files for which the caller has
> >>>> Smack read access. There may be cases where the cmd option may
> >>>> be closer to a write access than a read, but there is no way
> >>>> to make that determination.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>> -- 
> >>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >>>> index 001831458fa2..bffccdc494cb 100644
> >>>> --- a/security/smack/smack_lsm.c
> >>>> +++ b/security/smack/smack_lsm.c
> >>>> @@ -42,6 +42,7 @@
> >>>> #include <linux/fs_context.h>
> >>>> #include <linux/fs_parser.h>
> >>>> #include <linux/watch_queue.h>
> >>>> +#include <linux/io_uring.h>
> >>>> #include "smack.h"
> >>>>
> >>>> #define TRANS_TRUE    "TRUE"
> >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void)
> >>>>     return -EPERM;
> >>>> }
> >>>>
> >>>> +/**
> >>>> + * smack_uring_cmd - check on file operations for io_uring
> >>>> + * @ioucmd: the command in question
> >>>> + *
> >>>> + * Make a best guess about whether a io_uring "command" should
> >>>> + * be allowed. Use the same logic used for determining if the
> >>>> + * file could be opened for read in the absence of better criteria.
> >>>> + */
> >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
> >>>> +{
> >>>> +    struct file *file = ioucmd->file;
> >>>> +    struct smk_audit_info ad;
> >>>> +    struct task_smack *tsp;
> >>>> +    struct inode *inode;
> >>>> +    int rc;
> >>>> +
> >>>> +    if (!file)
> >>>> +        return -EINVAL;
> >>>> +
> >>>> +    tsp = smack_cred(file->f_cred);
> >>>> +    inode = file_inode(file);
> >>>> +
> >>>> +    smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
> >>>> +    smk_ad_setfield_u_fs_path(&ad, file->f_path);
> >>>> +    rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
> >>>> +    rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
> >>>> +
> >>>> +    return rc;
> >>>> +}
> >>>> +
> >>>> #endif /* CONFIG_IO_URING */
> >>>>
> >>>> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> >>>> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[]
> >>>> __lsm_ro_after_init = {
> >>>> #ifdef CONFIG_IO_URING
> >>>>     LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
> >>>>     LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
> >>>> +    LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
> >>>> #endif
> >>> Tried this on nvme device (/dev/ng0n1).
> >>> Took a while to come out of noob setup-related issues but I see that
> >>> smack is listed (in /sys/kernel/security/lsm), smackfs is present, and
> >>> the hook (smack_uring_cmd) gets triggered fine on doing I/O on
> >>> /dev/ng0n1.
> >>>
> >>> I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which
> >>> is set to floor).
> >>>
> >>> $ chsmack -L /dev/ng0n1
> >>> /dev/ng0n1 access="_"
> >> Setting the Smack on the object that the cmd operates on to
> >> something other than "_" would be the correct test. If that
> >> is /dev/ng0n1 you could use
> >>
> >> 	# chsmack -a Snap /dev/ng0n1
> >>
> >> The unprivileged user won't be able to read /dev/ng0n1 so you
> >> won't get as far as testing the cmd interface. I don't know
> >> io_uring and nvme well enough to know what other objects may
> >> be involved. Noob here, too.
> >>
> >>> I ran fio (/usr/bin/fio), which also has the same label.
> >>> Hope you expect the same outcome.
> >>>
> >>> Do you run something else to see that things are fine e.g. for
> >>> /dev/null, which also has the same label "_".
> >>> If yes, I can try the same on nvme side.
> >>>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]