only allow unprivileged passthrough for commands without effects

only allow unprivileged passthrough for commands without effects

[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.

Diffstat:
 host/core.c        |   47 ++++++++++++++++++++++++++---------------------
 host/ioctl.c       |   19 ++++++++++---------
 host/nvme.h        |    3 ++-
 target/admin-cmd.c |   37 +++++++++++++++++++++----------------
 target/passthru.c  |   25 +++++++++++++------------
 5 files changed, 72 insertions(+), 59 deletions(-)

[PATCH 1/7] nvmet: use NVME_CMD_EFFECTS_CSUPP instead of open coding it

[Christoph Hellwig]

And use multiple assignments to clean up the repeating assingments a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

Re: [PATCH 1/7] nvmet: use NVME_CMD_EFFECTS_CSUPP instead of open coding it

[Chaitanya Kulkarni]

On 12/13/22 08:24, Christoph Hellwig wrote:
> And use multiple assignments to clean up the repeating assingments a bit.
> 

's/And//g'

looks bit odd having than what we have but if others are okay with
it sure,

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH 2/7] 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>
---
 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

Re: [PATCH 2/7] nvmet: set the LBCC bit for commands that modify data

[Chaitanya Kulkarni]

On 12/13/22 08:24, Christoph Hellwig wrote:
> 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>

Thanks for fixing this, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH 3/7] 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>
---
 drivers/nvme/target/passthru.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 79af5140af8bfe..ad80ba61c42236 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -295,7 +295,6 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	struct nvme_ns *ns = NULL;
 	struct request *rq = NULL;
 	unsigned int timeout;
-	u32 effects;
 	u16 status;
 	int ret;
 
@@ -334,14 +333,16 @@ 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 there are any non-trivial effects for the command we are about to
+	 * execute, call nvme_execute_passthru_rq in a workqueue, so that the
+	 * effects can be properly handled by the calls to nvme_passthru_start
+	 * and nvme_passthru_end.
 	 */
-	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
-	if (req->p.use_workqueue || effects) {
+	if (nvme_command_effects(ctrl, ns, req->cmd->common.opcode) &
+	    ~NVME_CMD_EFFECTS_LBCC)
+		req->p.use_workqueue = true;
+
+	if (req->p.use_workqueue) {
 		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
 		req->p.rq = rq;
 		queue_work(nvmet_wq, &req->p.work);
-- 
2.35.1

Re: [PATCH 3/7] nvmet: allow async passthrough of commands that change logical block contents

[Chaitanya Kulkarni]

On 12/13/22 08:24, Christoph Hellwig wrote:
> 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>
> ---
>   drivers/nvme/target/passthru.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index 79af5140af8bfe..ad80ba61c42236 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -295,7 +295,6 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>   	struct nvme_ns *ns = NULL;
>   	struct request *rq = NULL;
>   	unsigned int timeout;
> -	u32 effects;
>   	u16 status;
>   	int ret;
>   
> @@ -334,14 +333,16 @@ 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 there are any non-trivial effects for the command we are about to
> +	 * execute, call nvme_execute_passthru_rq in a workqueue, so that the
> +	 * effects can be properly handled by the calls to nvme_passthru_start
> +	 * and nvme_passthru_end.
>   	 */
> -	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
> -	if (req->p.use_workqueue || effects) {
> +	if (nvme_command_effects(ctrl, ns, req->cmd->common.opcode) &
> +	    ~NVME_CMD_EFFECTS_LBCC)
> +		req->p.use_workqueue = true;
> +

How about this instead of calling function in if which is much cleaner
unless it has a bug ?

effect = nvme_command_effects(ctrl, ns, req->cmd->common.opcode); 

req->p.use_workqueue = effetcs & ~NVME_CMD_EFFECTS_LBCC;

irrespective of that, looks good :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 3/7] nvmet: allow async passthrough of commands that change logical block contents

[Christoph Hellwig]

On Wed, Dec 14, 2022 at 04:52:47AM +0000, Chaitanya Kulkarni wrote:
> > -	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
> > -	if (req->p.use_workqueue || effects) {
> > +	if (nvme_command_effects(ctrl, ns, req->cmd->common.opcode) &
> > +	    ~NVME_CMD_EFFECTS_LBCC)
> > +		req->p.use_workqueue = true;
> > +
> 
> How about this instead of calling function in if which is much cleaner
> unless it has a bug ?
> 
> effect = nvme_command_effects(ctrl, ns, req->cmd->common.opcode); 
> 
> req->p.use_workqueue = effetcs & ~NVME_CMD_EFFECTS_LBCC;

This would clear the bit if it is already set.  The being said
I'm not a fan of how the use_workqueue flag is set, and I now have
an idea on how to remove it entirely.

[PATCH 4/7] 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>
---
 drivers/nvme/host/core.c       | 18 ++++--------------
 drivers/nvme/host/ioctl.c      |  4 ++--
 drivers/nvme/host/nvme.h       |  3 ++-
 drivers/nvme/target/passthru.c |  8 ++++----
 4 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9854f5dbcf4ad0..955e960f2d908f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1003,7 +1003,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;
 
@@ -1014,6 +1014,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..2ed8fdd09bb35a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -208,8 +208,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, q->queuedata, cmd->common.opcode);
+	ret = nvme_execute_rq(req, false);
 
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6bbb73ef8b2548..37d97dfa39ea75 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);
+int nvme_execute_rq(struct request *rq, bool at_head);
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode);
 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 ad80ba61c42236..1b0863a537c874 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -216,13 +216,13 @@ 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;
+	u8 opcode = req->cmd->common.opcode;
 	u32 effects;
 	int status;
 
