astlpc: Introduce protocol v3 with integrity checks
v3 of the binding adds a CRC-32 value as a medium-specific trailer to
each packet passing over the binding interface.
The patch includes a naive bit-shift implementation of CRC-32, we can
improve it later as necessary.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I93a95bccef30010d56e10e29b6d84554268ab7af
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3b0097d..d13bab4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -5,7 +5,7 @@
add_definitions (-DMCTP_HAVE_STDIO)
add_definitions (-DMCTP_DEFAULT_ALLOC)
-add_library (mctp STATIC alloc.c astlpc.c core.c log.c libmctp.h serial.c)
+add_library (mctp STATIC alloc.c astlpc.c crc32.c core.c log.c libmctp.h serial.c)
target_include_directories (mctp PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
diff --git a/Makefile.am b/Makefile.am
index d630078..fc8e937 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -12,7 +12,7 @@
endif
if LIBMCTP_BINDING_astlpc
-libmctp_la_SOURCES += astlpc.c
+libmctp_la_SOURCES += astlpc.c crc32.c
include_HEADERS += libmctp-astlpc.h
endif
diff --git a/Makefile.inc b/Makefile.inc
index 1ee555d..6ea56b7 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -1,5 +1,5 @@
LIBMCTP_DIR ?= libmctp/
-LIBMCTP_OBJS = core.o alloc.o log.o
+LIBMCTP_OBJS = crc32.o core.o alloc.o log.o
LIBMCTP_BINDINGS ?= serial astlpc
LIBMCTP_OBJS += $(LIBMCTP_BINDINGS:%=%.o)
diff --git a/astlpc.c b/astlpc.c
index 95d8701..053de11 100644
--- a/astlpc.c
+++ b/astlpc.c
@@ -18,11 +18,12 @@
#define pr_fmt(x) "astlpc: " x
+#include "container_of.h"
+#include "crc32.h"
#include "libmctp.h"
#include "libmctp-alloc.h"
#include "libmctp-log.h"
#include "libmctp-astlpc.h"
-#include "container_of.h"
#include "range.h"
#ifdef MCTP_HAVE_FILEIO
@@ -53,6 +54,8 @@
uint16_t version;
uint32_t (*packet_size)(uint32_t body);
uint32_t (*body_size)(uint32_t packet);
+ void (*pktbuf_protect)(struct mctp_pktbuf *pkt);
+ bool (*pktbuf_validate)(struct mctp_pktbuf *pkt);
};
struct mctp_binding_astlpc {
@@ -103,7 +106,7 @@
/* Support testing of new binding protocols */
#ifndef ASTLPC_VER_CUR
-#define ASTLPC_VER_CUR 2
+#define ASTLPC_VER_CUR 3
#endif
/* clang-format on */
@@ -125,21 +128,79 @@
return packet - 4;
}
+void astlpc_pktbuf_protect_v1(struct mctp_pktbuf *pkt)
+{
+ (void)pkt;
+}
+
+bool astlpc_pktbuf_validate_v1(struct mctp_pktbuf *pkt)
+{
+ (void)pkt;
+ return true;
+}
+
+static uint32_t astlpc_packet_size_v3(uint32_t body)
+{
+ assert((body + 4 + 4) > body);
+
+ return body + 4 + 4;
+}
+
+static uint32_t astlpc_body_size_v3(uint32_t packet)
+{
+ assert((packet - 4 - 4) < packet);
+
+ return packet - 4 - 4;
+}
+
+void astlpc_pktbuf_protect_v3(struct mctp_pktbuf *pkt)
+{
+ uint32_t code;
+
+ code = htobe32(crc32(mctp_pktbuf_hdr(pkt), mctp_pktbuf_size(pkt)));
+ mctp_prdebug("%s: 0x%" PRIx32, __func__, code);
+ mctp_pktbuf_push(pkt, &code, 4);
+}
+
+bool astlpc_pktbuf_validate_v3(struct mctp_pktbuf *pkt)
+{
+ uint32_t code;
+ void *check;
+
+ code = be32toh(crc32(mctp_pktbuf_hdr(pkt), mctp_pktbuf_size(pkt) - 4));
+ mctp_prdebug("%s: 0x%" PRIx32, __func__, code);
+ check = mctp_pktbuf_pop(pkt, 4);
+ return check && !memcmp(&code, check, 4);
+}
+
static const struct mctp_astlpc_protocol astlpc_protocol_version[] = {
[0] = {
.version = 0,
.packet_size = NULL,
.body_size = NULL,
+ .pktbuf_protect = NULL,
+ .pktbuf_validate = NULL,
},
[1] = {
.version = 1,
.packet_size = astlpc_packet_size_v1,
.body_size = astlpc_body_size_v1,
+ .pktbuf_protect = astlpc_pktbuf_protect_v1,
+ .pktbuf_validate = astlpc_pktbuf_validate_v1,
},
[2] = {
.version = 2,
.packet_size = astlpc_packet_size_v1,
.body_size = astlpc_body_size_v1,
+ .pktbuf_protect = astlpc_pktbuf_protect_v1,
+ .pktbuf_validate = astlpc_pktbuf_validate_v1,
+ },
+ [3] = {
+ .version = 3,
+ .packet_size = astlpc_packet_size_v3,
+ .body_size = astlpc_body_size_v3,
+ .pktbuf_protect = astlpc_pktbuf_protect_v3,
+ .pktbuf_validate = astlpc_pktbuf_validate_v3,
},
};
@@ -710,18 +771,24 @@
__func__, len, hdr->src, hdr->dest, hdr->flags_seq_tag);
if (len > astlpc->proto->body_size(astlpc->layout.tx.size)) {
- astlpc_prwarn(astlpc, "invalid TX len 0x%x", len);
+ astlpc_prwarn(astlpc, "invalid TX len %" PRIu32 ": %" PRIu32, len,
+ astlpc->proto->body_size(astlpc->layout.tx.size));
return -1;
}
len_be = htobe32(len);
mctp_astlpc_lpc_write(astlpc, &len_be, astlpc->layout.tx.offset,
sizeof(len_be));
+
+ astlpc->proto->pktbuf_protect(pkt);
+ len = mctp_pktbuf_size(pkt);
+
mctp_astlpc_lpc_write(astlpc, hdr, astlpc->layout.tx.offset + 4, len);
mctp_binding_set_tx_enabled(b, false);
mctp_astlpc_kcs_send(astlpc, 0x1);
+
return 0;
}
@@ -850,32 +917,48 @@
static void mctp_astlpc_rx_start(struct mctp_binding_astlpc *astlpc)
{
struct mctp_pktbuf *pkt;
- uint32_t len;
+ uint32_t body, packet;
- mctp_astlpc_lpc_read(astlpc, &len, astlpc->layout.rx.offset,
- sizeof(len));
- len = be32toh(len);
+ mctp_astlpc_lpc_read(astlpc, &body, astlpc->layout.rx.offset,
+ sizeof(body));
+ body = be32toh(body);
- if (len > astlpc->proto->body_size(astlpc->layout.rx.size)) {
- astlpc_prwarn(astlpc, "invalid RX len 0x%x", len);
+ if (body > astlpc->proto->body_size(astlpc->layout.rx.size)) {
+ astlpc_prwarn(astlpc, "invalid RX len 0x%x", body);
return;
}
assert(astlpc->binding.pkt_size >= 0);
- if (len > (uint32_t)astlpc->binding.pkt_size) {
- mctp_prwarn("invalid RX len 0x%x", len);
- astlpc_prwarn(astlpc, "invalid RX len 0x%x", len);
+ if (body > (uint32_t)astlpc->binding.pkt_size) {
+ astlpc_prwarn(astlpc, "invalid RX len 0x%x", body);
return;
}
- pkt = mctp_pktbuf_alloc(&astlpc->binding, len);
+ /* Eliminate the medium-specific header that we just read */
+ packet = astlpc->proto->packet_size(body) - 4;
+ pkt = mctp_pktbuf_alloc(&astlpc->binding, packet);
if (!pkt)
goto out_complete;
+ /*
+ * Read payload and medium-specific trailer from immediately after the
+ * medium-specific header.
+ */
mctp_astlpc_lpc_read(astlpc, mctp_pktbuf_hdr(pkt),
- astlpc->layout.rx.offset + 4, len);
+ astlpc->layout.rx.offset + 4, packet);
- mctp_bus_rx(&astlpc->binding, pkt);
+ /*
+ * v3 will validate the CRC32 in the medium-specific trailer and adjust
+ * the packet size accordingly. On older protocols validation is a no-op
+ * that always returns true.
+ */
+ if (astlpc->proto->pktbuf_validate(pkt)) {
+ mctp_bus_rx(&astlpc->binding, pkt);
+ } else {
+ /* TODO: Drop any associated assembly */
+ mctp_pktbuf_free(pkt);
+ astlpc_prdebug(astlpc, "Dropped corrupt packet");
+ }
out_complete:
mctp_astlpc_kcs_send(astlpc, 0x2);
@@ -1049,8 +1132,8 @@
astlpc->binding.version = 1;
astlpc->binding.pkt_size =
MCTP_PACKET_SIZE(mtu > MCTP_BTU ? mtu : MCTP_BTU);
- astlpc->binding.pkt_header = 0;
- astlpc->binding.pkt_trailer = 0;
+ astlpc->binding.pkt_header = 4;
+ astlpc->binding.pkt_trailer = 4;
astlpc->binding.tx = mctp_binding_astlpc_tx;
if (mode == MCTP_BINDING_ASTLPC_MODE_BMC)
astlpc->binding.start = mctp_binding_astlpc_start_bmc;
diff --git a/core.c b/core.c
index 19202e8..2ec9046 100644
--- a/core.c
+++ b/core.c
@@ -156,6 +156,15 @@
return 0;
}
+void *mctp_pktbuf_pop(struct mctp_pktbuf *pkt, size_t len)
+{
+ if (len > mctp_pktbuf_size(pkt))
+ return NULL;
+
+ pkt->end -= len;
+ return pkt->data + pkt->end;
+}
+
/* Message reassembly */
static struct mctp_msg_ctx *mctp_msg_ctx_lookup(struct mctp *mctp,
uint8_t src, uint8_t dest, uint8_t tag)
diff --git a/crc32.c b/crc32.c
new file mode 100644
index 0000000..b5883c2
--- /dev/null
+++ b/crc32.c
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: Apache-2.0 */
+/* Copyright 2021 IBM Corp. */
+
+#include "crc32.h"
+
+#include <limits.h>
+
+/* Very dumb CRC-32 implementation */
+uint32_t crc32(const void *buf, size_t len)
+{
+ const uint8_t *buf8 = buf;
+ uint32_t rem = 0xffffffff;
+
+ for (; len; len--) {
+ int i;
+
+ rem = rem ^ *buf8;
+ for (i = 0; i < CHAR_BIT; i++)
+ rem = (rem >> 1) ^ ((rem & 1) * 0xEDB88320);
+
+ buf8++;
+ }
+
+ return rem ^ 0xffffffff;
+}
diff --git a/crc32.h b/crc32.h
new file mode 100644
index 0000000..c47a27e
--- /dev/null
+++ b/crc32.h
@@ -0,0 +1,9 @@
+#ifndef _CRC32_H
+#define _CRC32_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+uint32_t crc32(const void *buf, size_t len);
+
+#endif
diff --git a/libmctp.h b/libmctp.h
index 36e1483..46bda90 100644
--- a/libmctp.h
+++ b/libmctp.h
@@ -59,6 +59,7 @@
void *mctp_pktbuf_alloc_start(struct mctp_pktbuf *pkt, size_t size);
void *mctp_pktbuf_alloc_end(struct mctp_pktbuf *pkt, size_t size);
int mctp_pktbuf_push(struct mctp_pktbuf *pkt, void *data, size_t len);
+void *mctp_pktbuf_pop(struct mctp_pktbuf *pkt, size_t len);
/* MCTP core */
struct mctp;
diff --git a/tests/test_astlpc.c b/tests/test_astlpc.c
index 632cd6d..32c221f 100644
--- a/tests/test_astlpc.c
+++ b/tests/test_astlpc.c
@@ -4,7 +4,7 @@
#include "config.h"
#endif
-#define ASTLPC_VER_CUR 2
+#define ASTLPC_VER_CUR 3
#include "astlpc.c"
#ifdef pr_fmt
@@ -718,9 +718,9 @@
free(lpc_mem);
}
-#define BUFFER_MIN (MCTP_PACKET_SIZE(MCTP_BTU) + 4)
+#define BUFFER_MIN (MCTP_PACKET_SIZE(MCTP_BTU) + 4 + 4)
static const struct mctp_binding_astlpc astlpc_layout_ctx = {
- .proto = &astlpc_protocol_version[2],
+ .proto = &astlpc_protocol_version[3],
};
static void astlpc_test_buffers_rx_offset_overflow(void)
@@ -1173,6 +1173,111 @@
free(lpc_mem);
}
+static void astlpc_test_corrupt_host_tx(void)
+{
+ struct astlpc_test ctx = { 0 };
+ struct mctp_lpcmap_hdr *hdr;
+ uint8_t msg[MCTP_BTU];
+ uint32_t offset;
+ uint32_t code;
+ uint8_t *tlr;
+ int rc;
+
+ /* Test harness initialisation */
+
+ network_init(&ctx);
+
+ memset(&msg[0], 0xa5, MCTP_BTU);
+
+ ctx.msg = &msg[0];
+ ctx.count = 0;
+ mctp_set_rx_all(ctx.bmc.mctp, rx_message, &ctx);
+
+ /* Host sends the single-packet message */
+ rc = mctp_message_tx(ctx.host.mctp, 8, msg, sizeof(msg));
+ assert(rc == 0);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_IBF);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_DATA] == 0x01);
+
+ astlpc_assert_tx_packet(&ctx.host, &msg[0], MCTP_BTU);
+
+ /* Corrupt the CRC-32 in the message trailer */
+ hdr = (struct mctp_lpcmap_hdr *)ctx.lpc_mem;
+ offset = be32toh(hdr->layout.tx_offset);
+ tlr = (uint8_t *)&ctx.lpc_mem[offset] + 4 + sizeof(msg);
+ memcpy(&code, tlr, sizeof(code));
+ code = ~code;
+ memcpy(tlr, &code, sizeof(code));
+
+ /* BMC receives the single-packet message */
+ mctp_astlpc_poll(ctx.bmc.astlpc);
+ assert(ctx.count == 0);
+
+ /* BMC returns Tx area ownership to Host */
+ assert(!(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_IBF));
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_DATA] == 0x02);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_OBF);
+
+ /* Host dequeues ownership hand-over */
+ rc = mctp_astlpc_poll(ctx.host.astlpc);
+ assert(rc == 0);
+
+ network_destroy(&ctx);
+}
+
+static void astlpc_test_corrupt_bmc_tx(void)
+{
+ struct astlpc_test ctx = { 0 };
+ struct mctp_lpcmap_hdr *hdr;
+ uint8_t msg[MCTP_BTU];
+ uint32_t offset;
+ uint32_t code;
+ uint8_t *tlr;
+ int rc;
+
+ /* Test harness initialisation */
+
+ network_init(&ctx);
+
+ memset(&msg[0], 0x5a, MCTP_BTU);
+
+ ctx.msg = &msg[0];
+ ctx.count = 0;
+ mctp_set_rx_all(ctx.host.mctp, rx_message, &ctx);
+
+ /* BMC sends the single-packet message */
+ rc = mctp_message_tx(ctx.bmc.mctp, 9, msg, sizeof(msg));
+ assert(rc == 0);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_OBF);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_DATA] == 0x01);
+
+ /* Check that the BMC sent a fully-formed packet */
+ astlpc_assert_tx_packet(&ctx.bmc, &msg[0], MCTP_BTU);
+
+ /* Corrupt the CRC-32 in the message trailer */
+ hdr = (struct mctp_lpcmap_hdr *)ctx.lpc_mem;
+ offset = be32toh(hdr->layout.rx_offset);
+ tlr = (uint8_t *)&ctx.lpc_mem[offset] + 4 + sizeof(msg);
+ memcpy(&code, tlr, sizeof(code));
+ code = ~code;
+ memcpy(tlr, &code, sizeof(code));
+
+ /* Host drops the single-packet message */
+ mctp_astlpc_poll(ctx.host.astlpc);
+ assert(ctx.count == 0);
+
+ /* Host returns Rx area ownership to BMC */
+ assert(!(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_OBF));
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_DATA] == 0x02);
+ assert(ctx.kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_IBF);
+
+ /* BMC dequeues ownership hand-over */
+ rc = mctp_astlpc_poll(ctx.bmc.astlpc);
+ assert(rc == 0);
+
+ network_destroy(&ctx);
+}
+
/* clang-format off */
#define TEST_CASE(test) { #test, test }
static const struct {
@@ -1214,6 +1319,8 @@
TEST_CASE(astlpc_test_negotiate_mtu_low_high),
TEST_CASE(astlpc_test_send_large_packet),
TEST_CASE(astlpc_test_tx_before_channel_init),
+ TEST_CASE(astlpc_test_corrupt_host_tx),
+ TEST_CASE(astlpc_test_corrupt_bmc_tx),
};
/* clang-format on */