only allow unprivileged passthrough for commands without effects v2

only allow unprivileged passthrough for commands without effects v2

[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 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:
 host/core.c        |   95 ++++++++++++++++++++++++++++-------------------------
 host/ioctl.c       |   20 ++++++-----
 host/nvme.h        |    3 +
 target/admin-cmd.c |   37 +++++++++++---------
 target/nvmet.h     |    1 
 target/passthru.c  |   61 ++++++++++++++++++++--------------
 6 files changed, 122 insertions(+), 95 deletions(-)

[PATCH 1/9] 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 2/9] 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 3/9] nvme: remove nvme_execute_passthru_rq

[Christoph Hellwig]

After moving the nvme_passthru_end call to the callers of
nvme_execute_passthru_rq, this function has become quite pointless,
so remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c       | 18 ++++--------------
 drivers/nvme/host/ioctl.c      |  5 +++--
 drivers/nvme/host/nvme.h       |  3 ++-
 drivers/nvme/target/passthru.c |  5 +++--
 4 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e26b085a007aea..d6f3ac102e9488 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1004,7 +1004,7 @@ EXPORT_SYMBOL_GPL(nvme_setup_cmd);
  * >0: nvme controller's cqe status response
  * <0: kernel error in lieu of controller response
  */
-static int nvme_execute_rq(struct request *rq, bool at_head)
+int nvme_execute_rq(struct request *rq, bool at_head)
 {
 	blk_status_t status;
 
@@ -1015,6 +1015,7 @@ static int nvme_execute_rq(struct request *rq, bool at_head)
 		return nvme_req(rq)->status;
 	return blk_status_to_errno(status);
 }
+EXPORT_SYMBOL_NS_GPL(nvme_execute_rq, NVME_TARGET_PASSTHRU);
 
 /*
  * Returns 0 on success.  If the result is negative, it's a Linux error code;
@@ -1096,8 +1097,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 }
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);
 
-static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-			       u8 opcode)
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
 	u32 effects = nvme_command_effects(ctrl, ns, opcode);
 
@@ -1115,6 +1115,7 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	}
 	return effects;
 }
+EXPORT_SYMBOL_NS_GPL(nvme_passthru_start, NVME_TARGET_PASSTHRU);
 
 void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
 		       struct nvme_command *cmd, int status)
@@ -1156,17 +1157,6 @@ void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
 }
 EXPORT_SYMBOL_NS_GPL(nvme_passthru_end, NVME_TARGET_PASSTHRU);
 
-int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
-{
-	struct nvme_command *cmd = nvme_req(rq)->cmd;
-	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
-	struct nvme_ns *ns = rq->q->queuedata;
-
-	*effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
-	return nvme_execute_rq(rq, false);
-}
-EXPORT_SYMBOL_NS_GPL(nvme_execute_passthru_rq, NVME_TARGET_PASSTHRU);
-
 /*
  * Recommended frequency for KATO commands per NVMe 1.4 section 7.12.1:
  * 
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9ddda571f0461f..a371209ee5e6d4 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -187,6 +187,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
 		u32 meta_seed, u64 *result, unsigned timeout, bool vec)
 {
+	struct nvme_ns *ns = q->queuedata;
 	struct nvme_ctrl *ctrl;
 	struct request *req;
 	void *meta = NULL;
@@ -209,8 +210,8 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	bio = req->bio;
 	ctrl = nvme_req(req)->ctrl;
 
-	ret = nvme_execute_passthru_rq(req, &effects);
-
+	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
+	ret = nvme_execute_rq(req, false);
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
 	if (meta)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6bbb73ef8b2548..a52e854e894b70 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1070,7 +1070,8 @@ static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
 
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			 u8 opcode);
-int nvme_execute_passthru_rq(struct request *rq, u32 *effects);
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode);
+int nvme_execute_rq(struct request *rq, bool at_head);
 void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
 		       struct nvme_command *cmd, int status);
 struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 79af5140af8bfe..0b238fce2c11f2 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -216,11 +216,12 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
 	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
 	struct request *rq = req->p.rq;
 	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
+	struct nvme_ns *ns = rq->q->queuedata;
 	u32 effects;
 	int status;
 
-	status = nvme_execute_passthru_rq(rq, &effects);
-
+	effects = nvme_passthru_start(ctrl, ns, req->cmd->common.opcode);
+	status = nvme_execute_rq(rq, false);
 	if (status == NVME_SC_SUCCESS &&
 	    req->cmd->common.opcode == nvme_admin_identify) {
 		switch (req->cmd->identify.cns) {
-- 
2.35.1

[PATCH 4/9] nvmet: refactor passthru fixup code

[Christoph Hellwig]

Add a helper to check if a given command needs a fixup and use that
instead of setting the use_workqueue flag that can get out of sync.
Also move the fixup into a separate out of line function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/nvmet.h    |  1 -
 drivers/nvme/target/passthru.c | 50 ++++++++++++++++++++++------------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 89bedfcd974c41..b4f7d2aa869142 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -369,7 +369,6 @@ struct nvmet_req {
 			struct bio		inline_bio;
 			struct request		*rq;
 			struct work_struct      work;
-			bool			use_workqueue;
 		} p;
 #ifdef CONFIG_BLK_DEV_ZONED
 		struct {
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 0b238fce2c11f2..63fe8aaf968486 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -211,19 +211,20 @@ static u16 nvmet_passthru_override_id_ns(struct nvmet_req *req)
 	return status;
 }
 
-static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
+static bool nvmet_passthru_needs_fixup(struct nvmet_req *req)
 {
-	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
-	struct request *rq = req->p.rq;
-	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
-	struct nvme_ns *ns = rq->q->queuedata;
-	u32 effects;
-	int status;
+	/*
+	 * Some CNS values of Identify need fixups after I/O completion and
+	 * thus need to be handled in a workqueue.  Don't bother selecting the
+	 * specific ones as Identify performance doesn't matter too much.
+	 */
+	return req->cmd->common.opcode == nvme_admin_identify;
+}
 
