diff --git a/CHANGELOG.md b/CHANGELOG.md index 80b25c9..be6df7d 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/mctp-util.c b/src/mctp-util.c index 5226718..ed003cb 100644 --- a/src/mctp-util.c +++ b/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; +} diff --git a/src/mctp-util.h b/src/mctp-util.h index c56aade..a4bb008 100644 --- a/src/mctp-util.h +++ b/src/mctp-util.h @@ -1,5 +1,8 @@ +#include #include +#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); diff --git a/src/mctpd.c b/src/mctpd.c index 8234fb4..6843c21 100644 --- a/src/mctpd.c +++ b/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); diff --git a/tests/test_mctpd.py b/tests/test_mctpd.py index 4fabf96..d2806c8 100644 --- a/tests/test_mctpd.py +++ b/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):