mi-mctp: better handling for error/unexpected responses

Currently, the mi-mctp transport expects there to be at least a complete
header-sized reply - matching the caller's resp->hdr_size value,
otherwise we consider this a transport failure.

However, Admin command responses have a header that's larger than the
generic error response message, so we should be able to handle response
messages that are just the small header and no payload.

This change reworks the response-buffer handling in the MCTP transport.
Rather than storing the payload and MIC into an extra allocated buffer,
we just lay out the response iovec directly into the header, payload and
MIC buffers, and then handle the short-response cases as needed. This
means we can properly extract the error response, rather than just
having the transport report a generic "header too small" failure itself.

This is at high risk for off-by-one errors, so add tests to cover each
of the possible buffer-arrangement cases.

Signed-off-by: Jeremy Kerr <[email protected]>

Author: jk-ozlabs

src/nvme/mi-mctp.c

@@ -101,10 +101,9 @@ static int nvme_mi_mctp_submit(struct nvme_mi_ep *ep,
 			       struct nvme_mi_resp *resp)
 {
 	struct nvme_mi_transport_mctp *mctp;
-	struct iovec req_iov[3], resp_iov[2];
+	struct iovec req_iov[3], resp_iov[3];
 	struct msghdr req_msg, resp_msg;
 	struct sockaddr_mctp addr;
-	unsigned char *rspbuf;
 	ssize_t len;
 	__le32 mic;
 	int i;
@@ -153,49 +152,77 @@ static int nvme_mi_mctp_submit(struct nvme_mi_ep *ep,
 	resp_iov[0].iov_base = ((__u8 *)resp->hdr) + 1;
 	resp_iov[0].iov_len = resp->hdr_len - 1;
 
-	/* we use a temporary buffer to receive the response, and then
-	 * split into data & mic. This avoids having to re-arrange response
-	 * data on a recv that was shorter than expected */
-	rspbuf = malloc(resp->data_len + sizeof(mic));
-	if (!rspbuf)
-		return -ENOMEM;
+	resp_iov[1].iov_base = ((__u8 *)resp->data);
+	resp_iov[1].iov_len = resp->data_len;
 
-	resp_iov[1].iov_base = rspbuf;
-	resp_iov[1].iov_len = resp->data_len + sizeof(mic);
+	resp_iov[2].iov_base = &mic;
+	resp_iov[2].iov_len = sizeof(mic);
 
 	memset(&resp_msg, 0, sizeof(resp_msg));
 	resp_msg.msg_name = &addr;
 	resp_msg.msg_namelen = sizeof(addr);
 	resp_msg.msg_iov = resp_iov;
-	resp_msg.msg_iovlen = 2;
+	resp_msg.msg_iovlen = 3;
 
 	len = ops.recvmsg(mctp->sd, &resp_msg, 0);
 
 	if (len < 0) {
 		nvme_msg(ep->root, LOG_ERR,
 			 "Failure receiving MCTP message: %m\n");
-		free(rspbuf);
 		return len;
 	}
 
-	if (len < resp->hdr_len + sizeof(mic) - 1) {
+	if (len == 0) {
+		nvme_msg(ep->root, LOG_WARNING, "No data from MCTP endpoint\n");
+		return -1;
+	}
+
+	/* Re-add the type byte, so we can work on aligned lengths from here */
+	resp->hdr->type = MCTP_TYPE_NVME | MCTP_TYPE_MIC;
+	len += 1;
+
+	/* The smallest response data is 8 bytes: generic 4-byte message header
+	 * plus four bytes of error data (excluding MIC). Ensure we have enough.
+	 */
+	if (len < 8 + sizeof(mic)) {
 		nvme_msg(ep->root, LOG_ERR,
 			 "Invalid MCTP response: too short (%zd bytes, needed %zd)\n",
-			 len, resp->hdr_len + sizeof(mic) - 1);
-		free(rspbuf);
+			 len, 8 + sizeof(mic));
 		return -EIO;
 	}
-	resp->hdr->type = MCTP_TYPE_NVME | MCTP_TYPE_MIC;
-
-	len -= resp->hdr_len - 1;
 
-	memcpy(&mic, rspbuf + len - sizeof(mic), sizeof(mic));
-	len -= sizeof(mic);
+	/* We can't have header/payload data that isn't a multiple of 4 bytes */
+	if (len & 0x3) {
+		nvme_msg(ep->root, LOG_WARNING,
+			 "Response message has unaligned length (%zd)!\n",
+			 len);
+		return -EIO;
+	}
 
-	memcpy(resp->data, rspbuf, len);
-	resp->data_len = len;
+	/* If we have a shorter than expected response, we need to find the
+	 * MIC and the correct split between header & data. We know that the
+	 * split is 4-byte aligned, so the MIC will be entirely within one
+	 * of the iovecs.
+	 */
+	if (len == resp->hdr_len + resp->data_len + sizeof(mic)) {
+		/* Common case: expected data length. Header, data and MIC
+		 * are already laid-out correctly. Nothing to do. */
+
+	} else if (len < resp->hdr_len + sizeof(mic)) {
+		/* Response is smaller than the expected header. MIC is
+		 * somewhere in the header buf */
+		resp->hdr_len = len - sizeof(mic);
+		resp->data_len = 0;
+		memcpy(&mic, ((uint8_t *)resp->hdr) + resp->hdr_len,
+		       sizeof(mic));
 
-	free(rspbuf);
+	} else {
+		/* We have a full header, but data is truncated - possibly
+		 * zero bytes. MIC is somewhere in the data buf */
+		resp->data_len = len - resp->hdr_len - sizeof(mic);
+		memcpy(&mic, ((uint8_t *)resp->data) + resp->data_len,
+		       sizeof(mic));
+	}
 
 	resp->mic = le32_to_cpu(mic);
 

test/mi-mctp.c

@@ -172,6 +172,23 @@ static void test_rx_err(nvme_mi_ep_t ep, struct test_peer *peer)
 	assert(rc != 0);
 }
 
+static int tx_none(struct test_peer *peer, void *buf, size_t len)
+{
+	return 0;
+}
+
+static void test_tx_none(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct nvme_mi_read_nvm_ss_info ss_info;
+	int rc;
+
+	peer->tx_buf_len = 0;
+	peer->rx_fn = tx_none;
+
+	rc = nvme_mi_mi_read_mi_data_subsys(ep, &ss_info);
+	assert(rc != 0);
+}
+
 static void test_tx_err(nvme_mi_ep_t ep, struct test_peer *peer)
 {
 	struct nvme_mi_read_nvm_ss_info ss_info;
@@ -206,15 +223,102 @@ static void test_read_mi_data(nvme_mi_ep_t ep, struct test_peer *peer)
 	assert(rc == 0);
 }
 
+static void test_mi_resp_err(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct nvme_mi_read_nvm_ss_info ss_info;
+	int rc;
+
+	/* simple error response */
+	peer->tx_buf[4] = 0x02; /* internal error */
+	peer->tx_buf_len = 8;
+
+	rc = nvme_mi_mi_read_mi_data_subsys(ep, &ss_info);
+	assert(rc == 0x2);
+}
+
+static void test_admin_resp_err(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct nvme_id_ctrl id;
+	nvme_mi_ctrl_t ctrl;
+	int rc;
+
+	ctrl = nvme_mi_init_ctrl(ep, 1);
+	assert(ctrl);
+
+	/* Simple error response, will be shorter than the expected Admin
+	 * command response header. */
+	peer->tx_buf[4] = 0x02; /* internal error */
+	peer->tx_buf_len = 8;
+
+	rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
+	assert(rc == 0x2);
+}
+
+/* test: all 4-byte aligned response sizes - should be decoded into the
+ * response status value. We use an admin command here as the header size will
+ * be larger than the minimum header size (it contains the completion
+ * doublewords), and we need to ensure that an error response is correctly
+ * interpreted, including having the MIC extracted from the message.
+ */
+static void test_admin_resp_sizes(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct nvme_id_ctrl id;
+	nvme_mi_ctrl_t ctrl;
+	unsigned int i;
+	int rc;
+
+	ctrl = nvme_mi_init_ctrl(ep, 1);
+	assert(ctrl);
+
+	peer->tx_buf[4] = 0x02; /* internal error */
+
+	for (i = 8; i <= 4096 + 8; i+=4) {
+		peer->tx_buf_len = i;
+		rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
+		assert(rc == 2);
+	}
+
+	nvme_mi_close_ctrl(ctrl);
+}
+
+/* test: unaligned response sizes - should always report a transport error */
+static void test_admin_resp_sizes_unaligned(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct nvme_id_ctrl id;
+	nvme_mi_ctrl_t ctrl;
+	unsigned int i;
+	int rc;
+
+	ctrl = nvme_mi_init_ctrl(ep, 1);
+	assert(ctrl);
+
+	peer->tx_buf[4] = 0x02; /* internal error */
+
+	for (i = 8; i <= 4096 + 8; i++) {
+		peer->tx_buf_len = i;
+		if (!(i & 0x3))
+			continue;
+		rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
+		assert(rc < 0);
+	}
+
+	nvme_mi_close_ctrl(ctrl);
+}
+
 #define DEFINE_TEST(name) { #name, test_ ## name }
 struct test {
 	const char *name;
 	void (*fn)(nvme_mi_ep_t, struct test_peer *);
 } tests[] = {
 	DEFINE_TEST(rx_err),
+	DEFINE_TEST(tx_none),
 	DEFINE_TEST(tx_err),
 	DEFINE_TEST(tx_short),
 	DEFINE_TEST(read_mi_data),
+	DEFINE_TEST(mi_resp_err),
+	DEFINE_TEST(admin_resp_err),
+	DEFINE_TEST(admin_resp_sizes),
+	DEFINE_TEST(admin_resp_sizes_unaligned),
 };
 
 static void run_test(struct test *test, FILE *logfd, nvme_mi_ep_t ep,