only allow unprivileged passthrough for commands without effects v3

only allow unprivileged passthrough for commands without effects v3

[Christoph Hellwig]

Hi all,

this series first fixes a few minor issues in the CES log support in the
host and target drivers and then uses the log to deny unprivileged
passthrough of commands that have effects, where the only practically
relevant effect is the modification of contents of the data stored in the
namespace.

Changes since v2:
 - drop various cleanups and aim for a minimum viable fix for 6.2
 - fix the NVME_CMD_EFFECTS_CSE_MASK definition
 - don't allow unprivilged passthrough without a Commands Supported and
   Effects log

Changes since v1:
 - make sure ctrl->effects an nshead->effects is always available
 - initializse known effects at init time
 - remove the use_workqueue field in nvmet_req
 - fix up a commit message

Diffstat:
 drivers/nvme/host/core.c        |   25 +++++++++++++++++++------
 drivers/nvme/host/ioctl.c       |   28 ++++++++++++++++++++++++----
 drivers/nvme/target/admin-cmd.c |   37 +++++++++++++++++++++----------------
 drivers/nvme/target/passthru.c  |   11 +++++------
 include/linux/nvme.h            |    4 +++-
 5 files changed, 72 insertions(+), 33 deletions(-)

[PATCH 1/6] nvme: fix the NVME_CMD_EFFECTS_CSE_MASK definition

[Christoph Hellwig]

3 << 16 does not generate the correct mask for bits 16, 17 and 18.
Use the GENMASK macro to generate the correct mask instead.

Fixes: 84fef62d135b ("nvme: check admin passthru command effects")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/nvme.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d6be2a6861000e..d1cd53f2b6abd9 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -7,6 +7,7 @@
 #ifndef _LINUX_NVME_H
 #define _LINUX_NVME_H
 
+#include <linux/bits.h>
 #include <linux/types.h>
 #include <linux/uuid.h>
 
@@ -639,7 +640,7 @@ enum {
 	NVME_CMD_EFFECTS_NCC		= 1 << 2,
 	NVME_CMD_EFFECTS_NIC		= 1 << 3,
 	NVME_CMD_EFFECTS_CCC		= 1 << 4,
-	NVME_CMD_EFFECTS_CSE_MASK	= 3 << 16,
+	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
 	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
 };
 
-- 
2.35.1

[PATCH 2/6] nvmet: use NVME_CMD_EFFECTS_CSUPP instead of open coding it

[Christoph Hellwig]

Use NVME_CMD_EFFECTS_CSUPP instead of open coding it and assign a
single value to multiple array entries instead of repeated assignments.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 35 ++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 53a004ea320c1f..111a5cb6403fb0 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -164,26 +164,29 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 
 static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
 {
-	log->acs[nvme_admin_get_log_page]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_identify]		= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_abort_cmd]		= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_set_features]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_get_features]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_async_event]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_keep_alive]		= cpu_to_le32(1 << 0);
-
-	log->iocs[nvme_cmd_read]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_write]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_flush]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_dsm]			= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_write_zeroes]	= cpu_to_le32(1 << 0);
+	log->acs[nvme_admin_get_log_page] =
+	log->acs[nvme_admin_identify] =
+	log->acs[nvme_admin_abort_cmd] =
+	log->acs[nvme_admin_set_features] =
+	log->acs[nvme_admin_get_features] =
+	log->acs[nvme_admin_async_event] =
+	log->acs[nvme_admin_keep_alive] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
+
+	log->iocs[nvme_cmd_read] =
+	log->iocs[nvme_cmd_write] =
+	log->iocs[nvme_cmd_flush] =
+	log->iocs[nvme_cmd_dsm]	=
+	log->iocs[nvme_cmd_write_zeroes] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
 static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
 {
-	log->iocs[nvme_cmd_zone_append]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_zone_mgmt_send]	= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_zone_mgmt_recv]	= cpu_to_le32(1 << 0);
+	log->iocs[nvme_cmd_zone_append] =
+	log->iocs[nvme_cmd_zone_mgmt_send] =
+	log->iocs[nvme_cmd_zone_mgmt_recv] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
 static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
-- 
2.35.1

[PATCH 3/6] nvmet: set the LBCC bit for commands that modify data

[Christoph Hellwig]

Write, Write Zeroes, Zone append and a Zone Reset through
Zone Management Send modify the logical block content of a namespace,
so make sure the LBCC bit is reported for them.

Fіxes: b5d0b38c0475 ("nvmet: add Command Set Identifier support")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 111a5cb6403fb0..6a54ed6fb12144 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -174,17 +174,19 @@ static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 
 	log->iocs[nvme_cmd_read] =
-	log->iocs[nvme_cmd_write] =
 	log->iocs[nvme_cmd_flush] =
 	log->iocs[nvme_cmd_dsm]	=
