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

From: Kanchan Joshi <joshi.k@samsung.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>, Sagi Grimberg <sagi@grimberg.me>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	linux-nvme@lists.infradead.org
Subject: Re: [PATCH 9/9] nvme: don't allow unprivileged passthrough of commands that have effects
Date: Thu, 15 Dec 2022 12:44:05 +0530	[thread overview]
Message-ID: <20221215071405.GA27656@test-zns> (raw)
In-Reply-To: <20221214161347.764071-10-hch@lst.de>

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