From 548d12773965831811b2fe719df0c8c0da4e8a61 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 27 Apr 2022 18:01:40 -0700 Subject: [PATCH] Refactor code to use seccheck.SendToCheckers Updates #4805 PiperOrigin-RevId: 445017536 --- pkg/sentry/kernel/task_clone.go | 4 +++- pkg/sentry/kernel/task_exec.go | 4 +++- pkg/sentry/kernel/task_exit.go | 7 +++++- pkg/sentry/seccheck/BUILD | 3 --- pkg/sentry/seccheck/clone.go | 30 ----------------------- pkg/sentry/seccheck/execve.go | 36 ---------------------------- pkg/sentry/seccheck/exit.go | 35 --------------------------- pkg/sentry/seccheck/metadata.go | 6 +++++ pkg/sentry/seccheck/seccheck_test.go | 12 +++++++--- 9 files changed, 27 insertions(+), 110 deletions(-) delete mode 100644 pkg/sentry/seccheck/clone.go delete mode 100644 pkg/sentry/seccheck/execve.go delete mode 100644 pkg/sentry/seccheck/exit.go diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go index 9d8188744..fbf8a26bd 100644 --- a/pkg/sentry/kernel/task_clone.go +++ b/pkg/sentry/kernel/task_clone.go @@ -248,7 +248,9 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) { if seccheck.Global.Enabled(seccheck.PointClone) { mask, info := getCloneSeccheckInfo(t, nt) - if err := seccheck.Global.Clone(t, mask, info); err != nil { + if err := seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { + return c.Clone(t, mask, info) + }); err != nil { // nt has been visible to the rest of the system since NewTask, so // it may be blocking execve or a group stop, have been notified // for group signal delivery, had children reparented to it, etc. diff --git a/pkg/sentry/kernel/task_exec.go b/pkg/sentry/kernel/task_exec.go index 0d74c2f21..c91f30597 100644 --- a/pkg/sentry/kernel/task_exec.go +++ b/pkg/sentry/kernel/task_exec.go @@ -97,7 +97,9 @@ func (t *Task) Execve(newImage *TaskImage, argv, env []string, executable fsbrid // We can't clearly hold kernel package locks while stat'ing executable. if seccheck.Global.Enabled(seccheck.PointExecve) { mask, info := getExecveSeccheckInfo(t, argv, env, executable, pathname) - if err := seccheck.Global.Execve(t, mask, info); err != nil { + if err := seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { + return c.Execve(t, mask, info) + }); err != nil { newImage.release() return nil, err } diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go index c827914b5..08ff782be 100644 --- a/pkg/sentry/kernel/task_exit.go +++ b/pkg/sentry/kernel/task_exit.go @@ -31,6 +31,7 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/errors/linuxerr" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/seccheck" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" @@ -647,7 +648,11 @@ func (t *Task) exitNotifyLocked(fromPtraceDetach bool) { } if seccheck.Global.Enabled(seccheck.PointExitNotifyParent) { mask, info := getExitNotifyParentSeccheckInfo(t) - seccheck.Global.ExitNotifyParent(t, mask, info) + if err := seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { + return c.ExitNotifyParent(t, mask, info) + }); err != nil { + log.Infof("Ignoring error from ExitNotifyParent point: %v", err) + } } } } diff --git a/pkg/sentry/seccheck/BUILD b/pkg/sentry/seccheck/BUILD index 49742f466..972215062 100644 --- a/pkg/sentry/seccheck/BUILD +++ b/pkg/sentry/seccheck/BUILD @@ -17,10 +17,7 @@ go_template_instance( go_library( name = "seccheck", srcs = [ - "clone.go", "config.go", - "execve.go", - "exit.go", "metadata.go", "seccheck.go", "seqatomic_checkerslice_unsafe.go", diff --git a/pkg/sentry/seccheck/clone.go b/pkg/sentry/seccheck/clone.go deleted file mode 100644 index 9613e2c6f..000000000 --- a/pkg/sentry/seccheck/clone.go +++ /dev/null @@ -1,30 +0,0 @@ -// 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 seccheck - -import ( - "gvisor.dev/gvisor/pkg/context" - pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" -) - -// Clone is called at the Clone checkpoint. -func (s *State) Clone(ctx context.Context, fields FieldSet, info *pb.CloneInfo) error { - for _, c := range s.getCheckers() { - if err := c.Clone(ctx, fields, info); err != nil { - return err - } - } - return nil -} diff --git a/pkg/sentry/seccheck/execve.go b/pkg/sentry/seccheck/execve.go deleted file mode 100644 index 83a40db59..000000000 --- a/pkg/sentry/seccheck/execve.go +++ /dev/null @@ -1,36 +0,0 @@ -// 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 seccheck - -import ( - "gvisor.dev/gvisor/pkg/context" - pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" -) - -const ( - // ExecveFieldBinaryInfo is an optional field to collect information about the - // binary being executed. - ExecveFieldBinaryInfo Field = iota -) - -// Execve is called at the Execve checkpoint. -func (s *State) Execve(ctx context.Context, mask FieldSet, info *pb.ExecveInfo) error { - for _, c := range s.getCheckers() { - if err := c.Execve(ctx, mask, info); err != nil { - return err - } - } - return nil -} diff --git a/pkg/sentry/seccheck/exit.go b/pkg/sentry/seccheck/exit.go deleted file mode 100644 index 7f28ce2ce..000000000 --- a/pkg/sentry/seccheck/exit.go +++ /dev/null @@ -1,35 +0,0 @@ -// 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 seccheck - -import ( - "gvisor.dev/gvisor/pkg/context" - pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" -) - -// ExitNotifyParent is called at the ExitNotifyParent checkpoint. -// -// The ExitNotifyParent checkpoint occurs when a zombied thread group leader, -// not waiting for exit acknowledgement from a non-parent ptracer, becomes the -// last non-dead thread in its thread group and notifies its parent of its -// exiting. -func (s *State) ExitNotifyParent(ctx context.Context, fields FieldSet, info *pb.ExitNotifyParentInfo) error { - for _, c := range s.getCheckers() { - if err := c.ExitNotifyParent(ctx, fields, info); err != nil { - return err - } - } - return nil -} diff --git a/pkg/sentry/seccheck/metadata.go b/pkg/sentry/seccheck/metadata.go index 868835df0..a6c63cb54 100644 --- a/pkg/sentry/seccheck/metadata.go +++ b/pkg/sentry/seccheck/metadata.go @@ -27,6 +27,12 @@ const ( ContainerStartFieldEnv Field = iota ) +const ( + // ExecveFieldBinaryInfo is an optional field to collect information about the + // binary being executed. + ExecveFieldBinaryInfo Field = iota +) + var points = map[string]PointDesc{} var sinks = map[string]SinkDesc{} diff --git a/pkg/sentry/seccheck/seccheck_test.go b/pkg/sentry/seccheck/seccheck_test.go index 674509f31..109fbfb2e 100644 --- a/pkg/sentry/seccheck/seccheck_test.go +++ b/pkg/sentry/seccheck/seccheck_test.go @@ -75,7 +75,9 @@ func TestCheckerRegistered(t *testing.T) { if !fields.Context.Contains(FieldCtxtCredentials) { t.Errorf("fields.Context.Contains(PointContextCredentials): got false, wanted true") } - if err := s.Clone(context.Background(), fields, &pb.CloneInfo{}); err != nil { + if err := s.SendToCheckers(func(c Checker) error { + return c.Clone(context.Background(), fields, &pb.CloneInfo{}) + }); err != nil { t.Errorf("Clone(): got %v, wanted nil", err) } if !checkerCalled { @@ -112,7 +114,9 @@ func TestMultipleCheckersRegistered(t *testing.T) { // CloneReq() should return the union of requested fields from all calls to // AppendChecker. fields := s.GetFieldSet(PointClone) - if err := s.Clone(context.Background(), fields, &pb.CloneInfo{}); err != nil { + if err := s.SendToCheckers(func(c Checker) error { + return c.Clone(context.Background(), fields, &pb.CloneInfo{}) + }); err != nil { t.Errorf("Clone(): got %v, wanted nil", err) } for i := range checkersCalled { @@ -151,7 +155,9 @@ func TestCheckpointReturnsFirstCheckerError(t *testing.T) { if !s.Enabled(PointClone) { t.Errorf("Enabled(PointClone): got false, wanted true") } - if err := s.Clone(context.Background(), FieldSet{}, &pb.CloneInfo{}); err != errFirstChecker { + if err := s.SendToCheckers(func(c Checker) error { + return c.Clone(context.Background(), FieldSet{}, &pb.CloneInfo{}) + }); err != errFirstChecker { t.Errorf("Clone(): got %v, wanted %v", err, errFirstChecker) } if !checkersCalled[0] {