-	log->iocs[nvme_cmd_write_zeroes] =
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
+	log->iocs[nvme_cmd_write] =
+	log->iocs[nvme_cmd_write_zeroes] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC);
 }
 
 static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
 {
 	log->iocs[nvme_cmd_zone_append] =
 	log->iocs[nvme_cmd_zone_mgmt_send] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC);
 	log->iocs[nvme_cmd_zone_mgmt_recv] =
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
-- 
2.35.1

[PATCH 4/6] nvmet: don't defer passthrough commands with trivial effects to the workqueue

[Christoph Hellwig]

Mask out the "Command Supported" and "Logical Block Content Change" bits
and only defer execution of commands that have non-trivial effects to
the workqueue for synchronous execution.  This allows to execture admin
commands asynchronously on controllers that provide a Command Supported
and Effects log page, and will keep allowing to execute Write commands
asynchronously once command effects on I/O commands are taken into
account.

Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/passthru.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 79af5140af8bfe..adc0958755d66f 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -334,14 +334,13 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	}
 
 	/*
-	 * If there are effects for the command we are about to execute, or
-	 * an end_req function we need to use nvme_execute_passthru_rq()
-	 * synchronously in a work item seeing the end_req function and
-	 * nvme_passthru_end() can't be called in the request done callback
-	 * which is typically in interrupt context.
+	 * If a command needs post-execution fixups, or there are any
+	 * non-trivial effects, make sure to execute the command synchronously
+	 * in a workqueue so that nvme_passthru_end gets called.
 	 */
 	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
-	if (req->p.use_workqueue || effects) {
+	if (req->p.use_workqueue ||
+	    (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))) {
 		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
 		req->p.rq = rq;
 		queue_work(nvmet_wq, &req->p.work);
-- 
2.35.1

[PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

To be able to use the Commands Supported and Effects Log for allowing
unprivileged passtrough, it needs to be corretly reported for I/O
commands as well.  Return the I/O command effects from
nvme_command_effects, and also add a default list of effects for the
NVM command set.  For other command sets, the Commands Supported and
Effects log is required to be present already.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e26b085a007aea..4a04c12135702c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1074,6 +1074,18 @@ static u32 nvme_known_admin_effects(u8 opcode)
 	return 0;
 }
 
+static u32 nvme_known_nvm_effects(u8 opcode)
+{
+	switch (opcode) {
+	case nvme_cmd_write:
+	case nvme_cmd_write_zeroes:
+	case nvme_cmd_write_uncor:
+		 return NVME_CMD_EFFECTS_LBCC;
+	default:
+		return 0;
+	}
+}
+
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
 	u32 effects = 0;
@@ -1081,17 +1093,18 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 	if (ns) {
 		if (ns->head->effects)
 			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
+			effects |= nvme_known_nvm_effects(opcode);
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
-				"IO command:%02x has unhandled effects:%08x\n",
+				"IO command:%02x has unusual effects:%08x\n",
 				opcode, effects);
-		return 0;
+	} else {
+		if (ctrl->effects)
+			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+		effects |= nvme_known_admin_effects(opcode);
 	}
 
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
-
 	return effects;
 }
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);
-- 
2.35.1

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Keith Busch]

On Wed, Dec 21, 2022 at 11:10:49AM +0100, Christoph Hellwig wrote:
>  u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
>  {
>  	u32 effects = 0;
> @@ -1081,17 +1093,18 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
>  	if (ns) {
>  		if (ns->head->effects)
>  			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
> +		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
> +			effects |= nvme_known_nvm_effects(opcode);
>  		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
>  			dev_warn_once(ctrl->device,
> -				"IO command:%02x has unhandled effects:%08x\n",
> +				"IO command:%02x has unusual effects:%08x\n",
>  				opcode, effects);
> -		return 0;
> +	} else {
> +		if (ctrl->effects)
> +			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> +		effects |= nvme_known_admin_effects(opcode);
>  	}
>  
> -	if (ctrl->effects)
> -		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> -	effects |= nvme_known_admin_effects(opcode);
> -
>  	return effects;

Just noticed we need to mask out the CSE value for IO commands in case
some crazy controller sets it. If the driver were to see that value as
non-zero, it will freeze the IO queue the ioctl requires to to submit
its own IO request.

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

On Wed, Dec 21, 2022 at 02:42:47PM -0700, Keith Busch wrote:
> Just noticed we need to mask out the CSE value for IO commands in case
> some crazy controller sets it. If the driver were to see that value as
> non-zero, it will freeze the IO queue the ioctl requires to to submit
> its own IO request.

I'd rather reject passthrough of I/O commands with that effect
entirely, does that sound reasonable?

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Keith Busch]

On Thu, Dec 22, 2022 at 09:57:37AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 21, 2022 at 02:42:47PM -0700, Keith Busch wrote:
> > Just noticed we need to mask out the CSE value for IO commands in case
> > some crazy controller sets it. If the driver were to see that value as
> > non-zero, it will freeze the IO queue the ioctl requires to to submit
> > its own IO request.
> 
> I'd rather reject passthrough of I/O commands with that effect
> entirely, does that sound reasonable?

