[PATCH 0/2] Granular CAP_SYS_ADMIN

[PATCH 0/2] Granular CAP_SYS_ADMIN

[Kanchan Joshi]

Hi,

Patch 1 is for io-commands. It implements the shift to file-mode based
policy.
Patch 2 is to allow identify-namespace command. This is based on the feedback
received during ALPSS.

@Sagi: Since patch 1 is changed a bit (changelog below), I did not
apply the reviewed-by tag. Please take a look again.

Changes since v2:
- Add patch 2 that allows identify-ns
- Patch 1: Move nvme_cmd_allowed check further down, so that we can use CNS
  values for decision making in patch 2
- Patch 1: invert if condition (Sagi)

Changes since v1:
- Move nvme_cmd_allowed check at a place that allows using nvme_is_write
  helper (hch)
- Keep everything into single patch (chaitanya, hch)
- Comments cleanup (hch, chaitanya)
- Part of cover-letter moved to commit-description

Examples (after patches):
***************************

#1: Two NS, one with 666 another with 600
$: ls -l /dev/ng*
crw-rw-rw- 1 root root 242, 0 Oct 20 12:04 /dev/ng0n1
crw------- 1 root root 242, 1 Oct 20 12:04 /dev/ng0n2

#2: this should fail
$: nvme id-ns /dev/ng0n2
/dev/ng0n2: Permission denied
Usage: nvme id-ns <device> [OPTIONS]

Send an Identify Namespace command to the given device, returns properties of
the specified namespace in either human-readable or binary format. Can also
return binary vendor-specific namespace attributes.

Options:
  [  --namespace-id=<NUM>, -n <NUM> ]   --- identifier of desired namespace
  [  --force ]                          --- Return this namespace, even if
                                            not attaced (1.2 devices only)
  [  --vendor-specific, -v ]            --- dump binary vendor fields
  [  --raw-binary, -b ]                 --- show identify in binary format
  [  --output-format=<FMT>, -o <FMT> ]  --- Output format: normal|json|binary
  [  --human-readable, -H ]             --- show identify in readable format

#3: this should travel
$: nvme id-ns /dev/ng0n1
NVME Identify Namespace 1:
nsze    : 0x300000
ncap    : 0x300000
nuse    : 0x300000
nsfeat  : 0
nlbaf   : 7
flbas   : 0x4
mc      : 0
dpc     : 0
dps     : 0
nmic    : 0
rescap  : 0
fpi     : 0
dlfeat  : 9
nawun   : 0
nawupf  : 0
nacwu   : 0
nabsn   : 0
nabo    : 0
nabspf  : 0
noiob   : 0
nvmcap  : 0
mssrl   : 256
mcl     : 256
msrc    : 127
nulbaf  : 0
anagrpid: 0
nsattr  : 0
nvmsetid: 0
endgid  : 0
nguid   : 00000000000000000000000000000000
eui64   : 0000000000000000
lbaf  0 : ms:0   lbads:9  rp:0
lbaf  1 : ms:8   lbads:9  rp:0
lbaf  2 : ms:16  lbads:9  rp:0
lbaf  3 : ms:64  lbads:9  rp:0
lbaf  4 : ms:0   lbads:12 rp:0 (in use)
lbaf  5 : ms:8   lbads:12 rp:0
lbaf  6 : ms:16  lbads:12 rp:0
lbaf  7 : ms:64  lbads:12 rp:0

#4: this should not travel
$: nvme id-ctrl /dev/ng0n1
identify controller: Permission denied

#5: uring-passthru read on ng0n1 (should work)
$: ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -cmd_type=nvme -bs=4k -numjobs=1 -size=4k -filename=/dev/ng0n1 -name=pt
pt: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
fio-3.32-58-gb19c-dirty
Starting 1 process

pt: (groupid=0, jobs=1): err= 0: pid=56582: Thu Oct 20 12:12:50 2022
  read: IOPS=500, BW=2000KiB/s (2048kB/s)(4096B/2msec)
    slat (nsec): min=461505, max=461505, avg=461505.00, stdev= 0.00
    clat (nsec): min=544742, max=544742, avg=544742.00, stdev= 0.00
     lat (nsec): min=1006.2k, max=1006.2k, avg=1006247.00, stdev= 0.00
    clat percentiles (usec):
     |  1.00th=[  545],  5.00th=[  545], 10.00th=[  545], 20.00th=[  545],
     | 30.00th=[  545], 40.00th=[  545], 50.00th=[  545], 60.00th=[  545],
     | 70.00th=[  545], 80.00th=[  545], 90.00th=[  545], 95.00th=[  545],
     | 99.00th=[  545], 99.50th=[  545], 99.90th=[  545], 99.95th=[  545],
     | 99.99th=[  545]
  lat (usec)   : 750=100.00%
  cpu          : usr=0.00%, sys=200.00%, ctx=1, majf=0, minf=6
  IO depths    : 1=100.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.0%, 32=0.0%, >=64=0.0%
     submit    : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
     complete  : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
     issued rwts: total=1,0,0,0 short=0,0,0,0 dropped=0,0,0,0
     latency   : target=0, window=0, percentile=100.00%, depth=1

