From 9143fcd7fd38243dd40f927dafaeb75f6ef8ef49 Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Tue, 21 Jan 2020 14:47:17 -0800
Subject: Add UDP matchers.

---
 pkg/sentry/socket/netfilter/netfilter.go | 164 ++++++++++++++++++++++++++-----
 1 file changed, 141 insertions(+), 23 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index e1f2bacce..45296b339 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -131,6 +131,7 @@ func FillDefaultIPTables(stack *stack.Stack) {
 	stack.SetIPTables(ipt)
 }
 
+// TODO: Return proto.
 // convertNetstackToBinary converts the iptables as stored in netstack to the
 // format expected by the iptables tool. Linux stores each table as a binary
 // blob that can only be traversed by parsing a bit, reading some offsets,
@@ -318,10 +319,12 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		}
 		var entry linux.IPTEntry
 		buf := optVal[:linux.SizeOfIPTEntry]
-		optVal = optVal[linux.SizeOfIPTEntry:]
 		binary.Unmarshal(buf, usermem.ByteOrder, &entry)
-		if entry.TargetOffset != linux.SizeOfIPTEntry {
-			// TODO(gvisor.dev/issue/170): Support matchers.
+		initialOptValLen := len(optVal)
+		optVal = optVal[linux.SizeOfIPTEntry:]
+
+		if entry.TargetOffset < linux.SizeOfIPTEntry {
+			log.Warningf("netfilter: entry has too-small target offset %d", entry.TargetOffset)
 			return syserr.ErrInvalidArgument
 		}
 
@@ -332,19 +335,41 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 			return err
 		}
 
+		// TODO: Matchers (and maybe targets) can specify that they only work for certiain protocols, hooks, tables.
+		// Get matchers.
+		matchersSize := entry.TargetOffset - linux.SizeOfIPTEntry
+		if len(optVal) < int(matchersSize) {
+			log.Warningf("netfilter: entry doesn't have enough room for its matchers (only %d bytes remain)", len(optVal))
+		}
+		matchers, err := parseMatchers(filter, optVal[:matchersSize])
+		if err != nil {
+			log.Warningf("netfilter: failed to parse matchers: %v", err)
+			return err
+		}
+		optVal = optVal[matchersSize:]
+
 		// Get the target of the rule.
-		target, consumed, err := parseTarget(optVal)
+		targetSize := entry.NextOffset - entry.TargetOffset
+		if len(optVal) < int(targetSize) {
+			log.Warningf("netfilter: entry doesn't have enough room for its target (only %d bytes remain)", len(optVal))
+		}
+		target, err := parseTarget(optVal[:targetSize])
 		if err != nil {
 			return err
 		}
-		optVal = optVal[consumed:]
+		optVal = optVal[targetSize:]
 
 		table.Rules = append(table.Rules, iptables.Rule{
-			Filter: filter,
-			Target: target,
+			Filter:   filter,
+			Target:   target,
+			Matchers: matchers,
 		})
 		offsets = append(offsets, offset)
-		offset += linux.SizeOfIPTEntry + consumed
+		offset += uint32(entry.NextOffset)
+
+		if initialOptValLen-len(optVal) != int(entry.NextOffset) {
+			log.Warningf("netfilter: entry NextOffset is %d, but entry took up %d bytes", entry.NextOffset, initialOptValLen-len(optVal))
+		}
 	}
 
 	// Go through the list of supported hooks for this table and, for each
@@ -401,12 +426,105 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 	return nil
 }
 
