Re: [PATCH for-next 1/2] nvme: add the permission-policy for command control

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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