-	effects = nvme_passthru_start(ctrl, ns, req->cmd->common.opcode);
-	status = nvme_execute_rq(rq, false);
-	if (status == NVME_SC_SUCCESS &&
-	    req->cmd->common.opcode == nvme_admin_identify) {
+static void nvmet_passthru_fixup(struct nvmet_req *req)
+{
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_identify:
 		switch (req->cmd->identify.cns) {
 		case NVME_ID_CNS_CTRL:
 			nvmet_passthru_override_id_ctrl(req);
@@ -234,8 +235,26 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
 		case NVME_ID_CNS_NS_DESC_LIST:
 			nvmet_passthru_override_id_descs(req);
 			break;
+		default:
+			break;
 		}
-	} else if (status < 0)
+	}
+}
+
+static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
+	struct request *rq = req->p.rq;
+	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
+	struct nvme_ns *ns = rq->q->queuedata;
+	u32 effects;
+	int status;
+
+	effects = nvme_passthru_start(ctrl, ns, req->cmd->common.opcode);
+	status = nvme_execute_rq(rq, false);
+	if (status == NVME_SC_SUCCESS && nvmet_passthru_needs_fixup(req))
+		nvmet_passthru_fixup(req);
+	else if (status < 0)
 		status = NVME_SC_INTERNAL;
 
 	req->cqe->result = nvme_req(rq)->result;
@@ -342,7 +361,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	 * which is typically in interrupt context.
 	 */
 	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
