[PATCH v5 0/1] nvme : Add ctrl id to allowed passthru commands

[PATCH v5 0/1] nvme : Add ctrl id to allowed passthru commands

[Joel Granados]

What?
In this patch I add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the nvme
admin commands that are allowed in unprivileged passthru.

Why?
This will allow access to attributes that are needed to effectively write
to the char device in passthru.  Applications with write permissions should
not need to be privileged to write to the device. With Kanchan's latest patch
(https://lore.kernel.org/linux-nvme/20221020070205.57366-1-joshi.k@samsung.com/)
the nvme IO and identify commands in passthru now follow device
permissions; however there are still some controller attributes like
minimal data transfer size (MDTS) which need a privileged user to be
queried.

How?
Add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the allow list in
nvme_cmd_allowed.

V5:
* Drop the ioclt implementation in favor of just adding the controller id
  commands to the allow list in nvme_cmd_allowed. This resulted after a
  comment from Kieth Busch pointing out that adding a struct and an ioctl
  for stuff that is already there, makes no sense.  IMO the ioctl
  patch is mostly done and is there if we need it.

V4:
* Fixed an error where the ioctl number would change if new members were
  added. Now I use _IO instead of _IOWR to avoid leave the ioctl number
  static. This is very similar to the implementation contained in
  https://github.com/torvalds/linux/blob/master/include/uapi/linux/vfio.h

V3:
* Removed unneeded comments in nvme_ioctl.h
* Added a comment to the nvme_sectors_to_mps function
* Moved size checks to nvme_check_size in core.h
* Changed struct initialization to match what we use in nvme driver {} vs
  {0}

V2:
* Changed comment from // to /**/
* Took a call out from an if condition and assigned it to ret var.

Joel Granados (1):
  nvme : Add ctrl id to allowed passthru commands

 drivers/nvme/host/ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.30.2

[PATCH v5 1/1] nvme : Add ctrl id to allowed passthru commands

[Joel Granados]

Add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the nvme admin commands
that are allowed in unprivileged passthru. This will allow access to
attributes that are needed to effectively write to the char device in
passthru.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/nvme/host/ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9550a69029b3..9ddda571f046 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -34,6 +34,8 @@ 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:
+			case NVME_ID_CNS_CS_CTRL:
+			case NVME_ID_CNS_CTRL:
 				return true;
 			}
 		}
-- 
2.30.2

Re: [PATCH v5 1/1] nvme : Add ctrl id to allowed passthru commands

[Keith Busch]

On Mon, Dec 05, 2022 at 03:27:46PM +0100, Joel Granados wrote:
> Add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the nvme admin commands
> that are allowed in unprivileged passthru. This will allow access to
> attributes that are needed to effectively write to the char device in
> passthru.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>

I initially thought just open the entire nvme_identify_controller
opcode, but this looks good to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

> ---
>  drivers/nvme/host/ioctl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 9550a69029b3..9ddda571f046 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -34,6 +34,8 @@ 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:
> +			case NVME_ID_CNS_CS_CTRL:
> +			case NVME_ID_CNS_CTRL:
>  				return true;
>  			}
>  		}
> -- 
> 2.30.2
> 

Re: [PATCH v5 1/1] nvme : Add ctrl id to allowed passthru commands

[Chaitanya Kulkarni]

On 12/5/22 06:27, Joel Granados wrote:
> Add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the nvme admin commands
> that are allowed in unprivileged passthru. This will allow access to
> attributes that are needed to effectively write to the char device in
> passthru.
> 

Perhaps mention what attributes are needed for clarity.

Either way, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v5 1/1] nvme : Add ctrl id to allowed passthru commands

[Joel Granados]

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

On Tue, Dec 06, 2022 at 04:25:59AM +0000, Chaitanya Kulkarni wrote:
> On 12/5/22 06:27, Joel Granados wrote:
> > Add NVME_ID_CNS_CS_CTRL and NVME_ID_CNS_CTRL to the nvme admin commands
> > that are allowed in unprivileged passthru. This will allow access to
> > attributes that are needed to effectively write to the char device in
> > passthru.
> > 
> 
> Perhaps mention what attributes are needed for clarity.

I'll add two examples of the attribute names to the commit message and
leave it at that. Hope that is OK, as having a list of all attributes
that might be used for a write operation will get long.

> 
> Either way, looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> -ck
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5 1/1] nvme : Add ctrl id to allowed passthru commands

[Christoph Hellwig]

On Mon, Dec 05, 2022 at 03:35:17PM +0000, Keith Busch wrote:
> I initially thought just open the entire nvme_identify_controller
> opcode, but this looks good to me.

But that's not an opcode.  The opcode is identify, and then everything
else is multiplexed by the CNS value, even if the names of the
different CNS values sound like there might be another sub grouping.