ip,ip6: bound and validate what the local delivery nodes read - #743
Merged
Merged
Conversation
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
christophefontaine
approved these changes
Sep 16, 2026
ip_input() validates the IP header itself, but not the payload length the local nodes derive from it. ip_input_local() computes it as the total length minus the header length, ip6_input_local() takes the IPv6 payload length then subtracts each extension header it walks. Both results are uint16_t and neither was checked, so a header announcing less than it carries wraps the subtraction around. The following packets can be created using scapy: > p_ihl = Ether(dst='f0:0d:ac:dc:00:00')/IP(src='172.16.0.2', dst='172.16.0.1', ihl=10, len=20, proto=1)/ICMP() > p_ext = Ether(dst='f0:0d:ac:dc:00:00')/IPv6(src='fd00::2', dst='fd00::1', plen=4)/IPv6ExtHdrHopByHop()/ICMPv6EchoRequest() The first announces a 20 byte datagram with a 40 byte header, giving a payload length of 65516. The second claims 4 bytes of payload before a hop by hop option header that rte_ipv6_get_next_ext() sizes at 8, giving 65532. Both carry a valid checksum, so nothing upstream rejects them. Every node downstream sizes its reads with that value and reads them contiguously through rte_pktmbuf_mtod(). icmp_input() hands it straight to rte_raw_cksum(), which then walks tens of kilobytes past the mbuf. Refuse a length shorter than the headers, or longer than the first segment. Bounding it to the whole packet is not enough: no node here handles a payload spanning several segments, which is why ip_input() already validates against rte_pktmbuf_data_len(). The check belongs in the local nodes rather than in ip_input, because dnat44 and srv6_local hand packets over to them directly and those never went through the RFC 1812 validation. Fixes: 697e5c7 ("ip6: account for extension headers in payload length") Fixes: c71d3f3 ("ipv4: add support for icmp echo reply") Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
icmp6_input dereferenced the ICMPv6 preamble without checking that the payload was long enough to hold it, and nothing anywhere verified the checksum: the BAD_CHECKSUM edge was declared and wired to a drop node but never taken, and ip6_input_local only verifies UDP, TCP, SCTP and DCCP, ICMPv6 falling in its "no checksum to verify" default. The four ndp_*_input nodes already claim the checksum was "already checked in icmp6_input" among the RFC 4861 conditions they rely on, so they were validating neighbour discovery on a promise nobody kept. The same held for the echo requests answered straight from the datapath. The IPv6 header is gone by the time this node runs, so rebuild the pseudo header from the addresses and the upper layer length that ip6_input_local left in the mbuf private data. Fixes: 315e4e5 ("ip6: add basic icmp6 echo and reply support") Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
ip6_input_local only tagged the mbuf as IPv6 for the protocols whose checksum it verifies, while its IPv4 twin does it for whatever protocol it hands over. Anything else kept the packet type the receive path had left there, which is nothing at all on a PMD that does not fill it in. l4_loopback_output picks the address family from that field to build the header back, so it has to be right for every protocol that can reach the kernel, not just UDP and TCP. Fixes: d10b508 ("l4: ensure packet type is set before L4 processing") Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
An ICMP error message carries the packet that caused it, and until now the only thing checking that the quoted part was there at all sat in the control plane, inside the parser that walks it looking for one of our own echo requests. That parser tells a truncated message apart from a well formed one, but it only runs because a builtin ping might be waiting, and it mixes both verdicts into a single errno. Move the two length checks to the input nodes, which is where the rest of the ICMP validation already lives, so a malformed error message is dropped before the kernel ever sees it regardless of what the control plane does with the rest. RFC 792 wants the quoted header plus 64 bits of the datagram in error, and the quoted header length must agree with what the message holds. RFC 4443 only asks for the invoking IPv6 header, so the IPv6 side stops there, the extra 8 bytes it used to demand were a need of the echo matching, not of the protocol. Also punt parameter problem on the IPv4 side. Its IPv6 counterpart was already handed to the kernel, and it is an error about a packet the local stack sent, so the socket that sent it should hear about it. Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
ndp_ns_input asserts d.len >= 0 where the comment right above it, and RFC 4861 section 7.1.1, ask for 24 octets. len is a uint16_t so the condition never fires. An eight byte solicitation with a valid checksum passes the generic length check in icmp6_input, reaches this node, and the target address is read from beyond the announced payload. The three sibling nodes get it right: ndp_na_input checks 24, ndp_ra_input and ndp_rs_input size their own message. Check the same way here. Fixes: b51dfd9 ("ndp: avoid races between control and data planes") Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
ip_input_local accepts a datagram whose total length stops at the end of the IPv4 header, records a zero payload length and adjusts the header away. The mbuf then reaches l4_input_local empty: UDP reads dst_port out of it to pick an edge, and the trace path copies a whole UDP or TCP header on top. The minimum differs per protocol, so the local node cannot impose one. Check it where the header is read, for both protocols, and send a truncated datagram to a new bad_length drop node instead of punting it to the kernel. Fixes: 1d6f86a ("l4: add udp port handler") Signed-off-by: Maxime Leroy <maxime@leroys.fr> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
grout-bot
force-pushed
the
icmp-hardening
branch
from
September 16, 2026 07:01
ecf9aa8 to
7fb9957
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-existing bugs in the local delivery path, found while making ping(8) work
through a control plane TAP (#740). They stand on their own.
ip_local/ip6_localderived the payload length without bounding it,wrapping a
uint16_tto ~64k on a crafted header.icmp_inputfed that torte_raw_cksum().ndp_*_inputnodes documented relying on it.ip6_localdid not set the packet type for every locally delivered protocol.Testing
New unit test blocks for
ip_local,ip6_local,icmp_inputandicmp6_input, each checked to fail without its fix. Each of the 4 commitsbuilds and passes unit tests alone.