[PATCH] nvme: restrict management ioctls to admin

[PATCH] nvme: restrict management ioctls to admin

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The passthrough commands already have this restriction, but the other
operations do not. Require the same capabilities for all users as all of
these operations can be disruptive.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 27614bee7380..d23978e993c6 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -749,6 +749,9 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 	struct nvme_ctrl *ctrl = file->private_data;
 	void __user *argp = (void __user *)arg;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
 		return nvme_user_cmd(ctrl, NULL, argp);
-- 
2.30.2

Re: [PATCH] nvme: restrict management ioctls to admin

[Kanchan Joshi]

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

On Thu, Sep 08, 2022 at 02:35:17PM -0700, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>The passthrough commands already have this restriction, but the other
>operations do not. Require the same capabilities for all users as all of
>these operations can be disruptive.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/ioctl.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>index 27614bee7380..d23978e993c6 100644
>--- a/drivers/nvme/host/ioctl.c
>+++ b/drivers/nvme/host/ioctl.c
>@@ -749,6 +749,9 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd,
> 	struct nvme_ctrl *ctrl = file->private_data;
> 	void __user *argp = (void __user *)arg;
>
>+	if (!capable(CAP_SYS_ADMIN))
>+		return -EACCES;
>+

I too came across this and wondered why this check is not present here.

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

It's a different matter that I was looking at increasing the access
granularity (something like scsi whitelisting). Hope to have
conversations on a RFC soon.
>
>

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

Re: [PATCH] nvme: restrict management ioctls to admin

[Jens Axboe]

On 9/8/22 3:35 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The passthrough commands already have this restriction, but the other
> operations do not. Require the same capabilities for all users as all of
> these operations can be disruptive.

Would it be saner to require that you have write permissions on the
opened device? Not sure this CAP_SYS_ADMIN is really necessary,
and might break existing setups as this is new.

I do agree that passthrough and sync ioctl based issue should be
consistent, though.

-- 
Jens Axboe

Re: [PATCH] nvme: restrict management ioctls to admin

[Christoph Hellwig]

On Thu, Sep 08, 2022 at 02:35:17PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The passthrough commands already have this restriction, but the other
> operations do not. Require the same capabilities for all users as all of
> these operations can be disruptive.

Where "these operations" are: NVME_IOCTL_RESET, NVME_IOCTL_SUBSYS_RESET,
NVME_IOCTL_RESCAN.  Yes, I think those are very disruptive and the
definition of what CAP_SYS_ADMIN was designed for, but that should
be spelled out in the commit log.

That being said I think we should just do the capable() check in the
individul ioctl opcode to document things better, even if that does't
currently make any difference.  Any we really need a FIXES tag going
back to the addition of the first of these ioctls.

Re: [PATCH] nvme: restrict management ioctls to admin

[Sagi Grimberg]

>> The passthrough commands already have this restriction, but the other
>> operations do not. Require the same capabilities for all users as all of
>> these operations can be disruptive.
> 
> Where "these operations" are: NVME_IOCTL_RESET, NVME_IOCTL_SUBSYS_RESET,
> NVME_IOCTL_RESCAN.  Yes, I think those are very disruptive and the
> definition of what CAP_SYS_ADMIN was designed for, but that should
> be spelled out in the commit log.
> 
> That being said I think we should just do the capable() check in the
> individul ioctl opcode to document things better, even if that does't
> currently make any difference.

Agree.

>  Any we really need a FIXES tag going
> back to the addition of the first of these ioctls.

100% agree on the fixes tag