Always parse Transport headers

..including ICMP headers before delivering them to the
TransportDispatcher.

Updates #3810.

PiperOrigin-RevId: 404404002

2 files changed, 25 insertions, 34 deletions

diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go
index 1c3b0887f..3eff0bbd8 100644
--- a/pkg/tcpip/network/ipv4/icmp.go
+++ b/pkg/tcpip/network/ipv4/icmp.go
@@ -175,18 +175,14 @@ func (e *endpoint) handleControl(errInfo stack.TransportError, pkt *stack.Packet
 
 func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) {
 	received := e.stats.icmp.packetsReceived
-	// ICMP packets don't have their TransportHeader fields set. See
-	// icmp/protocol.go:protocol.Parse for a full explanation. Not all ICMP types
-	// require consuming the header, so we only call PullUp.
-	v, ok := pkt.Data().PullUp(header.ICMPv4MinimumSize)
-	if !ok {
+	h := header.ICMPv4(pkt.TransportHeader().View())
+	if len(h) < header.ICMPv4MinimumSize {
 		received.invalid.Increment()
 		return
 	}
-	h := header.ICMPv4(v)
 
 	// Only do in-stack processing if the checksum is correct.
-	if pkt.Data().AsRange().Checksum() != 0xffff {
+	if header.Checksum(h, pkt.Data().AsRange().Checksum()) != 0xffff {
 		received.invalid.Increment()
 		// It's possible that a raw socket expects to receive this regardless
 		// of checksum errors. If it's an echo request we know it's safe because
@@ -251,7 +247,7 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) {
 		// TODO(gvisor.dev/issue/4399): The copy may not be needed if there are no
 		// waiting endpoints. Consider moving responsibility for doing the copy to
 		// DeliverTransportPacket so that is is only done when needed.
-		replyData := pkt.Data().AsRange().ToOwnedView()
+		replyData := stack.PayloadSince(pkt.TransportHeader())
 		ipHdr := header.IPv4(pkt.NetworkHeader().View())
 		localAddressBroadcast := pkt.NetworkPacketInfo.LocalAddressBroadcast
 
@@ -344,9 +340,6 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) {
 
 		mtu := h.MTU()
 		code := h.Code()
-		if _, ok := pkt.Data().Consume(header.ICMPv4MinimumSize); !ok {
-			panic("could not consume ICMPv4MinimumSize bytes")
-		}
 		switch code {
 		case header.ICMPv4HostUnreachable:
 			e.handleControl(&icmpv4DestinationHostUnreachableSockError{}, pkt)
@@ -574,20 +567,6 @@ func (p *protocol) returnError(reason icmpReason, pkt *stack.PacketBuffer) tcpip
 
 	// Don't respond to icmp error packets.
 	if origIPHdr.Protocol() == uint8(header.ICMPv4ProtocolNumber) {
-		// TODO(gvisor.dev/issue/3810):
-		// Unfortunately the current stack pretty much always has ICMPv4 headers
-		// in the Data section of the packet but there is no guarantee that is the
-		// case. If this is the case grab the header to make it like all other
-		// packet types. When this is cleaned up the Consume should be removed.
-		if transportHeader.IsEmpty() {
-			var ok bool
-			transportHeader, ok = pkt.TransportHeader().Consume(header.ICMPv4MinimumSize)
-			if !ok {
-				return nil
-			}
-		} else if transportHeader.Size() < header.ICMPv4MinimumSize {
-			return nil
-		}
 		// We need to decide to explicitly name the packets we can respond to or
 		// the ones we can not respond to. The decision is somewhat arbitrary and
 		// if problems arise this could be reversed. It was judged less of a breach
diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go
index 6e52cc9bb..d1d509702 100644
--- a/pkg/tcpip/network/ipv4/ipv4.go
+++ b/pkg/tcpip/network/ipv4/ipv4.go
@@ -984,7 +984,7 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt *stack.PacketBuffer,
 		}
 
 		proto := h.Protocol()
-		resPkt, _, ready, err := e.protocol.fragmentation.Process(
+		resPkt, transProtoNum, ready, err := e.protocol.fragmentation.Process(
 			// As per RFC 791 section 2.3, the identification value is unique
 			// for a source-destination pair and protocol.
 			fragmentation.FragmentID{
@@ -1015,6 +1015,8 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt *stack.PacketBuffer,
 		h.SetTotalLength(uint16(pkt.Data().Size() + len(h)))
 		h.SetFlagsFragmentOffset(0, 0)
 
+		e.protocol.parseTransport(pkt, tcpip.TransportProtocolNumber(transProtoNum))
+
 		// Now that the packet is reassembled, it can be sent to raw sockets.
 		e.dispatcher.DeliverRawPacket(h.TransportProtocol(), pkt)
 	}
@@ -1310,19 +1312,29 @@ func (p *protocol) parseAndValidate(pkt *stack.PacketBuffer) (header.IPv4, bool)
 	}
 
 	if hasTransportHdr {
-		switch err := p.stack.ParsePacketBufferTransport(transProtoNum, pkt); err {
-		case stack.ParsedOK:
-		case stack.UnknownTransportProtocol, stack.TransportLayerParseError:
-			// The transport layer will handle unknown protocols and transport layer
-			// parsing errors.
-		default:
-			panic(fmt.Sprintf("unexpected error parsing transport header = %d", err))
-		}
+		p.parseTransport(pkt, transProtoNum)
 	}
 
 	return h, true
 }
 
+func (p *protocol) parseTransport(pkt *stack.PacketBuffer, transProtoNum tcpip.TransportProtocolNumber) {
+	if transProtoNum == header.ICMPv4ProtocolNumber {
+		// The transport layer will handle transport layer parsing errors.
+		_ = parse.ICMPv4(pkt)
+		return
+	}
+
+	switch err := p.stack.ParsePacketBufferTransport(transProtoNum, pkt); err {
+	case stack.ParsedOK:
+	case stack.UnknownTransportProtocol, stack.TransportLayerParseError:
+		// The transport layer will handle unknown protocols and transport layer
+		// parsing errors.
+	default:
+		panic(fmt.Sprintf("unexpected error parsing transport header = %d", err))
+	}
+}
+
 // Parse implements stack.NetworkProtocol.
 func (*protocol) Parse(pkt *stack.PacketBuffer) (proto tcpip.TransportProtocolNumber, hasTransportHdr bool, ok bool) {
 	if ok := parse.IPv4(pkt); !ok {