tun: clear virtioNetHdr when deleting item from tcpGROTable

Otherwise the deleted packet gets written to the TUN fd with an invalid
virtioNetHdr.

Fixes: 9e2f386 ("conn, device, tun: implement vectorized I/O on Linux")
Signed-off-by: Jordan Whited <jordan@tailscale.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

diff --git a/tun/offload_linux.go b/tun/offload_linux.go
index 5f0db062c39222921fb20271de4e6fc235855f65..7cd3206947661c7ea93edcd1c06a2fe9258359cb 100644 (file)
--- a/tun/offload_linux.go
+++ b/tun/offload_linux.go
@@ -608,6 +608,10 @@ func tcpGRO(bufs [][]byte, offset int, pktI int, table *tcpGROTable, isV6 bool)
                        case coalesceItemInvalidCSum:
                                // delete the item with an invalid csum
                                table.deleteAt(item.key, i)
+                               // The deleted item will not be re-visited in applyTCPCoalesceAccounting,
+                               // so we must zero the virtioNetHdr. clear() is the equivalent of
+                               // encoding a zero value virtioNetHdr.
+                               clear(bufs[item.bufsIndex][offset-virtioNetHdrLen : offset])
                        case coalescePktInvalidCSum:
                                // no point in inserting an item that we can't coalesce
                                return groResultNoop
@@ -667,11 +671,7 @@ func applyTCPCoalesceAccounting(bufs [][]byte, offset int, table *tcpGROTable) e
                                psum := pseudoHeaderChecksumNoFold(unix.IPPROTO_TCP, srcAddr, dstAddr, uint16(len(pkt)-int(item.iphLen)))
                                binary.BigEndian.PutUint16(pkt[hdr.csumStart+hdr.csumOffset:], checksum([]byte{}, psum))
                        } else {
-                               hdr := virtioNetHdr{}
-                               err := hdr.encode(bufs[item.bufsIndex][offset-virtioNetHdrLen:])
-                               if err != nil {
-                                       return err
-                               }
+                               clear(bufs[item.bufsIndex][offset-virtioNetHdrLen : offset])
                        }
                }
        }
@@ -727,11 +727,7 @@ func applyUDPCoalesceAccounting(bufs [][]byte, offset int, table *udpGROTable) e
                                psum := pseudoHeaderChecksumNoFold(unix.IPPROTO_UDP, srcAddr, dstAddr, uint16(len(pkt)-int(item.iphLen)))
                                binary.BigEndian.PutUint16(pkt[hdr.csumStart+hdr.csumOffset:], checksum([]byte{}, psum))
                        } else {
-                               hdr := virtioNetHdr{}
-                               err := hdr.encode(bufs[item.bufsIndex][offset-virtioNetHdrLen:])
-                               if err != nil {
-                                       return err
-                               }
+                               clear(bufs[item.bufsIndex][offset-virtioNetHdrLen : offset])
                        }
                }
        }
@@ -880,11 +876,7 @@ func handleGRO(bufs [][]byte, offset int, tcpTable *tcpGROTable, udpTable *udpGR
                }
                switch result {
                case groResultNoop:
-                       hdr := virtioNetHdr{}
-                       err := hdr.encode(bufs[i][offset-virtioNetHdrLen:])
-                       if err != nil {
-                               return err
-                       }
+                       clear(bufs[i][offset-virtioNetHdrLen : offset])
                        fallthrough
                case groResultTableInsert:
                        *toWrite = append(*toWrite, i)

diff --git a/tun/offload_linux_test.go b/tun/offload_linux_test.go
index d87e636127ab6fe1cb73682e6077c01a976945a6..3101f6dfef3a70e8bbe8d62ebf14ccae9a5f14fc 100644 (file)
--- a/tun/offload_linux_test.go
+++ b/tun/offload_linux_test.go
@@ -750,3 +750,51 @@ func Test_udpPacketsCanCoalesce(t *testing.T) {
                })
        }
 }
+
+func Test_handleGRO_invalidItemCsumClearsVirtioNetHdr(t *testing.T) {
+       pkts := [][]byte{
+               flipTCP4Checksum(tcp4Packet(ip4PortA, ip4PortB, header.TCPFlagAck, 100, 1)),
+               tcp4Packet(ip4PortA, ip4PortB, header.TCPFlagAck, 100, 101),
+               tcp4Packet(ip4PortA, ip4PortB, header.TCPFlagAck, 100, 201),
+       }
+       // Poison the virtioNetHdr region of pkts[0] so a missing clear() is detectable.
+       for i := 0; i < virtioNetHdrLen; i++ {
+               pkts[0][i] = 0xAB
+       }
+
+       table := newTCPGROTable()
+       toWrite := make([]int, 0, len(pkts))
+       if err := handleGRO(pkts, virtioNetHdrLen, table, newUDPGROTable(), false, &toWrite); err != nil {
+               t.Fatal(err)
+       }
+
+       // Verify pkts[0] is in toWrite where we expect
+       if toWrite[0] != 0 {
+               t.Fatal("pkts[0] not found in toWrite at expected, zero index")
+       }
+
+       // Verify pkts[0] is not in tcpGROTable
+       if len(table.itemsByFlow) != 1 {
+               t.Fatalf("unexpected tcpGROTable items len: %d", len(table.itemsByFlow))
+       }
+       for _, v := range table.itemsByFlow {
+               if len(v) != 1 {
+                       t.Fatalf("unexpected tcpGROItems slice len: %d", len(v))
+               }
+               item := v[0]
+               if item.sentSeq != 101 {
+                       t.Fatalf("unexpected starting seq num in tcpGROTable: %d", item.sentSeq)
+               }
+               if item.numMerged != 1 {
+                       t.Fatalf("unexpected numMerged in tcpGROTable: %d", item.numMerged)
+               }
+       }
+
+       // pkt 0 is in toWrite and not present in tcpGROTable, so its virtioNetHdr
+       // must have been cleared.
+       for i, b := range pkts[0][:virtioNetHdrLen] {
+               if b != 0 {
+                       t.Fatalf("pkts[0] virtioNetHdr[%d] = 0x%x, want 0", i, b)
+               }
+       }
+}