[RFC 0/2] nvme : Add whitelist for admin commands in passthru

[RFC 0/2] nvme : Add whitelist for admin commands in passthru

[Joel Granados]

What?
In this patch set we implement a dynamic whitelist for admin opcodes that
will allow the privileged user to define what opcodes can be used by the
unprivileged user in the passthru path. Applications will not only be
restricted by the whitelist, but will also need write permissions for the
device.

There are questions at the end of the cover letter for people to chime in.
I'll take these comments and hopefully send a V1 soon after.

Why?
Applications with write permissions should not need to be privileged to
write to the device. With Kanchan's latest patch
(https://lore.kernel.org/linux-nvme/20220927183620.12583-1-joshi.k@samsung.com/)
the nvme IO commands in passthru now follow device permissions however
privileged execution is still needed for admin commands like identify that
usually come before the actual write.  This patchset removes this requirement.

We can't foresee what subset of the existing admin commands will be used
nor what new ones will be added to future versions of the nvme
specification. Therefore we go with a dynamic whitelist instead of a
hardcoded one.

How?
We added an ioctl (NVME_IOCTL_PTHRU_WLIST) that controls adding, removing
and testing admin opcode to the whitelist for the passthru path. It can
only be used by privileged users (CAP_SYS_ADMIN).  Given that the nvme
identify opcode is usually needed to generate IO, we add it to the
whitelist by default.

I have rebased this on top of Kanchans "nvme: fine-granular CAP_SYS_ADMIN
for nvme io commands"
(https://lore.kernel.org/linux-nvme/20220927183620.12583-1-joshi.k@samsung.com/)
because this only makes sense if we can also do IO as unprivileged.

Questions:
1. I initialize the whitelist at the end of nvme_core_init. I put it there
   as I saw that that is where the module specific stuff what being
   initialized. Is there another function that is better?

2. Scope of the whitelist is the driver. There is only one whitelist for
   all the nvme devices. You can further control which devices use the
   whitelist with device file permissions. All devices with WRITE
   permissions are able to execute all whitelisted opcods. Any comments?

3. I went with the ioctl name of NVME_IOCTL_PTRHU_WLIST. Are there
   alternatives?

4. I have left the nvme_admin_identify cmd active by default as it is the
   one that I think would be used for simple most IO commands. Comments?

Joel Granados (2):
  nvme : Add dynamic whitelisting for passthru
  nvme : Add ioctls for passthru admin whitelisting

 drivers/nvme/host/core.c        | 10 ++++++++
 drivers/nvme/host/ioctl.c       | 43 +++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h        |  1 +
 include/linux/nvme.h            |  1 +
 include/uapi/linux/nvme_ioctl.h | 14 +++++++++++
 5 files changed, 67 insertions(+), 2 deletions(-)

-- 
2.30.2

[RFC 1/2] nvme : Add dynamic whitelisting for passthru

[Joel Granados]

Drive information such as block size and total usable LBAs are needed to
send IO down the nvme passthru path. Currently only privileged users can
get these parameters.  This patch implements a dynamic whitelist that
allows the privileged user to define what nvme opcodes are available for
the unprivileged.

A bitmap is added at the nvme driver level that controls what opcodes are
allowed. The unprivileged user will be able to execute an nvme opcode
(using passthru) when it is whitelisted and if mode matches FMODE_WRITE.

This contains only the whitelist implementation and is a preparation commit
for the ioctl calls.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/nvme/host/core.c  | 10 ++++++++++
 drivers/nvme/host/ioctl.c | 11 +++++++++--
 drivers/nvme/host/nvme.h  |  1 +
 include/linux/nvme.h      |  1 +
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 13080a017ecf..05d1e6fd633d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -107,6 +107,8 @@ EXPORT_SYMBOL_GPL(nvme_reset_wq);
 struct workqueue_struct *nvme_delete_wq;
 EXPORT_SYMBOL_GPL(nvme_delete_wq);
 
+DECLARE_BITMAP(nvme_admin_whitelist, nvme_admin_last);
+
 static LIST_HEAD(nvme_subsystems);
 static DEFINE_MUTEX(nvme_subsystems_lock);
 
@@ -125,6 +127,12 @@ static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
 static void nvme_update_keep_alive(struct nvme_ctrl *ctrl,
 				   struct nvme_command *cmd);
 
+static void nvme_init_admin_whitelist(void)
+{
+	bitmap_zero(nvme_admin_whitelist, nvme_admin_last);
+	__set_bit(nvme_admin_identify, nvme_admin_whitelist);
+}
+
 void nvme_queue_scan(struct nvme_ctrl *ctrl)
 {
 	/*
@@ -5231,6 +5239,8 @@ static int __init nvme_core_init(void)
 		goto unregister_generic_ns;
 	}
 
+	nvme_init_admin_whitelist();
+
 	return 0;
 
 unregister_generic_ns:
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index ebe04e977baa..73e4287d2c44 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -20,6 +20,14 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
 	return (void __user *)ptrval;
 }
 
+
+bool nvme_admin_cmd_allowed(u8 opcode, fmode_t mode)
+{
+	if (test_bit(opcode, nvme_admin_whitelist))
+		return (mode & FMODE_WRITE);
+	return false;
+}
+
 bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode)
 {
 	u8 opcode = c->common.opcode;
@@ -27,9 +35,8 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode)
 	if (capable(CAP_SYS_ADMIN))
 		return true;
 
-	/* admin commands are not allowed */
 	if (ns == NULL)
-		return false;
+		return nvme_admin_cmd_allowed(opcode, mode);
 
 	/* exclude vendor-specific io and fabrics commands */
 	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 216acbe953b3..18a55e3483bd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -46,6 +46,7 @@ extern unsigned int admin_timeout;
 extern struct workqueue_struct *nvme_wq;
 extern struct workqueue_struct *nvme_reset_wq;
 extern struct workqueue_struct *nvme_delete_wq;
+extern unsigned long nvme_admin_whitelist[];
 
 /*
  * List of workarounds for devices that required behavior not specified in
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 8396eb7ecb68..18a75496299c 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1123,6 +1123,7 @@ enum nvme_admin_opcode {
 	nvme_admin_sanitize_nvm		= 0x84,
 	nvme_admin_get_lba_status	= 0x86,
 	nvme_admin_vendor_start		= 0xC0,
+	nvme_admin_last,
 };
 
 #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
-- 
2.30.2

[RFC 2/2] nvme : Add ioctls for passthru admin whitelisting

[Joel Granados]

An ioctl (NVME_IOCTL_PTHRU_WLIST) is added in order to control the passthu
admin opcode whiltelist. A new struct (nvme_pthru_wlist) is used to pass
an action (Add, Remove, Test), opcode number and result data between user
and kernel space.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/nvme/host/ioctl.c       | 32 ++++++++++++++++++++++++++++++++
 include/uapi/linux/nvme_ioctl.h | 14 ++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 73e4287d2c44..76393d50a798 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -50,6 +50,36 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode)
 	return true;
 }
 
+static int nvme_admin_cmd_whitelist(struct nvme_pthru_wlist __user *u_op)
+{
+	__u8 opcode;
+	struct nvme_pthru_wlist nvme_pthru_wlist = {0};
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	if (copy_from_user(&nvme_pthru_wlist, u_op, sizeof(struct nvme_pthru_wlist)))
+		return -EFAULT;
+	opcode = nvme_pthru_wlist.opcode;
+	if (opcode >= nvme_admin_last || opcode < 0)
+		return -EINVAL;
+
+	switch (nvme_pthru_wlist.action) {
+	case NVME_PTHRU_WLIST_ADD:
+		__set_bit(opcode, nvme_admin_whitelist);
+		return 0;
+	case NVME_PTHRU_WLIST_REM:
+		__clear_bit(opcode, nvme_admin_whitelist);
+		return 0;
+	case NVME_PTHRU_WLIST_TEST:
+		if (put_user(test_bit(opcode, nvme_admin_whitelist),
+			     &u_op->result))
+			return -EFAULT;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 		unsigned len, u32 seed, bool write)
 {
@@ -586,6 +616,8 @@ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
 		return nvme_user_cmd64(ns->ctrl, ns, argp, false, mode);
 	case NVME_IOCTL_IO64_CMD_VEC:
 		return nvme_user_cmd64(ns->ctrl, ns, argp, true, mode);
+	case NVME_IOCTL_PTHRU_WLIST:
+		return nvme_admin_cmd_whitelist(argp);
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index 2f76cba67166..748ce70517ba 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -45,6 +45,19 @@ struct nvme_passthru_cmd {
 	__u32	result;
 };
 
+enum nvme_pthru_wlist_action {
+	NVME_PTHRU_WLIST_ADD,
+	NVME_PTHRU_WLIST_REM,
+	NVME_PTHRU_WLIST_TEST
+};
+
+struct nvme_pthru_wlist {
+	__u8				opcode;
+	__u8				flags;
+	__u32				result;
+	enum nvme_pthru_wlist_action	action;
+};
+
 struct nvme_passthru_cmd64 {
 	__u8	opcode;
 	__u8	flags;
@@ -104,6 +117,7 @@ struct nvme_uring_cmd {
 #define NVME_IOCTL_ADMIN64_CMD	_IOWR('N', 0x47, struct nvme_passthru_cmd64)
 #define NVME_IOCTL_IO64_CMD	_IOWR('N', 0x48, struct nvme_passthru_cmd64)
 #define NVME_IOCTL_IO64_CMD_VEC	_IOWR('N', 0x49, struct nvme_passthru_cmd64)
+#define NVME_IOCTL_PTHRU_WLIST	_IOWR('N', 0x50, struct nvme_pthru_wlist)
 
 /* io_uring async commands: */
 #define NVME_URING_CMD_IO	_IOWR('N', 0x80, struct nvme_uring_cmd)
-- 
2.30.2

Re: [RFC 0/2] nvme : Add whitelist for admin commands in passthru

[Keith Busch]

Just fyi, Documentation/process/coding-style.rst recommends against
using the term "whitelist" and has alternative suggestions.

I'll try to make time to provide more techincal feedback on the patch in
the next week, though I am travelling for much of it, so no promises.

Re: [RFC 0/2] nvme : Add whitelist for admin commands in passthru

[Joel Granados]

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

Hey Keith

Thx for the initial feedback.

On Fri, Oct 07, 2022 at 12:26:52PM -0600, Keith Busch wrote:
> Just fyi, Documentation/process/coding-style.rst recommends against
> using the term "whitelist" and has alternative suggestions.
I was not aware of this. Maybe allowlist is what fits more in this case.

> 
> I'll try to make time to provide more techincal feedback on the patch in
> the next week, though I am travelling for much of it, so no promises.
looking forward to it

Best

Joel


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