Minor fixes to comments and logging

7 files changed, 32 insertions, 43 deletions

diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go
index 8c7f3c7fc..b7867a576 100644
--- a/pkg/sentry/socket/netfilter/netfilter.go
+++ b/pkg/sentry/socket/netfilter/netfilter.go
@@ -157,9 +157,7 @@ func convertNetstackToBinary(name string, table iptables.Table) (linux.KernelIPT
 				meta.HookEntry[hook] = entries.Size
 			}
 		}
-		// Is this a chain underflow point? The underflow rule is the last rule
-		// in the chain, and is an unconditional rule (i.e. it matches any
-		// packet). This is enforced when saving iptables.
+		// Is this a chain underflow point?
 		for underflow, underflowRuleIdx := range table.Underflows {
 			if underflowRuleIdx == ruleIdx {
 				meta.Underflow[underflow] = entries.Size
@@ -290,6 +288,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 
 	// Get the basic rules data (struct ipt_replace).
 	if len(optVal) < linux.SizeOfIPTReplace {
+		log.Infof("netfilter.SetEntries: optVal has insufficient size for replace %d", len(optVal))
 		return syserr.ErrInvalidArgument
 	}
 	var replace linux.IPTReplace
@@ -313,6 +312,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 	for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ {
 		// Get the struct ipt_entry.
 		if len(optVal) < linux.SizeOfIPTEntry {
+			log.Infof("netfilter: optVal has insufficient size for entry %d", len(optVal))
 			return syserr.ErrInvalidArgument
 		}
 		var entry linux.IPTEntry
@@ -328,6 +328,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 		// filtering. We reject any nonzero IPTIP values for now.
 		emptyIPTIP := linux.IPTIP{}
 		if entry.IP != emptyIPTIP {
+			log.Infof("netfilter: non-empty struct iptip found")
 			return syserr.ErrInvalidArgument
 		}
 
@@ -386,6 +387,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error {
 // along with the number of bytes it occupies in optVal.
 func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 	if len(optVal) < linux.SizeOfXTEntryTarget {
+		log.Infof("netfilter: optVal has insufficient size for entry target %d", len(optVal))
 		return nil, 0, syserr.ErrInvalidArgument
 	}
 	var target linux.XTEntryTarget
@@ -395,6 +397,7 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 	case "":
 		// Standard target.
 		if len(optVal) < linux.SizeOfXTStandardTarget {
+			log.Infof("netfilter.SetEntries: optVal has insufficient size for standard target %d", len(optVal))
 			return nil, 0, syserr.ErrInvalidArgument
 		}
 		var target linux.XTStandardTarget
@@ -420,6 +423,7 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) {
 	case errorTargetName:
 		// Error target.
 		if len(optVal) < linux.SizeOfXTErrorTarget {
+			log.Infof("netfilter.SetEntries: optVal has insufficient size for error target %d", len(optVal))
 			return nil, 0, syserr.ErrInvalidArgument
 		}
 		var target linux.XTErrorTarget
diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go
index f7caa45b4..8c07eef4b 100644
--- a/pkg/sentry/socket/netstack/netstack.go
+++ b/pkg/sentry/socket/netstack/netstack.go
@@ -326,7 +326,7 @@ func AddressAndFamily(sfamily int, addr []byte, strict bool) (tcpip.FullAddress,
 	}
 
 	family := usermem.ByteOrder.Uint16(addr)
-	if family != uint16(sfamily) && (!strict && family != linux.AF_UNSPEC) {
+	if family != uint16(sfamily) && (strict || family != linux.AF_UNSPEC) {
 		return tcpip.FullAddress{}, family, syserr.ErrAddressFamilyNotSupported
 	}
 
@@ -1357,7 +1357,8 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa
 	}
 
 	if s.skType == linux.SOCK_RAW && level == linux.IPPROTO_IP {
-		if name == linux.IPT_SO_SET_REPLACE {
+		switch name {
+		case linux.IPT_SO_SET_REPLACE:
 			if len(optVal) < linux.SizeOfIPTReplace {
 				return syserr.ErrInvalidArgument
 			}
@@ -1371,7 +1372,8 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa
 				return err
 			}
 			return nil
-		} else if name == linux.IPT_SO_SET_ADD_COUNTERS {
+
+		case linux.IPT_SO_SET_ADD_COUNTERS:
 			// TODO(gvisor.dev/issue/170): Counter support.
 			return nil
 		}
diff --git a/pkg/sentry/syscalls/linux/sys_socket.go b/pkg/sentry/syscalls/linux/sys_socket.go
index 4b5aafcc0..cda517a81 100644
--- a/pkg/sentry/syscalls/linux/sys_socket.go
+++ b/pkg/sentry/syscalls/linux/sys_socket.go
@@ -41,7 +41,7 @@ const maxListenBacklog = 1024
 const maxAddrLen = 200
 
 // maxOptLen is the maximum sockopt parameter length we're willing to accept.
-const maxOptLen = 1024
+const maxOptLen = 1024 * 8
 
 // maxControlLen is the maximum length of the msghdr.msg_control buffer we're
 // willing to accept. Note that this limit is smaller than Linux, which allows
diff --git a/pkg/tcpip/iptables/targets.go b/pkg/tcpip/iptables/targets.go
index 03c9f19ff..2c3598e3d 100644
--- a/pkg/tcpip/iptables/targets.go
+++ b/pkg/tcpip/iptables/targets.go
@@ -34,7 +34,7 @@ func (UnconditionalDropTarget) Action(packet buffer.VectorisedView) (Verdict, st
 	return Drop, ""
 }
 
-// PanicTarget just panics.
+// PanicTarget just panics. It represents a target that should be unreachable.
 type PanicTarget struct{}
 
 // Actions implements Target.Action.
diff --git a/pkg/tcpip/iptables/types.go b/pkg/tcpip/iptables/types.go
index 76364ff1f..fe0394a31 100644
--- a/pkg/tcpip/iptables/types.go
+++ b/pkg/tcpip/iptables/types.go
@@ -107,20 +107,19 @@ type IPTables struct {
 	Priorities map[Hook][]string
 }
 
-// A Table defines a set of chains and hooks into the network stack. The
-// currently supported tables are:
-//   * nat
-//   * mangle
+// A Table defines a set of chains and hooks into the network stack. It is
+// really just a list of rules with some metadata for entrypoints and such.
 type Table struct {
-	// A table is just a list of rules with some entrypoints.
+	// Rules holds the rules that make up the table.
 	Rules []Rule
 
+	// BuiltinChains maps builtin chains to their entrypoints.
 	BuiltinChains map[Hook]int
 
+	// Underflows maps builtin chains to their underflow point (i.e. the
+	// rule to execute if the chain returns without a verdict).
 	Underflows map[Hook]int
 
-	// DefaultTargets map[Hook]int
-
 	// UserChains holds user-defined chains for the keyed by name. Users
 	// can give their chains arbitrary names.
 	UserChains map[string]int
@@ -149,21 +148,6 @@ func (table *Table) SetMetadata(metadata interface{}) {
 	table.metadata = metadata
 }
 
-//// A Chain defines a list of rules for packet processing. When a packet
-//// traverses a chain, it is checked against each rule until either a rule
-//// returns a verdict or the chain ends.
-////
-//// By convention, builtin chains end with a rule that matches everything and
-//// returns either Accept or Drop. User-defined chains end with Return. These
-//// aren't strictly necessary here, but the iptables tool writes tables this way.
-//type Chain struct {
-//	// Name is the chain name.
-//	Name string
-
-//	// Rules is the list of rules to traverse.
-//	Rules []Rule
-//}
-
 // A Rule is a packet processing rule. It consists of two pieces. First it
 // contains zero or more matchers, each of which is a specification of which
 // packets this rule applies to. If there are no matchers in the rule, it
diff --git a/test/iptables/filter_input.go b/test/iptables/filter_input.go
index 0cb668635..34a85db97 100644
--- a/test/iptables/filter_input.go
+++ b/test/iptables/filter_input.go
@@ -43,8 +43,7 @@ func (FilterInputDropUDP) Name() string {
 
 // ContainerAction implements TestCase.ContainerAction.
 func (FilterInputDropUDP) ContainerAction(ip net.IP) error {
-	if err := filterTable("-A", "INPUT", "-p", "udp", "-j", "DROP"); err != nil {
-		// if err := filterTable("-A", "INPUT", "-j", "ACCEPT"); err != nil {
+	if err := filterTable("-A", "INPUT", "-j", "ACCEPT"); err != nil {
 		return err
 	}
 
diff --git a/test/iptables/iptables_test.go b/test/iptables/iptables_test.go
index e761e0f2f..2465a4e65 100644
--- a/test/iptables/iptables_test.go
+++ b/test/iptables/iptables_test.go
@@ -167,14 +167,14 @@ func TestFilterInputDropUDP(t *testing.T) {
 	}
 }
 
-// func TestFilterInputDropUDPPort(t *testing.T) {
-// 	if err := singleTest(FilterInputDropUDPPort{}); err != nil {
-// 		t.Fatal(err)
-// 	}
-// }
-
-// func TestFilterInputDropDifferentUDPPort(t *testing.T) {
-// 	if err := singleTest(FilterInputDropDifferentUDPPort{}); err != nil {
-// 		t.Fatal(err)
-// 	}
-// }
+func TestFilterInputDropUDPPort(t *testing.T) {
+	if err := singleTest(FilterInputDropUDPPort{}); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestFilterInputDropDifferentUDPPort(t *testing.T) {
+	if err := singleTest(FilterInputDropDifferentUDPPort{}); err != nil {
+		t.Fatal(err)
+	}
+}