block dangerous passthrough operation

block dangerous passthrough operation

[Christoph Hellwig]

Hi all,

I recently had to debug some testing code that tried to send fabrics
commands and caused a major havoc.  This series adds checks for various
very low-level passthrough commands that have no business sent by anyone
but the driver itself.  The list might now be complete, so discussion
is welcome.

Diffstat:
 ioctl.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 18 deletions(-)

[PATCH 1/4] nvme: return an errno from nvme_cmd_allowed

[Christoph Hellwig]

Return an errno value instead of bool from nvme_cmd_allowed to prepare
it for returning different kinds of errors.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/ioctl.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9550a69029b36..9a2f0f5ccef57 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -8,11 +8,11 @@
 #include <linux/io_uring.h>
 #include "nvme.h"
 
-static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
+static int nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 		fmode_t mode)
 {
 	if (capable(CAP_SYS_ADMIN))
-		return true;
+		return 0;
 
 	/*
 	 * Do not allow unprivileged processes to send vendor specific or fabrics
@@ -20,7 +20,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 */
 	if (c->common.opcode >= nvme_cmd_vendor_start ||
 	    c->common.opcode == nvme_fabrics_command)
-		return false;
+		return -EACCES;
 
 	/*
 	 * Do not allow unprivileged passthrough of admin commands except
@@ -34,10 +34,10 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 			case NVME_ID_CNS_NS:
 			case NVME_ID_CNS_CS_NS:
 			case NVME_ID_CNS_NS_CS_INDEP:
-				return true;
+				return 0;
 			}
 		}
-		return false;
+		return -EACCES;
 	}
 
 	/*
@@ -45,9 +45,9 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 * special file is open for writing, but always allow I/O commands that
 	 * transfer data from the controller.
 	 */
-	if (nvme_is_write(c))
-		return mode & FMODE_WRITE;
-	return true;
+	if (nvme_is_write(c) && !(mode & FMODE_WRITE))
+		return -EACCES;
+	return 0;
 }
 
 /*
@@ -331,8 +331,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;
+	status = nvme_cmd_allowed(ns, &c, mode);
+	if (status)
+		return status;
 
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
@@ -379,8 +380,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;
+	status = nvme_cmd_allowed(ns, &c, mode);
+	if (status)
+		return status;
 
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
@@ -549,8 +551,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;
+	ret = nvme_cmd_allowed(ns, &c, ioucmd->file->f_mode);
+	if (ret)
+		return ret;
 
 	d.metadata = READ_ONCE(cmd->metadata);
 	d.addr = READ_ONCE(cmd->addr);
-- 
2.30.2

[PATCH 2/4] nvme: don't allow user space to send fabrics commands

[Christoph Hellwig]

Fabrics command are used to set up and control the NVMe over Fabrics
connection.  We can't let user space programs interfer with that.

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

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9a2f0f5ccef57..badbc55d29350 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -11,15 +11,29 @@
 static int nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 		fmode_t mode)
 {
+	/*
+	 * Do not allow anyone to send vendor fabrics commands as they are
+	 * used to manage the fabrics transports and we can't let user space
+	 * processes interfere with that.
+	 */
+	if (c->common.opcode == nvme_fabrics_command) {
+		dev_warn_ratelimited(ns->ctrl->device,
+			"rejecting passthrough of fabrics command 0x%x by %s\n",
+			c->common.opcode, current->comm);
+		return -EINVAL;
+	}
+
+	/*
+	 * Allow privileged processes to pass through any other command.
+	 */
 	if (capable(CAP_SYS_ADMIN))
 		return 0;
 
 	/*
-	 * Do not allow unprivileged processes to send vendor specific or fabrics
-	 * commands as we can't be sure about their effects.
+	 * Do not allow unprivileged processes to send vendor specific 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 (c->common.opcode >= nvme_cmd_vendor_start)
 		return -EACCES;
 
 	/*
-- 
2.30.2

[PATCH 3/4] nvme: don't allow userspace to set the Host Behavior Support feature

[Christoph Hellwig]

This feature is used to tell the controller what features are supported
by the driver.  We thus can't let userspace processes update it.

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

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index badbc55d29350..171c983191501 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -23,6 +23,20 @@ static int nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 		return -EINVAL;
 	}
 
+	if (!ns) {
+		switch (c->common.opcode) {
+		case nvme_admin_set_features:
+			switch (c->features.opcode) {
+			case NVME_FEAT_HOST_BEHAVIOR:
+				dev_warn_ratelimited(ns->ctrl->device,
+					"rejecting Host Behavior support update by %s\n",
+					current->comm);
+				return -EINVAL;
+			}
+			break;
+		}
+	}
+
 	/*
 	 * Allow privileged processes to pass through any other command.
 	 */
