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

[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

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