Run status group 0 (all jobs):
   READ: bw=2000KiB/s (2048kB/s), 2000KiB/s-2000KiB/s (2048kB/s-2048kB/s), io=4096B (4096B), run=2-2msec

#6: uring-passthru read on ng0n2 (should fail)
$: ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -cmd_type=nvme -bs=4k -numjobs=1 -size=4k -filename=/dev/ng0n2 -name=pt
pt: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
fio-3.32-58-gb19c-dirty
Starting 1 process



Run status group 0 (all jobs):


Kanchan Joshi (2):
  nvme: fine-granular CAP_SYS_ADMIN for nvme io commands
  nvme: identify-namespace without CAP_SYS_ADMIN

 drivers/nvme/host/ioctl.c | 107 ++++++++++++++++++++++++++------------
 include/linux/nvme.h      |   1 +
 2 files changed, 75 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH 1/2] nvme: fine-granular CAP_SYS_ADMIN for nvme io commands

[Kanchan Joshi]

Currently both io and admin commands are kept under a
coarse-granular CAP_SYS_ADMIN check, disregarding file mode completely.

$ ls -l /dev/ng*
crw-rw-rw- 1 root root 242, 0 Sep  9 19:20 /dev/ng0n1
crw------- 1 root root 242, 1 Sep  9 19:20 /dev/ng0n2

In the example above, ng0n1 appears as if it may allow unprivileged
read/write operation but it does not and behaves same as ng0n2.

This patch implements a shift from CAP_SYS_ADMIN to more fine-granular
control for io-commands.
If CAP_SYS_ADMIN is present, nothing else is checked as before.
Otherwise, following rules are in place
- any admin-cmd is not allowed
- vendor-specific and fabric commmand are not allowed
- io-commands that can write are allowed if matching FMODE_WRITE
permission is present
- io-commands that read are allowed

Add a helper nvme_cmd_allowed that implements above policy.
Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
for any decision making.
Since file open mode is counted for any approval/denial, change at
various places to keep file-mode information handy.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 97 ++++++++++++++++++++++++++-------------
 include/linux/nvme.h      |  1 +
 2 files changed, 65 insertions(+), 33 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 81f5550b670d..9c581b1a8956 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -8,6 +8,29 @@
 #include <linux/io_uring.h>
 #include "nvme.h"
 
+bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode)
+{
+	u8 opcode = c->common.opcode;
+
+	if (capable(CAP_SYS_ADMIN))
+		return true;
+
+	/* admin commands are not allowed */
+	if (!ns)
+		return false;
+
+	/* exclude vendor-specific io and fabrics commands */
+	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
+		return false;
+
+	/* allow write cmds only if matching FMODE is present */
+	if (nvme_is_write(c))
+		return mode & FMODE_WRITE;
+
+	/* allow read cmds as the device permission allows access */
+	return true;
+}
+
 /*
  * Convert integer values from ioctl structures to user pointers, silently
  * ignoring the upper bits in the compat case to match behaviour of 32-bit
@@ -261,7 +284,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
 }
 
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-			struct nvme_passthru_cmd __user *ucmd)
+			struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
 {
 	struct nvme_passthru_cmd cmd;
 	struct nvme_command c;
@@ -269,8 +292,6 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	u64 result;
 	int status;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EACCES;
 	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
 		return -EFAULT;
 	if (cmd.flags)
@@ -291,6 +312,9 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
+	if (!nvme_cmd_allowed(ns, &c, mode))
+		return -EACCES;
+
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
@@ -308,15 +332,14 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 }
 
 static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-			struct nvme_passthru_cmd64 __user *ucmd, bool vec)
+			struct nvme_passthru_cmd64 __user *ucmd, bool vec,
+			fmode_t mode)
 {
 	struct nvme_passthru_cmd64 cmd;
 	struct nvme_command c;
 	unsigned timeout = 0;
 	int status;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EACCES;
 	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
 		return -EFAULT;
 	if (cmd.flags)
@@ -337,6 +360,9 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
+	if (!nvme_cmd_allowed(ns, &c, mode))
+		return -EACCES;
+
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
@@ -483,9 +509,6 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	void *meta = NULL;
 	int ret;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EACCES;
-
 	c.common.opcode = READ_ONCE(cmd->opcode);
 	c.common.flags = READ_ONCE(cmd->flags);
 	if (c.common.flags)
@@ -507,6 +530,9 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(READ_ONCE(cmd->cdw14));
 	c.common.cdw15 = cpu_to_le32(READ_ONCE(cmd->cdw15));
 
+	if (!nvme_cmd_allowed(ns, &c, ioucmd->file->f_mode))
+		return -EACCES;
+
 	d.metadata = READ_ONCE(cmd->metadata);
 	d.addr = READ_ONCE(cmd->addr);
 	d.data_len = READ_ONCE(cmd->data_len);
@@ -570,13 +596,13 @@ static bool is_ctrl_ioctl(unsigned int cmd)
 }
 
 static int nvme_ctrl_ioctl(struct nvme_ctrl *ctrl, unsigned int cmd,
-		void __user *argp)
+		void __user *argp, fmode_t mode)
 {
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
-		return nvme_user_cmd(ctrl, NULL, argp);
+		return nvme_user_cmd(ctrl, NULL, argp, mode);
 	case NVME_IOCTL_ADMIN64_CMD:
-		return nvme_user_cmd64(ctrl, NULL, argp, false);
+		return nvme_user_cmd64(ctrl, NULL, argp, false, mode);
 	default:
 		return sed_ioctl(ctrl->opal_dev, cmd, argp);
 	}
@@ -601,14 +627,14 @@ struct nvme_user_io32 {
 #endif /* COMPAT_FOR_U64_ALIGNMENT */
 
 static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
-		void __user *argp)
+		void __user *argp, fmode_t mode)
 {
 	switch (cmd) {
 	case NVME_IOCTL_ID:
 		force_successful_syscall_return();
 		return ns->head->ns_id;
 	case NVME_IOCTL_IO_CMD:
-		return nvme_user_cmd(ns->ctrl, ns, argp);
+		return nvme_user_cmd(ns->ctrl, ns, argp, mode);
 	/*
 	 * struct nvme_user_io can have different padding on some 32-bit ABIs.
 	 * Just accept the compat version as all fields that are used are the
@@ -620,19 +646,20 @@ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
 	case NVME_IOCTL_SUBMIT_IO:
 		return nvme_submit_io(ns, argp);
 	case NVME_IOCTL_IO64_CMD:
-		return nvme_user_cmd64(ns->ctrl, ns, argp, false);
+		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);
+		return nvme_user_cmd64(ns->ctrl, ns, argp, true, mode);
 	default:
 		return -ENOTTY;
 	}
 }
 
-static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg)
+static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg,
+			fmode_t mode)
 {
-       if (is_ctrl_ioctl(cmd))
-               return nvme_ctrl_ioctl(ns->ctrl, cmd, arg);
-       return nvme_ns_ioctl(ns, cmd, arg);
+	if (is_ctrl_ioctl(cmd))
+		return nvme_ctrl_ioctl(ns->ctrl, cmd, arg, mode);
+	return nvme_ns_ioctl(ns, cmd, arg, mode);
 }
 
 int nvme_ioctl(struct block_device *bdev, fmode_t mode,
@@ -640,7 +667,7 @@ int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 {
 	struct nvme_ns *ns = bdev->bd_disk->private_data;
 
-	return __nvme_ioctl(ns, cmd, (void __user *)arg);
+	return __nvme_ioctl(ns, cmd, (void __user *)arg, mode);
 }
 
 long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -648,7 +675,7 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct nvme_ns *ns =
 		container_of(file_inode(file)->i_cdev, struct nvme_ns, cdev);
 
-	return __nvme_ioctl(ns, cmd, (void __user *)arg);
+	return __nvme_ioctl(ns, cmd, (void __user *)arg, file->f_mode);
 }
 
 static int nvme_uring_cmd_checks(unsigned int issue_flags)
@@ -716,7 +743,8 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
 }
 #ifdef CONFIG_NVME_MULTIPATH
 static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
-		void __user *argp, struct nvme_ns_head *head, int srcu_idx)
+		void __user *argp, struct nvme_ns_head *head, int srcu_idx,
+		fmode_t mode)
 	__releases(&head->srcu)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
@@ -724,7 +752,7 @@ static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
 
 	nvme_get_ctrl(ns->ctrl);
 	srcu_read_unlock(&head->srcu, srcu_idx);
-	ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp);
+	ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp, mode);
 
 	nvme_put_ctrl(ctrl);
 	return ret;
@@ -749,9 +777,10 @@ int nvme_ns_head_ioctl(struct block_device *bdev, fmode_t mode,
 	 * deadlock when deleting namespaces using the passthrough interface.
 	 */
 	if (is_ctrl_ioctl(cmd))
-		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
+		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
+					mode);
 
-	ret = nvme_ns_ioctl(ns, cmd, argp);
+	ret = nvme_ns_ioctl(ns, cmd, argp, mode);
 out_unlock:
 	srcu_read_unlock(&head->srcu, srcu_idx);
 	return ret;
@@ -773,9 +802,10 @@ long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
 		goto out_unlock;
 
 	if (is_ctrl_ioctl(cmd))
-		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
+		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
+				file->f_mode);
 
