mctpd: improve handling for out-of-spec Set Endpoint ID responses

CHANGELOG.md

@@ -31,6 +31,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
    `au.com.codecontruct.MCTP.Interface1` interface, allowing link-to-network
    lookups
 
+5. mctpd: Better handling of strange cases of Set Endpoint ID responses,
+   where the reported endpoint EID may either be different from expected,
+   or invalid
+
 ### Changed
 
 1. tests are now run with address sanitizer enabled (-fsanitize=address)

src/mctp-util.c

@@ -156,3 +156,8 @@ char* bytes_to_uuid(const uint8_t u[16])
 		u[8], u[9], u[10], u[11], u[12], u[13], u[14], u[15]);
 	return buf;
 }
+
+bool mctp_eid_is_valid_unicast(mctp_eid_t eid)
+{
+	return eid >= 8 && eid < 0xff;
+}

src/mctp-util.h

@@ -1,5 +1,8 @@
+#include <stdbool.h>
 #include <stdint.h>
 
+#include "mctp.h"
+
 #define max(a, b) ((a) > (b) ? (a) : (b))
 #define min(a, b) ((a) < (b) ? (a) : (b))
 
@@ -13,3 +16,4 @@ int parse_uint32(const char *str, uint32_t *out);
 int parse_int32(const char *str, int32_t *out);
 /* Returns a malloced pointer */
 char* bytes_to_uuid(const uint8_t u[16]);
+bool mctp_eid_is_valid_unicast(mctp_eid_t eid);

src/mctpd.c

@@ -1299,7 +1299,8 @@ static int endpoint_query_phys(struct ctx *ctx, const dest_phys *dest,
 }
 
 /* returns -ECONNREFUSED if the endpoint returns failure. */
