Enable all trace points for syscall tests

Enable all trace points while running syscall tests to catch possible
crashes and bugs that may exist. These are not verifying that the data
in the points are correct though. Since all trace points are platform
agnostic for now, only enable them for ptrace.

Updates #4805

PiperOrigin-RevId: 455232093

pkg/sentry/kernel/task_syscall.go

@@ -182,7 +182,6 @@ func (t *Task) executeSyscall(sysno uintptr, args arch.SyscallArguments) (rval u
 		})
 	}
 	if seccheck.Global.SyscallEnabled(seccheck.SyscallExit, sysno) {
-		cb := t.SyscallTable().LookupSyscallToProto(sysno)
 		fields := seccheck.Global.GetFieldSet(seccheck.GetPointForSyscall(seccheck.SyscallExit, sysno))
 		var ctxData *pb.ContextData
 		if !fields.Context.Empty() {
@@ -196,6 +195,7 @@ func (t *Task) executeSyscall(sysno uintptr, args arch.SyscallArguments) (rval u
 			Rval:  rval,
 			Errno: ExtractErrno(err, int(sysno)),
 		}
+		cb := t.SyscallTable().LookupSyscallToProto(sysno)
 		msg, msgType := cb(t, fields, ctxData, info)
 		seccheck.Global.SendToCheckers(func(c seccheck.Checker) error {
 			return c.Syscall(t, fields, ctxData, msgType, msg)

pkg/sentry/seccheck/checkers/null/BUILD

@@ -0,0 +1,13 @@
+load("//tools:defs.bzl", "go_library")
+
+package(licenses = ["notice"])
+
+go_library(
+    name = "null",
+    srcs = ["null.go"],
+    visibility = ["//:sandbox"],
+    deps = [
+        "//pkg/fd",
+        "//pkg/sentry/seccheck",
+    ],
+)

pkg/sentry/seccheck/checkers/null/null.go

@@ -0,0 +1,40 @@
+// Copyright 2021 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 null defines a seccheck.Checker that does nothing with the trace
+// points, akin to /dev/null.
+package null
+
+import (
+	"gvisor.dev/gvisor/pkg/fd"
+	"gvisor.dev/gvisor/pkg/sentry/seccheck"
+)
+
+func init() {
+	seccheck.RegisterSink(seccheck.SinkDesc{
+		Name: "null",
+		New:  new,
+	})
+}
+
+// null is a checker that does nothing with the trace points.
+type null struct {
+	seccheck.CheckerDefaults
+}
+
+var _ seccheck.Checker = (*null)(nil)
+
+func new(_ map[string]interface{}, _ *fd.FD) (seccheck.Checker, error) {
+	return &null{}, nil
+}

pkg/sentry/seccheck/syscall.go

@@ -63,5 +63,9 @@ func GetPointForSyscall(typ SyscallType, sysno uintptr) Point {
 
 // SyscallEnabled checks if the corresponding point for the syscall is enabled.
 func (s *State) SyscallEnabled(typ SyscallType, sysno uintptr) bool {
+	// Prevent overflow.
+	if sysno >= syscallsMax {
+		return false
+	}
 	return s.Enabled(GetPointForSyscall(typ, sysno))
 }

runsc/boot/BUILD

@@ -73,6 +73,7 @@ go_library(
         "//pkg/sentry/pgalloc",
         "//pkg/sentry/platform",
         "//pkg/sentry/seccheck",
+        "//pkg/sentry/seccheck/checkers/null",
         "//pkg/sentry/seccheck/checkers/remote",
         "//pkg/sentry/seccheck/points:points_go_proto",
         "//pkg/sentry/socket/hostinet",

runsc/boot/seccheck.go

@@ -23,6 +23,7 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/seccheck"
 
 	// Register supported of checkers.
+	_ "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/null"
 	_ "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote"
 )
 

test/runner/BUILD

@@ -13,9 +13,11 @@ go_binary(
     visibility = ["//:sandbox"],
     deps = [
         "//pkg/log",
+        "//pkg/sentry/seccheck",
         "//pkg/test/testutil",
         "//runsc/specutils",
         "//test/runner/gtest",
+        "//test/trace/config",
         "//test/uds",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@com_github_syndtr_gocapability//capability:go_default_library",

test/runner/defs.bzl

@@ -145,6 +145,10 @@ def _syscall_test(
         "--container=" + str(container),
     ]
 
+    # Trace points are platform agnostic, so enable them for ptrace only.
+    if platform == "ptrace":
+        runner_args.append("--trace")
+
     # Call the rule above.
     _runner_test(
         name = name,

test/runner/main.go

@@ -33,9 +33,11 @@ import (
 	"github.com/syndtr/gocapability/capability"
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/log"
+	"gvisor.dev/gvisor/pkg/sentry/seccheck"
 	"gvisor.dev/gvisor/pkg/test/testutil"
 	"gvisor.dev/gvisor/runsc/specutils"
 	"gvisor.dev/gvisor/test/runner/gtest"
+	"gvisor.dev/gvisor/test/trace/config"
 	"gvisor.dev/gvisor/test/uds"
 )
 
@@ -52,6 +54,7 @@ var (
 	lisafs             = flag.Bool("lisafs", false, "enable lisafs protocol if vfs2 is also enabled")
 	container          = flag.Bool("container", false, "run tests in their own namespaces (user ns, network ns, etc), pretending to be root. Implicitly enabled if network=host, or if using network namespaces")
 	setupContainerPath = flag.String("setup-container", "", "path to setup_container binary (for use with --container)")
+	trace              = flag.Bool("trace", false, "enables all trace points")
 
 	addUDSTree = flag.Bool("add-uds-tree", false, "expose a tree of UDS utilities for use in tests")
 	// TODO(gvisor.dev/issue/4572): properly support leak checking for runsc, and
@@ -217,6 +220,14 @@ func runRunsc(tc gtest.TestCase, spec *specs.Spec) error {
 	if *leakCheck {
 		args = append(args, "-ref-leak-mode=log-names")
 	}
+	if *trace {
+		flag, err := enableAllTraces(rootDir)
+		if err != nil {
+			return fmt.Errorf("enabling all traces: %w", err)
+		}
+		log.Infof("Enabling all trace points: %s", flag)
+		args = append(args, flag)
+	}
 
 	testLogDir := ""
 	if undeclaredOutputsDir, ok := unix.Getenv("TEST_UNDECLARED_OUTPUTS_DIR"); ok {
@@ -562,3 +573,24 @@ func main() {
 
 	testing.Main(matchString, tests, nil, nil)
 }
+
+func enableAllTraces(dir string) (string, error) {
+	builder := config.Builder{}
+	if err := builder.LoadAllPoints(specutils.ExePath); err != nil {
+		return "", err
+	}
+	builder.AddSink(seccheck.SinkConfig{
+		Name: "null",
+	})
+	path := filepath.Join(dir, "pod_init.json")
+	cfgFile, err := os.Create(path)
+	if err != nil {
+		return "", err
+	}
+	defer cfgFile.Close()
+
+	if err := builder.WriteInitConfig(cfgFile); err != nil {
+		return "", fmt.Errorf("writing config file: %w", err)
+	}
+	return "--pod-init-config=" + path, nil
+}

test/trace/BUILD

@@ -20,7 +20,7 @@ go_test(
         "//pkg/sentry/seccheck/checkers/remote/test",
         "//pkg/sentry/seccheck/points:points_go_proto",
         "//pkg/test/testutil",
-        "//runsc/boot",
+        "//test/trace/config",
         "@org_golang_google_protobuf//proto:go_default_library",
     ],
 )

test/trace/config/BUILD

@@ -0,0 +1,14 @@
+load("//tools:defs.bzl", "go_library")
+
+package(licenses = ["notice"])
+
+go_library(
+    name = "config",
+    testonly = 1,
+    srcs = ["config.go"],
+    visibility = ["//:sandbox"],
+    deps = [
+        "//pkg/sentry/seccheck",
+        "//runsc/boot",
+    ],
+)

test/trace/config/config.go

@@ -0,0 +1,110 @@
+// Copyright 2022 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 config providides helper functions to configure trace sessions.
+package config
+
+import (
+	"bufio"
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	"os/exec"
+	"strings"
+
+	"gvisor.dev/gvisor/pkg/sentry/seccheck"
+	"gvisor.dev/gvisor/runsc/boot"
+)
+
+// Builder helps with building of trace session configuration.
+type Builder struct {
+	points []seccheck.PointConfig
+	sinks  []seccheck.SinkConfig
+}
+
+// WriteInitConfig writes the current configuration in a format compatible with
+// the flag --pod-init-config.
+func (b *Builder) WriteInitConfig(w io.Writer) error {
+	init := &boot.InitConfig{
+		TraceSession: seccheck.SessionConfig{
+			Name:   seccheck.DefaultSessionName,
+			Points: b.points,
+			Sinks:  b.sinks,
+		},
+	}
+
+	encoder := json.NewEncoder(w)
+	return encoder.Encode(&init)
+}
+
+// LoadAllPoints enables all points together with all optional and context
+// fields.
+func (b *Builder) LoadAllPoints(runscPath string) error {
+	cmd := exec.Command(runscPath, "trace", "metadata")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		return err
+	}
+
+	// The command above produces an output like the following:
+	//   POINTS (907)
+	//   Name: container/start, optional fields: [], context fields: [time|thread_id]
+	scanner := bufio.NewScanner(bytes.NewReader(out))
+	if !scanner.Scan() {
+		return fmt.Errorf("%q returned empty", cmd)
+	}
+	if !scanner.Scan() {
+		return fmt.Errorf("%q returned empty", cmd)
+	}
+	for line := scanner.Text(); scanner.Scan(); line = scanner.Text() {
+		elems := strings.Split(line, ",")
+		if len(elems) != 3 {
+			return fmt.Errorf("invalid line: %q", line)
+		}
+		name := strings.TrimPrefix(elems[0], "Name: ")
+		optFields, err := parseFields(elems[1], "optional fields: ")
+		if err != nil {
+			return err
+		}
+		ctxFields, err := parseFields(elems[2], "context fields: ")
+		if err != nil {
+			return err
+		}
+		b.points = append(b.points, seccheck.PointConfig{
+			Name:           name,
+			OptionalFields: optFields,
+			ContextFields:  ctxFields,
+		})
+	}
+	return scanner.Err()
+}
+
+func parseFields(elem, prefix string) ([]string, error) {
+	stripped := strings.TrimPrefix(strings.TrimSpace(elem), prefix)
+	switch {
+	case len(stripped) < 2:
+		return nil, fmt.Errorf("invalid %s format: %q", prefix, elem)
+	case len(stripped) == 2:
+		return nil, nil
+	}
+	// Remove [] from `stripped`.
+	clean := stripped[1 : len(stripped)-1]
+	return strings.Split(clean, "|"), nil
+}
+
+// AddSink adds the sink to the configuration.
+func (b *Builder) AddSink(sink seccheck.SinkConfig) {
+	b.sinks = append(b.sinks, sink)
+}

test/trace/trace_test.go

@@ -16,11 +16,7 @@
 package trace
 
 import (
-	"bufio"
-	"bytes"
-	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"strings"
@@ -32,7 +28,7 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/test"
 	pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto"
 	"gvisor.dev/gvisor/pkg/test/testutil"
-	"gvisor.dev/gvisor/runsc/boot"
+	"gvisor.dev/gvisor/test/trace/config"
 )
 
 // TestAll enabled all trace points in the system with all optional and context
@@ -48,19 +44,24 @@ func TestAll(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	cfg, err := buildPodConfig(runsc, server.Endpoint)
-	if err != nil {
+	builder := config.Builder{}
+	if err := builder.LoadAllPoints(runsc); err != nil {
 		t.Fatal(err)
 	}
+	builder.AddSink(seccheck.SinkConfig{
+		Name: "remote",
+		Config: map[string]interface{}{
+			"endpoint": server.Endpoint,
+		},
+	})
 
-	cfgFile, err := ioutil.TempFile(testutil.TmpDir(), "config")
+	cfgFile, err := os.CreateTemp(testutil.TmpDir(), "config")
 	if err != nil {
 		t.Fatalf("error creating tmp file: %v", err)
 	}
 	defer cfgFile.Close()
-	encoder := json.NewEncoder(cfgFile)
-	if err := encoder.Encode(&cfg); err != nil {
-		t.Fatalf("JSON encode: %v", err)
+	if err := builder.WriteInitConfig(cfgFile); err != nil {
+		t.Fatalf("writing config file: %v", err)
 	}
 
 	workload, err := testutil.FindFile("test/trace/workload/workload")
@@ -84,84 +85,6 @@ func TestAll(t *testing.T) {
 	matchPoints(t, server.GetPoints())
 }
 
-func buildPodConfig(runscPath, endpoint string) (*boot.InitConfig, error) {
-	pts, err := allPoints(runscPath)
-	if err != nil {
-		return nil, err
-	}
-	return &boot.InitConfig{
-		TraceSession: seccheck.SessionConfig{
-			Name:   seccheck.DefaultSessionName,
-			Points: pts,
-			Sinks: []seccheck.SinkConfig{
-				{
-					Name: "remote",
-					Config: map[string]interface{}{
-						"endpoint": endpoint,
-					},
-				},
-			},
-		},
-	}, nil
-}
-
-func allPoints(runscPath string) ([]seccheck.PointConfig, error) {
-	cmd := exec.Command(runscPath, "trace", "metadata")
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		return nil, err
-	}
-
-	// The command above produces an output like the following:
-	//   POINTS (907)
-	//   Name: container/start, optional fields: [], context fields: [time|thread_id]
-	scanner := bufio.NewScanner(bytes.NewReader(out))
-	if !scanner.Scan() {
-		return nil, fmt.Errorf("%q returned empty", cmd)
-	}
-	if !scanner.Scan() {
-		return nil, fmt.Errorf("%q returned empty", cmd)
-	}
-	var points []seccheck.PointConfig
-	for line := scanner.Text(); scanner.Scan(); line = scanner.Text() {
-		elems := strings.Split(line, ",")
-		if len(elems) != 3 {
-			return nil, fmt.Errorf("invalid line: %q", line)
-		}
-		name := strings.TrimPrefix(elems[0], "Name: ")
-		optFields, err := parseFields(elems[1], "optional fields: ")
-		if err != nil {
-			return nil, err
-		}
-		ctxFields, err := parseFields(elems[2], "context fields: ")
-		if err != nil {
-			return nil, err
-		}
-		points = append(points, seccheck.PointConfig{
-			Name:           name,
-			OptionalFields: optFields,
-			ContextFields:  ctxFields,
-		})
-	}
-	if scanner.Err() != nil {
-		return nil, scanner.Err()
-	}
-	return points, nil
-}
-
-func parseFields(elem, prefix string) ([]string, error) {
-	stripped := strings.TrimPrefix(strings.TrimSpace(elem), prefix)
-	switch {
-	case len(stripped) < 2:
-		return nil, fmt.Errorf("invalid %s format: %q", prefix, elem)
-	case len(stripped) == 2:
-		return nil, nil
-	}
-	// Remove [] from `stripped`.
-	clean := stripped[1 : len(stripped)-1]
-	return strings.Split(clean, "|"), nil
-}
-
 func matchPoints(t *testing.T, msgs []test.Message) {
 	// Register functions that verify each available point.
 	matchers := map[pb.MessageType]*struct {