* Re: [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs
2022-11-29 9:00 ` [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs Christoph Hellwig
@ 2022-11-29 17:54 ` Chaitanya Kulkarni
2022-11-30 8:34 ` Sagi Grimberg
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Chaitanya Kulkarni @ 2022-11-29 17:54 UTC (permalink / raw)
To: Christoph Hellwig, kbusch@kernel.org, sagi@grimberg.me,
joshi.k@samsung.com
Cc: linux-nvme@lists.infradead.org
On 11/29/22 01:00, Christoph Hellwig wrote:
> Unfortunately Write Zeroes is coded as a no data transfer opcode in NVMe,
> so don't allow it on a read-only FD for unprivileged users.
>
> Fixes: 855b7717f44b ("nvme: fine-granular CAP_SYS_ADMIN for nvme io commands")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 9550a69029b368..8aefe9c904dc9a 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -45,7 +45,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> * special file is open for writing, but always allow I/O commands that
> * transfer data from the controller.
> */
> - if (nvme_is_write(c))
> + if (nvme_is_write(c) || c->common.opcode == nvme_cmd_write_zeroes)
> return mode & FMODE_WRITE;
> return true;
> }
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs
2022-11-29 9:00 ` [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs Christoph Hellwig
2022-11-29 17:54 ` Chaitanya Kulkarni
@ 2022-11-30 8:34 ` Sagi Grimberg
2022-11-30 23:20 ` Kanchan Joshi
2022-12-01 16:07 ` Keith Busch
3 siblings, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2022-11-30 8:34 UTC (permalink / raw)
To: Christoph Hellwig, kbusch, joshi.k; +Cc: linux-nvme
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs
2022-11-29 9:00 ` [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs Christoph Hellwig
2022-11-29 17:54 ` Chaitanya Kulkarni
2022-11-30 8:34 ` Sagi Grimberg
@ 2022-11-30 23:20 ` Kanchan Joshi
2022-12-01 16:07 ` Keith Busch
3 siblings, 0 replies; 6+ messages in thread
From: Kanchan Joshi @ 2022-11-30 23:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: kbusch, sagi, linux-nvme
[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]
On Tue, Nov 29, 2022 at 10:00:16AM +0100, Christoph Hellwig wrote:
>Unfortunately Write Zeroes is coded as a no data transfer opcode in NVMe,
>so don't allow it on a read-only FD for unprivileged users.
>
>Fixes: 855b7717f44b ("nvme: fine-granular CAP_SYS_ADMIN for nvme io commands")
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> drivers/nvme/host/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>index 9550a69029b368..8aefe9c904dc9a 100644
>--- a/drivers/nvme/host/ioctl.c
>+++ b/drivers/nvme/host/ioctl.c
>@@ -45,7 +45,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> * special file is open for writing, but always allow I/O commands that
> * transfer data from the controller.
> */
>- if (nvme_is_write(c))
>+ if (nvme_is_write(c) || c->common.opcode == nvme_cmd_write_zeroes)
> return mode & FMODE_WRITE;
I was thinking why check for write_zeroes should not go inside nvme_is_write
itself. Then I saw various callers of nvme_is_write, and that killed the
thought.
Another thought is - does it make sense to include nvme_cmd_flush too?
That is also declared as no-data-transfer in spec. Flush alone can't
make any difference when writes are not allowed in first place. So this
is about whether we care for empty flushes.
And on spec - not sure whether the criteria has changed of late. copy
command also does not involve data-transfer but bit is set there.
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs
2022-11-29 9:00 ` [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs Christoph Hellwig
` (2 preceding siblings ...)
2022-11-30 23:20 ` Kanchan Joshi
@ 2022-12-01 16:07 ` Keith Busch
2022-12-01 16:09 ` Christoph Hellwig
3 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2022-12-01 16:07 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, joshi.k, linux-nvme
On Tue, Nov 29, 2022 at 10:00:16AM +0100, Christoph Hellwig wrote:
> - if (nvme_is_write(c))
> + if (nvme_is_write(c) || c->common.opcode == nvme_cmd_write_zeroes)
> return mode & FMODE_WRITE;
Write Uncorrectable should also be checked, and any future opcodes that
can modify media. Maybe use Command Effects Log's LBCC field instead? We
can preload known effects for older nvme's that don't support that log
page.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme: don't allow unprivileged Write Zeroes passthrough on read-only FDs
2022-12-01 16:07 ` Keith Busch
@ 2022-12-01 16:09 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2022-12-01 16:09 UTC (permalink / raw)
To: Keith Busch; +Cc: Christoph Hellwig, sagi, joshi.k, linux-nvme
On Thu, Dec 01, 2022 at 09:07:38AM -0700, Keith Busch wrote:
> On Tue, Nov 29, 2022 at 10:00:16AM +0100, Christoph Hellwig wrote:
> > - if (nvme_is_write(c))
> > + if (nvme_is_write(c) || c->common.opcode == nvme_cmd_write_zeroes)
> > return mode & FMODE_WRITE;
>
> Write Uncorrectable should also be checked, and any future opcodes that
> can modify media. Maybe use Command Effects Log's LBCC field instead? We
> can preload known effects for older nvme's that don't support that log
> page.
Yes, that might be a better idea. I'll try to find some time in
the next days to implement that.
^ permalink raw reply [flat|nested] 6+ messages in thread