All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nick Bowler <nbowler@draconx.ca>
To: linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
Cc: Sagi Grimberg <sagi@grimberg.me>,
	Christoph Hellwig <hch@infradead.org>,
	Keith Busch <kbusch@kernel.org>
Subject: [PATCH v2 1/2] nvme: Fix compat NVME_IOCTL_SUBMIT_IO numbering
Date: Sat, 28 Mar 2020 01:09:08 -0400	[thread overview]
Message-ID: <20200328050909.30639-2-nbowler@draconx.ca> (raw)
In-Reply-To: <20200328050909.30639-1-nbowler@draconx.ca>

When __u64 has 64-bit alignment, the nvme_user_io structure has trailing
padding.  This causes problems in the compat case with 32-bit userspace
that has less strict alignment because the size of the structure differs.

Since the NVME_IOCTL_SUBMIT_IO macro encodes the structure size itself,
the result is that this ioctl does not work at all in such a scenario:

  # nvme read /dev/nvme0n1 -z 512
  submit-io: Inappropriate ioctl for device

But by the same token, this makes it easy to handle both cases and
since the structures differ only in unused trailing padding bytes
we can simply not read those bytes.

Signed-off-by: Nick Bowler <nbowler@draconx.ca>
---
 drivers/nvme/host/core.c        | 19 +++++++++++++------
 include/uapi/linux/nvme_ioctl.h | 25 +++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a4d8c90ee7cc..9eccf56494de 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1248,14 +1248,19 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 
-static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
+static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio,
+			  size_t uio_size)
 {
 	struct nvme_user_io io;
 	struct nvme_command c;
 	unsigned length, meta_len;
 	void __user *metadata;
 
-	if (copy_from_user(&io, uio, sizeof(io)))
+	/*
+	 * To handle the compat case the amount of data read may be reduced as
+	 * the only difference is in unused padding at the end of the structure.
+	 */
+	if (copy_from_user(&io, uio, uio_size))
 		return -EFAULT;
 	if (io.flags)
 		return -EINVAL;
@@ -1559,6 +1564,11 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	if (is_ctrl_ioctl(cmd))
 		return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
 
+	if (cmd == NVME_IOCTL_SUBMIT_IO || cmd == NVME_IOCTL_SUBMIT_IO_COMPAT) {
+		ret = nvme_submit_io(ns, argp, _IOC_SIZE(cmd));
+		goto out;
+	}
+
 	switch (cmd) {
 	case NVME_IOCTL_ID:
 		force_successful_syscall_return();
@@ -1567,9 +1577,6 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	case NVME_IOCTL_IO_CMD:
 		ret = nvme_user_cmd(ns->ctrl, ns, argp);
 		break;
-	case NVME_IOCTL_SUBMIT_IO:
-		ret = nvme_submit_io(ns, argp);
-		break;
 	case NVME_IOCTL_IO64_CMD:
 		ret = nvme_user_cmd64(ns->ctrl, ns, argp);
 		break;
@@ -1579,7 +1586,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		else
 			ret = -ENOTTY;
 	}
-
+out:
 	nvme_put_ns_from_disk(head, srcu_idx);
 	return ret;
 }
diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index d99b5a772698..60e20f23bec9 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -9,6 +9,31 @@
 
 #include <linux/types.h>
 
