All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kanchan Joshi <joshi.k@samsung.com>
To: hch@lst.de, kbusch@kernel.org, axboe@kernel.dk, sagi@grimberg.me
Cc: linux-nvme@lists.infradead.org, vincentfu@gmail.com,
	ankit.kumar@samsung.com, joshiiitr@gmail.com, cpgs@samsung.com,
	Kanchan Joshi <joshi.k@samsung.com>,
	stable@vger.kernel.org, Vincent Fu <vincent.fu@samsung.com>
Subject: [PATCH v3] nvme: fix memory corruption for passthrough metadata
Date: Fri,  6 Oct 2023 19:17:06 +0530	[thread overview]
Message-ID: <1891546521.01696823881551.JavaMail.epsvc@epcpadp4> (raw)
In-Reply-To: CGME20231006135322epcas5p1c9acf38b04f35017181c715c706281dc@epcas5p1.samsung.com

User can specify a smaller meta buffer than what the device is
wired to update/access. Kernel makes a copy of the meta buffer into
which the device does DMA.
As a result, the device overwrites the unrelated kernel memory, causing
random kernel crashes.

Same issue is possible for extended-lba case also. When user specifies a
short unaligned buffer, the kernel makes a copy and uses that for DMA.

Detect these situations for sync/uring passthrough, and bail out.

Fixes: 456cba386e94 ("nvme: wire-up uring-cmd support for io-passthru on char-device")
Cc: stable@vger.kernel.org

Reported-by: Vincent Fu <vincent.fu@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---

Changes since v2:
- Handle extended-lba case: short unaligned buffer IO
- Reduce the scope of check to only well-known commands
- Do not check early. Move it deeper so that check gets executed less
  often
- Combine two patches into one.

Changes since v1:
- Revise the check to exclude PRACT=1 case

Random crash example:

[ 6815.014478] general protection fault, probably for non-canonical address 0x70e3cdbe9133b7a6: 0000 [#1] PREEMPT SMP PTI
[ 6815.014505] CPU: 1 PID: 434 Comm: systemd-timesyn Tainted: G           OE      6.4.0-rc3+ #5
[ 6815.014516] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.2-0-gea1b7a073390-prebuilt.qemu.org 04/01/2014
[ 6815.014522] RIP: 0010:__kmem_cache_alloc_node+0x100/0x440
[ 6815.014551] Code: 48 85 c0 0f 84 fb 02 00 00 41 83 ff ff 74 10 48 8b 00 48 c1 e8 36 41 39 c7 0f 85 e5 02 00 00 41 8b 45 28 49 8b 7d 00 4c 01 e0 <48> 8b 18 48 89 c1 49 33 9d b8 00 00 00 4c 89 e0 48 0f c9 48 31 cb
[ 6815.014559] RSP: 0018:ffffb510c0577d18 EFLAGS: 00010216
[ 6815.014569] RAX: 70e3cdbe9133b7a6 RBX: ffff8a9ec1042300 RCX: 0000000000000010
[ 6815.014575] RDX: 00000000048b0001 RSI: 0000000000000dc0 RDI: 0000000000037060
[ 6815.014581] RBP: ffffb510c0577d58 R08: ffffffffb9ffa280 R09: 0000000000000000
[ 6815.014586] R10: ffff8a9ecbcab1f0 R11: 0000000000000000 R12: 70e3cdbe9133b79e
[ 6815.014591] R13: ffff8a9ec1042300 R14: 0000000000000dc0 R15: 00000000ffffffff
[ 6815.014597] FS:  00007fce590d6940(0000) GS:ffff8a9f3dd00000(0000) knlGS:0000000000000000
[ 6815.014604] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6815.014609] CR2: 00005579abbb6498 CR3: 000000000d9b0000 CR4: 00000000000006e0
[ 6815.014622] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 6815.014627] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 6815.014632] Call Trace:
[ 6815.014650]  <TASK>
[ 6815.014655]  ? apparmor_sk_alloc_security+0x40/0x80
[ 6815.014673]  kmalloc_trace+0x2a/0xa0
[ 6815.014684]  apparmor_sk_alloc_security+0x40/0x80
[ 6815.014694]  security_sk_alloc+0x3f/0x60
[ 6815.014703]  sk_prot_alloc+0x75/0x110
[ 6815.014712]  sk_alloc+0x31/0x200
[ 6815.014721]  inet_create+0xd8/0x3a0
[ 6815.014734]  __sock_create+0x11b/0x220
[ 6815.014749]  __sys_socket_create.part.0+0x49/0x70
[ 6815.014756]  ? __secure_computing+0x94/0xf0
[ 6815.014768]  __sys_socket+0x3c/0xc0
[ 6815.014776]  __x64_sys_socket+0x1a/0x30
[ 6815.014783]  do_syscall_64+0x3b/0x90
[ 6815.014794]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[ 6815.014804] RIP: 0033:0x7fce59aa795b

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

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index d8ff796fd5f2..8147750beff4 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -158,6 +158,67 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 	return req;
 }
 