-	status = nvme_execute_passthru_rq(rq, &effects);
-
-	if (status == NVME_SC_SUCCESS &&
-	    req->cmd->common.opcode == nvme_admin_identify) {
+	effects = nvme_passthru_start(ctrl, rq->q->queuedata, opcode);
+	status = nvme_execute_rq(rq, false);
+	if (status == NVME_SC_SUCCESS && opcode == nvme_admin_identify) {
 		switch (req->cmd->identify.cns) {
 		case NVME_ID_CNS_CTRL:
 			nvmet_passthru_override_id_ctrl(req);
-- 
2.35.1

Re: [PATCH 4/7] nvme: remove nvme_execute_passthru_rq

[Chaitanya Kulkarni]

On 12/13/22 08:24, Christoph Hellwig wrote:
> 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>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH 5/7] 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 955e960f2d908f..e9b721bbb5f95f 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 6/7] 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 limit for the NVM
command set.  For other command sets we do require the log page
to be present.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e9b721bbb5f95f..94e7224a696899 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1075,6 +1075,17 @@ 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:
+		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;
@@ -1082,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);
-
 	/* 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 |
-- 
2.35.1

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

[Keith Busch]

On Tue, Dec 13, 2022 at 05:24:52PM +0100, Christoph Hellwig wrote:
> +static u32 nvme_known_nvm_effects(u8 opcode)
> +{
> +	switch (opcode) {
> +	case nvme_cmd_write:
> +	case nvme_cmd_write_zeroes:
> +		return NVME_CMD_EFFECTS_LBCC;
> +	default:
> +		return 0;
> +	}
> +}

Also nvme_cmd_write_uncorr.

I think you'd need to know the namespace's command set to assume these
opcodes, though.

I was thinking instead of appending effects per-IO, we could always set
ns->head->effects even if the device doesn't support the log page, then
overwrite the log page's opcodes with known LBCC effects during
initialization. That way we don't need a per-io check. Does that sound
reasonable?

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

[Christoph Hellwig]

On Tue, Dec 13, 2022 at 10:53:07AM -0700, Keith Busch wrote:
> Also nvme_cmd_write_uncorr.

Indeed.

> I think you'd need to know the namespace's command set to assume these
> opcodes, though.

ctrl->effects always is NVM command set if supported, or otherwise
empty for I/O commands.

> I was thinking instead of appending effects per-IO, we could always set
> ns->head->effects even if the device doesn't support the log page, then
> overwrite the log page's opcodes with known LBCC effects during
> initialization. That way we don't need a per-io check. Does that sound
> reasonable?

I'd prefer to keep OR ingin the known worst cases.  But we can do
that at setup time.

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

[Keith Busch]

On Tue, Dec 13, 2022 at 07:54:15PM +0100, Christoph Hellwig wrote:
> On Tue, Dec 13, 2022 at 10:53:07AM -0700, Keith Busch wrote:
> > Also nvme_cmd_write_uncorr.
> 
> Indeed.
> 
> > I think you'd need to know the namespace's command set to assume these
> > opcodes, though.
> 
> ctrl->effects always is NVM command set if supported, or otherwise
> empty for I/O commands.
> 
> > I was thinking instead of appending effects per-IO, we could always set
> > ns->head->effects even if the device doesn't support the log page, then
> > overwrite the log page's opcodes with known LBCC effects during
> > initialization. That way we don't need a per-io check. Does that sound
> > reasonable?
> 
> I'd prefer to keep OR ingin the known worst cases.  But we can do
> that at setup time.

This is what I was thinking (but please ignore that I missed the
endian conversion in this example):

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d11e0d5b231df..b8672dc28beee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1060,39 +1060,19 @@ 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;
-
 	if (ns) {
-		if (ns->head->effects)
-			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		u32 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",
 				opcode, effects);
-		return 0;
+		return effects;
 	}
 
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
-
-	return effects;
+	return le32_to_cpu(ctrl->effects->acs[opcode]);
 }
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);
 
@@ -3100,6 +3080,21 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
+{
+	struct nvme_effects_log	*log = ctrl->effects;
+
+	log->acs[nvme_admin_format_nvm] |= NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_NCC |
+						NVME_CMD_EFFECTS_CSE_MASK;
+	log->acs[nvme_admin_sanitize_nvm] |= NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_CSE_MASK;
+
+	log->iocs[nvme_cmd_write] |= NVME_CMD_EFFECTS_LBCC;
+	log->iocs[nvme_cmd_write_zeroes] |= NVME_CMD_EFFECTS_LBCC;
+	log->iocs[nvme_cmd_write_uncor] |= NVME_CMD_EFFECTS_LBCC;
+}
+
 static int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -3113,10 +3108,21 @@ 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->effects) {
+		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;
+		} else {
+			ctrl->effects = kzalloc(sizeof(*ctrl->effects),
+						GFP_KERNEL);
+			if (!ctrl->effects) {
+				ret = -ENOMEM;
+				goto out_free;
+			}
+		}
+		nvme_init_known_nvm_effects(ctrl);
 	}
 
 	if (!(ctrl->ops->flags & NVME_F_FABRICS))
--

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

[Christoph Hellwig]

On Tue, Dec 13, 2022 at 12:18:47PM -0700, Keith Busch wrote:
> +	if (!ctrl->effects) {
> +		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;
> +		} else {
> +			ctrl->effects = kzalloc(sizeof(*ctrl->effects),
> +						GFP_KERNEL);
> +			if (!ctrl->effects) {
> +				ret = -ENOMEM;
> +				goto out_free;
> +			}
> +		}
> +		nvme_init_known_nvm_effects(ctrl);

I'd probably move this entire block into a helper, but otherwise
this looks reasonable.

[PATCH 7/7] 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 2ed8fdd09bb35a..e71d136816eae0 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