From 8bc861ca71d38c5430f5b8f2d8501dc245713104 Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Thu, 22 Jul 2021 16:22:20 +0200 Subject: [PATCH] nic_router: do not re-use ARP request as reply So far, in order to create an ARP reply, the NIC router merely created a copy of the corresponding ARP request and modified only those values that differ. This approach has the disadvantage of re-using bad parameters from a broken request. The specific use-case that made this visible was an early version of the Pine board network driver that used to forward ARP requests with a greater size than required. The ARP replies of the router re-used this size and confused other network nodes with that. In general, the NIC router should rely on the data of incoming packets the least possible. Therefore, with this commit, the router creates a new ARP reply from scratch and uses only those values required from the corresponding ARP request. Fixes #4235 --- repos/os/src/server/nic_router/interface.cc | 69 +++++++++++++-------- repos/os/src/server/nic_router/interface.h | 5 +- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/repos/os/src/server/nic_router/interface.cc b/repos/os/src/server/nic_router/interface.cc index deb992d64b..f9907b03ed 100644 --- a/repos/os/src/server/nic_router/interface.cc +++ b/repos/os/src/server/nic_router/interface.cc @@ -43,6 +43,24 @@ using Genode::Signal_transmitter; ** Utilities ** ***************/ +enum { + + ETHERNET_HEADER_SIZE = sizeof(Ethernet_frame), + + ETHERNET_DATA_SIZE_WITH_ARP = + sizeof(Arp_packet) + ETHERNET_HEADER_SIZE < Ethernet_frame::MIN_SIZE ? + Ethernet_frame::MIN_SIZE - ETHERNET_HEADER_SIZE : + sizeof(Arp_packet), + + ETHERNET_CRC_SIZE = sizeof(Genode::uint32_t), + + ARP_PACKET_SIZE = + ETHERNET_HEADER_SIZE + + ETHERNET_DATA_SIZE_WITH_ARP + + ETHERNET_CRC_SIZE, +}; + + template static void _destroy_dissolved_links(Link_list &dissolved_links, Deallocator &dealloc) @@ -1265,15 +1283,7 @@ void Interface::_handle_ip(Ethernet_frame ð, void Interface::_broadcast_arp_request(Ipv4_address const &src_ip, Ipv4_address const &dst_ip) { - enum { - ETH_HDR_SZ = sizeof(Ethernet_frame), - ETH_DAT_SZ = sizeof(Arp_packet) + ETH_HDR_SZ >= Ethernet_frame::MIN_SIZE ? - sizeof(Arp_packet) : - Ethernet_frame::MIN_SIZE - ETH_HDR_SZ, - ETH_CRC_SZ = sizeof(Genode::uint32_t), - PKT_SIZE = ETH_HDR_SZ + ETH_DAT_SZ + ETH_CRC_SZ, - }; - send(PKT_SIZE, [&] (void *pkt_base, Size_guard &size_guard) { + send(ARP_PACKET_SIZE, [&] (void *pkt_base, Size_guard &size_guard) { /* write Ethernet header */ Ethernet_frame ð = Ethernet_frame::construct_at(pkt_base, size_guard); @@ -1340,22 +1350,31 @@ void Interface::_handle_arp_reply(Ethernet_frame ð, } -void Interface::_send_arp_reply(Ethernet_frame ð, - Size_guard &size_guard, - Arp_packet &arp) +void Interface::_send_arp_reply(Ethernet_frame &request_eth, + Arp_packet &request_arp) { - /* interchange source and destination MAC and IP addresses */ - Ipv4_address dst_ip = arp.dst_ip(); - arp.dst_ip(arp.src_ip()); - arp.dst_mac(arp.src_mac()); - eth.dst(eth.src()); - arp.src_ip(dst_ip); - arp.src_mac(_router_mac); - eth.src(_router_mac); + send(ARP_PACKET_SIZE, [&] (void *reply_base, Size_guard &reply_guard) { - /* mark packet as reply and send it back to its sender */ - arp.opcode(Arp_packet::REPLY); - send(eth, size_guard); + Ethernet_frame &reply_eth { + Ethernet_frame::construct_at(reply_base, reply_guard) }; + + reply_eth.dst(request_eth.src()); + reply_eth.src(_router_mac); + reply_eth.type(Ethernet_frame::Type::ARP); + + Arp_packet &reply_arp { + reply_eth.construct_at_data(reply_guard) }; + + reply_arp.hardware_address_type(Arp_packet::ETHERNET); + reply_arp.protocol_address_type(Arp_packet::IPV4); + reply_arp.hardware_address_size(sizeof(Mac_address)); + reply_arp.protocol_address_size(sizeof(Ipv4_address)); + reply_arp.opcode(Arp_packet::REPLY); + reply_arp.src_mac(_router_mac); + reply_arp.src_ip(request_arp.dst_ip()); + reply_arp.dst_mac(request_arp.src_mac()); + reply_arp.dst_ip(request_arp.src_ip()); + }); } @@ -1380,7 +1399,7 @@ void Interface::_handle_arp_request(Ethernet_frame ð, if (_config().verbose()) { log("[", local_domain, "] answer ARP request for router IP " "with router MAC"); } - _send_arp_reply(eth, size_guard, arp); + _send_arp_reply(eth, arp); } else { @@ -1405,7 +1424,7 @@ void Interface::_handle_arp_request(Ethernet_frame ð, if (_config().verbose()) { log("[", local_domain, "] answer ARP request for foreign IP " "with router MAC"); } - _send_arp_reply(eth, size_guard, arp); + _send_arp_reply(eth, arp); } } } diff --git a/repos/os/src/server/nic_router/interface.h b/repos/os/src/server/nic_router/interface.h index d76d5b8ff0..88852cd4b7 100644 --- a/repos/os/src/server/nic_router/interface.h +++ b/repos/os/src/server/nic_router/interface.h @@ -217,9 +217,8 @@ class Net::Interface : private Interface_list::Element Arp_packet &arp, Domain &local_domain); - void _send_arp_reply(Ethernet_frame ð, - Size_guard &size_guard, - Arp_packet &arp); + void _send_arp_reply(Ethernet_frame &request_eth, + Arp_packet &request_arp); void _handle_dhcp_request(Ethernet_frame ð, Dhcp_packet &dhcp,