-- 
2.30.2

[PATCH 4/4] nvme: reject passthrough of queue creation / deletion commands

[Christoph Hellwig]

Creating and deleting queues interferes directly with the low-level
operation of the driver.  Don't allow user space processes to do that.

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

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 171c983191501..271f85ee04e9b 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -34,6 +34,14 @@ static int nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 				return -EINVAL;
 			}
 			break;
+		case nvme_admin_create_cq:
+		case nvme_admin_create_sq:
+		case nvme_admin_delete_cq:
+		case nvme_admin_delete_sq:
+			dev_warn_ratelimited(ns->ctrl->device,
+				"rejecting queue create/delete command (0x%x) by %s\n",
+				c->common.opcode, current->comm);
+			return -EINVAL;
 		}
 	}
 
-- 
2.30.2

Re: block dangerous passthrough operation

[Kanchan Joshi]

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

On Wed, Nov 16, 2022 at 02:01:00PM +0100, Christoph Hellwig wrote:
>Hi all,
>
>I recently had to debug some testing code that tried to send fabrics
>commands and caused a major havoc.  This series adds checks for various
>very low-level passthrough commands that have no business sent by anyone
>but the driver itself.  The list might now be complete, so discussion
>is welcome.

Sorry to hear about the trouble, but I am confused how did this
happen.
The old code also did not allow fabrics command and any other
admin command except id-ns variants. Yet this series had to
explicitly disallow admin cmds such create/delete sq/cq and
set-features.

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

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 06:55:02PM +0530, Kanchan Joshi wrote:
> Sorry to hear about the trouble, but I am confused how did this
> happen.
> The old code also did not allow fabrics command and any other
> admin command except id-ns variants. Yet this series had to
> explicitly disallow admin cmds such create/delete sq/cq and
> set-features.

This is not a regression in your code, sorry if I made that
impression.  This is a long-standing issue since these ioctls
were originally added.

Re: block dangerous passthrough operation

[Kanchan Joshi]

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

On Wed, Nov 16, 2022 at 02:38:39PM +0100, Christoph Hellwig wrote:
>On Wed, Nov 16, 2022 at 06:55:02PM +0530, Kanchan Joshi wrote:
>> Sorry to hear about the trouble, but I am confused how did this
>> happen.
>> The old code also did not allow fabrics command and any other
>> admin command except id-ns variants. Yet this series had to
>> explicitly disallow admin cmds such create/delete sq/cq and
>> set-features.
>
>This is not a regression in your code, sorry if I made that
>impression.  This is a long-standing issue since these ioctls
>were originally added.

I see, good to know. I am still missing something.
This series is on top of nvme-6.2, since nvme_cmd_allowed did not exist
earlier.
In that case having this series or not having - gives the same effect,
no?

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

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 07:13:22PM +0530, Kanchan Joshi wrote:
> I see, good to know. I am still missing something.
> This series is on top of nvme-6.2, since nvme_cmd_allowed did not exist
> earlier.
> In that case having this series or not having - gives the same effect,
> no?

Yes, no change due to the series introducing nvme_cmd_allowed.
It is just a convenient place to put the checks.

Re: block dangerous passthrough operation

[Kanchan Joshi]

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

On Wed, Nov 16, 2022 at 04:44:15PM +0100, Christoph Hellwig wrote:
>On Wed, Nov 16, 2022 at 07:13:22PM +0530, Kanchan Joshi wrote:
>> I see, good to know. I am still missing something.
>> This series is on top of nvme-6.2, since nvme_cmd_allowed did not exist
>> earlier.
>> In that case having this series or not having - gives the same effect,
>> no?
>
>Yes, no change due to the series introducing nvme_cmd_allowed.
>It is just a convenient place to put the checks.

Got it now. The series is about restricting root/admin itself from doing
certain things.
If we end up going this route, putting a new helper seems clearer to me.
Something like this: 

if (capable(CAP_SYS_ADMIN)) {
	return admin_only_checks();
}
/* regular user checks as before */