-// parseTarget parses a target from the start of optVal and returns the target
-// along with the number of bytes it occupies in optVal.
-func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
+// parseMatchers parses 0 or more matchers from optVal. optVal should contain
+// only the matchers.
+func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Matcher, *syserr.Error) {
+	var matchers []iptables.Matcher
+	for len(optVal) > 0 {
+		log.Infof("parseMatchers: optVal has len %d", len(optVal))
+		// Get the XTEntryMatch.
+		if len(optVal) < linux.SizeOfXTEntryMatch {
+			log.Warningf("netfilter: optVal has insufficient size for entry match: %d", len(optVal))
+			return nil, syserr.ErrInvalidArgument
+		}
+		var match linux.XTEntryMatch
+		buf := optVal[:linux.SizeOfXTEntryMatch]
+		binary.Unmarshal(buf, usermem.ByteOrder, &match)
+		log.Infof("parseMatchers: parsed entry match %q: %+v", match.Name.String(), match)
+
+		// Check some invariants.
+		if match.MatchSize < linux.SizeOfXTEntryMatch {
+			log.Warningf("netfilter: match size is too small, must be at least %d", linux.SizeOfXTEntryMatch)
+			return nil, syserr.ErrInvalidArgument
+		}
+		if len(optVal) < int(match.MatchSize) {
+			log.Warningf("netfilter: optVal has insufficient size for match: %d", len(optVal))
+			return nil, syserr.ErrInvalidArgument
+		}
+
+		buf = optVal[linux.SizeOfXTEntryMatch:match.MatchSize]
+		var matcher iptables.Matcher
+		var err error
+		switch match.Name.String() {
+		case "tcp":
+			if len(buf) < linux.SizeOfXTTCP {
+				log.Warningf("netfilter: optVal has insufficient size for TCP match: %d", len(optVal))
+				return nil, syserr.ErrInvalidArgument
+			}
+			var matchData linux.XTTCP
+			// For alignment reasons, the match's total size may exceed what's
+			// strictly necessary to hold matchData.
+			binary.Unmarshal(buf[:linux.SizeOfXTUDP], usermem.ByteOrder, &matchData)
+			log.Infof("parseMatchers: parsed XTTCP: %+v", matchData)
+			matcher, err = iptables.NewTCPMatcher(filter, iptables.TCPMatcherData{
+				SourcePortStart:      matchData.SourcePortStart,
+				SourcePortEnd:        matchData.SourcePortEnd,
+				DestinationPortStart: matchData.DestinationPortStart,
+				DestinationPortEnd:   matchData.DestinationPortEnd,
+				Option:               matchData.Option,
+				FlagMask:             matchData.FlagMask,
+				FlagCompare:          matchData.FlagCompare,
+				InverseFlags:         matchData.InverseFlags,
+			})
+			if err != nil {
+				log.Warningf("netfilter: failed to create TCP matcher: %v", err)
+				return nil, syserr.ErrInvalidArgument
+			}
+
+		case "udp":
+			if len(buf) < linux.SizeOfXTUDP {
+				log.Warningf("netfilter: optVal has insufficient size for UDP match: %d", len(optVal))
+				return nil, syserr.ErrInvalidArgument
+			}
+			var matchData linux.XTUDP
+			// For alignment reasons, the match's total size may exceed what's
+			// strictly necessary to hold matchData.
+			binary.Unmarshal(buf[:linux.SizeOfXTUDP], usermem.ByteOrder, &matchData)
+			log.Infof("parseMatchers: parsed XTUDP: %+v", matchData)
+			matcher, err = iptables.NewUDPMatcher(filter, iptables.UDPMatcherData{
+				SourcePortStart:      matchData.SourcePortStart,
+				SourcePortEnd:        matchData.SourcePortEnd,
+				DestinationPortStart: matchData.DestinationPortStart,
+				DestinationPortEnd:   matchData.DestinationPortEnd,
+				InverseFlags:         matchData.InverseFlags,
+			})
+			if err != nil {
+				log.Warningf("netfilter: failed to create UDP matcher: %v", err)
+				return nil, syserr.ErrInvalidArgument
+			}
+
+		default:
+			log.Warningf("netfilter: unsupported matcher with name %q", match.Name.String())
+			return nil, syserr.ErrInvalidArgument
+		}
+
+		matchers = append(matchers, matcher)
+
+		// TODO: Support revision.
+		// TODO: Support proto -- matchers usually specify which proto(s) they work with.
+		optVal = optVal[match.MatchSize:]
+	}
+
+	// TODO: Check that optVal is exhausted.
+	return matchers, nil
+}
+
+// parseTarget parses a target from optVal. optVal should contain only the
+// target.
+func parseTarget(optVal []byte) (iptables.Target, *syserr.Error) {
 	if len(optVal) < linux.SizeOfXTEntryTarget {
 		log.Warningf("netfilter: optVal has insufficient size for entry target %d", len(optVal))
-		return nil, 0, syserr.ErrInvalidArgument
+		return nil, syserr.ErrInvalidArgument
 	}
 	var target linux.XTEntryTarget
 	buf := optVal[:linux.SizeOfXTEntryTarget]
@@ -414,9 +532,9 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 	switch target.Name.String() {
 	case "":
 		// Standard target.
-		if len(optVal) < linux.SizeOfXTStandardTarget {
-			log.Warningf("netfilter.SetEntries: optVal has insufficient size for standard target %d", len(optVal))
-			return nil, 0, syserr.ErrInvalidArgument
+		if len(optVal) != linux.SizeOfXTStandardTarget {
+			log.Warningf("netfilter.SetEntries: optVal has wrong size for standard target %d", len(optVal))
+			return nil, syserr.ErrInvalidArgument
 		}
 		var standardTarget linux.XTStandardTarget
 		buf = optVal[:linux.SizeOfXTStandardTarget]
@@ -424,22 +542,22 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 
 		verdict, err := translateToStandardVerdict(standardTarget.Verdict)
 		if err != nil {
-			return nil, 0, err
+			return nil, err
 		}
 		switch verdict {
 		case iptables.Accept:
-			return iptables.UnconditionalAcceptTarget{}, linux.SizeOfXTStandardTarget, nil
+			return iptables.UnconditionalAcceptTarget{}, nil
 		case iptables.Drop:
-			return iptables.UnconditionalDropTarget{}, linux.SizeOfXTStandardTarget, nil
+			return iptables.UnconditionalDropTarget{}, nil
 		default:
 			panic(fmt.Sprintf("Unknown verdict: %v", verdict))
 		}
 
 	case errorTargetName:
 		// Error target.
-		if len(optVal) < linux.SizeOfXTErrorTarget {
+		if len(optVal) != linux.SizeOfXTErrorTarget {
 			log.Infof("netfilter.SetEntries: optVal has insufficient size for error target %d", len(optVal))
-			return nil, 0, syserr.ErrInvalidArgument
+			return nil, syserr.ErrInvalidArgument
 		}
 		var errorTarget linux.XTErrorTarget
 		buf = optVal[:linux.SizeOfXTErrorTarget]
@@ -454,16 +572,16 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 		//   rules have an error with the name of the chain.
 		switch errorTarget.Name.String() {
 		case errorTargetName:
-			return iptables.ErrorTarget{}, linux.SizeOfXTErrorTarget, nil
+			return iptables.ErrorTarget{}, nil
 		default:
 			log.Infof("Unknown error target %q doesn't exist or isn't supported yet.", errorTarget.Name.String())
-			return nil, 0, syserr.ErrInvalidArgument
+			return nil, syserr.ErrInvalidArgument
 		}
 	}
 
 	// Unknown target.
 	log.Infof("Unknown target %q doesn't exist or isn't supported yet.", target.Name.String())
-	return nil, 0, syserr.ErrInvalidArgument
+	return nil, syserr.ErrInvalidArgument
 }
 
 func filterFromIPTIP(iptip linux.IPTIP) (iptables.IPHeaderFilter, *syserr.Error) {
-- 
cgit v1.2.3


From 2661101ad470548cb15dce0afc694296668d780a Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Tue, 21 Jan 2020 14:51:28 -0800
Subject: Removed TCP work (saved in ipt-tcp-match).

---
 pkg/abi/linux/netfilter.go               |  52 -------------
 pkg/sentry/socket/netfilter/netfilter.go |  26 -------
 pkg/tcpip/iptables/tcp_matcher.go        | 122 -------------------------------
 3 files changed, 200 deletions(-)
 delete mode 100644 pkg/tcpip/iptables/tcp_matcher.go

(limited to 'pkg/sentry/socket')

diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go
index fb4588272..f0e544f9c 100644
--- a/pkg/abi/linux/netfilter.go
+++ b/pkg/abi/linux/netfilter.go
@@ -341,58 +341,6 @@ func goString(cstring []byte) string {
 	return string(cstring)
 }
 
-// XTTCP holds data for matching TCP packets. It corresponds to struct xt_tcp
-// in include/uapi/linux/netfilter/xt_tcpudp.h.
-type XTTCP struct {
-	// SourcePortStart specifies the inclusive start of the range of source
-	// ports to which the matcher applies.
-	SourcePortStart uint16
-
-	// SourcePortEnd specifies the inclusive end of the range of source ports
-	// to which the matcher applies.
-	SourcePortEnd uint16
-
-	// DestinationPortStart specifies the start of the destination port
-	// range to which the matcher applies.
-	DestinationPortStart uint16
-
-	// DestinationPortEnd specifies the start of the destination port
-	// range to which the matcher applies.
-	DestinationPortEnd uint16
-
-	// Option specifies that a particular TCP option must be set.
-	Option uint8
-
-	// FlagMask masks the FlagCompare byte when comparing to the TCP flag
-	// fields.
-	FlagMask uint8
-
-	// FlagCompare is binary and-ed with the TCP flag fields.
-	FlagCompare uint8
-
-	// InverseFlags flips the meaning of certain fields. See the
-	// TX_TCP_INV_* flags.
-	InverseFlags uint8
-}
-
-// SizeOfXTTCP is the size of an XTTCP.
-const SizeOfXTTCP = 12
-
-// Flags in XTTCP.InverseFlags. Corresponding constants are in
-// include/uapi/linux/netfilter/xt_tcpudp.h.
-const (
-	// Invert the meaning of SourcePortStart/End.
-	XT_TCP_INV_SRCPT = 0x01
-	// Invert the meaning of DestinationPortStart/End.
-	XT_TCP_INV_DSTPT = 0x02
-	// Invert the meaning of FlagCompare.
-	XT_TCP_INV_FLAGS = 0x04
-	// Invert the meaning of Option.
-	XT_TCP_INV_OPTION = 0x08
-	// Enable all flags.
-	XT_TCP_INV_MASK = 0x0F
-)
-
 // XTUDP holds data for matching UDP packets. It corresponds to struct xt_udp
 // in include/uapi/linux/netfilter/xt_tcpudp.h.
 type XTUDP struct {
diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index 45296b339..f8ed1acbc 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -131,7 +131,6 @@ func FillDefaultIPTables(stack *stack.Stack) {
 	stack.SetIPTables(ipt)
 }
 
-// TODO: Return proto.
 // convertNetstackToBinary converts the iptables as stored in netstack to the
 // format expected by the iptables tool. Linux stores each table as a binary
 // blob that can only be traversed by parsing a bit, reading some offsets,
@@ -456,31 +455,6 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 		var matcher iptables.Matcher
 		var err error
 		switch match.Name.String() {
-		case "tcp":
-			if len(buf) < linux.SizeOfXTTCP {
-				log.Warningf("netfilter: optVal has insufficient size for TCP match: %d", len(optVal))
-				return nil, syserr.ErrInvalidArgument
-			}
-			var matchData linux.XTTCP
-			// For alignment reasons, the match's total size may exceed what's
-			// strictly necessary to hold matchData.
-			binary.Unmarshal(buf[:linux.SizeOfXTUDP], usermem.ByteOrder, &matchData)
-			log.Infof("parseMatchers: parsed XTTCP: %+v", matchData)
-			matcher, err = iptables.NewTCPMatcher(filter, iptables.TCPMatcherData{
-				SourcePortStart:      matchData.SourcePortStart,
-				SourcePortEnd:        matchData.SourcePortEnd,
-				DestinationPortStart: matchData.DestinationPortStart,
-				DestinationPortEnd:   matchData.DestinationPortEnd,
-				Option:               matchData.Option,
-				FlagMask:             matchData.FlagMask,
-				FlagCompare:          matchData.FlagCompare,
-				InverseFlags:         matchData.InverseFlags,
-			})
-			if err != nil {
-				log.Warningf("netfilter: failed to create TCP matcher: %v", err)
-				return nil, syserr.ErrInvalidArgument
-			}
-
 		case "udp":
 			if len(buf) < linux.SizeOfXTUDP {
 				log.Warningf("netfilter: optVal has insufficient size for UDP match: %d", len(optVal))
diff --git a/pkg/tcpip/iptables/tcp_matcher.go b/pkg/tcpip/iptables/tcp_matcher.go
deleted file mode 100644
index 6acbd6eb9..000000000
--- a/pkg/tcpip/iptables/tcp_matcher.go
+++ /dev/null
@@ -1,122 +0,0 @@
-// Copyright 2020 The gVisor Authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package iptables
-
-import (
-	"fmt"
-
-	"gvisor.dev/gvisor/pkg/log"
-	"gvisor.dev/gvisor/pkg/tcpip"
-	"gvisor.dev/gvisor/pkg/tcpip/header"
-)
-
-type TCPMatcher struct {
-	data TCPMatcherData
-
-	// tablename string
-	// unsigned int matchsize;
-	// unsigned int usersize;
-	// #ifdef CONFIG_COMPAT
-	// unsigned int compatsize;
-	// #endif
-	// unsigned int hooks;
-	// unsigned short proto;
-	// unsigned short family;
-}
-
-// TODO: Delete?
-// MatchCheckEntryParams
-
-type TCPMatcherData struct {
-	// Filter IPHeaderFilter
-
-	SourcePortStart      uint16
-	SourcePortEnd        uint16
-	DestinationPortStart uint16
-	DestinationPortEnd   uint16
-	Option               uint8
-	FlagMask             uint8
-	FlagCompare          uint8
-	InverseFlags         uint8
-}
-
-func NewTCPMatcher(filter IPHeaderFilter, data TCPMatcherData) (Matcher, error) {
-	// TODO: We currently only support source port and destination port.
-	log.Infof("Adding rule with TCPMatcherData: %+v", data)
-
-	if data.Option != 0 ||
-		data.FlagMask != 0 ||
-		data.FlagCompare != 0 ||
-		data.InverseFlags != 0 {
-		return nil, fmt.Errorf("unsupported TCP matcher flags set")
-	}
-
-	if filter.Protocol != header.TCPProtocolNumber {
-		log.Warningf("TCP matching is only valid for protocol %d.", header.TCPProtocolNumber)
-	}
-
-	return &TCPMatcher{data: data}, nil
-}
-
-// TODO: Check xt_tcpudp.c. Need to check for same things (e.g. fragments).
-func (tm *TCPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName string) (bool, bool) {
-	netHeader := header.IPv4(pkt.NetworkHeader)
-
-	// TODO: Do we check proto here or elsewhere? I think elsewhere (check
-	// codesearch).
-	if netHeader.TransportProtocol() != header.TCPProtocolNumber {
-		return false, false
-	}
-
-	// We dont't match fragments.
-	if frag := netHeader.FragmentOffset(); frag != 0 {
-		if frag == 1 {
-			log.Warningf("Dropping TCP packet: malicious packet with fragment with fragment offest of 1.")
-			return false, true
-		}
-		return false, false
-	}
-
-	// Now we need the transport header. However, this may not have been set
-	// yet.
-	// TODO
-	var tcpHeader header.TCP
-	if pkt.TransportHeader != nil {
-		tcpHeader = header.TCP(pkt.TransportHeader)
-	} else {
-		// The TCP header hasn't been parsed yet. We have to do it here.
-		if len(pkt.Data.First()) < header.TCPMinimumSize {
-			// There's no valid TCP header here, so we hotdrop the
-			// packet.
-			// TODO: Stats.
-			log.Warningf("Dropping TCP packet: size to small.")
-			return false, true
-		}
-		tcpHeader = header.TCP(pkt.Data.First())
-	}
-
-	// Check whether the source and destination ports are within the
-	// matching range.
-	sourcePort := tcpHeader.SourcePort()
-	destinationPort := tcpHeader.DestinationPort()
-	if sourcePort < tm.data.SourcePortStart || tm.data.SourcePortEnd < sourcePort {
-		return false, false
-	}
-	if destinationPort < tm.data.DestinationPortStart || tm.data.DestinationPortEnd < destinationPort {
-		return false, false
-	}
-
-	return true, false
-}
-- 
cgit v1.2.3


From 538053538dfb378aa8bc512d484ea305177e617b Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Tue, 21 Jan 2020 16:51:17 -0800
Subject: Adding serialization.

---
 pkg/sentry/socket/netfilter/netfilter.go | 29 ++++++++++++++++++++++++++++-
 pkg/tcpip/iptables/udp_matcher.go        | 14 +++++++-------
 2 files changed, 35 insertions(+), 8 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index f8ed1acbc..3caabca9a 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -196,7 +196,9 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 }
 
 func marshalMatcher(matcher iptables.Matcher) []byte {
-	switch matcher.(type) {
+	switch m := matcher.(type) {
+	case *iptables.UDPMatcher:
+		return marshalUDPMatcher(m)
 	default:
 		// TODO(gvisor.dev/issue/170): We don't support any matchers
 		// yet, so any call to marshalMatcher will panic.
@@ -204,6 +206,31 @@ func marshalMatcher(matcher iptables.Matcher) []byte {
 	}
 }
 
+func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
+	type udpMatch struct {
+		linux.XTEntryMatch
+		linux.XTUDP
+	}
+	linuxMatcher := udpMatch{
+		XTEntryMatch: linux.XTEntryMatch{
+			MatchSize: linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP,
+			// Name:      "udp",
+		},
+		XTUDP: linux.XTUDP{
+			SourcePortStart:      matcher.Data.SourcePortStart,
+			SourcePortEnd:        matcher.Data.SourcePortEnd,
+			DestinationPortStart: matcher.Data.DestinationPortStart,
+			DestinationPortEnd:   matcher.Data.DestinationPortEnd,
+			InverseFlags:         matcher.Data.InverseFlags,
+		},
+	}
+	copy(linuxMatcher.Name[:], "udp")
+
+	var buf [linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP]byte
+	binary.Marshal(buf[:], usermem.ByteOrder, linuxMatcher)
+	return buf[:]
+}
+
 func marshalTarget(target iptables.Target) []byte {
 	switch target.(type) {
 	case iptables.UnconditionalAcceptTarget:
diff --git a/pkg/tcpip/iptables/udp_matcher.go b/pkg/tcpip/iptables/udp_matcher.go
index ce4368a3d..fca457199 100644
--- a/pkg/tcpip/iptables/udp_matcher.go
+++ b/pkg/tcpip/iptables/udp_matcher.go
@@ -24,7 +24,7 @@ import (
 )
 
 type UDPMatcher struct {
-	data UDPMatcherData
+	Data UDPMatcherData
 
 	// tablename string
 	// unsigned int matchsize;
@@ -62,11 +62,11 @@ func NewUDPMatcher(filter IPHeaderFilter, data UDPMatcherData) (Matcher, error)
 		log.Warningf("UDP matching is only valid for protocol %d.", header.UDPProtocolNumber)
 	}
 
-	return &UDPMatcher{data: data}, nil
+	return &UDPMatcher{Data: data}, nil
 }
 
 // TODO: Check xt_tcpudp.c. Need to check for same things (e.g. fragments).
-func (tm *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName string) (bool, bool) {
+func (um *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName string) (bool, bool) {
 	log.Infof("UDPMatcher called from: %s", string(debug.Stack()))
 	netHeader := header.IPv4(pkt.NetworkHeader)
 
@@ -114,12 +114,12 @@ func (tm *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName str
 	destinationPort := udpHeader.DestinationPort()
 	log.Infof("UDPMatcher: sport and dport are %d and %d. sports and dport start and end are (%d, %d) and (%d, %d)",
 		udpHeader.SourcePort(), udpHeader.DestinationPort(),
-		tm.data.SourcePortStart, tm.data.SourcePortEnd,
-		tm.data.DestinationPortStart, tm.data.DestinationPortEnd)
-	if sourcePort < tm.data.SourcePortStart || tm.data.SourcePortEnd < sourcePort {
+		um.Data.SourcePortStart, um.Data.SourcePortEnd,
+		um.Data.DestinationPortStart, um.Data.DestinationPortEnd)
+	if sourcePort < um.Data.SourcePortStart || um.Data.SourcePortEnd < sourcePort {
 		return false, false
 	}
-	if destinationPort < tm.data.DestinationPortStart || tm.data.DestinationPortEnd < destinationPort {
+	if destinationPort < um.Data.DestinationPortStart || um.Data.DestinationPortEnd < destinationPort {
 		return false, false
 	}
 
-- 
cgit v1.2.3


From b7853f688b4bcd3465c0c3087fcbd8d53bdf26ae Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Wed, 22 Jan 2020 14:46:15 -0800
Subject: Error marshalling the matcher.

The iptables binary is looking for libxt_.so when it should be looking
for libxt_udp.so, so it's having an issue reading the data in
xt_match_entry. I think it may be an alignment issue.

Trying to fix this is leading to me fighting with the metadata struct,
so I'm gonna go kill that.
---
 pkg/abi/linux/netfilter.go               |  5 +++++
 pkg/sentry/socket/netfilter/netfilter.go | 35 ++++++++++++++++++++------------
 pkg/tcpip/iptables/udp_matcher.go        |  2 +-
 3 files changed, 28 insertions(+), 14 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go
index f0e544f9c..effed7976 100644
--- a/pkg/abi/linux/netfilter.go
+++ b/pkg/abi/linux/netfilter.go
@@ -198,6 +198,11 @@ type XTEntryMatch struct {
 // SizeOfXTEntryMatch is the size of an XTEntryMatch.
 const SizeOfXTEntryMatch = 32
 
+type KernelXTEntryMatch struct {
+	XTEntryMatch
+	Data []byte
+}
+
 // XTEntryTarget holds a target for a rule. For example, it can specify that
 // packets matching the rule should DROP, ACCEPT, or use an extension target.
 // iptables-extension(8) has a list of possible targets.
diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index 3caabca9a..b49fe5b3e 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -207,26 +207,34 @@ func marshalMatcher(matcher iptables.Matcher) []byte {
 }
 
 func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
-	type udpMatch struct {
-		linux.XTEntryMatch
-		linux.XTUDP
-	}
-	linuxMatcher := udpMatch{
+	linuxMatcher := linux.KernelXTEntryMatch{
 		XTEntryMatch: linux.XTEntryMatch{
 			MatchSize: linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP,
 			// Name:      "udp",
 		},
-		XTUDP: linux.XTUDP{
-			SourcePortStart:      matcher.Data.SourcePortStart,
-			SourcePortEnd:        matcher.Data.SourcePortEnd,
-			DestinationPortStart: matcher.Data.DestinationPortStart,
-			DestinationPortEnd:   matcher.Data.DestinationPortEnd,
-			InverseFlags:         matcher.Data.InverseFlags,
-		},
+		Data: make([]byte, linux.SizeOfXTUDP+22),
 	}
+	// copy(linuxMatcher.Name[:], "udp")
 	copy(linuxMatcher.Name[:], "udp")
 
-	var buf [linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP]byte
+	// TODO: Must be aligned.
+	xtudp := linux.XTUDP{
+		SourcePortStart:      matcher.Data.SourcePortStart,
+		SourcePortEnd:        matcher.Data.SourcePortEnd,
+		DestinationPortStart: matcher.Data.DestinationPortStart,
+		DestinationPortEnd:   matcher.Data.DestinationPortEnd,
+		InverseFlags:         matcher.Data.InverseFlags,
+	}
+	binary.Marshal(linuxMatcher.Data[:linux.SizeOfXTUDP], usermem.ByteOrder, xtudp)
+
+	if binary.Size(linuxMatcher)%64 != 0 {
+		panic(fmt.Sprintf("size is actually: %d", binary.Size(linuxMatcher)))
+	}
+
+	var buf [linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 22]byte
+	if len(buf)%64 != 0 {
+		panic(fmt.Sprintf("len is actually: %d", len(buf)))
+	}
 	binary.Marshal(buf[:], usermem.ByteOrder, linuxMatcher)
 	return buf[:]
 }
@@ -245,6 +253,7 @@ func marshalTarget(target iptables.Target) []byte {
 }
 
 func marshalStandardTarget(verdict iptables.Verdict) []byte {
+	// TODO: Must be aligned.
 	// The target's name will be the empty string.
 	target := linux.XTStandardTarget{
 		Target: linux.XTEntryTarget{
diff --git a/pkg/tcpip/iptables/udp_matcher.go b/pkg/tcpip/iptables/udp_matcher.go
index fca457199..65ae7f9e0 100644
--- a/pkg/tcpip/iptables/udp_matcher.go
+++ b/pkg/tcpip/iptables/udp_matcher.go
@@ -59,7 +59,7 @@ func NewUDPMatcher(filter IPHeaderFilter, data UDPMatcherData) (Matcher, error)
 	}
 
 	if filter.Protocol != header.UDPProtocolNumber {
-		log.Warningf("UDP matching is only valid for protocol %d.", header.UDPProtocolNumber)
+		return nil, fmt.Errorf("UDP matching is only valid for protocol %d.", header.UDPProtocolNumber)
 	}
 
 	return &UDPMatcher{Data: data}, nil
-- 
cgit v1.2.3


From 2946fe81627afa223853769ed736e2a56e0144b7 Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Fri, 24 Jan 2020 17:12:03 -0800
Subject: We can now actually write out the udp matcher.

---
 pkg/sentry/socket/netfilter/netfilter.go | 78 ++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 20 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index 3ca22932d..6c88a50a6 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -36,7 +36,7 @@ const errorTargetName = "ERROR"
 
 // metadata is opaque to netstack. It holds data that we need to translate
 // between Linux's and netstack's iptables representations.
-// TODO(gvisor.dev/issue/170): This might be removable.
+// TODO(gvisor.dev/issue/170): Use metadata to check correctness.
 type metadata struct {
 	HookEntry  [linux.NF_INET_NUMHOOKS]uint32
 	Underflow  [linux.NF_INET_NUMHOOKS]uint32
@@ -44,6 +44,14 @@ type metadata struct {
 	Size       uint32
 }
 
+const enableDebugLog = true
+
+func nflog(format string, args ...interface{}) {
+	if enableDebugLog {
+		log.Infof("netfilter: "+format, args...)
+	}
+}
+
 // GetInfo returns information about iptables.
 func GetInfo(t *kernel.Task, stack *stack.Stack, outPtr usermem.Addr) (linux.IPTGetinfo, *syserr.Error) {
 	// Read in the struct and table name.
@@ -72,6 +80,8 @@ func GetInfo(t *kernel.Task, stack *stack.Stack, outPtr usermem.Addr) (linux.IPT
 	info.NumEntries = metadata.NumEntries
 	info.Size = metadata.Size
 
+	nflog("GetInfo returning info: %+v", info)
+
 	return info, nil
 }
 
@@ -80,21 +90,26 @@ func GetEntries(t *kernel.Task, stack *stack.Stack, outPtr usermem.Addr, outLen
 	// Read in the struct and table name.
 	var userEntries linux.IPTGetEntries
 	if _, err := t.CopyIn(outPtr, &userEntries); err != nil {
+		log.Warningf("netfilter: couldn't copy in entries %q", userEntries.Name)
 		return linux.KernelIPTGetEntries{}, syserr.FromError(err)
 	}
 
 	// Find the appropriate table.
 	table, err := findTable(stack, userEntries.Name)
 	if err != nil {
+		log.Warningf("netfilter: couldn't find table %q", userEntries.Name)
 		return linux.KernelIPTGetEntries{}, err
 	}
 
 	// Convert netstack's iptables rules to something that the iptables
 	// tool can understand.
-	entries, _, err := convertNetstackToBinary(userEntries.Name.String(), table)
+	entries, meta, err := convertNetstackToBinary(userEntries.Name.String(), table)
 	if err != nil {
 		return linux.KernelIPTGetEntries{}, err
 	}
+	if meta != table.Metadata().(metadata) {
+		panic(fmt.Sprintf("Table %q metadata changed between writing and reading. Was saved as %+v, but is now %+v", userEntries.Name.String(), table.Metadata().(metadata), meta))
+	}
 	if binary.Size(entries) > uintptr(outLen) {
 		log.Warningf("Insufficient GetEntries output size: %d", uintptr(outLen))
 		return linux.KernelIPTGetEntries{}, syserr.ErrInvalidArgument
@@ -148,15 +163,19 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 	copy(entries.Name[:], tablename)
 
 	for ruleIdx, rule := range table.Rules {
+		nflog("Current offset: %d", entries.Size)
+
 		// Is this a chain entry point?
 		for hook, hookRuleIdx := range table.BuiltinChains {
 			if hookRuleIdx == ruleIdx {
+				nflog("Found hook %d at offset %d", hook, entries.Size)
 				meta.HookEntry[hook] = entries.Size
 			}
 		}
 		// Is this a chain underflow point?
 		for underflow, underflowRuleIdx := range table.Underflows {
 			if underflowRuleIdx == ruleIdx {
+				nflog("Found underflow %d at offset %d", underflow, entries.Size)
 				meta.Underflow[underflow] = entries.Size
 			}
 		}
@@ -176,6 +195,10 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 			// Serialize the matcher and add it to the
 			// entry.
 			serialized := marshalMatcher(matcher)
+			nflog("matcher serialized as: %v", serialized)
+			if len(serialized)%8 != 0 {
+				panic(fmt.Sprintf("matcher %T is not 64-bit aligned", matcher))
+			}
 			entry.Elems = append(entry.Elems, serialized...)
 			entry.NextOffset += uint16(len(serialized))
 			entry.TargetOffset += uint16(len(serialized))
@@ -183,18 +206,25 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 
 		// Serialize and append the target.
 		serialized := marshalTarget(rule.Target)
+		if len(serialized)%8 != 0 {
+			panic(fmt.Sprintf("target %T is not 64-bit aligned", rule.Target))
+		}
 		entry.Elems = append(entry.Elems, serialized...)
 		entry.NextOffset += uint16(len(serialized))
 
+		nflog("Adding entry: %+v", entry)
+
 		entries.Size += uint32(entry.NextOffset)
 		entries.Entrytable = append(entries.Entrytable, entry)
 		meta.NumEntries++
 	}
 
+	nflog("Finished with an marshalled size of %d", meta.Size)
 	meta.Size = entries.Size
 	return entries, meta, nil
 }
 
+// TODO: SOMEHOW THIS IS NOT GETTING APPENDED!
 func marshalMatcher(matcher iptables.Matcher) []byte {
 	switch m := matcher.(type) {
 	case *iptables.UDPMatcher:
@@ -207,17 +237,17 @@ func marshalMatcher(matcher iptables.Matcher) []byte {
 }
 
 func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
+	nflog("Marshalling UDP matcher: %+v", matcher)
+
 	linuxMatcher := linux.KernelXTEntryMatch{
 		XTEntryMatch: linux.XTEntryMatch{
-			MatchSize: linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP,
+			MatchSize: linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 6,
 			// Name:      "udp",
 		},
-		Data: make([]byte, linux.SizeOfXTUDP+22),
+		Data: make([]byte, 0, linux.SizeOfXTUDP),
 	}
-	// copy(linuxMatcher.Name[:], "udp")
 	copy(linuxMatcher.Name[:], "udp")
 
-	// TODO: Must be aligned.
 	xtudp := linux.XTUDP{
 		SourcePortStart:      matcher.Data.SourcePortStart,
 		SourcePortEnd:        matcher.Data.SourcePortEnd,
@@ -225,17 +255,17 @@ func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
 		DestinationPortEnd:   matcher.Data.DestinationPortEnd,
 		InverseFlags:         matcher.Data.InverseFlags,
 	}
-	binary.Marshal(linuxMatcher.Data[:linux.SizeOfXTUDP], usermem.ByteOrder, xtudp)
-
-	if binary.Size(linuxMatcher)%64 != 0 {
-		panic(fmt.Sprintf("size is actually: %d", binary.Size(linuxMatcher)))
-	}
-
-	var buf [linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 22]byte
-	if len(buf)%64 != 0 {
-		panic(fmt.Sprintf("len is actually: %d", len(buf)))
-	}
-	binary.Marshal(buf[:], usermem.ByteOrder, linuxMatcher)
+	nflog("marshalUDPMatcher: xtudp: %+v", xtudp)
+	linuxMatcher.Data = binary.Marshal(linuxMatcher.Data, usermem.ByteOrder, xtudp)
+	nflog("marshalUDPMatcher: linuxMatcher: %+v", linuxMatcher)
+
+	// We have to pad this struct size to a multiple of 8 bytes, so we make
+	// this a little longer than it needs to be.
+	buf := make([]byte, 0, linux.SizeOfXTEntryMatch+linux.SizeOfXTUDP+6)
+	buf = binary.Marshal(buf, usermem.ByteOrder, linuxMatcher)
+	buf = append(buf, []byte{0, 0, 0, 0, 0, 0}...)
+	nflog("Marshalled into matcher of size %d", len(buf))
+	nflog("marshalUDPMatcher: buf is: %v", buf)
 	return buf[:]
 }
 
@@ -253,6 +283,8 @@ func marshalTarget(target iptables.Target) []byte {
 }
 
 func marshalStandardTarget(verdict iptables.Verdict) []byte {
+	nflog("Marshalling standard target with size %d", linux.SizeOfXTStandardTarget)
+
 	// TODO: Must be aligned.
 	// The target's name will be the empty string.
 	target := linux.XTStandardTarget{
@@ -321,7 +353,7 @@ func translateToStandardVerdict(val int32) (iptables.Verdict, *syserr.Error) {
 // SetEntries sets iptables rules for a single table. See
 // net/ipv4/netfilter/ip_tables.c:translate_table for reference.
 func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
-	printReplace(optVal)
+	// printReplace(optVal)
 
 	// Get the basic rules data (struct ipt_replace).
 	if len(optVal) < linux.SizeOfIPTReplace {
@@ -343,10 +375,14 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		return syserr.ErrInvalidArgument
 	}
 
+	nflog("Setting entries in table %q", replace.Name.String())
+
 	// Convert input into a list of rules and their offsets.
 	var offset uint32
 	var offsets []uint32
 	for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ {
+		nflog("Processing entry at offset %d", offset)
+
 		// Get the struct ipt_entry.
 		if len(optVal) < linux.SizeOfIPTEntry {
 			log.Warningf("netfilter: optVal has insufficient size for entry %d", len(optVal))
@@ -464,9 +500,10 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 // parseMatchers parses 0 or more matchers from optVal. optVal should contain
 // only the matchers.
 func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Matcher, *syserr.Error) {
+	nflog("Parsing matchers of size %d", len(optVal))
 	var matchers []iptables.Matcher
 	for len(optVal) > 0 {
-		log.Infof("parseMatchers: optVal has len %d", len(optVal))
+		nflog("parseMatchers: optVal has len %d", len(optVal))
 		// Get the XTEntryMatch.
 		if len(optVal) < linux.SizeOfXTEntryMatch {
 			log.Warningf("netfilter: optVal has insufficient size for entry match: %d", len(optVal))
@@ -475,7 +512,7 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 		var match linux.XTEntryMatch
 		buf := optVal[:linux.SizeOfXTEntryMatch]
 		binary.Unmarshal(buf, usermem.ByteOrder, &match)
-		log.Infof("parseMatchers: parsed entry match %q: %+v", match.Name.String(), match)
+		nflog("parseMatchers: parsed entry match %q: %+v", match.Name.String(), match)
 
 		// Check some invariants.
 		if match.MatchSize < linux.SizeOfXTEntryMatch {
@@ -532,6 +569,7 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 // parseTarget parses a target from optVal. optVal should contain only the
 // target.
 func parseTarget(optVal []byte) (iptables.Target, *syserr.Error) {
+	nflog("Parsing target of size %d", len(optVal))
 	if len(optVal) < linux.SizeOfXTEntryTarget {
 		log.Warningf("netfilter: optVal has insufficient size for entry target %d", len(optVal))
 		return nil, syserr.ErrInvalidArgument
-- 
cgit v1.2.3


From 29316e66adfc49c158425554761e34c12338f1d9 Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Mon, 27 Jan 2020 12:27:04 -0800
Subject: Cleanup for GH review.

---
 pkg/abi/linux/netfilter.go               |  14 +--
 pkg/sentry/socket/netfilter/netfilter.go | 144 ++++++++++++-------------------
 pkg/tcpip/iptables/types.go              |  15 ----
 pkg/tcpip/iptables/udp_matcher.go        |  62 ++++++-------
 test/iptables/filter_input.go            |   6 +-
 5 files changed, 88 insertions(+), 153 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go
index effed7976..8e40bcc62 100644
--- a/pkg/abi/linux/netfilter.go
+++ b/pkg/abi/linux/netfilter.go
@@ -198,6 +198,8 @@ type XTEntryMatch struct {
 // SizeOfXTEntryMatch is the size of an XTEntryMatch.
 const SizeOfXTEntryMatch = 32
 
+// KernelXTEntryMatch is identical to XTEntryMatch, but contains
+// variable-length Data field.
 type KernelXTEntryMatch struct {
 	XTEntryMatch
 	Data []byte
@@ -349,19 +351,19 @@ func goString(cstring []byte) string {
 // XTUDP holds data for matching UDP packets. It corresponds to struct xt_udp
 // in include/uapi/linux/netfilter/xt_tcpudp.h.
 type XTUDP struct {
-	// SourcePortStart specifies the inclusive start of the range of source
-	// ports to which the matcher applies.
+	// SourcePortStart is the inclusive start of the range of source ports
+	// to which the matcher applies.
 	SourcePortStart uint16
 
-	// SourcePortEnd specifies the inclusive end of the range of source ports
-	// to which the matcher applies.
+	// SourcePortEnd is the inclusive end of the range of source ports to
+	// which the matcher applies.
 	SourcePortEnd uint16
 
-	// DestinationPortStart specifies the start of the destination port
+	// DestinationPortStart is the inclusive start of the destination port
 	// range to which the matcher applies.
 	DestinationPortStart uint16
 
-	// DestinationPortEnd specifies the start of the destination port
+	// DestinationPortEnd is the inclusive end of the destination port
 	// range to which the matcher applies.
 	DestinationPortEnd uint16
 
diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index 6c88a50a6..b8848f08a 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -34,9 +34,16 @@ import (
 // shouldn't be reached - an error has occurred if we fall through to one.
 const errorTargetName = "ERROR"
 
-// metadata is opaque to netstack. It holds data that we need to translate
-// between Linux's and netstack's iptables representations.
-// TODO(gvisor.dev/issue/170): Use metadata to check correctness.
+const (
+	matcherNameUDP = "udp"
+)
+
+// Metadata is used to verify that we are correctly serializing and
+// deserializing iptables into structs consumable by the iptables tool. We save
+// a metadata struct when the tables are written, and when they are read out we
+// verify that certain fields are the same.
+//
+// metadata is opaque to netstack.
 type metadata struct {
 	HookEntry  [linux.NF_INET_NUMHOOKS]uint32
 	Underflow  [linux.NF_INET_NUMHOOKS]uint32
@@ -44,10 +51,12 @@ type metadata struct {
 	Size       uint32
 }
 
-const enableDebugLog = true
+const enableDebug = false
 
+// nflog logs messages related to the writing and reading of iptables, but only
+// when enableDebug is true.
 func nflog(format string, args ...interface{}) {
-	if enableDebugLog {
+	if enableDebug {
 		log.Infof("netfilter: "+format, args...)
 	}
 }
@@ -80,7 +89,7 @@ func GetInfo(t *kernel.Task, stack *stack.Stack, outPtr usermem.Addr) (linux.IPT
 	info.NumEntries = metadata.NumEntries
 	info.Size = metadata.Size
 
-	nflog("GetInfo returning info: %+v", info)
+	nflog("returning info: %+v", info)
 
 	return info, nil
 }
@@ -163,19 +172,19 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 	copy(entries.Name[:], tablename)
 
 	for ruleIdx, rule := range table.Rules {
-		nflog("Current offset: %d", entries.Size)
+		nflog("convert to binary: current offset: %d", entries.Size)
 
 		// Is this a chain entry point?
 		for hook, hookRuleIdx := range table.BuiltinChains {
 			if hookRuleIdx == ruleIdx {
-				nflog("Found hook %d at offset %d", hook, entries.Size)
+				nflog("convert to binary: found hook %d at offset %d", hook, entries.Size)
 				meta.HookEntry[hook] = entries.Size
 			}
 		}
 		// Is this a chain underflow point?
 		for underflow, underflowRuleIdx := range table.Underflows {
 			if underflowRuleIdx == ruleIdx {
-				nflog("Found underflow %d at offset %d", underflow, entries.Size)
+				nflog("convert to binary: found underflow %d at offset %d", underflow, entries.Size)
 				meta.Underflow[underflow] = entries.Size
 			}
 		}
@@ -195,7 +204,7 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 			// Serialize the matcher and add it to the
 			// entry.
 			serialized := marshalMatcher(matcher)
-			nflog("matcher serialized as: %v", serialized)
+			nflog("convert to binary: matcher serialized as: %v", serialized)
 			if len(serialized)%8 != 0 {
 				panic(fmt.Sprintf("matcher %T is not 64-bit aligned", matcher))
 			}
@@ -212,14 +221,14 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 		entry.Elems = append(entry.Elems, serialized...)
 		entry.NextOffset += uint16(len(serialized))
 
-		nflog("Adding entry: %+v", entry)
+		nflog("convert to binary: adding entry: %+v", entry)
 
 		entries.Size += uint32(entry.NextOffset)
 		entries.Entrytable = append(entries.Entrytable, entry)
 		meta.NumEntries++
 	}
 
-	nflog("Finished with an marshalled size of %d", meta.Size)
+	nflog("convert to binary: finished with an marshalled size of %d", meta.Size)
 	meta.Size = entries.Size
 	return entries, meta, nil
 }
@@ -237,16 +246,18 @@ func marshalMatcher(matcher iptables.Matcher) []byte {
 }
 
 func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
-	nflog("Marshalling UDP matcher: %+v", matcher)
+	nflog("convert to binary: marshalling UDP matcher: %+v", matcher)
+
+	// We have to pad this struct size to a multiple of 8 bytes.
+	const size = linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 6
 
 	linuxMatcher := linux.KernelXTEntryMatch{
 		XTEntryMatch: linux.XTEntryMatch{
-			MatchSize: linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 6,
-			// Name:      "udp",
+			MatchSize: size,
 		},
 		Data: make([]byte, 0, linux.SizeOfXTUDP),
 	}
-	copy(linuxMatcher.Name[:], "udp")
+	copy(linuxMatcher.Name[:], matcherNameUDP)
 
 	xtudp := linux.XTUDP{
 		SourcePortStart:      matcher.Data.SourcePortStart,
@@ -255,17 +266,12 @@ func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
 		DestinationPortEnd:   matcher.Data.DestinationPortEnd,
 		InverseFlags:         matcher.Data.InverseFlags,
 	}
-	nflog("marshalUDPMatcher: xtudp: %+v", xtudp)
 	linuxMatcher.Data = binary.Marshal(linuxMatcher.Data, usermem.ByteOrder, xtudp)
-	nflog("marshalUDPMatcher: linuxMatcher: %+v", linuxMatcher)
 
-	// We have to pad this struct size to a multiple of 8 bytes, so we make
-	// this a little longer than it needs to be.
-	buf := make([]byte, 0, linux.SizeOfXTEntryMatch+linux.SizeOfXTUDP+6)
+	buf := make([]byte, 0, size)
 	buf = binary.Marshal(buf, usermem.ByteOrder, linuxMatcher)
 	buf = append(buf, []byte{0, 0, 0, 0, 0, 0}...)
-	nflog("Marshalled into matcher of size %d", len(buf))
-	nflog("marshalUDPMatcher: buf is: %v", buf)
+	nflog("convert to binary: marshalled UDP matcher into %v", buf)
 	return buf[:]
 }
 
@@ -283,9 +289,8 @@ func marshalTarget(target iptables.Target) []byte {
 }
 
 func marshalStandardTarget(verdict iptables.Verdict) []byte {
-	nflog("Marshalling standard target with size %d", linux.SizeOfXTStandardTarget)
+	nflog("convert to binary: marshalling standard target with size %d", linux.SizeOfXTStandardTarget)
 
-	// TODO: Must be aligned.
 	// The target's name will be the empty string.
 	target := linux.XTStandardTarget{
 		Target: linux.XTEntryTarget{
@@ -353,8 +358,6 @@ func translateToStandardVerdict(val int32) (iptables.Verdict, *syserr.Error) {
 // SetEntries sets iptables rules for a single table. See
 // net/ipv4/netfilter/ip_tables.c:translate_table for reference.
 func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
-	// printReplace(optVal)
-
 	// Get the basic rules data (struct ipt_replace).
 	if len(optVal) < linux.SizeOfIPTReplace {
 		log.Warningf("netfilter.SetEntries: optVal has insufficient size for replace %d", len(optVal))
@@ -375,13 +378,13 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		return syserr.ErrInvalidArgument
 	}
 
-	nflog("Setting entries in table %q", replace.Name.String())
+	nflog("set entries: setting entries in table %q", replace.Name.String())
 
 	// Convert input into a list of rules and their offsets.
 	var offset uint32
 	var offsets []uint32
 	for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ {
-		nflog("Processing entry at offset %d", offset)
+		nflog("set entries: processing entry at offset %d", offset)
 
 		// Get the struct ipt_entry.
 		if len(optVal) < linux.SizeOfIPTEntry {
@@ -406,11 +409,13 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 			return err
 		}
 
-		// TODO: Matchers (and maybe targets) can specify that they only work for certiain protocols, hooks, tables.
+		// TODO(gvisor.dev/issue/170): Matchers and targets can specify
+		// that they only work for certiain protocols, hooks, tables.
 		// Get matchers.
 		matchersSize := entry.TargetOffset - linux.SizeOfIPTEntry
 		if len(optVal) < int(matchersSize) {
 			log.Warningf("netfilter: entry doesn't have enough room for its matchers (only %d bytes remain)", len(optVal))
+			return syserr.ErrInvalidArgument
 		}
 		matchers, err := parseMatchers(filter, optVal[:matchersSize])
 		if err != nil {
@@ -423,6 +428,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		targetSize := entry.NextOffset - entry.TargetOffset
 		if len(optVal) < int(targetSize) {
 			log.Warningf("netfilter: entry doesn't have enough room for its target (only %d bytes remain)", len(optVal))
+			return syserr.ErrInvalidArgument
 		}
 		target, err := parseTarget(optVal[:targetSize])
 		if err != nil {
@@ -500,10 +506,11 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 // parseMatchers parses 0 or more matchers from optVal. optVal should contain
 // only the matchers.
 func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Matcher, *syserr.Error) {
-	nflog("Parsing matchers of size %d", len(optVal))
+	nflog("set entries: parsing matchers of size %d", len(optVal))
 	var matchers []iptables.Matcher
 	for len(optVal) > 0 {
-		nflog("parseMatchers: optVal has len %d", len(optVal))
+		nflog("set entries: optVal has len %d", len(optVal))
+
 		// Get the XTEntryMatch.
 		if len(optVal) < linux.SizeOfXTEntryMatch {
 			log.Warningf("netfilter: optVal has insufficient size for entry match: %d", len(optVal))
@@ -512,7 +519,7 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 		var match linux.XTEntryMatch
 		buf := optVal[:linux.SizeOfXTEntryMatch]
 		binary.Unmarshal(buf, usermem.ByteOrder, &match)
-		nflog("parseMatchers: parsed entry match %q: %+v", match.Name.String(), match)
+		nflog("set entries: parsed entry match %q: %+v", match.Name.String(), match)
 
 		// Check some invariants.
 		if match.MatchSize < linux.SizeOfXTEntryMatch {
@@ -528,17 +535,17 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 		var matcher iptables.Matcher
 		var err error
 		switch match.Name.String() {
-		case "udp":
+		case matcherNameUDP:
 			if len(buf) < linux.SizeOfXTUDP {
 				log.Warningf("netfilter: optVal has insufficient size for UDP match: %d", len(optVal))
 				return nil, syserr.ErrInvalidArgument
 			}
+			// For alignment reasons, the match's total size may
+			// exceed what's strictly necessary to hold matchData.
 			var matchData linux.XTUDP
-			// For alignment reasons, the match's total size may exceed what's
-			// strictly necessary to hold matchData.
 			binary.Unmarshal(buf[:linux.SizeOfXTUDP], usermem.ByteOrder, &matchData)
 			log.Infof("parseMatchers: parsed XTUDP: %+v", matchData)
-			matcher, err = iptables.NewUDPMatcher(filter, iptables.UDPMatcherData{
+			matcher, err = iptables.NewUDPMatcher(filter, iptables.UDPMatcherParams{
 				SourcePortStart:      matchData.SourcePortStart,
 				SourcePortEnd:        matchData.SourcePortEnd,
 				DestinationPortStart: matchData.DestinationPortStart,
@@ -557,19 +564,22 @@ func parseMatchers(filter iptables.IPHeaderFilter, optVal []byte) ([]iptables.Ma
 
 		matchers = append(matchers, matcher)
 
-		// TODO: Support revision.
-		// TODO: Support proto -- matchers usually specify which proto(s) they work with.
+		// TODO(gvisor.dev/issue/170): Check the revision field.
 		optVal = optVal[match.MatchSize:]
 	}
 
-	// TODO: Check that optVal is exhausted.
+	if len(optVal) != 0 {
+		log.Warningf("netfilter: optVal should be exhausted after parsing matchers")
+		return nil, syserr.ErrInvalidArgument
+	}
+
 	return matchers, nil
 }
 
 // parseTarget parses a target from optVal. optVal should contain only the
 // target.
 func parseTarget(optVal []byte) (iptables.Target, *syserr.Error) {
-	nflog("Parsing target of size %d", len(optVal))
+	nflog("set entries: parsing target of size %d", len(optVal))
 	if len(optVal) < linux.SizeOfXTEntryTarget {
 		log.Warningf("netfilter: optVal has insufficient size for entry target %d", len(optVal))
 		return nil, syserr.ErrInvalidArgument
@@ -598,7 +608,8 @@ func parseTarget(optVal []byte) (iptables.Target, *syserr.Error) {
 		case iptables.Drop:
 			return iptables.UnconditionalDropTarget{}, nil
 		default:
-			panic(fmt.Sprintf("Unknown verdict: %v", verdict))
+			log.Warningf("Unknown verdict: %v", verdict)
+			return nil, syserr.ErrInvalidArgument
 		}
 
 	case errorTargetName:
@@ -673,52 +684,3 @@ func hookFromLinux(hook int) iptables.Hook {
 	}
 	panic(fmt.Sprintf("Unknown hook %d does not correspond to a builtin chain", hook))
 }
-
-// printReplace prints information about the struct ipt_replace in optVal. It
-// is only for debugging.
-func printReplace(optVal []byte) {
-	// Basic replace info.
-	var replace linux.IPTReplace
-	replaceBuf := optVal[:linux.SizeOfIPTReplace]
-	optVal = optVal[linux.SizeOfIPTReplace:]
-	binary.Unmarshal(replaceBuf, usermem.ByteOrder, &replace)
-	log.Infof("Replacing table %q: %+v", replace.Name.String(), replace)
-
-	// Read in the list of entries at the end of replace.
-	var totalOffset uint16
-	for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ {
-		var entry linux.IPTEntry
-		entryBuf := optVal[:linux.SizeOfIPTEntry]
-		binary.Unmarshal(entryBuf, usermem.ByteOrder, &entry)
-		log.Infof("Entry %d (total offset %d): %+v", entryIdx, totalOffset, entry)
-
-		totalOffset += entry.NextOffset
-		if entry.TargetOffset == linux.SizeOfIPTEntry {
-			log.Infof("Entry has no matches.")
-		} else {
-			log.Infof("Entry has matches.")
-		}
-
-		var target linux.XTEntryTarget
-		targetBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTEntryTarget]
-		binary.Unmarshal(targetBuf, usermem.ByteOrder, &target)
-		log.Infof("Target named %q: %+v", target.Name.String(), target)
-
-		switch target.Name.String() {
-		case "":
-			var standardTarget linux.XTStandardTarget
-			stBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTStandardTarget]
-			binary.Unmarshal(stBuf, usermem.ByteOrder, &standardTarget)
-			log.Infof("Standard target with verdict %q (%d).", linux.VerdictStrings[standardTarget.Verdict], standardTarget.Verdict)
-		case errorTargetName:
-			var errorTarget linux.XTErrorTarget
-			etBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTErrorTarget]
-			binary.Unmarshal(etBuf, usermem.ByteOrder, &errorTarget)
-			log.Infof("Error target with name %q.", errorTarget.Name.String())
-		default:
-			log.Infof("Unknown target type.")
-		}
-
-		optVal = optVal[entry.NextOffset:]
-	}
-}
diff --git a/pkg/tcpip/iptables/types.go b/pkg/tcpip/iptables/types.go
index d47447d40..ba5ed75b4 100644
--- a/pkg/tcpip/iptables/types.go
+++ b/pkg/tcpip/iptables/types.go
@@ -169,8 +169,6 @@ type IPHeaderFilter struct {
 	Protocol tcpip.TransportProtocolNumber
 }
 
-// TODO: Should these be able to marshal/unmarshal themselves?
-// TODO: Something has to map the name to the matcher.
 // A Matcher is the interface for matching packets.
 type Matcher interface {
 	// Match returns whether the packet matches and whether the packet
@@ -179,19 +177,6 @@ type Matcher interface {
 	//
 	// Precondition: packet.NetworkHeader is set.
 	Match(hook Hook, packet tcpip.PacketBuffer, interfaceName string) (matches bool, hotdrop bool)
-
-	// TODO: Make this typesafe by having each Matcher have their own, typed CheckEntry?
-	// CheckEntry(params MatchCheckEntryParams) bool
-}
-
-// TODO: Unused?
-type MatchCheckEntryParams struct {
-	Table  string // TODO: Tables should be an enum...
-	Filter IPHeaderFilter
-	Info   interface{} // TODO: Type unsafe.
-	// HookMask       uint8
-	// Family         uint8
-	// NFTCompat      bool
 }
 
 // A Target is the interface for taking an action for a packet.
diff --git a/pkg/tcpip/iptables/udp_matcher.go b/pkg/tcpip/iptables/udp_matcher.go
index 65ae7f9e0..f59ca2027 100644
--- a/pkg/tcpip/iptables/udp_matcher.go
+++ b/pkg/tcpip/iptables/udp_matcher.go
@@ -16,33 +16,28 @@ package iptables
 
 import (
 	"fmt"
-	"runtime/debug"
 
 	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/tcpip"
 	"gvisor.dev/gvisor/pkg/tcpip/header"
 )
 
+// TODO(gvisor.dev/issue/170): The following per-matcher params should be
+// supported:
+// - Table name
+// - Match size
+// - User size
+// - Hooks
+// - Proto
+// - Family
+
+// UDPMatcher matches UDP packets and their headers. It implements Matcher.
 type UDPMatcher struct {
-	Data UDPMatcherData
-
-	// tablename string
-	// unsigned int matchsize;
-	// unsigned int usersize;
-	// #ifdef CONFIG_COMPAT
-	// unsigned int compatsize;
-	// #endif
-	// unsigned int hooks;
-	// unsigned short proto;
-	// unsigned short family;
+	Data UDPMatcherParams
 }
 
-// TODO: Delete?
-// MatchCheckEntryParams
-
-type UDPMatcherData struct {
-	// Filter IPHeaderFilter
-
+// UDPMatcherParams are the parameters used to create a UDPMatcher.
+type UDPMatcherParams struct {
 	SourcePortStart      uint16
 	SourcePortEnd        uint16
 	DestinationPortStart uint16
@@ -50,12 +45,12 @@ type UDPMatcherData struct {
 	InverseFlags         uint8
 }
 
-func NewUDPMatcher(filter IPHeaderFilter, data UDPMatcherData) (Matcher, error) {
-	// TODO: We currently only support source port and destination port.
-	log.Infof("Adding rule with UDPMatcherData: %+v", data)
+// NewUDPMatcher returns a new instance of UDPMatcher.
+func NewUDPMatcher(filter IPHeaderFilter, data UDPMatcherParams) (Matcher, error) {
+	log.Infof("Adding rule with UDPMatcherParams: %+v", data)
 
 	if data.InverseFlags != 0 {
-		return nil, fmt.Errorf("unsupported UDP matcher flags set")
+		return nil, fmt.Errorf("unsupported UDP matcher inverse flags set")
 	}
 
 	if filter.Protocol != header.UDPProtocolNumber {
@@ -65,21 +60,18 @@ func NewUDPMatcher(filter IPHeaderFilter, data UDPMatcherData) (Matcher, error)
 	return &UDPMatcher{Data: data}, nil
 }
 
-// TODO: Check xt_tcpudp.c. Need to check for same things (e.g. fragments).
+// Match implements Matcher.Match.
 func (um *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName string) (bool, bool) {
-	log.Infof("UDPMatcher called from: %s", string(debug.Stack()))
 	netHeader := header.IPv4(pkt.NetworkHeader)
 
-	// TODO: Do we check proto here or elsewhere? I think elsewhere (check
-	// codesearch).
+	// TODO(gvisor.dev/issue/170): Proto checks should ultimately be moved
+	// into the iptables.Check codepath as matchers are added.
 	if netHeader.TransportProtocol() != header.UDPProtocolNumber {
-		log.Infof("UDPMatcher: wrong protocol number")
 		return false, false
 	}
 
 	// We dont't match fragments.
 	if frag := netHeader.FragmentOffset(); frag != 0 {
-		log.Infof("UDPMatcher: it's a fragment")
 		if frag == 1 {
 			return false, true
 		}
@@ -89,20 +81,18 @@ func (um *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName str
 
 	// Now we need the transport header. However, this may not have been set
 	// yet.
-	// TODO
+	// TODO(gvisor.dev/issue/170): Parsing the transport header should
+	// ultimately be moved into the iptables.Check codepath as matchers are
+	// added.
 	var udpHeader header.UDP
 	if pkt.TransportHeader != nil {
-		log.Infof("UDPMatcher: transport header is not nil")
 		udpHeader = header.UDP(pkt.TransportHeader)
 	} else {
-		log.Infof("UDPMatcher: transport header is nil")
-		log.Infof("UDPMatcher: is network header nil: %t", pkt.NetworkHeader == nil)
 		// The UDP header hasn't been parsed yet. We have to do it here.
 		if len(pkt.Data.First()) < header.UDPMinimumSize {
 			// There's no valid UDP header here, so we hotdrop the
 			// packet.
-			// TODO: Stats.
-			log.Warningf("Dropping UDP packet: size to small.")
+			log.Warningf("Dropping UDP packet: size too small.")
 			return false, true
 		}
 		udpHeader = header.UDP(pkt.Data.First())
@@ -112,10 +102,6 @@ func (um *UDPMatcher) Match(hook Hook, pkt tcpip.PacketBuffer, interfaceName str
 	// matching range.
 	sourcePort := udpHeader.SourcePort()
 	destinationPort := udpHeader.DestinationPort()
-	log.Infof("UDPMatcher: sport and dport are %d and %d. sports and dport start and end are (%d, %d) and (%d, %d)",
-		udpHeader.SourcePort(), udpHeader.DestinationPort(),
-		um.Data.SourcePortStart, um.Data.SourcePortEnd,
-		um.Data.DestinationPortStart, um.Data.DestinationPortEnd)
 	if sourcePort < um.Data.SourcePortStart || um.Data.SourcePortEnd < sourcePort {
 		return false, false
 	}
diff --git a/test/iptables/filter_input.go b/test/iptables/filter_input.go
index bc963d40e..e9f0978eb 100644
--- a/test/iptables/filter_input.go
+++ b/test/iptables/filter_input.go
@@ -264,9 +264,9 @@ func (FilterInputMultiUDPRules) ContainerAction(ip net.IP) error {
 	if err := filterTable("-A", "INPUT", "-p", "udp", "-m", "udp", "--destination-port", fmt.Sprintf("%d", dropPort), "-j", "DROP"); err != nil {
 		return err
 	}
-	// if err := filterTable("-A", "INPUT", "-p", "udp", "-m", "udp", "--destination-port", fmt.Sprintf("%d", acceptPort), "-j", "ACCEPT"); err != nil {
-	// 	return err
-	// }
+	if err := filterTable("-A", "INPUT", "-p", "udp", "-m", "udp", "--destination-port", fmt.Sprintf("%d", acceptPort), "-j", "ACCEPT"); err != nil {
+		return err
+	}
 	return filterTable("-L")
 }
 
-- 
cgit v1.2.3


From d6a2e01d3e57e0837c7e5cfda3b56c4dcfbb4627 Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Mon, 27 Jan 2020 16:40:46 -0800
Subject: Address GH comments.

---
 pkg/sentry/socket/netfilter/netfilter.go | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

(limited to 'pkg/sentry/socket')

diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index b8848f08a..a06562743 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -43,7 +43,7 @@ const (
 // a metadata struct when the tables are written, and when they are read out we
 // verify that certain fields are the same.
 //
-// metadata is opaque to netstack.
+// metadata is used by this serialization/deserializing code, not netstack.
 type metadata struct {
 	HookEntry  [linux.NF_INET_NUMHOOKS]uint32
 	Underflow  [linux.NF_INET_NUMHOOKS]uint32
@@ -51,14 +51,10 @@ type metadata struct {
 	Size       uint32
 }
 
-const enableDebug = false
-
 // nflog logs messages related to the writing and reading of iptables, but only
 // when enableDebug is true.
 func nflog(format string, args ...interface{}) {
-	if enableDebug {
-		log.Infof("netfilter: "+format, args...)
-	}
+	log.Infof("netfilter: "+format, args...)
 }
 
 // GetInfo returns information about iptables.
@@ -233,14 +229,12 @@ func convertNetstackToBinary(tablename string, table iptables.Table) (linux.Kern
 	return entries, meta, nil
 }
 
-// TODO: SOMEHOW THIS IS NOT GETTING APPENDED!
 func marshalMatcher(matcher iptables.Matcher) []byte {
 	switch m := matcher.(type) {
 	case *iptables.UDPMatcher:
 		return marshalUDPMatcher(m)
 	default:
-		// TODO(gvisor.dev/issue/170): We don't support any matchers
-		// yet, so any call to marshalMatcher will panic.
+		// TODO(gvisor.dev/issue/170): Support other matchers.
 		panic(fmt.Errorf("unknown matcher of type %T", matcher))
 	}
 }
@@ -249,11 +243,11 @@ func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
 	nflog("convert to binary: marshalling UDP matcher: %+v", matcher)
 
 	// We have to pad this struct size to a multiple of 8 bytes.
-	const size = linux.SizeOfXTEntryMatch + linux.SizeOfXTUDP + 6
+	size := alignUp(linux.SizeOfXTEntryMatch+linux.SizeOfXTUDP, 8)
 
 	linuxMatcher := linux.KernelXTEntryMatch{
 		XTEntryMatch: linux.XTEntryMatch{
-			MatchSize: size,
+			MatchSize: uint16(size),
 		},
 		Data: make([]byte, 0, linux.SizeOfXTUDP),
 	}
@@ -270,7 +264,7 @@ func marshalUDPMatcher(matcher *iptables.UDPMatcher) []byte {
 
 	buf := make([]byte, 0, size)
 	buf = binary.Marshal(buf, usermem.ByteOrder, linuxMatcher)
-	buf = append(buf, []byte{0, 0, 0, 0, 0, 0}...)
+	buf = append(buf, make([]byte, size-len(buf))...)
 	nflog("convert to binary: marshalled UDP matcher into %v", buf)
 	return buf[:]
 }
@@ -410,7 +404,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		}
 
 		// TODO(gvisor.dev/issue/170): Matchers and targets can specify
-		// that they only work for certiain protocols, hooks, tables.
+		// that they only work for certain protocols, hooks, tables.
 		// Get matchers.
 		matchersSize := entry.TargetOffset - linux.SizeOfIPTEntry
 		if len(optVal) < int(matchersSize) {
@@ -684,3 +678,8 @@ func hookFromLinux(hook int) iptables.Hook {
 	}
 	panic(fmt.Sprintf("Unknown hook %d does not correspond to a builtin chain", hook))
 }
+
+// alignUp rounds a length up to an alignment. align must be a power of 2.
+func alignUp(length int, align uint) int {
+	return (length + int(align) - 1) & ^(int(align) - 1)
+}
-- 
cgit v1.2.3