Remove panic when reassembly fails.

Reassembly can fail due to an invalid sequence of fragments
being received. eg. Multiple fragments with same id which
claim to be the last one by setting the more flag to 0 etc.
It's safer to just drop the reassembler and increment a metric
than to panic when reassembly fails.

PiperOrigin-RevId: 274920901

pkg/tcpip/network/fragmentation/BUILD

@@ -28,6 +28,7 @@ go_library(
     visibility = ["//:sandbox"],
     deps = [
         "//pkg/log",
+        "//pkg/tcpip",
         "//pkg/tcpip/buffer",
     ],
 )

pkg/tcpip/network/fragmentation/fragmentation.go

@@ -17,6 +17,7 @@
 package fragmentation
 
 import (
+	"fmt"
 	"log"
 	"sync"
 	"time"
@@ -82,7 +83,7 @@ func NewFragmentation(highMemoryLimit, lowMemoryLimit int, reassemblingTimeout t
 
 // Process processes an incoming fragment belonging to an ID
 // and returns a complete packet when all the packets belonging to that ID have been received.
-func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool) {
+func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool, error) {
 	f.mu.Lock()
 	r, ok := f.reassemblers[id]
 	if ok && r.tooOld(f.timeout) {
@@ -97,8 +98,15 @@ func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv buf
 	}
 	f.mu.Unlock()
 
-	res, done, consumed := r.process(first, last, more, vv)
-
+	res, done, consumed, err := r.process(first, last, more, vv)
+	if err != nil {
+		// We probably got an invalid sequence of fragments. Just
+		// discard the reassembler and move on.
+		f.mu.Lock()
+		f.release(r)
+		f.mu.Unlock()
+		return buffer.VectorisedView{}, false, fmt.Errorf("fragmentation processing error: %v", err)
+	}
 	f.mu.Lock()
 	f.size += consumed
 	if done {
@@ -114,7 +122,7 @@ func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv buf
 		}
 	}
 	f.mu.Unlock()
-	return res, done
+	return res, done, nil
 }
 
 func (f *Fragmentation) release(r *reassembler) {

pkg/tcpip/network/fragmentation/fragmentation_test.go

@@ -83,7 +83,10 @@ func TestFragmentationProcess(t *testing.T) {
 		t.Run(c.comment, func(t *testing.T) {
 			f := NewFragmentation(1024, 512, DefaultReassembleTimeout)
 			for i, in := range c.in {
-				vv, done := f.Process(in.id, in.first, in.last, in.more, in.vv)
+				vv, done, err := f.Process(in.id, in.first, in.last, in.more, in.vv)
+				if err != nil {
+					t.Fatalf("f.Process(%+v, %+d, %+d, %t, %+v) failed: %v", in.id, in.first, in.last, in.more, in.vv, err)
+				}
 				if !reflect.DeepEqual(vv, c.out[i].vv) {
 					t.Errorf("got Process(%d) = %+v, want = %+v", i, vv, c.out[i].vv)
 				}
@@ -114,7 +117,10 @@ func TestReassemblingTimeout(t *testing.T) {
 	time.Sleep(2 * timeout)
 	// Send another fragment that completes a packet.
 	// However, no packet should be reassembled because the fragment arrived after the timeout.
-	_, done := f.Process(0, 1, 1, false, vv(1, "1"))
+	_, done, err := f.Process(0, 1, 1, false, vv(1, "1"))
+	if err != nil {
+		t.Fatalf("f.Process(0, 1, 1, false, vv(1, \"1\")) failed: %v", err)
+	}
 	if done {
 		t.Errorf("Fragmentation does not respect the reassembling timeout.")
 	}

pkg/tcpip/network/fragmentation/reassembler.go

@@ -78,7 +78,7 @@ func (r *reassembler) updateHoles(first, last uint16, more bool) bool {
 	return used
 }
 
-func (r *reassembler) process(first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool, int) {
+func (r *reassembler) process(first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool, int, error) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	consumed := 0
@@ -86,7 +86,7 @@ func (r *reassembler) process(first, last uint16, more bool, vv buffer.Vectorise
 		// A concurrent goroutine might have already reassembled
 		// the packet and emptied the heap while this goroutine
 		// was waiting on the mutex. We don't have to do anything in this case.
-		return buffer.VectorisedView{}, false, consumed
+		return buffer.VectorisedView{}, false, consumed, nil
 	}
 	if r.updateHoles(first, last, more) {
 		// We store the incoming packet only if it filled some holes.
@@ -96,13 +96,13 @@ func (r *reassembler) process(first, last uint16, more bool, vv buffer.Vectorise
 	}
 	// Check if all the holes have been deleted and we are ready to reassamble.
 	if r.deleted < len(r.holes) {
-		return buffer.VectorisedView{}, false, consumed
+		return buffer.VectorisedView{}, false, consumed, nil
 	}
 	res, err := r.heap.reassemble()
 	if err != nil {
-		panic(fmt.Sprintf("reassemble failed with: %v. There is probably a bug in the code handling the holes.", err))
+		return buffer.VectorisedView{}, false, consumed, fmt.Errorf("fragment reassembly failed: %v", err)
 	}
-	return res, true, consumed
+	return res, true, consumed, nil
 }
 
 func (r *reassembler) tooOld(timeout time.Duration) bool {

pkg/tcpip/network/ipv4/ipv4.go

@@ -326,7 +326,13 @@ func (e *endpoint) HandlePacket(r *stack.Route, vv buffer.VectorisedView) {
 			return
 		}
 		var ready bool
-		vv, ready = e.fragmentation.Process(hash.IPv4FragmentHash(h), h.FragmentOffset(), last, more, vv)
+		var err error
+		vv, ready, err = e.fragmentation.Process(hash.IPv4FragmentHash(h), h.FragmentOffset(), last, more, vv)
+		if err != nil {
+			r.Stats().IP.MalformedPacketsReceived.Increment()
+			r.Stats().IP.MalformedFragmentsReceived.Increment()
+			return
+		}
 		if !ready {
 			return
 		}

pkg/tcpip/network/ipv4/ipv4_test.go

@@ -441,6 +441,16 @@ func TestInvalidFragments(t *testing.T) {
 			1,
 			1,
 		},
+		{
+			"multiple_fragments_with_more_fragments_set_to_false",
+			[][]byte{
+				{0x45, 0x00, 0x00, 0x1c, 0x30, 0x40, 0x00, 0x10, 0x00, 0x06, 0x34, 0x69, 0x73, 0x73, 0x69, 0x6e, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+				{0x45, 0x00, 0x00, 0x1c, 0x30, 0x40, 0x00, 0x01, 0x61, 0x06, 0x34, 0x69, 0x73, 0x73, 0x69, 0x6e, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+				{0x45, 0x00, 0x00, 0x1c, 0x30, 0x40, 0x20, 0x00, 0x00, 0x06, 0x34, 0x1e, 0x73, 0x73, 0x69, 0x6e, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			},
+			1,
+			1,
+		},
 	}
 
 	for _, tc := range testCases {