Sounds good to me.

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

On Thu, Dec 22, 2022 at 08:38:08AM -0700, Keith Busch wrote:
> On Thu, Dec 22, 2022 at 09:57:37AM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 21, 2022 at 02:42:47PM -0700, Keith Busch wrote:
> > > Just noticed we need to mask out the CSE value for IO commands in case
> > > some crazy controller sets it. If the driver were to see that value as
> > > non-zero, it will freeze the IO queue the ioctl requires to to submit
> > > its own IO request.
> > 
> > I'd rather reject passthrough of I/O commands with that effect
> > entirely, does that sound reasonable?
> 
> Sounds good to me.

I ended up to actually implement your suggestion of clearing it.
This does not block command execution, and we already warn about
unusual effects.

[PATCH 6/6] nvme: consult the CSE log page for unprivileged passthrough

[Christoph Hellwig]

Commands like Write Zeros can change the contents of a namespaces without
actually transferring data.  To protect against this, check the Commands
Supported and Effects log is supported by the controller for any
unprivileg command passthrough and refuse unprivileged passthrough if the
command has any effects that can change data or metadata.

Note: While the Commands Support and Effects log page has only been
mandatory since NVMe 2.0, it is widely supported because Windows requires
it for any command passthrough from userspace.

Fixes: e4fbcf32c860 ("nvme: identify-namespace without CAP_SYS_ADMIN")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/ioctl.c | 28 ++++++++++++++++++++++++----
 include/linux/nvme.h      |  1 +
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9ddda571f0461f..a8639919237e6a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -11,6 +11,8 @@
 static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 		fmode_t mode)
 {
+	u32 effects;
+
 	if (capable(CAP_SYS_ADMIN))
 		return true;
 
@@ -43,11 +45,29 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	}
 
 	/*
-	 * Only allow I/O commands that transfer data to the controller if the
-	 * special file is open for writing, but always allow I/O commands that
-	 * transfer data from the controller.
+	 * Check if the controller provides a Commands Supported and Effects log
+	 * and marks this command as supported.  If not reject unprivileged
+	 * passthrough.
+	 */
+	effects = nvme_command_effects(ns->ctrl, ns, c->common.opcode);
+	if (!(effects & NVME_CMD_EFFECTS_CSUPP))
+		return false;
+
+	/*
+	 * Don't allow passthrough for command that have intrusive (or unknown)
+	 * effects.
+	 */
+	if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
+			NVME_CMD_EFFECTS_UUID_SEL |
+			NVME_CMD_EFFECTS_SCOPE_MASK))
+		return false;
+
+	/*
+	 * Only allow I/O commands that transfer data to the controller or that
+	 * change the logical block contents if the file descriptor is open for
+	 * writing.
 	 */
-	if (nvme_is_write(c))
+	if (nvme_is_write(c) || (effects & NVME_CMD_EFFECTS_LBCC))
 		return mode & FMODE_WRITE;
 	return true;
 }
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d1cd53f2b6abd9..4fad4aa245fb06 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -642,6 +642,7 @@ enum {
 	NVME_CMD_EFFECTS_CCC		= 1 << 4,
 	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
 	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
+	NVME_CMD_EFFECTS_SCOPE_MASK	= GENMASK(31, 20),
 };
 
 struct nvme_effects_log {
-- 
2.35.1

Re: only allow unprivileged passthrough for commands without effects v3

[Kanchan Joshi]

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

On Wed, Dec 21, 2022 at 11:10:44AM +0100, Christoph Hellwig wrote:
>Hi all,
>
>this series first fixes a few minor issues in the CES log support in the
>host and target drivers and then uses the log to deny unprivileged
>passthrough of commands that have effects, where the only practically
>relevant effect is the modification of contents of the data stored in the
>namespace.

LGTM.
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>


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

Re: only allow unprivileged passthrough for commands without effects v3

[Christoph Hellwig]

Btw, any chance you could write some tests for blktests to verify
the unprivilged passthrough operations?

Re: only allow unprivileged passthrough for commands without effects v3

[Kanchan Joshi]

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

On Fri, Dec 23, 2022 at 08:45:45AM +0100, Christoph Hellwig wrote:
>Btw, any chance you could write some tests for blktests to verify
>the unprivilged passthrough operations?

Good point, will look into that.

FWIW I tested v3 with fio/liburing test and few nvme-cli commands. 

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

Re: only allow unprivileged passthrough for commands without effects v3

[Christoph Hellwig]

On Fri, Dec 23, 2022 at 06:58:56PM +0530, Kanchan Joshi wrote:
> On Fri, Dec 23, 2022 at 08:45:45AM +0100, Christoph Hellwig wrote:
>> Btw, any chance you could write some tests for blktests to verify
>> the unprivilged passthrough operations?
>
> Good point, will look into that.
>
> FWIW I tested v3 with fio/liburing test and few nvme-cli commands. 

I've done testing with nvme-cli and su, but it might be worth wiring
that up.