But if there are people using the upstream driver for testing
nvme-hardware, restricting may not go well. Stuff like creating SQ/CQ
in early stages of new SSD/controller development may just be
the thing they want to test.


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

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Thu, Nov 17, 2022 at 08:43:36AM +0530, Kanchan Joshi wrote:
> But if there are people using the upstream driver for testing
> nvme-hardware, restricting may not go well. Stuff like creating SQ/CQ
> in early stages of new SSD/controller development may just be
> the thing they want to test.

How would that actually work?  If you want to play with low-level
details of the controller you better have something like a vfio
driver that is entirely in control.

Re: block dangerous passthrough operation

[Keith Busch]

On Wed, Nov 16, 2022 at 02:01:00PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> I recently had to debug some testing code that tried to send fabrics
> commands and caused a major havoc.  This series adds checks for various
> very low-level passthrough commands that have no business sent by anyone
> but the driver itself.  The list might now be complete, so discussion
> is welcome.

People do use the fabrics command for 'get property' to check on their
device, which is a harmless command that you've blocked.

There are still other harmful things a user could do, like Doorbell
Buffer Config or Set Feature Host Memory Buffer that could really screw
things up for the driver. But I think this sets a bad precedence that
the driver is going to protect an admin user from doing stupid things.
As more destructive opcodes and features are added in the future, we'd
be taking on a maintenance burden to analyze all these. Meanwhile, older
drivers won't provide that protection, so the user is expected to simply
not do such actions, so why can't they just do that now?

There's nothing preventing a vendor specific command from being just as
destructive either, but you can't realistically fence those off either.
I've always said the driver should not police this user interface as
that inevitably gets in the way of its flexibility.

Re: block dangerous passthrough operation

[Kanchan Joshi]

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

On Wed, Nov 16, 2022 at 09:12:08AM -0700, Keith Busch wrote:
>On Wed, Nov 16, 2022 at 02:01:00PM +0100, Christoph Hellwig wrote:
>> Hi all,
>>
>> I recently had to debug some testing code that tried to send fabrics
>> commands and caused a major havoc.  This series adds checks for various
>> very low-level passthrough commands that have no business sent by anyone
>> but the driver itself.  The list might now be complete, so discussion
>> is welcome.
>
>People do use the fabrics command for 'get property' to check on their
>device, which is a harmless command that you've blocked.

Not about this series, but should not fabrics command be allowed for
unprivileged users? Trying to understand the harm linked to that.

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

Re: block dangerous passthrough operation

[Keith Busch]

On Thu, Nov 17, 2022 at 09:21:21AM +0530, Kanchan Joshi wrote:
> On Wed, Nov 16, 2022 at 09:12:08AM -0700, Keith Busch wrote:
> > On Wed, Nov 16, 2022 at 02:01:00PM +0100, Christoph Hellwig wrote:
> > > Hi all,
> > > 
> > > I recently had to debug some testing code that tried to send fabrics
> > > commands and caused a major havoc.  This series adds checks for various
> > > very low-level passthrough commands that have no business sent by anyone
> > > but the driver itself.  The list might now be complete, so discussion
> > > is welcome.
> > 
> > People do use the fabrics command for 'get property' to check on their
> > device, which is a harmless command that you've blocked.
> 
> Not about this series, but should not fabrics command be allowed for
> unprivileged users? Trying to understand the harm linked to that.

The driver needs to own the fabrics connection setup. If users mess with
that directly, it can really confuse the driver as to the state of the
target and things will mysteriously stop working. Instead of issuing
passthrough fabrics commands, users should use the /dev/nvme-fabrics
special file.

Re: block dangerous passthrough operation

[Chaitanya Kulkarni]

> There's nothing preventing a vendor specific command from being just as
> destructive either, but you can't realistically fence those off either.
> I've always said the driver should not police this user interface as
> that inevitably gets in the way of its flexibility.
> 

The list will grow over time and we'll have to deal with all sorts of
patches/spec modifications and even if we all do that as mentioned
earlier I don't see any way to block Vendor specific commands ...

-ck

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 09:12:08AM -0700, Keith Busch wrote:
> People do use the fabrics command for 'get property' to check on their
> device, which is a harmless command that you've blocked.

Yeah, I guess read fabrics commnds are ok.