-	if (req->p.use_workqueue || effects) {
+	if (nvmet_passthru_needs_fixup(req) || effects) {
 		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
 		req->p.rq = rq;
 		queue_work(nvmet_wq, &req->p.work);
@@ -404,7 +423,6 @@ static void nvmet_passthru_set_host_behaviour(struct nvmet_req *req)
 
 static u16 nvmet_setup_passthru_command(struct nvmet_req *req)
 {
-	req->p.use_workqueue = false;
 	req->execute = nvmet_passthru_execute_cmd;
 	return NVME_SC_SUCCESS;
 }
@@ -538,25 +556,21 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 		switch (req->cmd->identify.cns) {
 		case NVME_ID_CNS_CTRL:
 			req->execute = nvmet_passthru_execute_cmd;
-			req->p.use_workqueue = true;
 			return NVME_SC_SUCCESS;
 		case NVME_ID_CNS_CS_CTRL:
 			switch (req->cmd->identify.csi) {
 			case NVME_CSI_ZNS:
 				req->execute = nvmet_passthru_execute_cmd;
-				req->p.use_workqueue = true;
 				return NVME_SC_SUCCESS;
 			}
 			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 		case NVME_ID_CNS_NS:
 			req->execute = nvmet_passthru_execute_cmd;
-			req->p.use_workqueue = true;
 			return NVME_SC_SUCCESS;
 		case NVME_ID_CNS_CS_NS:
 			switch (req->cmd->identify.csi) {
 			case NVME_CSI_ZNS:
 				req->execute = nvmet_passthru_execute_cmd;
-				req->p.use_workqueue = true;
 				return NVME_SC_SUCCESS;
 			}
 			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
-- 
2.35.1

[PATCH 5/9] nvmet: allow async passthrough of commands that change logical block contents

[Christoph Hellwig]

Mask out the logical block change effect to allow to keep supporting
passthrough for Write commands once I/O Command effects are properly
propagated.

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

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 63fe8aaf968486..f31537dd5714b1 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -312,10 +312,10 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 {
 	struct nvme_ctrl *ctrl = nvmet_req_subsys(req)->passthru_ctrl;
 	struct request_queue *q = ctrl->admin_q;
+	u8 opcode = req->cmd->common.opcode;
 	struct nvme_ns *ns = NULL;
 	struct request *rq = NULL;
 	unsigned int timeout;
-	u32 effects;
 	u16 status;
 	int ret;
 
@@ -354,14 +354,12 @@ 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-exectuion fixups, or there are any non-trivial
+	 * effects, make sure to execute the command in a workqueue so that we
+	 * can execute the fixups and call nvme_passthru_end.
 	 */
-	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
-	if (nvmet_passthru_needs_fixup(req) || effects) {
+	if (nvmet_passthru_needs_fixup(req) ||
+	    (nvme_command_effects(ctrl, ns, opcode) & ~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 6/9] nvme: only return actual effects from nvme_command_effects

[Christoph Hellwig]

Only return actual destructive effects from the command, and not misc
information like if the command is supported, the scope and support for
the UUID index.

Note that this causes the nvmet passthrough code to now execute
admin commands asynchronously, which it previously did not due to the
supported by in the returned effects.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d6f3ac102e9488..945a947f90b757 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1093,7 +1093,10 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
 	effects |= nvme_known_admin_effects(opcode);
 
-	return effects;
+	/* only return actual command effects and not misc information */
+	return effects & (NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
+			  NVME_CMD_EFFECTS_NCC | NVME_CMD_EFFECTS_NIC |
+			  NVME_CMD_EFFECTS_CCC | NVME_CMD_EFFECTS_CSE_MASK);
 }
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);
 
-- 
2.35.1

[PATCH 7/9] nvme: assign known effects at controller initialization time

[Christoph Hellwig]

Always allocate an in-memory commands and effects log in
nvme_init_identify, and assign the known effects there to remove branches
that check for the logs presence in the runtime passthrough path.

Note that instead of checking for ctrl->effects this also moves the
initialization in the initialized branch, which has the same effect, but
is a little easier to reason about.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 945a947f90b757..9529abe22c9cdb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1061,27 +1061,12 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 }
 EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
-static u32 nvme_known_admin_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_admin_format_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
-			NVME_CMD_EFFECTS_CSE_MASK;
-	case nvme_admin_sanitize_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK;
-	default:
-		break;
-	}
-	return 0;
-}
-
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
-	u32 effects = 0;
+	u32 effects;
 
 	if (ns) {
-		if (ns->head->effects)
-			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
 				"IO command:%02x has unhandled effects:%08x\n",
@@ -1089,9 +1074,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		return 0;
 	}
 
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
+	effects = le32_to_cpu(ctrl->effects->acs[opcode]);
 
 	/* only return actual command effects and not misc information */
 	return effects & (NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
@@ -3066,6 +3049,33 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
+{
+	int ret;
+
+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
+		if (ret < 0)
+			return ret;
+	} else {
+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
+		if (!ctrl->effects)
+			return -ENOMEM;
+	}
+
+	/*
+	 * Initialize known admin command set effects for controllers don't
+	 * provide the log or are buggy enough to not correctly mark the
+	 * usual effects.
+	 */
+	ctrl->effects->acs[nvme_admin_format_nvm] |=
+		cpu_to_le32(NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
+			    NVME_CMD_EFFECTS_CSE_MASK);
+	ctrl->effects->acs[nvme_admin_sanitize_nvm] |=
+		cpu_to_le32(NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK);
+	return 0;
+}
+
 static int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -3079,18 +3089,16 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return -EIO;
 	}
 
