From 21c757b60f22d9a570ac92b24b27fafede3b0224 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 15 Jun 2022 15:19:33 -0700 Subject: [PATCH] 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 | 2 +- pkg/sentry/seccheck/checkers/null/BUILD | 13 +++ pkg/sentry/seccheck/checkers/null/null.go | 40 ++++++++ pkg/sentry/seccheck/syscall.go | 4 + runsc/boot/BUILD | 1 + runsc/boot/seccheck.go | 1 + test/runner/BUILD | 2 + test/runner/defs.bzl | 4 + test/runner/main.go | 32 +++++++ test/trace/BUILD | 2 +- test/trace/config/BUILD | 14 +++ test/trace/config/config.go | 110 ++++++++++++++++++++++ test/trace/trace_test.go | 101 +++----------------- 13 files changed, 235 insertions(+), 91 deletions(-) create mode 100644 pkg/sentry/seccheck/checkers/null/BUILD create mode 100644 pkg/sentry/seccheck/checkers/null/null.go create mode 100644 test/trace/config/BUILD create mode 100644 test/trace/config/config.go diff --git a/pkg/sentry/kernel/task_syscall.go b/pkg/sentry/kernel/task_syscall.go index edd36e519..48be595f2 100644 --- a/pkg/sentry/kernel/task_syscall.go +++ b/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) diff --git a/pkg/sentry/seccheck/checkers/null/BUILD b/pkg/sentry/seccheck/checkers/null/BUILD new file mode 100644 index 000000000..1136dc5bc --- /dev/null +++ b/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", + ], +) diff --git a/pkg/sentry/seccheck/checkers/null/null.go b/pkg/sentry/seccheck/checkers/null/null.go new file mode 100644 index 000000000..9837952cd --- /dev/null +++ b/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 +} diff --git a/pkg/sentry/seccheck/syscall.go b/pkg/sentry/seccheck/syscall.go index 43d0cce84..4dea53546 100644 --- a/pkg/sentry/seccheck/syscall.go +++ b/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)) } diff --git a/runsc/boot/BUILD b/runsc/boot/BUILD index 5a7782c97..912c2faac 100644 --- a/runsc/boot/BUILD +++ b/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", diff --git a/runsc/boot/seccheck.go b/runsc/boot/seccheck.go index df4a49ff0..5d9363820 100644 --- a/runsc/boot/seccheck.go +++ b/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" ) diff --git a/test/runner/BUILD b/test/runner/BUILD index 7c6bebd3b..084cdbf5a 100644 --- a/test/runner/BUILD +++ b/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", diff --git a/test/runner/defs.bzl b/test/runner/defs.bzl index 7cbae734d..a14818c15 100644 --- a/test/runner/defs.bzl +++ b/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, diff --git a/test/runner/main.go b/test/runner/main.go index 313d9939f..97f8b96a4 100644 --- a/test/runner/main.go +++ b/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 +} diff --git a/test/trace/BUILD b/test/trace/BUILD index bd3564de8..74597ab6d 100644 --- a/test/trace/BUILD +++ b/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", ], ) diff --git a/test/trace/config/BUILD b/test/trace/config/BUILD new file mode 100644 index 000000000..b87bf4b53 --- /dev/null +++ b/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", + ], +) diff --git a/test/trace/config/config.go b/test/trace/config/config.go new file mode 100644 index 000000000..c3f4343d7 --- /dev/null +++ b/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) +} diff --git a/test/trace/trace_test.go b/test/trace/trace_test.go index 76f370270..617c7f0b8 100644 --- a/test/trace/trace_test.go +++ b/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 {