-	ret = nvme_ns_ioctl(ns, cmd, argp);
+	ret = nvme_ns_ioctl(ns, cmd, argp, file->f_mode);
 out_unlock:
 	srcu_read_unlock(&head->srcu, srcu_idx);
 	return ret;
@@ -849,7 +879,8 @@ int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
 	return ret;
 }
 
-static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
+static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp,
+		fmode_t mode)
 {
 	struct nvme_ns *ns;
 	int ret;
@@ -873,7 +904,7 @@ static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
 	kref_get(&ns->kref);
 	up_read(&ctrl->namespaces_rwsem);
 
-	ret = nvme_user_cmd(ctrl, ns, argp);
+	ret = nvme_user_cmd(ctrl, ns, argp, mode);
 	nvme_put_ns(ns);
 	return ret;
 
@@ -890,11 +921,11 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
-		return nvme_user_cmd(ctrl, NULL, argp);
+		return nvme_user_cmd(ctrl, NULL, argp, file->f_mode);
 	case NVME_IOCTL_ADMIN64_CMD:
-		return nvme_user_cmd64(ctrl, NULL, argp, false);
+		return nvme_user_cmd64(ctrl, NULL, argp, false, file->f_mode);
 	case NVME_IOCTL_IO_CMD:
-		return nvme_dev_user_cmd(ctrl, argp);
+		return nvme_dev_user_cmd(ctrl, argp, file->f_mode);
 	case NVME_IOCTL_RESET:
 		if (!capable(CAP_SYS_ADMIN))
 			return -EACCES;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 050d7d0cd81b..1d102b662e88 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -797,6 +797,7 @@ enum nvme_opcode {
 	nvme_cmd_zone_mgmt_send	= 0x79,
 	nvme_cmd_zone_mgmt_recv	= 0x7a,
 	nvme_cmd_zone_append	= 0x7d,
+	nvme_cmd_vendor_start	= 0x80,
 };
 
 #define nvme_opcode_name(opcode)	{ opcode, #opcode }
-- 
2.25.1

[PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN

[Kanchan Joshi]

Allow all identify-namespace variants (CNS 00h, 05h and 08h) without
requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is
needed to form IO commands for passthrough interface.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9c581b1a8956..9273db147872 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -15,9 +15,19 @@ 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)
+	/* policy for admin commands */
+	if (!ns) {
+		if (opcode == nvme_admin_identify) {
+			switch (c->identify.cns) {
+			case NVME_ID_CNS_NS:
+			case NVME_ID_CNS_CS_NS:
+			case NVME_ID_CNS_NS_CS_INDEP:
+				return true;
+			}
+		}
+		/* other admin commands are not allowed */
 		return false;
+	}
 
 	/* exclude vendor-specific io and fabrics commands */
 	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
-- 
2.25.1

Re: [PATCH 0/2] Granular CAP_SYS_ADMIN

[Jens Axboe]

On 10/20/22 1:02 AM, Kanchan Joshi wrote:
> Hi,
> 
> Patch 1 is for io-commands. It implements the shift to file-mode based
> policy.
> Patch 2 is to allow identify-namespace command. This is based on the feedback
> received during ALPSS.
> 
> @Sagi: Since patch 1 is changed a bit (changelog below), I did not
> apply the reviewed-by tag. Please take a look again.

I generally despise any kind of listing of commands, but this looks
pretty reasonable.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH 0/2] Granular CAP_SYS_ADMIN

[Keith Busch]

On Thu, Oct 20, 2022 at 12:32:03PM +0530, Kanchan Joshi wrote:
> #1: Two NS, one with 666 another with 600
> $: ls -l /dev/ng*
> crw-rw-rw- 1 root root 242, 0 Oct 20 12:04 /dev/ng0n1
> crw------- 1 root root 242, 1 Oct 20 12:04 /dev/ng0n2
> 
> #2: this should fail
> $: nvme id-ns /dev/ng0n2
> /dev/ng0n2: Permission denied