-	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
-		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
-		if (ret < 0)
-			goto out_free;
-	}
-
 	if (!(ctrl->ops->flags & NVME_F_FABRICS))
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
 
 	if (!ctrl->identified) {
 		unsigned int i;
 
+		ret = nvme_init_effects(ctrl, id);
+		if (ret < 0)
+			goto out_free;
+
 		/*
 		 * Check for quirks.  Quirk can depend on firmware version,
 		 * so, in principle, the set of quirks present can change
-- 
2.35.1

[PATCH 8/9] 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 we do require the log page
to be present already.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9529abe22c9cdb..c50678390def38 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1069,13 +1069,12 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		effects = le32_to_cpu(ns->head->effects->iocs[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 {
+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
 	}
 
-	effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-
 	/* only return actual command effects and not misc information */
 	return effects & (NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
 			  NVME_CMD_EFFECTS_NCC | NVME_CMD_EFFECTS_NIC |
@@ -3064,8 +3063,8 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	}
 
 	/*
-	 * Initialize known admin command set effects for controllers don't
-	 * provide the log or are buggy enough to not correctly mark the
+	 * Initialize known admin and NVM command set effects for controllers
+	 * don't provide the log or are buggy enough to not correctly mark the
 	 * usual effects.
 	 */
 	ctrl->effects->acs[nvme_admin_format_nvm] |=
@@ -3073,6 +3072,13 @@ static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 			    NVME_CMD_EFFECTS_CSE_MASK);
 	ctrl->effects->acs[nvme_admin_sanitize_nvm] |=
 		cpu_to_le32(NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK);
+
+	ctrl->effects->iocs[nvme_cmd_write] |=
+		cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	ctrl->effects->iocs[nvme_cmd_write_zeroes] |=
+		cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	ctrl->effects->iocs[nvme_cmd_write_uncor] |=
+		cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
 	return 0;
 }
 
-- 
2.35.1

[PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects

[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 and refuse unprivileged passthrough if the
command has any effects.  This also includes more intrusive effects which
currently can't happen for I/O commands.

Fixes: e4fbcf32c860 ("nvme: identify-namespace without CAP_SYS_ADMIN")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/ioctl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index a371209ee5e6d4..90e3a4a711bd17 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)
 {
+	u8 opcode = c->common.opcode;
+
 	if (capable(CAP_SYS_ADMIN))
 		return true;
 
@@ -18,8 +20,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 * Do not allow unprivileged processes to send vendor specific or fabrics
 	 * commands as we can't be sure about their effects.
 	 */
-	if (c->common.opcode >= nvme_cmd_vendor_start ||
-	    c->common.opcode == nvme_fabrics_command)
+	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
 		return false;
 
 	/*
@@ -29,7 +30,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 * potentially sensitive information.
 	 */
 	if (!ns) {
-		if (c->common.opcode == nvme_admin_identify) {
+		if (opcode == nvme_admin_identify) {
 			switch (c->identify.cns) {
 			case NVME_ID_CNS_NS:
 			case NVME_ID_CNS_CS_NS:
@@ -43,11 +44,11 @@ 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.
+	 * Only allow I/O commands that transfer data to the controller, change
+	 * the logical block content or have any other intrusive effects if the
+	 * special file is open for writing. 
 	 */
-	if (nvme_is_write(c))
+	if (nvme_is_write(c) || nvme_command_effects(ns->ctrl, ns, opcode))
 		return mode & FMODE_WRITE;
 	return true;
 }
-- 
2.35.1

Re: [PATCH 7/9] nvme: assign known effects at controller initialization time

[Keith Busch]

On Wed, Dec 14, 2022 at 05:13:45PM +0100, Christoph Hellwig wrote:
>  
> +static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> +{
> +	int ret;
> +
> +	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
> +		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
> +		if (ret < 0)
> +			return ret;

In the unlikely case where ret > 0, like the controller is very
confused, we need to fall through to allocate the default ctrl->effects.

> +	} else {
> +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> +		if (!ctrl->effects)
> +			return -ENOMEM;
> +	}

Re: only allow unprivileged passthrough for commands without effects v2

[Keith Busch]

I only have just the one comment on patch 7. The rest looks good!

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

Re: [PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects

[Kanchan Joshi]

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

On Wed, Dec 14, 2022 at 05:13:47PM +0100, Christoph Hellwig wrote:
>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 and refuse unprivileged passthrough if the
>command has any effects.  This also includes more intrusive effects which
>currently can't happen for I/O commands.
>
>Fixes: e4fbcf32c860 ("nvme: identify-namespace without CAP_SYS_ADMIN")
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> drivers/nvme/host/ioctl.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>index a371209ee5e6d4..90e3a4a711bd17 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)
> {
>+	u8 opcode = c->common.opcode;
>+
> 	if (capable(CAP_SYS_ADMIN))
> 		return true;
>
>@@ -18,8 +20,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> 	 * Do not allow unprivileged processes to send vendor specific or fabrics
> 	 * commands as we can't be sure about their effects.
> 	 */
>-	if (c->common.opcode >= nvme_cmd_vendor_start ||
>-	    c->common.opcode == nvme_fabrics_command)
>+	if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
> 		return false;
>
> 	/*
>@@ -29,7 +30,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> 	 * potentially sensitive information.
> 	 */
> 	if (!ns) {
>-		if (c->common.opcode == nvme_admin_identify) {
>+		if (opcode == nvme_admin_identify) {
> 			switch (c->identify.cns) {
> 			case NVME_ID_CNS_NS:
> 			case NVME_ID_CNS_CS_NS:
>@@ -43,11 +44,11 @@ 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.
>+	 * Only allow I/O commands that transfer data to the controller, change
>+	 * the logical block content or have any other intrusive effects if the
>+	 * special file is open for writing.

nit: trailing whitespace at the end of above line.

> 	 */
>-	if (nvme_is_write(c))
>+	if (nvme_is_write(c) || nvme_command_effects(ns->ctrl, ns, opcode))
> 		return mode & FMODE_WRITE;

So even for operation that do not alter anything (e.g. nvme_cmd_read)
nvme_is_write will return false, but nvme_command_effects will return
true and we will ask for FMODE_WRITE. Is that intentional?

I think doing 
"nvme_command_effects(ctrl, ns, opcode) & ~NVME_CMD_EFFECTS_CSUPP"
is better to avoid that?

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

Re: [PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects

[Christoph Hellwig]

On Thu, Dec 15, 2022 at 12:44:05PM +0530, Kanchan Joshi wrote:
> So even for operation that do not alter anything (e.g. nvme_cmd_read)
> nvme_is_write will return false, but nvme_command_effects will return
> true and we will ask for FMODE_WRITE. Is that intentional?
>
> I think doing "nvme_command_effects(ctrl, ns, opcode) & 
> ~NVME_CMD_EFFECTS_CSUPP"
> is better to avoid that?

Take a look at patch 6.

Re: [PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects

[Kanchan Joshi]

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

On Thu, Dec 15, 2022 at 09:18:29AM +0100, Christoph Hellwig wrote:
>On Thu, Dec 15, 2022 at 12:44:05PM +0530, Kanchan Joshi wrote:
>> So even for operation that do not alter anything (e.g. nvme_cmd_read)
>> nvme_is_write will return false, but nvme_command_effects will return
>> true and we will ask for FMODE_WRITE. Is that intentional?
>>
>> I think doing "nvme_command_effects(ctrl, ns, opcode) &
>> ~NVME_CMD_EFFECTS_CSUPP"
>> is better to avoid that?
>
>Take a look at patch 6.

But that patch still takes NVME_CMD_EFFECTS_CSUPP into account while
returning effects. It should be removed from there, and we need nothing
else here.

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

Re: [PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects

[Christoph Hellwig]

On Thu, Dec 15, 2022 at 01:54:10PM +0530, Kanchan Joshi wrote:
> But that patch still takes NVME_CMD_EFFECTS_CSUPP into account while
> returning effects. It should be removed from there, and we need nothing
> else here.

Ooops, indeed.  I accidentally broke that again.

Re: [PATCH 7/9] nvme: assign known effects at controller initialization time

[Kanchan Joshi]

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

On Wed, Dec 14, 2022 at 05:13:45PM +0100, Christoph Hellwig wrote:

>+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>+{
>+	int ret;
>+
>+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
>+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
>+		if (ret < 0)
>+			return ret;
>+	} else {
>+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
>+		if (!ctrl->effects)
>+			return -ENOMEM;

Maybe I am missing something obvious, but when ctrl->effects gets
freed if it was allocated via the else path above?
It's not going to that "ctrl->cels" xarray which gets freed during
nvme_free_ctrl.

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