+#ifdef __KERNEL__
+/*
+ * The nvme_user_io structure has excess padding at the end when __u64 has
+ * 64-bit alignment.  In the compat case with less strict alignment, there
+ * is no such padding.  The nvme_user_io_compat structure has otherwise
+ * identical layout to nvme_user_io as there is no padding between members.
+ */
+struct nvme_user_io_compat {
+	__u8	opcode;
+	__u8	flags;
+	__u16	control;
+	__u16	nblocks;
+	__u16	rsvd;
+	__u64	metadata;
+	__u64	addr;
+	__u64	slba;
+	__u32	dsmgmt;
+	__u32	reftag;
+	__u16	apptag;
+	__u16	appmask;
+} __attribute__((packed));
+
+#define NVME_IOCTL_SUBMIT_IO_COMPAT _IOW('N', 0x42, struct nvme_user_io_compat)
+#endif
+
 struct nvme_user_io {
 	__u8	opcode;
 	__u8	flags;
-- 
2.24.1


WARNING: multiple messages have this Message-ID (diff)
From: Nick Bowler <nbowler@draconx.ca>
To: linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
Cc: Christoph Hellwig <hch@infradead.org>,
	Keith Busch <kbusch@kernel.org>, Sagi Grimberg <sagi@grimberg.me>
Subject: [PATCH v2 1/2] nvme: Fix compat NVME_IOCTL_SUBMIT_IO numbering
Date: Sat, 28 Mar 2020 01:09:08 -0400	[thread overview]
Message-ID: <20200328050909.30639-2-nbowler@draconx.ca> (raw)
In-Reply-To: <20200328050909.30639-1-nbowler@draconx.ca>

When __u64 has 64-bit alignment, the nvme_user_io structure has trailing
padding.  This causes problems in the compat case with 32-bit userspace
that has less strict alignment because the size of the structure differs.

Since the NVME_IOCTL_SUBMIT_IO macro encodes the structure size itself,
the result is that this ioctl does not work at all in such a scenario:

  # nvme read /dev/nvme0n1 -z 512
  submit-io: Inappropriate ioctl for device

But by the same token, this makes it easy to handle both cases and
since the structures differ only in unused trailing padding bytes
we can simply not read those bytes.

Signed-off-by: Nick Bowler <nbowler@draconx.ca>
---
 drivers/nvme/host/core.c        | 19 +++++++++++++------
 include/uapi/linux/nvme_ioctl.h | 25 +++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a4d8c90ee7cc..9eccf56494de 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1248,14 +1248,19 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 
-static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
+static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio,
+			  size_t uio_size)
 {
 	struct nvme_user_io io;
 	struct nvme_command c;
 	unsigned length, meta_len;
 	void __user *metadata;
 
-	if (copy_from_user(&io, uio, sizeof(io)))
+	/*
+	 * To handle the compat case the amount of data read may be reduced as
+	 * the only difference is in unused padding at the end of the structure.
+	 */
+	if (copy_from_user(&io, uio, uio_size))
 		return -EFAULT;
 	if (io.flags)
 		return -EINVAL;
@@ -1559,6 +1564,11 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	if (is_ctrl_ioctl(cmd))
 		return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
 
+	if (cmd == NVME_IOCTL_SUBMIT_IO || cmd == NVME_IOCTL_SUBMIT_IO_COMPAT) {
+		ret = nvme_submit_io(ns, argp, _IOC_SIZE(cmd));
+		goto out;
+	}
+
 	switch (cmd) {
 	case NVME_IOCTL_ID:
 		force_successful_syscall_return();
@@ -1567,9 +1577,6 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	case NVME_IOCTL_IO_CMD:
 		ret = nvme_user_cmd(ns->ctrl, ns, argp);
 		break;
-	case NVME_IOCTL_SUBMIT_IO:
-		ret = nvme_submit_io(ns, argp);
-		break;
 	case NVME_IOCTL_IO64_CMD:
 		ret = nvme_user_cmd64(ns->ctrl, ns, argp);
 		break;
@@ -1579,7 +1586,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		else
 			ret = -ENOTTY;
 	}
-
+out:
 	nvme_put_ns_from_disk(head, srcu_idx);
 	return ret;
 }
diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index d99b5a772698..60e20f23bec9 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -9,6 +9,31 @@
 
 #include <linux/types.h>
 
+#ifdef __KERNEL__
+/*
+ * The nvme_user_io structure has excess padding at the end when __u64 has
+ * 64-bit alignment.  In the compat case with less strict alignment, there
+ * is no such padding.  The nvme_user_io_compat structure has otherwise
+ * identical layout to nvme_user_io as there is no padding between members.
+ */
+struct nvme_user_io_compat {
+	__u8	opcode;
+	__u8	flags;
+	__u16	control;
+	__u16	nblocks;
+	__u16	rsvd;
+	__u64	metadata;
+	__u64	addr;
+	__u64	slba;
+	__u32	dsmgmt;
+	__u32	reftag;
+	__u16	apptag;
+	__u16	appmask;
+} __attribute__((packed));
+
+#define NVME_IOCTL_SUBMIT_IO_COMPAT _IOW('N', 0x42, struct nvme_user_io_compat)
+#endif
+
 struct nvme_user_io {
 	__u8	opcode;
 	__u8	flags;
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2020-03-28  5:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-28  5:09 [PATCH v2 0/2] nvme: compat ioctl fixes Nick Bowler
2020-03-28  5:09 ` Nick Bowler
2020-03-28  5:09 ` Nick Bowler [this message]
2020-03-28  5:09   ` [PATCH v2 1/2] nvme: Fix compat NVME_IOCTL_SUBMIT_IO numbering Nick Bowler
2020-03-28  8:26   ` Christoph Hellwig
2020-03-28  8:26     ` Christoph Hellwig
2020-03-28 13:56     ` Nick Bowler
2020-03-28 13:56       ` Nick Bowler
2020-03-28  5:09 ` [PATCH v2 2/2] nvme: Fix compat address handling in several ioctls Nick Bowler
2020-03-28  5:09   ` Nick Bowler
2020-03-28  8:26   ` Christoph Hellwig
2020-03-28  8:26     ` Christoph Hellwig
2020-03-31 14:17   ` Christoph Hellwig
2020-03-31 14:17     ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200328050909.30639-2-nbowler@draconx.ca \
    --to=nbowler@draconx.ca \
    --cc=hch@infradead.org \
    --cc=kbusch@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.