* [PATCH for-next 0/2] Fine-granular CAP_SYS_ADMIN
[not found] <CGME20220926150436epcas5p4fd7f1945793cded05910da5c5094805e@epcas5p4.samsung.com>
@ 2022-09-26 14:54 ` Kanchan Joshi
[not found] ` <CGME20220926150439epcas5p160506693c35f94eec80f26e34027988f@epcas5p1.samsung.com>
[not found] ` <CGME20220926150442epcas5p2d2258d9799e47a49523d36b1a852dc9c@epcas5p2.samsung.com>
0 siblings, 2 replies; 9+ messages in thread
From: Kanchan Joshi @ 2022-09-26 14:54 UTC (permalink / raw)
To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi
Current nvme passthrough interface is more useful than it used to be.
Now that user-space has more reasons to pick this path, the existing
CAP_SYS_ADMIN based checks are worth a revisit.
Currently both io and admin commands are kept under a
coarse-granular CAP_SYS_ADMIN check, without any regard to file open
mode.
$ ls -l /dev/ng*
crw-rw-rw- 1 root root 242, 0 Sep 9 19:20 /dev/ng0n1
crw------- 1 root root 242, 1 Sep 9 19:20 /dev/ng0n2
In the example above, ng0n1 appears as if it may allow unprivileged
read/write operation but it does not and behaves same as ng0n2.
The series attempts a shift from CAP_SYS_ADMIN to more fine-granular
control for io-commands. This is somewhat similar to scsi whitelisting.
Patch 1: contains the new policy for io command control
Patch 2: changes the callers to use that
Kanchan Joshi (2):
nvme: add the permission-policy for command control
nvme: Make CAP_SYS_ADMIN fine-granular
drivers/nvme/host/ioctl.c | 89 +++++++++++++++++++++++++--------------
include/linux/nvme.h | 1 +
2 files changed, 58 insertions(+), 32 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH for-next 1/2] nvme: add the permission-policy for command control
[not found] ` <CGME20220926150439epcas5p160506693c35f94eec80f26e34027988f@epcas5p1.samsung.com>
@ 2022-09-26 14:54 ` Kanchan Joshi
2022-09-26 22:25 ` Chaitanya Kulkarni
2022-09-27 7:31 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Kanchan Joshi @ 2022-09-26 14:54 UTC (permalink / raw)
To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi
If CAP_SYS_ADMIN is present, nothing else is checked as before.
Otherwise takes the decision to allow the command based on following
rules:
- any admin-cmd is not allowed
- vendor-specific and fabric commmand are not allowed
- io-commands that can write are allowed if matching FMODE_WRITE
permission is present
- io-commands that read are allowed
Add a helper nvme_cmd_allowed that implements above policy. This is a prep
patch.
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
drivers/nvme/host/ioctl.c | 19 +++++++++++++++++++
include/linux/nvme.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 548aca8b5b9f..6ca6477dd899 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -20,6 +20,25 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
return (void __user *)ptrval;
}
+bool nvme_cmd_allowed(struct nvme_ns *ns, u8 opcode, fmode_t mode)
+{
+ /* root can do anything */
+ if (capable(CAP_SYS_ADMIN))
+ return true;
+ /* admin commands are not allowed */
+ if (ns == NULL)
+ return false;
+ /* exclude vendor-specific io and fabrics commands */
+ if (opcode >= nvme_cmd_vendor_start ||
+ opcode== nvme_fabrics_command)
+ return false;
+ /* allow write cmds only if matching FMODE is present */
+ if (opcode & 1)
+ return mode & FMODE_WRITE;
+ /* allow read cmds */
+ return true;
+}
+
static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
unsigned len, u32 seed, bool write)
{
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index ae53d74f3696..8396eb7ecb68 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -797,6 +797,7 @@ enum nvme_opcode {
nvme_cmd_zone_mgmt_send = 0x79,
nvme_cmd_zone_mgmt_recv = 0x7a,
nvme_cmd_zone_append = 0x7d,
+ nvme_cmd_vendor_start = 0x80,
};
#define nvme_opcode_name(opcode) { opcode, #opcode }
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular
[not found] ` <CGME20220926150442epcas5p2d2258d9799e47a49523d36b1a852dc9c@epcas5p2.samsung.com>
@ 2022-09-26 14:54 ` Kanchan Joshi
2022-09-26 22:30 ` Chaitanya Kulkarni
2022-09-27 7:32 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Kanchan Joshi @ 2022-09-26 14:54 UTC (permalink / raw)
To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi
Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
for any decision making.
Since file open mode is taken into consideration for any
approval/denial, change at various places to keep file-mode information
handy.
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
1 file changed, 38 insertions(+), 32 deletions(-)
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 6ca6477dd899..4e53a01e702d 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
}
static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
- struct nvme_passthru_cmd __user *ucmd)
+ struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
{
struct nvme_passthru_cmd cmd;
struct nvme_command c;
@@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
u64 result;
int status;
- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
return -EFAULT;
+ if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
+ return -EACCES;
if (cmd.flags)
return -EINVAL;
if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
@@ -306,17 +306,18 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
}
static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
- struct nvme_passthru_cmd64 __user *ucmd, bool vec)
+ struct nvme_passthru_cmd64 __user *ucmd, bool vec,
+ fmode_t mode)
{
struct nvme_passthru_cmd64 cmd;
struct nvme_command c;
unsigned timeout = 0;
int status;
- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
return -EFAULT;
+ if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
+ return -EACCES;
if (cmd.flags)
return -EINVAL;
if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
@@ -438,14 +439,14 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
blk_mq_req_flags_t blk_flags = 0;
void *meta = NULL;
- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
-
c.common.opcode = READ_ONCE(cmd->opcode);
c.common.flags = READ_ONCE(cmd->flags);
if (c.common.flags)
return -EINVAL;
+ if (!nvme_cmd_allowed(ns, c.common.opcode, ioucmd->file->f_mode))
+ return -EACCES;
+
c.common.command_id = 0;
c.common.nsid = cpu_to_le32(cmd->nsid);
if (!nvme_validate_passthru_nsid(ctrl, ns, le32_to_cpu(c.common.nsid)))
@@ -517,13 +518,13 @@ static bool is_ctrl_ioctl(unsigned int cmd)
}
static int nvme_ctrl_ioctl(struct nvme_ctrl *ctrl, unsigned int cmd,
- void __user *argp)
+ void __user *argp, fmode_t mode)
{
switch (cmd) {
case NVME_IOCTL_ADMIN_CMD:
- return nvme_user_cmd(ctrl, NULL, argp);
+ return nvme_user_cmd(ctrl, NULL, argp, mode);
case NVME_IOCTL_ADMIN64_CMD:
- return nvme_user_cmd64(ctrl, NULL, argp, false);
+ return nvme_user_cmd64(ctrl, NULL, argp, false, mode);
default:
return sed_ioctl(ctrl->opal_dev, cmd, argp);
}
@@ -548,14 +549,14 @@ struct nvme_user_io32 {
#endif /* COMPAT_FOR_U64_ALIGNMENT */
static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
- void __user *argp)
+ void __user *argp, fmode_t mode)
{
switch (cmd) {
case NVME_IOCTL_ID:
force_successful_syscall_return();
return ns->head->ns_id;
case NVME_IOCTL_IO_CMD:
- return nvme_user_cmd(ns->ctrl, ns, argp);
+ return nvme_user_cmd(ns->ctrl, ns, argp, mode);
/*
* struct nvme_user_io can have different padding on some 32-bit ABIs.
* Just accept the compat version as all fields that are used are the
@@ -567,19 +568,20 @@ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
case NVME_IOCTL_SUBMIT_IO:
return nvme_submit_io(ns, argp);
case NVME_IOCTL_IO64_CMD:
- return nvme_user_cmd64(ns->ctrl, ns, argp, false);
+ return nvme_user_cmd64(ns->ctrl, ns, argp, false, mode);
case NVME_IOCTL_IO64_CMD_VEC:
- return nvme_user_cmd64(ns->ctrl, ns, argp, true);
+ return nvme_user_cmd64(ns->ctrl, ns, argp, true, mode);
default:
return -ENOTTY;
}
}
-static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg)
+static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg,
+ fmode_t mode)
{
if (is_ctrl_ioctl(cmd))
- return nvme_ctrl_ioctl(ns->ctrl, cmd, arg);
- return nvme_ns_ioctl(ns, cmd, arg);
+ return nvme_ctrl_ioctl(ns->ctrl, cmd, arg, mode);
+ return nvme_ns_ioctl(ns, cmd, arg, mode);
}
int nvme_ioctl(struct block_device *bdev, fmode_t mode,
@@ -587,7 +589,7 @@ int nvme_ioctl(struct block_device *bdev, fmode_t mode,
{
struct nvme_ns *ns = bdev->bd_disk->private_data;
- return __nvme_ioctl(ns, cmd, (void __user *)arg);
+ return __nvme_ioctl(ns, cmd, (void __user *)arg, mode);
}
long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -595,7 +597,7 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
struct nvme_ns *ns =
container_of(file_inode(file)->i_cdev, struct nvme_ns, cdev);
- return __nvme_ioctl(ns, cmd, (void __user *)arg);
+ return __nvme_ioctl(ns, cmd, (void __user *)arg, file->f_mode);
}
static int nvme_uring_cmd_checks(unsigned int issue_flags)
@@ -663,7 +665,8 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
}
#ifdef CONFIG_NVME_MULTIPATH
static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
- void __user *argp, struct nvme_ns_head *head, int srcu_idx)
+ void __user *argp, struct nvme_ns_head *head, int srcu_idx,
+ fmode_t mode)
__releases(&head->srcu)
{
struct nvme_ctrl *ctrl = ns->ctrl;
@@ -671,7 +674,7 @@ static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
nvme_get_ctrl(ns->ctrl);
srcu_read_unlock(&head->srcu, srcu_idx);
- ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp);
+ ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp, mode);
nvme_put_ctrl(ctrl);
return ret;
@@ -696,9 +699,10 @@ int nvme_ns_head_ioctl(struct block_device *bdev, fmode_t mode,
* deadlock when deleting namespaces using the passthrough interface.
*/
if (is_ctrl_ioctl(cmd))
- return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
+ return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
+ mode);
- ret = nvme_ns_ioctl(ns, cmd, argp);
+ ret = nvme_ns_ioctl(ns, cmd, argp, mode);
out_unlock:
srcu_read_unlock(&head->srcu, srcu_idx);
return ret;
@@ -720,9 +724,10 @@ long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
goto out_unlock;
if (is_ctrl_ioctl(cmd))
- return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
+ return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
+ file->f_mode);
- ret = nvme_ns_ioctl(ns, cmd, argp);
+ ret = nvme_ns_ioctl(ns, cmd, argp, file->f_mode);
out_unlock:
srcu_read_unlock(&head->srcu, srcu_idx);
return ret;
@@ -796,7 +801,8 @@ int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
return ret;
}
-static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
+static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp,
+ fmode_t mode)
{
struct nvme_ns *ns;
int ret;
@@ -820,7 +826,7 @@ static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
kref_get(&ns->kref);
up_read(&ctrl->namespaces_rwsem);
- ret = nvme_user_cmd(ctrl, ns, argp);
+ ret = nvme_user_cmd(ctrl, ns, argp, mode);
nvme_put_ns(ns);
return ret;
@@ -837,11 +843,11 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd,
switch (cmd) {
case NVME_IOCTL_ADMIN_CMD:
- return nvme_user_cmd(ctrl, NULL, argp);
+ return nvme_user_cmd(ctrl, NULL, argp, file->f_mode);
case NVME_IOCTL_ADMIN64_CMD:
- return nvme_user_cmd64(ctrl, NULL, argp, false);
+ return nvme_user_cmd64(ctrl, NULL, argp, false, file->f_mode);
case NVME_IOCTL_IO_CMD:
- return nvme_dev_user_cmd(ctrl, argp);
+ return nvme_dev_user_cmd(ctrl, argp, file->f_mode);
case NVME_IOCTL_RESET:
dev_warn(ctrl->device, "resetting controller\n");
return nvme_reset_ctrl_sync(ctrl);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 1/2] nvme: add the permission-policy for command control
2022-09-26 14:54 ` [PATCH for-next 1/2] nvme: add the permission-policy for command control Kanchan Joshi
@ 2022-09-26 22:25 ` Chaitanya Kulkarni
2022-09-27 7:31 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Chaitanya Kulkarni @ 2022-09-26 22:25 UTC (permalink / raw)
To: Kanchan Joshi, hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev
On 9/26/22 07:54, Kanchan Joshi wrote:
> If CAP_SYS_ADMIN is present, nothing else is checked as before.
> Otherwise takes the decision to allow the command based on following
> rules:
> - any admin-cmd is not allowed
> - vendor-specific and fabric commmand are not allowed
> - io-commands that can write are allowed if matching FMODE_WRITE
> permission is present
> - io-commands that read are allowed
>
> Add a helper nvme_cmd_allowed that implements above policy. This is a prep
> patch.
>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
> drivers/nvme/host/ioctl.c | 19 +++++++++++++++++++
> include/linux/nvme.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 548aca8b5b9f..6ca6477dd899 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -20,6 +20,25 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
> return (void __user *)ptrval;
> }
>
> +bool nvme_cmd_allowed(struct nvme_ns *ns, u8 opcode, fmode_t mode)
> +{
> + /* root can do anything */
I think above comment is redundant, CAP_SYS_ADMIN is self explanatory.
> + if (capable(CAP_SYS_ADMIN))
> + return true;
> + /* admin commands are not allowed */
by looking at the callers of this function it is evident that ns is NULL
for admin-cmds only so maybe we can remove above comment also.
> + if (ns == NULL)
> + return false;
> + /* exclude vendor-specific io and fabrics commands */
again very clear if condition is self explanatory.
> + if (opcode >= nvme_cmd_vendor_start ||
> + opcode== nvme_fabrics_command)
> + return false;
> + /* allow write cmds only if matching FMODE is present */
> + if (opcode & 1)
> + return mode & FMODE_WRITE;
> + /* allow read cmds */
> + return true;
> +}
> +
This patch needs to be merged with the next patch as there are no
callers to above function in this patch, unless there is a specific
review comment that says that we need a prep patch for this.
-ck
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular
2022-09-26 14:54 ` [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular Kanchan Joshi
@ 2022-09-26 22:30 ` Chaitanya Kulkarni
2022-09-27 17:06 ` Kanchan Joshi
2022-09-27 7:32 ` Christoph Hellwig
1 sibling, 1 reply; 9+ messages in thread
From: Chaitanya Kulkarni @ 2022-09-26 22:30 UTC (permalink / raw)
To: Kanchan Joshi; +Cc: linux-nvme, hch, axboe, kbusch, gost.dev, sagi
On 9/26/22 07:54, Kanchan Joshi wrote:
> Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
> for any decision making.
> Since file open mode is taken into consideration for any
> approval/denial, change at various places to keep file-mode information
> handy.
>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
> drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
> 1 file changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 6ca6477dd899..4e53a01e702d 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
> }
>
> static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> - struct nvme_passthru_cmd __user *ucmd)
> + struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
> {
> struct nvme_passthru_cmd cmd;
> struct nvme_command c;
> @@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> u64 result;
> int status;
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
> return -EFAULT;
> + if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
> + return -EACCES;
you are chaning the order of the check CAP_SYS_ADMIN, unless there is a
specific reason for it (that is not listed in the commit log) move
nvme_cmd_allowed() where CAP_SYS_ADMIN is to retain the original
behaviour which seems right since you are avoiding kernel copy in case
cmds are not allowed.
> if (cmd.flags)
> return -EINVAL;
> if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
> @@ -306,17 +306,18 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> }
>
> static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> - struct nvme_passthru_cmd64 __user *ucmd, bool vec)
> + struct nvme_passthru_cmd64 __user *ucmd, bool vec,
> + fmode_t mode)
> {
> struct nvme_passthru_cmd64 cmd;
> struct nvme_command c;
> unsigned timeout = 0;
> int status;
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
> return -EFAULT;
> + if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
> + return -EACCES;
same as above.
> if (cmd.flags)
> return -EINVAL;
> if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
> @@ -438,14 +439,14 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> blk_mq_req_flags_t blk_flags = 0;
> void *meta = NULL;
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> -
> c.common.opcode = READ_ONCE(cmd->opcode);
> c.common.flags = READ_ONCE(cmd->flags);
> if (c.common.flags)
> return -EINVAL;
>
> + if (!nvme_cmd_allowed(ns, c.common.opcode, ioucmd->file->f_mode))
> + return -EACCES;
> +
same as above, why initialize c.common.xxx if cmd is not allowed.
> c.common.command_id = 0;
> c.common.nsid = cpu_to_le32(cmd->nsid);
> if (!nvme_validate_passthru_nsid(ctrl, ns, le32_to_cpu(c.common.nsid)))
> @@ -517,13 +518,13 @@ static bool is_ctrl_ioctl(unsigned int cmd)
> }
>
-ck
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 1/2] nvme: add the permission-policy for command control
2022-09-26 14:54 ` [PATCH for-next 1/2] nvme: add the permission-policy for command control Kanchan Joshi
2022-09-26 22:25 ` Chaitanya Kulkarni
@ 2022-09-27 7:31 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2022-09-27 7:31 UTC (permalink / raw)
To: Kanchan Joshi; +Cc: hch, kbusch, sagi, axboe, linux-nvme, gost.dev
> +bool nvme_cmd_allowed(struct nvme_ns *ns, u8 opcode, fmode_t mode)
This adds an unused function, so I think it should be merged into the
next patch to have one coherent change.
> +{
> + /* root can do anything */
> + if (capable(CAP_SYS_ADMIN))
> + return true;
> + /* admin commands are not allowed */
Empty lines between the check would be nice for readability.
> + if (ns == NULL)
if (!ns)
> + /* exclude vendor-specific io and fabrics commands */
> + if (opcode >= nvme_cmd_vendor_start ||
> + opcode== nvme_fabrics_command)
Odd indentation here, this should be:
if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command)
> + /* allow write cmds only if matching FMODE is present */
> + if (opcode & 1)
> + return mode & FMODE_WRITE;
> + /* allow read cmds */
/* allow read cmds when the device permissions allow access */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular
2022-09-26 14:54 ` [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular Kanchan Joshi
2022-09-26 22:30 ` Chaitanya Kulkarni
@ 2022-09-27 7:32 ` Christoph Hellwig
2022-09-27 17:50 ` Kanchan Joshi
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2022-09-27 7:32 UTC (permalink / raw)
To: Kanchan Joshi; +Cc: hch, kbusch, sagi, axboe, linux-nvme, gost.dev
On Mon, Sep 26, 2022 at 08:24:30PM +0530, Kanchan Joshi wrote:
> Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
> for any decision making.
> Since file open mode is taken into consideration for any
> approval/denial, change at various places to keep file-mode information
> handy.
>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
> drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
> 1 file changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 6ca6477dd899..4e53a01e702d 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
> }
>
> static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> - struct nvme_passthru_cmd __user *ucmd)
> + struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
> {
> struct nvme_passthru_cmd cmd;
> struct nvme_command c;
> @@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> u64 result;
> int status;
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
> return -EFAULT;
> + if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
> + return -EACCES;
Btw, what speaks against moving this check a little bit later,
so that we can pass the nvme_command to nvme_cmd_allowed, and thus
can simply use nvme_is_write?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular
2022-09-26 22:30 ` Chaitanya Kulkarni
@ 2022-09-27 17:06 ` Kanchan Joshi
0 siblings, 0 replies; 9+ messages in thread
From: Kanchan Joshi @ 2022-09-27 17:06 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: linux-nvme, hch, axboe, kbusch, gost.dev, sagi
[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]
On Mon, Sep 26, 2022 at 10:30:14PM +0000, Chaitanya Kulkarni wrote:
>On 9/26/22 07:54, Kanchan Joshi wrote:
>> Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
>> for any decision making.
>> Since file open mode is taken into consideration for any
>> approval/denial, change at various places to keep file-mode information
>> handy.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>> drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
>> 1 file changed, 38 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>> index 6ca6477dd899..4e53a01e702d 100644
>> --- a/drivers/nvme/host/ioctl.c
>> +++ b/drivers/nvme/host/ioctl.c
>> @@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
>> }
>>
>> static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>> - struct nvme_passthru_cmd __user *ucmd)
>> + struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
>> {
>> struct nvme_passthru_cmd cmd;
>> struct nvme_command c;
>> @@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>> u64 result;
>> int status;
>>
>> - if (!capable(CAP_SYS_ADMIN))
>> - return -EACCES;
>> if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
>> return -EFAULT;
>> + if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
>> + return -EACCES;
>
>you are chaning the order of the check CAP_SYS_ADMIN, unless there is a
>specific reason for it (that is not listed in the commit log) move
>nvme_cmd_allowed() where CAP_SYS_ADMIN is to retain the original
>behaviour which seems right since you are avoiding kernel copy in case
>cmds are not allowed.
cmd.opcode is required to make the decision making. So it cannot be
moved any up.
User-space does not come to know whether error comes before/after
kernel-copy, so that part does not fall into behavior-change category.
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular
2022-09-27 7:32 ` Christoph Hellwig
@ 2022-09-27 17:50 ` Kanchan Joshi
0 siblings, 0 replies; 9+ messages in thread
From: Kanchan Joshi @ 2022-09-27 17:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: kbusch, sagi, axboe, linux-nvme, gost.dev
[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]
On Tue, Sep 27, 2022 at 09:32:50AM +0200, Christoph Hellwig wrote:
>On Mon, Sep 26, 2022 at 08:24:30PM +0530, Kanchan Joshi wrote:
>> Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
>> for any decision making.
>> Since file open mode is taken into consideration for any
>> approval/denial, change at various places to keep file-mode information
>> handy.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>> drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
>> 1 file changed, 38 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>> index 6ca6477dd899..4e53a01e702d 100644
>> --- a/drivers/nvme/host/ioctl.c
>> +++ b/drivers/nvme/host/ioctl.c
>> @@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
>> }
>>
>> static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>> - struct nvme_passthru_cmd __user *ucmd)
>> + struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
>> {
>> struct nvme_passthru_cmd cmd;
>> struct nvme_command c;
>> @@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>> u64 result;
>> int status;
>>
>> - if (!capable(CAP_SYS_ADMIN))
>> - return -EACCES;
>> if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
>> return -EFAULT;
>> + if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
>> + return -EACCES;
>
>Btw, what speaks against moving this check a little bit later,
>so that we can pass the nvme_command to nvme_cmd_allowed, and thus
>can simply use nvme_is_write?
Moving down is good to me. Just after we have put the opcode into
nvme_command. Will do that in v2.
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-27 18:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20220926150436epcas5p4fd7f1945793cded05910da5c5094805e@epcas5p4.samsung.com>
2022-09-26 14:54 ` [PATCH for-next 0/2] Fine-granular CAP_SYS_ADMIN Kanchan Joshi
[not found] ` <CGME20220926150439epcas5p160506693c35f94eec80f26e34027988f@epcas5p1.samsung.com>
2022-09-26 14:54 ` [PATCH for-next 1/2] nvme: add the permission-policy for command control Kanchan Joshi
2022-09-26 22:25 ` Chaitanya Kulkarni
2022-09-27 7:31 ` Christoph Hellwig
[not found] ` <CGME20220926150442epcas5p2d2258d9799e47a49523d36b1a852dc9c@epcas5p2.samsung.com>
2022-09-26 14:54 ` [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular Kanchan Joshi
2022-09-26 22:30 ` Chaitanya Kulkarni
2022-09-27 17:06 ` Kanchan Joshi
2022-09-27 7:32 ` Christoph Hellwig
2022-09-27 17:50 ` Kanchan Joshi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).