This looks good to me. The only additional thought I had was to restrict
the identification's nsid parameter to ones readable to the user because
they could just open /dev/ng0n1 instead and override the nsid with the
'--nampespace-id=2' parameter.

But that doesn't look very straight now that I look at it again.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN

[Chaitanya Kulkarni]

On 10/20/22 00:02, Kanchan Joshi wrote:
> Allow all identify-namespace variants (CNS 00h, 05h and 08h) without
> requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is
> needed to form IO commands for passthrough interface.
> 
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
>   drivers/nvme/host/ioctl.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 9c581b1a8956..9273db147872 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -15,9 +15,19 @@ 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)
> +	/* policy for admin commands */

above comment is not needed as it is clear from the opcode below
you are dealing with admin commands only that too specific cns
values ..

> +	if (!ns) {
> +		if (opcode == nvme_admin_identify) {
> +			switch (c->identify.cns) {
> +			case NVME_ID_CNS_NS:
> +			case NVME_ID_CNS_CS_NS:
> +			case NVME_ID_CNS_NS_CS_INDEP:
> +				return true;
> +			}
> +		}
> +		/* other admin commands are not allowed */

same here..

>   		return false;

if and swicth and two returns are looking confusing, I'd use
nested switch case default here..

> +	}
>   
>   	/* exclude vendor-specific io and fabrics commands */
>   	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)


-ck

Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN

[Kanchan Joshi]

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

On Mon, Oct 31, 2022 at 06:55:56AM +0000, Chaitanya Kulkarni wrote:
>On 10/20/22 00:02, Kanchan Joshi wrote:
>> Allow all identify-namespace variants (CNS 00h, 05h and 08h) without
>> requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is
>> needed to form IO commands for passthrough interface.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>>   drivers/nvme/host/ioctl.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>> index 9c581b1a8956..9273db147872 100644
>> --- a/drivers/nvme/host/ioctl.c
>> +++ b/drivers/nvme/host/ioctl.c
>> @@ -15,9 +15,19 @@ 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)
>> +	/* policy for admin commands */
>
>above comment is not needed as it is clear from the opcode below
>you are dealing with admin commands only that too specific cns
>values ..
>
>> +	if (!ns) {
>> +		if (opcode == nvme_admin_identify) {
>> +			switch (c->identify.cns) {
>> +			case NVME_ID_CNS_NS:
>> +			case NVME_ID_CNS_CS_NS:
>> +			case NVME_ID_CNS_NS_CS_INDEP:
>> +				return true;
>> +			}
>> +		}
>> +		/* other admin commands are not allowed */
>
>same here..

All right, will kill these. 
>
>>   		return false;
>
>if and swicth and two returns are looking confusing, I'd use
>nested switch case default here..

Do you think that'll give better looking code?
I did not write that because it did not seem good fit for the
situtation. It involved aligning more curly braces: 

-       /* admin commands are not allowed */
-       if (!ns)
+       if (!ns) {
+               switch (opcode) {
+               case nvme_admin_identify: {
+                       switch (c->identify.cns) {
+                       case NVME_ID_CNS_NS:
+                       case NVME_ID_CNS_CS_NS:
+                       case NVME_ID_CNS_NS_CS_INDEP:
+                               return true;
+                       }
+               }
+               }
                return false;
+       }

Above is without default. And with two defaults, it just gets more
wordy.
And future growth in above admin opcodes is not expected too.

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

Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN

[Chaitanya Kulkarni]

> Do you think that'll give better looking code?
> I did not write that because it did not seem good fit for the
> situtation. It involved aligning more curly braces:
> -       /* admin commands are not allowed */
> -       if (!ns)
> +       if (!ns) {
> +               switch (opcode) {
> +               case nvme_admin_identify: {
> +                       switch (c->identify.cns) {
> +                       case NVME_ID_CNS_NS:
> +                       case NVME_ID_CNS_CS_NS:
> +                       case NVME_ID_CNS_NS_CS_INDEP:
> +                               return true;
> +                       }
> +               }
> +               }
>                 return false;
> +       }
> 

this is exact pattern we have used in the target when it comes to
nvme_admin_identify handling, so it makes sense to keep the same,
unless there is any strong objection..

> Above is without default. And with two defaults, it just gets more
> wordy.
> And future growth in above admin opcodes is not expected too.
> 

-ck