Message-ID: <20230103205937.1126626-1-rrichter@amd.com>
Date:   Tue, 3 Jan 2023 21:59:36 +0100
From:   Robert Richter <rrichter@....com>
To:     Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Dave Jiang <dave.jiang@...el.com>
CC:     Robert Richter <rrichter@....com>, <linux-cxl@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: [PATCH] cxl/mbox: Fix Payload Length check for Get Log command

Commit 2aeaf663b85e introduced strict checking for variable length
payload size validation. The payload length of received data must
match the size of the requested data by the caller except for the case
where the min_out value is set.

The Get Log command does not have a header with a length field set.
The Log size is determined by the Get Supported Logs command (CXL 3.0,
8.2.9.5.1). However, the actual size can be smaller and the number of
valid bytes in the payload output must be determined reading the
Payload Length field (CXL 3.0, Table 8-36, Note 2).

Two issues arise: The command can successfully complete with a payload
length of zero. And, the valid payload length must then also be
consumed by the caller.

Change cxl_xfer_log() to pass the number of payload bytes back to the
caller to determine the number of log entries.

Logs can be bigger than the maximum payload length and multiple Get
Log commands can be issued. If the received payload size is smaller
than the maximum payload size we can assume all valid bytes have been
fetched. Stop sending further Get Log commands then.

Also, implement CXL_NO_PAYLOAD_SIZE_VALIDATION as special value to
@min_out to skip the payload size validation check. A value of zero
for @min_out is already widespread in use as default if the size must
match @size_out. Thus, zero can not be passed to allow zero length
variable payloads, CXL_NO_PAYLOAD_SIZE_VALIDATION should be used
instead for this case.

On that occasion, change debug messages to also report supported
opcodes.

There could be other variable payloads commands affected by this
strict check, the implementation of GET_LSA and SET_LSA in this kernel
could possibly be broken too. A fix of this is not scope of this
patch.

Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands")
Signed-off-by: Robert Richter <rrichter@....com>
---
 drivers/cxl/core/mbox.c | 41 ++++++++++++++++++++++++++++++-----------
 drivers/cxl/cxlmem.h    |  5 +++++
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index b03fba212799..0c2056ae8aff 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -183,11 +183,16 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
 	 * Variable sized output needs to at least satisfy the caller's
 	 * minimum if not the fully requested size.
 	 */
+
+	if (min_out == CXL_NO_PAYLOAD_SIZE_VALIDATION)
+		return 0;
+
 	if (min_out == 0)
 		min_out = out_size;
 
 	if (mbox_cmd->size_out < min_out)
 		return -EIO;
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
@@ -554,6 +559,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 {
 	u32 remaining = size;
 	u32 offset = 0;
+	u32 size_out;
 
 	while (remaining) {
 		u32 xfer_size = min_t(u32, remaining, cxlds->payload_size);
@@ -572,6 +578,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 			.size_in = sizeof(log),
 			.payload_in = &log,
 			.size_out = xfer_size,
+			/* Any size is allowed (CXL 3.0, Table 8-36). */
+			.min_out = CXL_NO_PAYLOAD_SIZE_VALIDATION,
 			.payload_out = out,
 		};
 
@@ -579,12 +587,24 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 		if (rc < 0)
 			return rc;
 
-		out += xfer_size;
-		remaining -= xfer_size;
-		offset += xfer_size;
+		size_out = mbox_cmd.size_out;
+		if (size_out > xfer_size)
+			return -ENXIO;
+
+		out += size_out;
+		remaining -= size_out;
+		offset += size_out;
+
+		/*
+		 * A smaller output payload length indicates all valid
+		 * bytes have been fetched.
+		 */
+		if (size_out < xfer_size)
+			break;
 	}
 
-	return 0;
+	/* Total number of bytes fetched. */
+	return offset;
 }
 
 /**
@@ -608,13 +628,11 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
 		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
 		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
 
-		if (!cmd) {
-			dev_dbg(cxlds->dev,
-				"Opcode 0x%04x unsupported by driver", opcode);
-			continue;
-		}
+		if (cmd)
+			set_bit(cmd->info.id, cxlds->enabled_cmds);
 
-		set_bit(cmd->info.id, cxlds->enabled_cmds);
+		dev_dbg(cxlds->dev, "Opcode 0x%04x %ssupported by driver",
+			opcode, cmd ? "" : "un");
 	}
 }
 
@@ -695,11 +713,12 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
 		}
 
 		rc = cxl_xfer_log(cxlds, &uuid, size, log);
-		if (rc) {
+		if (rc < 0) {
 			kvfree(log);
 			goto out;
 		}
 
+		size = (u32)rc;
 		cxl_walk_cel(cxlds, size, log);
 		kvfree(log);
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ab138004f644..2db24b062913 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -102,6 +102,10 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
  *            variable sized output commands, it tells the exact number of bytes
  *            written.
  * @min_out: (input) internal command output payload size validation
+ *  * %0:   Payload size must match @size_out.
+ *  * %>0:  Minimum payload size.
+ *  * %CXL_NO_PAYLOAD_SIZE_VALIDATION: Skip payload size validation check.
+ *
  * @return_code: (output) Error code returned from hardware.
  *
  * This is the primary mechanism used to send commands to the hardware.
@@ -117,6 +121,7 @@ struct cxl_mbox_cmd {
 	size_t size_in;
 	size_t size_out;
 	size_t min_out;
+#define CXL_NO_PAYLOAD_SIZE_VALIDATION	SIZE_MAX
 	u16 return_code;
 };
 
-- 
2.30.2

Powered by blists - more mailing lists