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

We may have endpoints reporting strange EID values in their set endpoint
ID response, even when reporting that the assignement was accepted.
Particularly, if the EID is in the invalid range, we will still attempt
to add neighour and route entries for this invalid EID.

We still want to accommodate valid IDs that were not the same as the one
assigned, but ensure the EID is valid before processing.

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

Author: jk-ozlabs

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,37 @@ 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"
+
 """ Ensure a response with an invalid IID is discarded """
 async def test_learn_endpoint_invalid_response_iid(dbus, mctpd):
     class InvalidIIDEndpoint(Endpoint):