[PATCH 0/2] Fix memory-corruption for passthrough metadata

[PATCH 0/2] Fix memory-corruption for passthrough metadata

[Kanchan Joshi]

Malformed user-space application can specify smaller meta-buffer and
larger data-buffer.
For DIX namespace, nvme-driver allocates a meta-buffer (of same small size that
user specified) and that is passed to the device for DMA.
Device can do DMA writes (of larger length) into unrelated kernel
memory, leading to random crashes [1].


Patch 1: avoids the above for uring passthrough
Patch 2: avoid the same for sync passthrough


[1]
[ 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


Kanchan Joshi (2):
  nvme: fix memory corruption for passthrough metadata
  nvme: avoid memory corruption for sync passthrough

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

-- 
2.25.1

[PATCH 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

User can specify a smaller meta buffer than what the device is
wired to update/access.
This may lead to Device doing a larger DMA operation, overwriting
unrelated kernel memory.

Detect this situation for 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>
---
 drivers/nvme/host/ioctl.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 19a5177bc360..fb73fa95f090 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -320,6 +320,30 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			meta_len, lower_32_bits(io.slba), NULL, 0, 0);
 }
 
+static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
+					struct nvme_ns *ns,
+					struct nvme_command *c,
+					__u64 meta, __u32 meta_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.
+	 */
+	if (ns && (meta_len || meta)) {
+		u16 nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
+
+		if (meta_len != (nlb + 1) * ns->ms) {
+			dev_err(ctrl->device,
+			"%s: metadata length does not match!\n", current->comm);
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
 					struct nvme_ns *ns, __u32 nsid)
 {
@@ -593,6 +617,10 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	d.metadata_len = READ_ONCE(cmd->metadata_len);
 	d.timeout_ms = READ_ONCE(cmd->timeout_ms);
 
+	if (!nvme_validate_passthru_meta(ctrl, ns, &c, d.metadata,
+					 d.metadata_len))
+		return -EINVAL;
+
 	if (issue_flags & IO_URING_F_NONBLOCK) {
 		rq_flags |= REQ_NOWAIT;
 		blk_flags = BLK_MQ_REQ_NOWAIT;
-- 
2.25.1

[PATCH 2/2] nvme: avoid memory corruption for sync passthrough

[Kanchan Joshi]

Sync passthrough metadata handling also needs to avoid the situation of
device writing into unrelated kernel memory.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index fb73fa95f090..f7adabbc9e9f 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -388,6 +388,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
+	if (!nvme_validate_passthru_meta(ctrl, ns, &c, cmd.metadata,
+					 cmd.metadata_len))
+		return -EINVAL;
+
 	if (!nvme_cmd_allowed(ns, &c, 0, open_for_write))
 		return -EACCES;
 
@@ -435,6 +439,10 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
+	if (!nvme_validate_passthru_meta(ctrl, ns, &c, cmd.metadata,
+					 cmd.metadata_len))
+		return -EINVAL;
+
 	if (!nvme_cmd_allowed(ns, &c, flags, open_for_write))
 		return -EACCES;
 
-- 
2.25.1

Re: [PATCH 1/2] nvme: fix memory corruption for passthrough metadata

[Keith Busch]

On Fri, Aug 11, 2023 at 09:29:05PM +0530, Kanchan Joshi wrote:
> +static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
> +					struct nvme_ns *ns,
> +					struct nvme_command *c,
> +					__u64 meta, __u32 meta_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.
> +	 */
> +	if (ns && (meta_len || meta)) {
> +		u16 nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
> +
> +		if (meta_len != (nlb + 1) * ns->ms) {
> +			dev_err(ctrl->device,
> +			"%s: metadata length does not match!\n", current->comm);
> +			return false;
> +		}

Don't you need to check the command PRINFO PRACT bit to know if metadata
length is striped/generated on the controller side?

Re: [PATCH 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

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

On Fri, Aug 11, 2023 at 10:57:36AM -0600, Keith Busch wrote:
>On Fri, Aug 11, 2023 at 09:29:05PM +0530, Kanchan Joshi wrote:
>> +static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
>> +					struct nvme_ns *ns,
>> +					struct nvme_command *c,
>> +					__u64 meta, __u32 meta_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.
>> +	 */
>> +	if (ns && (meta_len || meta)) {
>> +		u16 nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
>> +
>> +		if (meta_len != (nlb + 1) * ns->ms) {
>> +			dev_err(ctrl->device,
>> +			"%s: metadata length does not match!\n", current->comm);
>> +			return false;
>> +		}
>
>Don't you need to check the command PRINFO PRACT bit to know if metadata
>length is striped/generated on the controller side?

Good point. Will add that check in v2.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]