+static inline bool nvme_nlb_in_cdw12(u8 opcode)
+{
+	switch (opcode) {
+	case nvme_cmd_read:
+	case nvme_cmd_write:
+	case nvme_cmd_compare:
+	case nvme_cmd_zone_append:
+		return true;
+	}
+	return false;
+}
+
+static bool nvme_validate_passthru_meta(struct nvme_ns *ns,
+					struct nvme_command *c,
+					__u32 meta_len,
+					unsigned data_len)
+{
+	/*
+	 * User may specify smaller meta-buffer with a larger data-buffer.
+	 * Driver allocated meta buffer will also be small.
+	 * Device can do larger dma into that, overwriting unrelated kernel
+	 * memory.
+	 * For extended lba case, if user provides a short unaligned buffer,
+	 * the device will corrupt kernel memory.
+	 * Avoid running into corruption in both situations.
+	 */
+	bool ext_lba = ns->features & NVME_NS_EXT_LBAS;
+	u16 nlb, control;
+	unsigned dlen, mlen;
+
+	/* Exclude commands that do not have nlb in cdw12 */
+	if (!nvme_nlb_in_cdw12(c->common.opcode))
+		return true;
+
+	control = upper_16_bits(le32_to_cpu(c->common.cdw12));
+	/* Exclude when meta transfer from/to host is not done */
+	if (control & NVME_RW_PRINFO_PRACT && ns->ms == ns->pi_size)
+		return true;
+
+	nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
+	mlen = (nlb + 1) * ns->ms;
+
+	/* sanity for interleaved buffer */
+	if (ext_lba) {
+		dlen = (nlb + 1) << ns->lba_shift;
+		if (data_len < (dlen + mlen))
+			goto out_false;
+		return true;
+	}
+	/* sanity for separate meta buffer */
+	if (meta_len < mlen)
+		goto out_false;
+
+	return true;
+
+out_false:
+	dev_err(ns->ctrl->device,
+		"%s: metadata length is small!\n", current->comm);
+	return false;
+}
+
 static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
 		u32 meta_seed, void **metap, struct io_uring_cmd *ioucmd,
@@ -194,6 +255,12 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		bio_set_dev(bio, bdev);
 
 	if (bdev && meta_buffer && meta_len) {
+		if (!nvme_validate_passthru_meta(ns, nvme_req(req)->cmd,
+					meta_len, bufflen)) {
+			ret = -EINVAL;
+			goto out_unmap;
+		}
+
 		meta = nvme_add_user_metadata(req, meta_buffer, meta_len,
 				meta_seed);
 		if (IS_ERR(meta)) {
@@ -203,6 +270,15 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		*metap = meta;
 	}
 
+	/* guard another case when kernel memory is being used */
+	if (bio->bi_private && ns && ns->features & NVME_NS_EXT_LBAS) {
+		if (!nvme_validate_passthru_meta(ns, nvme_req(req)->cmd,
+					meta_len, bufflen)) {
+			ret = -EINVAL;
+			goto out_unmap;
+		}
+	}
+
 	return ret;
 
 out_unmap:
-- 
2.25.1




       reply	other threads:[~2023-10-09  3:58 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20231006135322epcas5p1c9acf38b04f35017181c715c706281dc@epcas5p1.samsung.com>
2023-10-06 13:47 ` Kanchan Joshi [this message]
2023-10-10  7:46   ` [PATCH v3] nvme: fix memory corruption for passthrough metadata Christoph Hellwig
2023-10-10 13:39     ` Kanchan Joshi
2023-10-10 15:31       ` Clay Mayers
2023-10-11  5:03         ` Christoph Hellwig
2023-10-11  5:02       ` Christoph Hellwig
2023-10-11  5:26         ` Kanchan Joshi
2023-10-11  6:36           ` Christoph Hellwig
2023-10-11 17:04         ` Keith Busch
2023-10-12  4:36           ` Christoph Hellwig
2023-10-12 15:31             ` Keith Busch
2023-10-12 15:46               ` Christoph Hellwig
2023-10-13  2:19               ` Kanchan Joshi
2023-10-13  4:38                 ` Christoph Hellwig
2023-10-13  5:50                   ` Kanchan Joshi

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=1891546521.01696823881551.JavaMail.epsvc@epcpadp4 \
    --to=joshi.k@samsung.com \
    --cc=ankit.kumar@samsung.com \
    --cc=axboe@kernel.dk \
    --cc=cpgs@samsung.com \
    --cc=hch@lst.de \
    --cc=joshiiitr@gmail.com \
    --cc=kbusch@kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    --cc=stable@vger.kernel.org \
    --cc=vincent.fu@samsung.com \
    --cc=vincentfu@gmail.com \
    /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.