diff --git a/.gitignore b/.gitignore index 467902d..bb26e7f 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,9 @@ dhcp4relay/debian/* !dhcp4relay/debian/compat !dhcp4relay/debian/control !dhcp4relay/debian/rules + +# Build artifacts +*.buildinfo +*.changes +dhcp4relay/PcapPlusPlus-*/ +dhcp4relay/pcappp.stamp diff --git a/dhcp4relay/src/dhcp4_sender.cpp b/dhcp4relay/src/dhcp4_sender.cpp index 7b98c7e..299c3b4 100644 --- a/dhcp4relay/src/dhcp4_sender.cpp +++ b/dhcp4relay/src/dhcp4_sender.cpp @@ -20,15 +20,20 @@ * * @return boolean True if packet successfully sent */ +void bootp_pad(uint8_t *out, const uint8_t *buffer, uint32_t *len, bool pad) { + if (pad && *len < BOOTP_MIN_LEN) { + memcpy(out, buffer, *len); + *len = BOOTP_MIN_LEN; + } +} + #ifndef UNIT_TEST bool send_udp(int sock, uint8_t *buffer, struct sockaddr_in target, uint32_t len, in_addr src_ip, bool use_src_ip, bool pad) { - /* Pad additional bytes if length is lesser than 300 - * to make DHCP packet length to minimum of 300 bytes */ - if (pad && len < BOOTP_MIN_LEN) { - auto pad_len = BOOTP_MIN_LEN - len; - memset(buffer+len, 0, pad_len); - len = BOOTP_MIN_LEN; - } + uint8_t padded[BOOTP_MIN_LEN] = {}; + uint32_t orig_len = len; + bootp_pad(padded, buffer, &len, pad); + if (len != orig_len) + buffer = padded; if (use_src_ip && src_ip.s_addr != 0) { // Enable IP_PKTINFO on the socket diff --git a/dhcp4relay/src/dhcp4_sender.h b/dhcp4relay/src/dhcp4_sender.h index e8e65e1..70f0cd0 100644 --- a/dhcp4relay/src/dhcp4_sender.h +++ b/dhcp4relay/src/dhcp4_sender.h @@ -6,6 +6,25 @@ #include #define BOOTP_MIN_LEN 300 + +/** + * @brief Apply BOOTP minimum-length padding. + * + * Copies *buffer into out and updates *len to BOOTP_MIN_LEN when pad is true + * and *len < BOOTP_MIN_LEN. out must point to at least BOOTP_MIN_LEN + * zero-initialised bytes; the caller is responsible for switching to out as + * the send buffer when the returned length differs from the input length. + * + * Compiled in both production and UNIT_TEST builds so tests can exercise the + * padding logic directly without linking the real send_udp(). + * + * @param out destination buffer (>= BOOTP_MIN_LEN bytes, zero-filled) + * @param buffer source packet bytes + * @param len packet length; updated to BOOTP_MIN_LEN if padding applied + * @param pad enable padding + */ +void bootp_pad(uint8_t *out, const uint8_t *buffer, uint32_t *len, bool pad); + /** * @code bool send_udp(int sock, uint8_t *buffer, struct sockaddr_in target, uint32_t len, const char* src_ip, bool use_src_ip); * @@ -13,7 +32,7 @@ * * @param *buffer message buffer * @param sockaddr_in target target socket - * @param len length of message + * @param len length of message * @param src_ip source IP address as string (optional) * @param use_src_ip if true, use src_ip as source address * @param pad if true, do padding diff --git a/dhcp4relay/src/dhcp4relay.cpp b/dhcp4relay/src/dhcp4relay.cpp index 324a498..6d965d6 100644 --- a/dhcp4relay/src/dhcp4relay.cpp +++ b/dhcp4relay/src/dhcp4relay.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -23,9 +24,10 @@ struct event_base *base; struct event *ev_sigint; struct event *ev_sigterm; -extern bool feature_dhcp_server_enabled; +extern std::atomic feature_dhcp_server_enabled; extern std::string global_dhcp_server_ip; extern metadata_config m_config; +extern std::mutex m_config_mutex; static uint8_t client_recv_buffer[BUFFER_SIZE]; int config_pipe[2]; @@ -260,12 +262,15 @@ void prepare_relay_interface_config(relay_config &interface_config) { return; } - if (m_config.is_dualTor) { - /* If DualTor is enabled, we set source interface to "Loopback0" - and link_selection option will be enabled during encoding if is_dualTor is enabled */ - interface_config.source_interface = "Loopback0"; - SWSS_LOG_INFO("[DHCPV4_INFO][DualTor] %s: link_selection_opt is enabled and source interface is set to %s", - interface_config.vlan.c_str(), interface_config.source_interface.c_str()); + { + std::lock_guard lk(m_config_mutex); + if (m_config.is_dualTor) { + /* If DualTor is enabled, we set source interface to "Loopback0" + and link_selection option will be enabled during encoding if is_dualTor is enabled */ + interface_config.source_interface = "Loopback0"; + SWSS_LOG_INFO("[DHCPV4_INFO][DualTor] %s: link_selection_opt is enabled and source interface is set to %s", + interface_config.vlan.c_str(), interface_config.source_interface.c_str()); + } } if (interface_config.source_interface.length() > 0) { @@ -452,12 +457,19 @@ int prepare_vlan_sockets(relay_config &config) { return 0; } -uint8_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, uint8_t *v) { - *buf = t; - *(buf + DHCP_SUB_OPT_TLV_LENGTH_OFFSET) = l; - memcpy((buf + DHCP_SUB_OPT_TLV_HEADER_LEN), v, l); - return (l + DHCP_SUB_OPT_TLV_HEADER_LEN); +/* + * Writes one TLV sub-option into buf if remaining space allows. + * Returns bytes written (type + length + value), or 0 on overflow. + */ +size_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, const uint8_t *v, size_t remaining) { + size_t needed = (size_t)l + DHCP_SUB_OPT_TLV_HEADER_LEN; + if (needed > remaining) + return 0; + buf[0] = t; + buf[DHCP_SUB_OPT_TLV_LENGTH_OFFSET] = l; + memcpy(buf + DHCP_SUB_OPT_TLV_HEADER_LEN, v, l); + return needed; } std::string get_mac_address(const std::string &ifname) { @@ -473,12 +485,19 @@ std::string get_mac_address(const std::string &ifname) { } void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) { - uint8_t buf[256] = {0}; + uint8_t buf[DHCP_OPTION_VALUE_MAX_LEN] = {0}; uint8_t buf_offset = 0; std::string bm_mac; auto vrf = vlan_vrf_map[config->vlan.c_str()]; + /* Snapshot m_config once to avoid races with the config thread. */ + metadata_config snap; + { + std::lock_guard lk(m_config_mutex); + snap = m_config; + } + /* Get interface alias */ std::string intf_alias; if (phy_interface_alias_map.find(config->phy_interface) != phy_interface_alias_map.end()) { @@ -489,61 +508,96 @@ void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) { /* | 1 | 4 | hostname:interface_alias:vlan | */ std::string circuit_id; if (feature_dhcp_server_enabled) { - circuit_id = m_config.hostname + ":" + intf_alias; + circuit_id = snap.hostname + ":" + intf_alias; } else { - circuit_id = m_config.hostname + ":" + intf_alias + ":" + config->vlan; + circuit_id = snap.hostname + ":" + intf_alias + ":" + config->vlan; + } + + if (circuit_id.length() > UINT8_MAX) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] circuit-id length %zu exceeds maximum on %s, dropping option 82", + circuit_id.length(), config->vlan.c_str()); + return; + } + auto offset = encode_tlv(buf, OPTION82_SUBOPT_CIRCUIT_ID, (uint8_t)circuit_id.length(), + (uint8_t *)circuit_id.c_str(), sizeof(buf)); + if (!offset) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] circuit-id length %zu exceeds buffer on %s, dropping option 82", + circuit_id.length(), config->vlan.c_str()); + return; } - auto offset = encode_tlv(buf, OPTION82_SUBOPT_CIRCUIT_ID, circuit_id.length(), - (uint8_t *)circuit_id.c_str()); buf_offset += offset; - if (!m_config.midplane_bridge.empty()) { - bm_mac = get_mac_address(m_config.midplane_bridge); + if (!snap.midplane_bridge.empty()) { + bm_mac = get_mac_address(snap.midplane_bridge); } - /* Encode remote ID sub-option */ + /* Encode remote ID sub-option (required, like circuit-id) */ /* | 2 | 6 | my_mac| */ /* if its SmartSwitch we need to fetch mac of bridge-midplane */ - if ((m_config.is_SmartSwitch) && (!bm_mac.empty())) { + if ((snap.is_SmartSwitch) && (!bm_mac.empty())) { + uint8_t len = (uint8_t)std::min((size_t)MAC_ADDR_STR_LEN, bm_mac.length()); offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_REMOTE_ID, - MAC_ADDR_STR_LEN, (uint8_t *)(bm_mac.c_str())); - buf_offset += offset; + len, (uint8_t *)(bm_mac.c_str()), sizeof(buf) - buf_offset); } else { + uint8_t len = (uint8_t)std::min((size_t)MAC_ADDR_STR_LEN, snap.host_mac_addr.length()); offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_REMOTE_ID, - MAC_ADDR_STR_LEN, (uint8_t *)(m_config.host_mac_addr.c_str())); - buf_offset += offset; + len, (uint8_t *)(snap.host_mac_addr.c_str()), sizeof(buf) - buf_offset); } + if (!offset) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] remote-id does not fit after circuit-id on %s, dropping option 82", + config->vlan.c_str()); + return; + } + buf_offset += offset; /* TODO: this sub-option should be set if source interface selection is enabled */ /* | 5 | 4 | ipv4 | */ - if (m_config.is_dualTor || config->link_selection_opt == "enable") { + if (snap.is_dualTor || config->link_selection_opt == "enable") { /* RFC 3527 specifies an address contained in the client subnet; match ISC's VLAN address. */ uint32_t link_sel_ip = config->link_address.sin_addr.s_addr; - offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_LINK_SELECTION, sizeof(uint32_t), - (uint8_t *)&link_sel_ip); + offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_LINK_SELECTION, + sizeof(uint32_t), (uint8_t *)&link_sel_ip, sizeof(buf) - buf_offset); + if (!offset) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] link-selection does not fit on %s, dropping option 82", + config->vlan.c_str()); + return; + } buf_offset += offset; } /* | 11 | 4 | ipv4 | */ if (config->server_id_override_opt == "enable") { - offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_SERVER_OVERRIDE, sizeof(uint32_t), - (uint8_t *)(&(config->link_address.sin_addr.s_addr))); + offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_SERVER_OVERRIDE, + sizeof(uint32_t), (uint8_t *)(&(config->link_address.sin_addr.s_addr)), + sizeof(buf) - buf_offset); + if (!offset) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] server-override does not fit on %s, dropping option 82", + config->vlan.c_str()); + return; + } buf_offset += offset; } /* Encode VSS sub-option 151 if client is not default VRF */ /* | 151 | vrf_len | 0 | vrf_name | */ - uint8_t vss_buf[32] = {0}; /* Enable VSS only if client and server are in two different VRF's */ if ((config->vrf_selection_opt == "enable") && (vrf != "default") && (config->vrf != vrf)) { - uint8_t zero_encode = 0; - memcpy(vss_buf, &zero_encode, sizeof(uint8_t)); - memcpy((vss_buf + 1), (uint8_t *)vrf.c_str(), (uint8_t)vrf.length()); - - offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_VIRTUAL_SUBNET, - (uint8_t)(vrf.length() + 1), vss_buf); - buf_offset += offset; + if (vrf.length() > OPTION82_VSS_VRF_MAX_LEN) { + SWSS_LOG_WARN("[DHCPV4_RELAY] VRF name '%s' exceeds %d bytes, skipping VSS sub-option", + vrf.c_str(), OPTION82_VSS_VRF_MAX_LEN); + } else { + uint8_t vss_buf[OPTION82_VSS_VRF_MAX_LEN + 1] = {0}; + memcpy(vss_buf + 1, vrf.c_str(), vrf.length()); + offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_VIRTUAL_SUBNET, + (uint8_t)(vrf.length() + 1), vss_buf, sizeof(buf) - buf_offset); + if (!offset) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] VSS sub-option does not fit on %s, dropping option 82", + config->vlan.c_str()); + return; + } + buf_offset += offset; + } } /* We shouldn't append relay information if packet size is exceeding MTU size */ @@ -571,6 +625,12 @@ void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) { * @return none */ void from_client(pcpp::DhcpLayer *dhcp_pkt, relay_config &config) { + if (dhcp_pkt->getHeaderLen() < sizeof(pcpp::dhcp_header)) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping short DHCP packet from client on %s: len %zu < %zu", + config.vlan.c_str(), dhcp_pkt->getHeaderLen(), sizeof(pcpp::dhcp_header)); + dhcp_cntr_table.increment_counter(config.vlan, "RX", DHCPv4_MESSAGE_TYPE_DROP); + return; + } /* Update giaddr */ if (!(dhcp_pkt->getDhcpHeader()->gatewayIpAddress)) { const bool is_dhcp = @@ -636,9 +696,12 @@ void from_client(pcpp::DhcpLayer *dhcp_pkt, relay_config &config) { // Backward compatibility for deployment_id 8. If deployment_id is 8, use client interface IP as source IP bool use_intf_ip_as_src_ip = false; in_addr src_ip = {0}; - if (m_config.deployment_id == 8) { - use_intf_ip_as_src_ip = true; - src_ip.s_addr = config.link_address.sin_addr.s_addr; + { + std::lock_guard lk(m_config_mutex); + if (m_config.deployment_id == 8) { + use_intf_ip_as_src_ip = true; + src_ip.s_addr = config.link_address.sin_addr.s_addr; + } } for (auto server : config.servers_sock) { @@ -664,7 +727,7 @@ uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_ while (temp && ((offset + DHCP_SUB_OPT_TLV_HEADER_LEN) <= options_total_size)) { len = *(temp + DHCP_SUB_OPT_TLV_LENGTH_OFFSET); - if ((offset + DHCP_SUB_OPT_TLV_LENGTH_OFFSET + len) > options_total_size) { + if ((offset + DHCP_SUB_OPT_TLV_HEADER_LEN + len) > options_total_size) { /* Malformed packet */ SWSS_LOG_ERROR("[DHCPV4_INFO] Failed to decode relay agent sub-option %d" " exceeded total option len %d offset %d sub-option len %d", @@ -696,7 +759,12 @@ uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_ * @return none */ void to_client(pcpp::DhcpLayer *dhcp_pkt, std::unordered_map *vlans, - std::string src_ip) { + std::string src_ip, const std::string &ingress_intf) { + if (dhcp_pkt->getHeaderLen() < sizeof(pcpp::dhcp_header)) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping short DHCP server reply from %s: len %zu < %zu", + src_ip.c_str(), dhcp_pkt->getHeaderLen(), sizeof(pcpp::dhcp_header)); + return; + } struct ifaddrs *ifa, *ifa_tmp; struct sockaddr_in target_addr = {0}; uint32_t giaddr = dhcp_pkt->getDhcpHeader()->gatewayIpAddress; @@ -717,6 +785,15 @@ void to_client(pcpp::DhcpLayer *dhcp_pkt, std::unordered_mapgetOptionData(pcpp::DHCPOPT_DHCP_AGENT_OPTIONS); auto options_ptr = agent_option.getValue(); auto agent_option_size = agent_option.getDataSize(); @@ -962,6 +1039,9 @@ void pkt_in_callback(evutil_socket_t fd, short event, void *arg) { } return; } + /* Prevent a preceding frame from priming header bytes that fall past + * the current frame's boundary (e.g. magic-cookie priming attack). */ + memset(client_recv_buffer + buffer_sz, 0, sizeof(client_recv_buffer) - buffer_sz); /* Find ingress VLAN */ sll = (struct sockaddr_ll *)msg.msg_name; @@ -989,15 +1069,22 @@ void pkt_in_callback(evutil_socket_t fd, short event, void *arg) { } std::string vlan_str; + bool snap_is_SmartSwitch; + std::string snap_midplane_bridge; + { + std::lock_guard lk(m_config_mutex); + snap_is_SmartSwitch = m_config.is_SmartSwitch; + snap_midplane_bridge = m_config.midplane_bridge; + } if (vlan_id == 0) { /* vlan_id can be 0 when we receive packet from the server */ auto vlan = vlan_map.find(intf); if (vlan == vlan_map.end()) { if (intf.find(CLIENT_IF_PREFIX) != std::string::npos) { SWSS_LOG_WARN("[DHCPV4_RELAY] Invalid input interface %s", interface_name); - } else if ((m_config.is_SmartSwitch) && (intf.rfind("dpu", 0) == 0) && !m_config.midplane_bridge.empty()) { + } else if (snap_is_SmartSwitch && (intf.rfind("dpu", 0) == 0) && !snap_midplane_bridge.empty()) { // if its SmartSwitch, we need to check for bridge_midplane interface - vlan_str = m_config.midplane_bridge; + vlan_str = snap_midplane_bridge; } } else { vlan_str = vlan->second; @@ -1074,6 +1161,17 @@ void pkt_in_callback(evutil_socket_t fd, short event, void *arg) { continue; } + /* Reject a truncated DHCP layer before any getDhcpHeader()/getMessageType() + * access below dereferences fields past the received bytes. */ + if (dhcp_pkt->getHeaderLen() < sizeof(pcpp::dhcp_header)) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping short DHCP packet from interface %s: len %zu < %zu", + intf.c_str(), dhcp_pkt->getHeaderLen(), sizeof(pcpp::dhcp_header)); + if (!vlan_str.empty()) { + dhcp_cntr_table.increment_counter(vlan_str, "RX", DHCPv4_MESSAGE_TYPE_MALFORMED); + } + continue; + } + if (dhcp_pkt->getDhcpHeader()->opCode == BOOTPREQUEST) { if (vlan_str.empty()) { continue; @@ -1092,7 +1190,7 @@ void pkt_in_callback(evutil_socket_t fd, short event, void *arg) { dhcp_cntr_table.increment_counter(config.vlan, "RX", (int)dhcp_pkt->getMessageType()); from_client(dhcp_pkt, config_itr->second); } else if (dhcp_pkt->getDhcpHeader()->opCode == BOOTPREPLY) { - to_client(dhcp_pkt, vlans, src_ip); + to_client(dhcp_pkt, vlans, src_ip, intf); } else { if (!vlan_str.empty()) { dhcp_cntr_table.increment_counter(vlan_str, "RX", DHCPv4_MESSAGE_TYPE_UNKNOWN); @@ -1402,11 +1500,16 @@ static void apply_config_event(const event_config &received_event, delete_all_relay_configs(vlans); } else if (received_event.type == DHCPv4_SERVER_IP_UPDATE) { SWSS_LOG_INFO("[DHCPV4_RELAY] dhcp_server IP update in state DB event received"); + std::string server_ip; + { + std::lock_guard lk(m_config_mutex); + server_ip = global_dhcp_server_ip; + } for (auto it = vlans->begin(); it != vlans->end(); ++it) { relay_config &config = it->second; config.servers.clear(); config.servers_sock.clear(); - config.servers.push_back(global_dhcp_server_ip); + config.servers.push_back(server_ip); prepare_relay_server_config(config); } } else if (received_event.type == DHCPv4_RELAY_DUAL_TOR_UPDATE) { diff --git a/dhcp4relay/src/dhcp4relay.h b/dhcp4relay/src/dhcp4relay.h index 1654ed6..6e14bb0 100644 --- a/dhcp4relay/src/dhcp4relay.h +++ b/dhcp4relay/src/dhcp4relay.h @@ -48,6 +48,14 @@ #define DHCP_SUB_OPT_TLV_LENGTH_OFFSET 1 #define DHCP_SUB_OPT_TLV_HEADER_LEN 2 +/* DHCP option value is length-prefixed with a 1-byte field, so max 255 bytes. */ +#define DHCP_OPTION_VALUE_MAX_LEN 255 +/* + * VRF names in SONiC are Linux network interfaces (IFNAMSIZ = 16, null-terminated), + * so the max usable length is IF_NAMESIZE - 1 = 15. + */ +#define OPTION82_VSS_VRF_MAX_LEN (IF_NAMESIZE - 1) + #define lengthof(A) (sizeof(A) / sizeof(A)[0]) extern char vrf_single[IF_NAMESIZE]; @@ -320,5 +328,5 @@ void update_vlan_mapping(std::string vlan, bool is_add); */ void pkt_in_callback(evutil_socket_t fd, short event, void *arg); void config_event_callback(evutil_socket_t fd, short event, void *arg); +size_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, const uint8_t *v, size_t remaining); uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_total_size); -uint8_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, uint8_t *v); diff --git a/dhcp4relay/src/dhcp4relay_mgr.cpp b/dhcp4relay/src/dhcp4relay_mgr.cpp index 7fabec0..19412f0 100644 --- a/dhcp4relay/src/dhcp4relay_mgr.cpp +++ b/dhcp4relay/src/dhcp4relay_mgr.cpp @@ -13,11 +13,12 @@ using namespace swss; metadata_config m_config; -bool feature_dhcp_server_enabled = false; +std::atomic feature_dhcp_server_enabled{false}; std::shared_ptr config_db_dhcp_server_ipv4_ptr = NULL; std::shared_ptr state_db_dhcp_server_ipv4_ip_ptr = NULL; std::shared_ptr config_db_relaymgr_table_ptr = NULL; std::string global_dhcp_server_ip; +std::mutex m_config_mutex; /** * @brief Initializes the configuration listener for the DHCP manager. * @@ -129,28 +130,40 @@ void DHCPMgr::handle_swss_notification() { continue; } - if (!feature_dhcp_server_enabled) { - if (config_db_relaymgr_table_ptr && selectable == config_db_relaymgr_table_ptr.get()) { - config_db_relaymgr_table_ptr->pops(entries); + /* Always pops() whatever selectable fired so its keyspace-event buffer + drains, then gate processing on feature_dhcp_server_enabled. Leaving a + feature-gated selectable undrained keeps hasCachedData() true, which + makes swss::Select re-queue it and spin at 100% CPU. */ + if (config_db_relaymgr_table_ptr && selectable == config_db_relaymgr_table_ptr.get()) { + config_db_relaymgr_table_ptr->pops(entries); + if (!feature_dhcp_server_enabled) { process_relay_notification(entries); - } else if (selectable == static_cast(&config_db_interface_table)) { - config_db_interface_table.pops(entries); + } + } else if (selectable == static_cast(&config_db_interface_table)) { + config_db_interface_table.pops(entries); + if (!feature_dhcp_server_enabled) { process_interface_notification(entries); - } else if (selectable == static_cast(&config_db_loopback_table)) { - config_db_loopback_table.pops(entries); + } + } else if (selectable == static_cast(&config_db_loopback_table)) { + config_db_loopback_table.pops(entries); + if (!feature_dhcp_server_enabled) { process_interface_notification(entries); - } else if (selectable == static_cast(&config_db_portchannel_table)) { - config_db_portchannel_table.pops(entries); + } + } else if (selectable == static_cast(&config_db_portchannel_table)) { + config_db_portchannel_table.pops(entries); + if (!feature_dhcp_server_enabled) { process_interface_notification(entries); - } - } else { - if (config_db_dhcp_server_ipv4_ptr && selectable == config_db_dhcp_server_ipv4_ptr.get()) { - config_db_dhcp_server_ipv4_ptr->pops(entries); + } + } else if (config_db_dhcp_server_ipv4_ptr && selectable == config_db_dhcp_server_ipv4_ptr.get()) { + config_db_dhcp_server_ipv4_ptr->pops(entries); + if (feature_dhcp_server_enabled) { process_dhcp_server_ipv4_notification(entries); - } else if (state_db_dhcp_server_ipv4_ip_ptr && selectable == state_db_dhcp_server_ipv4_ip_ptr.get()) { - state_db_dhcp_server_ipv4_ip_ptr->pops(entries); + } + } else if (state_db_dhcp_server_ipv4_ip_ptr && selectable == state_db_dhcp_server_ipv4_ip_ptr.get()) { + state_db_dhcp_server_ipv4_ip_ptr->pops(entries); + if (feature_dhcp_server_enabled) { process_dhcp_server_ipv4_ip_notification(entries, swss_select, config_db_ptr); - } + } } if (selectable == static_cast(&config_db_device_metadata_table)) { @@ -204,18 +217,25 @@ void DHCPMgr::process_device_metadata_notification(std::deque lk(m_config_mutex); + new_config = m_config; + } + for (auto &field : field_values) { std::string f = fvField(field); std::string v = fvValue(field); if (f == "hostname") { - m_config.hostname = v; + new_config.hostname = v; } else if (f == "mac") { std::transform(v.begin(), v.end(), v.begin(), ::tolower); - m_config.host_mac_addr = v; + new_config.host_mac_addr = v; } else if (f == "deployment_id") { try { - m_config.deployment_id = static_cast(std::stoul(v)); + new_config.deployment_id = static_cast(std::stoul(v)); } catch (const std::exception &e) { SWSS_LOG_WARN("[DHCPV4_RELAY] Invalid deployment_id value '%s': %s", v.c_str(), e.what()); } @@ -224,58 +244,74 @@ void DHCPMgr::process_device_metadata_notification(std::deque lk(m_config_mutex); + m_config = std::move(new_config); + } - if (m_config.is_dualTor) { - relay_msg->is_add = true; - } else { - relay_msg->is_add = false; - } + /* Publish the DualToR update only after m_config is committed, so the + * main thread's prepare_relay_interface_config() observes the fresh + * is_dualTor instead of a stale value. Publishing once here (rather than + * per field inside the loop) also avoids duplicate events. */ + if (send_dualTor_event) { + relay_config *relay_msg = nullptr; + try { + relay_msg = new relay_config(); + } catch (const std::bad_alloc &e) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] Memory allocation failed: %s", e.what()); + return; + } + relay_msg->is_add = is_dualTor_final; - event_config event; - event.type = DHCPv4_RELAY_DUAL_TOR_UPDATE; - event.msg = static_cast(relay_msg); - // Write the pointer address to the pipe - if (write(config_pipe[1], &event, sizeof(event)) == -1) { - SWSS_LOG_ERROR("[DHCPV4_RELAY] Failed to write to config update pipe: %s", strerror(errno)); - delete relay_msg; - } - } - } - /* Re-set hostname to default value if hostname is deleted */ - if (m_config.hostname.length() == 0) { - m_config.hostname = "sonic"; + event_config event; + event.type = DHCPv4_RELAY_DUAL_TOR_UPDATE; + event.msg = static_cast(relay_msg); + // Write the pointer address to the pipe + if (write(config_pipe[1], &event, sizeof(event)) == -1) { + SWSS_LOG_ERROR("[DHCPV4_RELAY] Failed to write to config update pipe: %s", strerror(errno)); + delete relay_msg; + } } } } @@ -516,7 +552,10 @@ void DHCPMgr::process_feature_notification(std::deque lk(m_config_mutex); + global_dhcp_server_ip.clear(); + } vlans_copy.clear(); //Delete the old auto generated relay config in main thread event_config event; @@ -577,18 +616,28 @@ void DHCPMgr::process_dhcp_server_ipv4_ip_notification(std::deque lk(m_config_mutex); + is_modify = !global_dhcp_server_ip.empty() && (global_dhcp_server_ip != server_ip); + if (is_modify) { + prev_ip = global_dhcp_server_ip; + } + global_dhcp_server_ip = server_ip; + } + if (is_modify) { event_config event; event.type = DHCPv4_SERVER_IP_UPDATE; if (write(config_pipe[1], &event, sizeof(event)) == -1) { SWSS_LOG_ERROR("[DHCPV4_RELAY] Failed to send delete event for dhcp_server IP update"); + std::lock_guard lk(m_config_mutex); + global_dhcp_server_ip = prev_ip; return; } - is_modify = true; } - global_dhcp_server_ip = server_ip; //Since the server IP see newly added, restart the listener for the dhcp_server config. if (!is_modify) { SWSS_LOG_INFO("[DHCPV4_RELAY] Restarting the dhcp_server listener"); @@ -607,7 +656,10 @@ void DHCPMgr::process_dhcp_server_ipv4_ip_notification(std::deque lk(m_config_mutex); + global_dhcp_server_ip.clear(); + } vlans_copy.clear(); } } @@ -755,14 +807,23 @@ void DHCPMgr::process_dhcp_server_ipv4_notification(std::deque lk(m_config_mutex); + server_ip = global_dhcp_server_ip; + } + if (server_ip.empty()) { std::shared_ptr state_db_ptr = std::make_shared("STATE_DB", 0); swss::Table ip_tbl(state_db_ptr.get(), "DHCP_SERVER_IPV4_SERVER_IP"); std::string ip; ip_tbl.hget("eth0", "ip", ip); if (!ip.empty()) { - global_dhcp_server_ip = ip; + { + std::lock_guard lk(m_config_mutex); + global_dhcp_server_ip = ip; + server_ip = ip; + } SWSS_LOG_INFO("[DHCPV4_RELAY] Fetched DHCPv4 server IP from STATE_DB: %s", ip.c_str()); } else { SWSS_LOG_ERROR("[DHCPV4_RELAY] Failed to get DHCPv4 server IP from STATE_DB"); @@ -771,7 +832,7 @@ void DHCPMgr::process_dhcp_server_ipv4_notification(std::dequeis_add = true; - relay_msg->servers.push_back(global_dhcp_server_ip); + relay_msg->servers.push_back(server_ip); relay_msg->vrf = "default"; } else if (state == "disabled") { relay_msg->is_add = false; //In case of modify in state field need to delete the entry diff --git a/dhcp4relay/test/mock_relay.cpp b/dhcp4relay/test/mock_relay.cpp index 510b923..d9f9d03 100644 --- a/dhcp4relay/test/mock_relay.cpp +++ b/dhcp4relay/test/mock_relay.cpp @@ -10,6 +10,7 @@ #include "gmock/gmock.h" #include "mock_relay.h" #include "mock_table.h" +#include "../src/dhcp4_sender.h" #include #include @@ -29,7 +30,7 @@ MOCK_GLOBAL_FUNC7(send_udp, bool(int, uint8_t *, struct sockaddr_in, uint32_t, i void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config); void to_client(pcpp::DhcpLayer* dhcp_pkt, std::unordered_map *vlans, - std::string src_ip); + std::string src_ip, const std::string &ingress_intf); void from_client(pcpp::DhcpLayer *dhcp_pkt, relay_config &config); ssize_t RealWrite(int fd, const void *buf, size_t count) { @@ -86,7 +87,7 @@ TEST(EncodeDecodeTLV, EncodeAndDecode) { uint8_t value[3] = {0x11, 0x22, 0x33}; uint8_t length = 0; - uint8_t encoded_length = encode_tlv(buffer, 1, 3, value); + size_t encoded_length = encode_tlv(buffer, 1, 3, value, sizeof(buffer)); EXPECT_EQ(encoded_length, 5); EXPECT_EQ(buffer[0], 1); EXPECT_EQ(buffer[1], 3); @@ -102,6 +103,31 @@ TEST(EncodeDecodeTLV, EncodeAndDecode) { EXPECT_EQ(decoded_value[2], 0x33); } +/* A sub-option whose length runs one byte past options_total_size must be + rejected. The value spans offset+2 .. offset+2+len-1, so the last byte needs + options_total_size to be at least offset + TLV_HEADER + len. Here type+len + occupy 2 bytes and len=3, so 5 value+header bytes need size 5; declaring the + buffer as size 4 makes the final value byte (index 4) out of bounds. */ +TEST(EncodeDecodeTLV, DecodeRejectsTruncatedSubOption) { + uint8_t buffer[5] = {1, 3, 0x11, 0x22, 0x33}; + uint8_t length = 7; + + /* options_total_size deliberately one byte short of the full TLV. */ + uint8_t *decoded_value = decode_tlv(buffer, 1, length, 4); + EXPECT_EQ(decoded_value, nullptr); + EXPECT_EQ(length, 0); +} + +/* Boundary: a sub-option that exactly fills options_total_size is valid. */ +TEST(EncodeDecodeTLV, DecodeAcceptsExactFit) { + uint8_t buffer[5] = {1, 3, 0x11, 0x22, 0x33}; + uint8_t length = 0; + + uint8_t *decoded_value = decode_tlv(buffer, 1, length, 5); + ASSERT_NE(decoded_value, nullptr); + EXPECT_EQ(length, 3); +} + TEST(sock, sock_open) { struct sock_filter ether_relay_filter[] = { { 0x6, 0, 0, 0x00040000 }, @@ -687,7 +713,7 @@ TEST(DHCPMgrTest, dhcp_server_feature_enable) { std::this_thread::sleep_for(std::chrono::milliseconds(100)); EXPECT_EQ(global_dhcp_server_ip, "240.127.1.2"); - feature_dhcp_server_enabled = false; + feature_dhcp_server_enabled.store(false); global_dhcp_server_ip.clear(); } @@ -702,13 +728,13 @@ TEST(DHCPMgrTest, dhcp_server_feature_disable) { std::vector> disable_dhcp_server = { {"state", "disabled"}, }; - feature_dhcp_server_enabled = true; + feature_dhcp_server_enabled.store(true); feature_table.set("dhcp_server", disable_dhcp_server); std::this_thread::sleep_for(std::chrono::seconds(1)); dhcpMgr.stop_db_updates();; std::this_thread::sleep_for(std::chrono::milliseconds(100)); - feature_dhcp_server_enabled = false; + feature_dhcp_server_enabled.store(false); } TEST(DHCPMgrTest, dhcp_server_ip_modification) { @@ -946,7 +972,35 @@ TEST(DHCPRelayTest, to_client) { EXPECT_EQ((dhcp_hdr->gatewayIpAddress), inet_addr("192.168.1.1")); return true; }); - to_client(&dhcpLayer, &vlans, "172.22.178.234"); + to_client(&dhcpLayer, &vlans, "172.22.178.234", "Ethernet4"); +} + +TEST(DHCPRelayTest, to_client_ingress_on_client_vlan) { + std::unordered_map vlans; + + pcpp::MacAddress clientMac(std::string("00:0e:86:11:c0:75")); + pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_OFFER, clientMac); + dhcpLayer.getDhcpHeader()->hops = 1; + dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.1"); + dhcpLayer.getDhcpHeader()->opCode = 1; + + relay_config config = {}; + config.vlan = "Vlan10"; + config.client_sock = 1; + vlan_vrf_map["Vlan10"] = "default"; + vlans["Vlan10"] = config; + /* Simulate Ethernet12 being a client-facing VLAN member port */ + vlan_map["Ethernet12"] = "Vlan10"; + + /* giaddr must resolve to Vlan10 so removing the ingress drop reaches send_udp */ + struct ifaddrs *mock_ifaddrs = CreateMockIfaddrs("192.168.1.1", "255.255.255.0", "Vlan10", "192.168.1.2", "Ethernet4"); + EXPECT_GLOBAL_CALL(getifaddrs, getifaddrs(_)).WillOnce(DoAll( + testing::SetArgPointee<0>(mock_ifaddrs), Return(0))); + EXPECT_GLOBAL_CALL(freeifaddrs, freeifaddrs(_)).Times(1); + EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0); + to_client(&dhcpLayer, &vlans, "10.0.0.1", "Ethernet12"); + vlan_map.erase("Ethernet12"); + FreeMockIfaddrs(mock_ifaddrs); } TEST(DHCPRelayTest, from_client) { @@ -1155,3 +1209,204 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_discard) { EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0); from_client(&dhcpLayer, config); } +/* Short DHCP packet (< sizeof(dhcp_header)): must be dropped before touching header fields. */ +TEST(DHCPRelayTest, from_client_short_header) { + /* Allocate on the heap: pcpp::Layer(data, len, nullptr, nullptr) sets + * m_IsAllocatedInPacket=false so ~Layer() always calls delete[] m_Data. + * Stack storage would cause an ASAN invalid-free. */ + uint8_t* raw = new uint8_t[50](); + raw[0] = 0x01; /* opcode = BOOTREQUEST */ + raw[1] = 0x01; raw[2] = 0x06; + raw[3] = 0x00; /* hops */ + { + uint32_t giaddr = inet_addr("192.168.1.1"); + memcpy(raw + 24, &giaddr, sizeof(giaddr)); /* relay-from-relay path */ + } + pcpp::DhcpLayer short_dhcp(raw, 50, nullptr, nullptr); + + relay_config config = {}; + config.vlan = "Vlan10"; + config.agent_relay_mode = "forward"; + config.max_hop_count = 16; + config.vrf_sock = 1; + config.link_address.sin_addr.s_addr = inet_addr("192.168.10.1"); + struct sockaddr_in srv_sock = {}; + srv_sock.sin_family = AF_INET; + srv_sock.sin_addr.s_addr = inet_addr("10.0.0.1"); + config.servers_sock = {srv_sock}; + config.servers = {"10.0.0.1"}; + + EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0); + from_client(&short_dhcp, config); +} + +/* Short DHCP server reply (< sizeof(dhcp_header)): must be dropped before touching header fields. */ +TEST(DHCPRelayTest, to_client_short_header) { + /* Heap-allocated for the same ASAN reason as from_client_short_header. */ + uint8_t* raw = new uint8_t[50](); + raw[0] = 0x02; /* opcode = BOOTREPLY */ + raw[1] = 0x01; raw[2] = 0x06; + { + uint32_t giaddr = inet_addr("192.168.1.1"); + memcpy(raw + 24, &giaddr, sizeof(giaddr)); + } + pcpp::DhcpLayer short_dhcp(raw, 50, nullptr, nullptr); + + std::unordered_map vlans; + relay_config config = {}; + config.vlan = "Vlan10"; + config.client_sock = 1; + vlans["Vlan10"] = config; + + /* to_client() returns before getifaddrs() on a truncated DHCP layer */ + EXPECT_GLOBAL_CALL(getifaddrs, getifaddrs(_)).Times(0); + EXPECT_GLOBAL_CALL(freeifaddrs, freeifaddrs(_)).Times(0); + EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0); + to_client(&short_dhcp, &vlans, "172.22.178.234", "Ethernet4"); +} + +/* bootp_pad() pads short packets to BOOTP_MIN_LEN and is compiled in both + * production and UNIT_TEST builds, allowing direct unit coverage of the logic + * that send_udp() uses without linking the real send_udp(). */ +TEST(DHCPRelayTest, bootp_pad_extends_short_packet) { + uint8_t src[50] = {}; + src[0] = 0x01; /* BOOTREQUEST */ + uint8_t out[BOOTP_MIN_LEN] = {}; + uint32_t len = sizeof(src); + bootp_pad(out, src, &len, true); + EXPECT_EQ(len, (uint32_t)BOOTP_MIN_LEN); + EXPECT_EQ(out[0], 0x01); + EXPECT_EQ(out[49], 0x00); /* zero-padded */ +} + +TEST(DHCPRelayTest, bootp_pad_no_op_when_already_min_len) { + uint8_t src[BOOTP_MIN_LEN] = {}; + src[0] = 0x02; /* BOOTREPLY */ + uint8_t out[BOOTP_MIN_LEN] = {}; + uint32_t len = BOOTP_MIN_LEN; + bootp_pad(out, src, &len, true); + EXPECT_EQ(len, (uint32_t)BOOTP_MIN_LEN); + /* out was not written — src was not copied */ + EXPECT_EQ(out[0], 0x00); +} + +TEST(DHCPRelayTest, bootp_pad_disabled_when_pad_false) { + uint8_t src[50] = {}; + uint8_t out[BOOTP_MIN_LEN] = {}; + uint32_t len = sizeof(src); + bootp_pad(out, src, &len, false); + EXPECT_EQ(len, (uint32_t)sizeof(src)); /* unchanged */ +} +TEST(DHCPRelayTest, encode_relay_option_long_circuit_id) { + interface_list.clear(); + phy_interface_alias_map.clear(); + vlan_vrf_map.clear(); + m_config = {}; + + pcpp::MacAddress clientMac("00:0e:86:11:c0:75"); + pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_DISCOVER, clientMac); + + interface_list.push_back("Ethernet12"); + phy_interface_alias_map["Ethernet12"] = "eth12"; + + relay_config config = {}; + config.phy_interface = "Ethernet12"; + config.vlan = "Vlan10"; + vlan_vrf_map["Vlan10"] = "default"; + + // circuit-id = hostname + ":eth12:Vlan10" (13 fixed chars). + // With no optional sub-options, cap = DHCP_OPTION_VALUE_MAX_LEN(255) - remote-id(19) - circuit-id hdr(2) = 234. + // Use 225-char hostname → circuit-id = 238 > 234. + m_config.hostname = std::string(225, 'a'); + m_config.host_mac_addr = "12:32:54:24:95:36"; + + encode_relay_option(&dhcpLayer, &config); + + auto agent_option = dhcpLayer.getOptionData(pcpp::DHCPOPT_DHCP_AGENT_OPTIONS); + EXPECT_TRUE(agent_option.isNull()) << "option 82 must not be added when circuit-id exceeds available buffer space"; +} + +TEST(DHCPRelayTest, encode_relay_option_long_vrf_name) { + interface_list.clear(); + phy_interface_alias_map.clear(); + vlan_vrf_map.clear(); + m_config = {}; + + pcpp::MacAddress clientMac("00:0e:86:11:c0:75"); + pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_DISCOVER, clientMac); + + interface_list.push_back("Ethernet12"); + phy_interface_alias_map["Ethernet12"] = "eth12"; + + relay_config config = {}; + config.phy_interface = "Ethernet12"; + config.vlan = "Vlan10"; + config.vrf_selection_opt = "enable"; + config.vrf = "Vrf00"; + vlan_vrf_map["Vlan10"] = std::string(32, 'v'); // 32-char VRF name overflows vss_buf[32] + + m_config.hostname = "host"; + m_config.host_mac_addr = "12:32:54:24:95:36"; + + encode_relay_option(&dhcpLayer, &config); + + auto agent_option = dhcpLayer.getOptionData(pcpp::DHCPOPT_DHCP_AGENT_OPTIONS); + ASSERT_FALSE(agent_option.isNull()) << "option 82 must still be present"; + + // VSS sub-option (151) must be absent; circuit-id (1) and remote-id (2) must be present + const uint8_t *data = agent_option.getValue(); + size_t len = agent_option.getDataSize(); + bool has_circuit_id = false, has_remote_id = false, has_vss = false; + for (size_t i = 0; i + 1 < len; ) { + uint8_t type = data[i]; + uint8_t slen = data[i + 1]; + if (type == 1) has_circuit_id = true; + if (type == 2) has_remote_id = true; + if (type == 151) has_vss = true; + i += 2 + slen; + } + EXPECT_TRUE(has_circuit_id); + EXPECT_TRUE(has_remote_id); + EXPECT_FALSE(has_vss) << "VSS sub-option must be skipped for oversized VRF name"; +} + +TEST(DHCPRelayTest, encode_relay_option_short_mac) { + interface_list.clear(); + phy_interface_alias_map.clear(); + vlan_vrf_map.clear(); + m_config = {}; + + pcpp::MacAddress clientMac("00:0e:86:11:c0:75"); + pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_DISCOVER, clientMac); + + interface_list.push_back("Ethernet12"); + phy_interface_alias_map["Ethernet12"] = "eth12"; + + relay_config config = {}; + config.phy_interface = "Ethernet12"; + config.vlan = "Vlan10"; + vlan_vrf_map["Vlan10"] = "default"; + + m_config.hostname = "host"; + m_config.host_mac_addr = "ab:cd"; // shorter than MAC_ADDR_STR_LEN (17) + + encode_relay_option(&dhcpLayer, &config); + + auto agent_option = dhcpLayer.getOptionData(pcpp::DHCPOPT_DHCP_AGENT_OPTIONS); + ASSERT_FALSE(agent_option.isNull()); + + const uint8_t *data = agent_option.getValue(); + size_t len = agent_option.getDataSize(); + bool has_remote_id = false; + for (size_t i = 0; i + 1 < len; ) { + uint8_t type = data[i]; + uint8_t slen = data[i + 1]; + if (type == 2) { + has_remote_id = true; + EXPECT_EQ(slen, (uint8_t)m_config.host_mac_addr.length()) + << "remote-id length must equal actual MAC string length, not MAC_ADDR_STR_LEN"; + } + i += 2 + slen; + } + EXPECT_TRUE(has_remote_id) << "remote-id sub-option must be present"; +} diff --git a/dhcp4relay/test/mock_relay.h b/dhcp4relay/test/mock_relay.h index d492abe..468f774 100644 --- a/dhcp4relay/test/mock_relay.h +++ b/dhcp4relay/test/mock_relay.h @@ -18,7 +18,7 @@ extern std::unordered_map vrf_sock_map; extern std::unordered_map phy_interface_alias_map; extern std::vector interface_list; extern metadata_config m_config; -extern bool feature_dhcp_server_enabled; +extern std::atomic feature_dhcp_server_enabled; extern std::unordered_map vlans_copy; extern std::string global_dhcp_server_ip; extern std::shared_ptr config_db;