> There are still other harmful things a user could do, like Doorbell
> Buffer Config or Set Feature Host Memory Buffer that could really screw
> things up for the driver. But I think this sets a bad precedence that
> the driver is going to protect an admin user from doing stupid things.
> As more destructive opcodes and features are added in the future, we'd
> be taking on a maintenance burden to analyze all these. Meanwhile, older
> drivers won't provide that protection, so the user is expected to simply
> not do such actions, so why can't they just do that now?

It's a little less about preventing all possible problems, than about
at least letting people known they're doing something totally broken
and not report bugs to me about them..

Re: block dangerous passthrough operation

[Jens Axboe]

On 11/16/22 6:01 AM, Christoph Hellwig wrote:
> Hi all,
> 
> I recently had to debug some testing code that tried to send fabrics
> commands and caused a major havoc.  This series adds checks for various
> very low-level passthrough commands that have no business sent by anyone
> but the driver itself.  The list might now be complete, so discussion
> is welcome.

Honestly, I think this is a mess, and it's exactly why I didn't like
going down this path to begin with. I'm assuming that last 'now' above
should be a 'not'? This approach is not going to be maintainable.

-- 
Jens Axboe

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 08:49:42PM -0700, Jens Axboe wrote:
> Honestly, I think this is a mess, and it's exactly why I didn't like
> going down this path to begin with. I'm assuming that last 'now' above
> should be a 'not'? This approach is not going to be maintainable.

What else can be we?  Allow people to put the driver into all kinds of
weird states without us even knowing and just deal with it?

And yes, it is a mess but so is the complete lack of layering in
NVMe (and a lot else..).

Re: block dangerous passthrough operation

[Keith Busch]

On Mon, Nov 21, 2022 at 08:46:47AM +0100, Christoph Hellwig wrote:
> On Wed, Nov 16, 2022 at 08:49:42PM -0700, Jens Axboe wrote:
> > Honestly, I think this is a mess, and it's exactly why I didn't like
> > going down this path to begin with. I'm assuming that last 'now' above
> > should be a 'not'? This approach is not going to be maintainable.
> 
> What else can be we?  Allow people to put the driver into all kinds of
> weird states without us even knowing and just deal with it?

It's not like just anyone can do this kind of craziness. It's like a
soft equivalent of pulling a cable. There are usually access
restrictions for untrusted people.

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Mon, Nov 21, 2022 at 08:35:42AM -0700, Keith Busch wrote:
> It's not like just anyone can do this kind of craziness. It's like a
> soft equivalent of pulling a cable. There are usually access
> restrictions for untrusted people.

Yes, there are.  But that doesn't change us having to deal with the
fallout.

Re: block dangerous passthrough operation

[Sagi Grimberg]

>> It's not like just anyone can do this kind of craziness. It's like a
>> soft equivalent of pulling a cable. There are usually access
>> restrictions for untrusted people.
> 
> Yes, there are.  But that doesn't change us having to deal with the
> fallout.

Maybe we should do the inverse?
Whitelist what userspace is known to be allowed to do, and anything
outside of that, we don't prevent but rather log a big fat log message
that the driver behavior is unexpected?

That would prevent loss of flexibility, but still tell users that this
is not intended to be supported...

Re: block dangerous passthrough operation

[Christoph Hellwig]

On Tue, Nov 22, 2022 at 12:38:01PM +0200, Sagi Grimberg wrote:
> Maybe we should do the inverse?
> Whitelist what userspace is known to be allowed to do, and anything
> outside of that, we don't prevent but rather log a big fat log message
> that the driver behavior is unexpected?

Hmm, that list is for sure going to be a lot bigger..

Re: block dangerous passthrough operation

[Keith Busch]

On Tue, Nov 22, 2022 at 07:47:43AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 21, 2022 at 08:35:42AM -0700, Keith Busch wrote:
> > It's not like just anyone can do this kind of craziness. It's like a
> > soft equivalent of pulling a cable. There are usually access
> > restrictions for untrusted people.
> 
> Yes, there are.  But that doesn't change us having to deal with the
> fallout.

What exactly happened here? If I got a bug report that says "it broke
when I did this admin command", the answer is "don't do that. *plonk*".

Did you spend too much time on a bug report that didn't include repro
steps or something?

If you really need to do this, I think augmenting the
nvme_known_admin_effects() with a driver specific flag saying "Do Not
Execute" or something like that provides a more centralized location for
this type of check.