-static int endpoint_send_set_endpoint_id(const struct peer *peer, mctp_eid_t *new_eid)
+static int endpoint_send_set_endpoint_id(const struct peer *peer,
+					 mctp_eid_t *new_eidp)
 {
 	struct sockaddr_mctp_ext addr;
 	struct mctp_ctrl_cmd_set_eid req = {0};
@@ -1309,6 +1310,7 @@ static int endpoint_send_set_endpoint_id(const struct peer *peer, mctp_eid_t *ne
 	size_t buf_size;
 	uint8_t iid, stat, alloc;
 	const dest_phys *dest = &peer->phys;
+	mctp_eid_t new_eid;
 
 	rc = -1;
 
@@ -1331,18 +1333,34 @@ static int endpoint_send_set_endpoint_id(const struct peer *peer, mctp_eid_t *ne
 	resp = (void*)buf;
 
 	stat = resp->status >> 4 & 0x3;
+	new_eid = resp->eid_set;
+
+	// For both accepted and rejected cases, we learn the new EID of the
+	// endpoint. If this is a valid ID, we are likely to be able to handle
+	// this, as the caller may be able to change_peer_eid() to the
+	// newly-reported eid
 	if (stat == 0x01) {
-		// changed eid
+		if (!mctp_eid_is_valid_unicast(new_eid)) {
+			warnx("%s rejected assignment eid %d, and reported invalid eid %d",
+				dest_phys_tostr(dest), peer->eid, new_eid);
+			rc = -ECONNREFUSED;
+			goto out;
+		}
 	} else if (stat == 0x00) {
-		if (resp->eid_set != peer->eid) {
+		if (!mctp_eid_is_valid_unicast(new_eid)) {
+			warnx("%s eid %d replied with invalid eid %d, but 'accepted'",
+				dest_phys_tostr(dest), peer->eid, new_eid);
+			rc = -ECONNREFUSED;
+			goto out;
+		} else if (new_eid != peer->eid) {
 			warnx("%s eid %d replied with different eid %d, but 'accepted'",
-				dest_phys_tostr(dest), peer->eid, resp->eid_set);
+				dest_phys_tostr(dest), peer->eid, new_eid);
 		}
 	} else {
 		warnx("%s unexpected status 0x%02x",
 			dest_phys_tostr(dest), resp->status);
 	}
-	*new_eid = resp->eid_set;
+	*new_eidp = new_eid;
 
 	alloc = resp->status & 0x3;
 	if (alloc != 0) {
@@ -1519,6 +1537,9 @@ static int change_peer_eid(struct peer *peer, mctp_eid_t new_eid)
 	struct net *n = NULL;
 	int rc;
 
+	if (!mctp_eid_is_valid_unicast(new_eid))
+		return -EINVAL;
+
 	n = lookup_net(peer->ctx, peer->net);
 	if (!n) {
 		bug_warn("%s: Bad net %u", __func__, peer->net);

tests/test_mctpd.py

@@ -446,6 +446,107 @@ async def handle_mctp_control(self, sock, src_addr, msg):
 
     assert str(ex.value) == "Request failed"
 
+""" During a SetupEndpoint's Set Endpoint ID exchange, return a response
+that indicates that the EID has been set, but report an invalid (0) EID
+in the response."""
+async def test_setup_endpoint_invalid_set_eid_response(dbus, mctpd):
+    class InvalidEndpoint(Endpoint):
+        async def handle_mctp_control(self, sock, src_addr, msg):
+            flags, opcode = msg[0:2]
+            if opcode != 1:
+                return await super().handle_mctp_control(sock, src_addr, msg)
+            dst_addr = MCTPSockAddr.for_ep_resp(self, src_addr, sock.addr_ext)
+            self.eid = msg[3]
+            msg = bytes([
+                flags & 0x1f, # Rsp
+                0x01, # opcode: Set Endpoint ID
+                0x00, # cc: success
+                0x00, # assignment accepted, no pool
+                0x00, # set EID: invalid
+                0x00, # pool size: 0
+            ])
+            await sock.send(dst_addr, msg)
+
+    iface = mctpd.system.interfaces[0]
+    ep = InvalidEndpoint(iface, bytes([0x1e]), eid = 0)
+    mctpd.network.add_endpoint(ep)
+    mctp = await mctpd_mctp_iface_obj(dbus, iface)
+
+    with pytest.raises(asyncdbus.errors.DBusError) as ex:
+        rc = await mctp.call_setup_endpoint(ep.lladdr)
+
+    assert str(ex.value) == "Endpoint returned failure to Set Endpoint ID"
+
+""" During a SetupEndpoint's Set Endpoint ID exchange, return a response
+that indicates that the EID has been set, but report a different set EID
+in the response."""
+async def test_setup_endpoint_vary_set_eid_response(dbus, mctpd):
+    class VaryEndpoint(Endpoint):
+        async def handle_mctp_control(self, sock, src_addr, msg):
+            flags, opcode = msg[0:2]
+            if opcode != 1:
+                return await super().handle_mctp_control(sock, src_addr, msg)
+            dst_addr = MCTPSockAddr.for_ep_resp(self, src_addr, sock.addr_ext)
+            self.eid = msg[3] + 1
+            msg = bytes([
+                flags & 0x1f, # Rsp
+                0x01, # opcode: Set Endpoint ID
+                0x00, # cc: success
+                0x00, # assignment accepted, no pool
+                self.eid, # set EID: valid, but not what was assigned
+                0x00, # pool size: 0
+            ])
+            await sock.send(dst_addr, msg)
+
+    iface = mctpd.system.interfaces[0]
+    ep = VaryEndpoint(iface, bytes([0x1e]))
+    mctpd.network.add_endpoint(ep)
+    mctp = await mctpd_mctp_iface_obj(dbus, iface)
+
+    (eid, _, _, _) = await mctp.call_setup_endpoint(ep.lladdr)
+
+    assert eid == ep.eid
+
+""" During a SetupEndpoint's Set Endpoint ID exchange, return a response
+that indicates that the EID has been set, but report a different set EID
+in the response, which conflicts with another endpoint"""
+async def test_setup_endpoint_conflicting_set_eid_response(dbus, mctpd):
+
+    class ConflictingEndpoint(Endpoint):
+        def __init__(self, iface, lladdr, conflict_eid):
+            super().__init__(iface, lladdr)
+            self.conflict_eid = conflict_eid
+
+        async def handle_mctp_control(self, sock, src_addr, msg):
+            flags, opcode = msg[0:2]
+            if opcode != 1:
+                return await super().handle_mctp_control(sock, src_addr, msg)
+            dst_addr = MCTPSockAddr.for_ep_resp(self, src_addr, sock.addr_ext)
+            # reject reality, use a conflicting eid
+            self.eid = self.conflict_eid
+            msg = bytes([
+                flags & 0x1f, # Rsp
+                0x01, # opcode: Set Endpoint ID
+                0x00, # cc: success
+                0x00, # assignment accepted, no pool
+                self.eid, # set EID: valid, but not what was assigned
+                0x00, # pool size: 0
+            ])
+            await sock.send(dst_addr, msg)
+
+    iface = mctpd.system.interfaces[0]
+    ep1 = mctpd.network.endpoints[0]
+    mctp = await mctpd_mctp_iface_obj(dbus, iface)
+    (eid1, _, _, _) = await mctp.call_setup_endpoint(ep1.lladdr)
+    assert eid1 == ep1.eid
+
+    ep2 = ConflictingEndpoint(iface, bytes([0x1f]), ep1.eid)
+    mctpd.network.add_endpoint(ep2)
+    with pytest.raises(asyncdbus.errors.DBusError) as ex:
+        await mctp.call_setup_endpoint(ep2.lladdr)
+
+    assert "already used" in str(ex.value)
+
 """ Ensure a response with an invalid IID is discarded """
 async def test_learn_endpoint_invalid_response_iid(dbus, mctpd):
     class InvalidIIDEndpoint(Endpoint):