[PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular

[Chaitanya Kulkarni]

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 at 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