From cc1f5503f1fe220ed813875d1305003e04530051 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 27 Aug 2024 13:41:38 -0700 Subject: [PATCH] runsc: always run the sandbox process in a new pid namespace The sandbox process was executed in the current pid namespace if a target platform used ptrace. It was the workaround for the kernel issue that was fixed by 8fb335e07837 ("kernel/exit.c: release ptraced tasks before zap_pid_ns_processes"). This fix was back-ported to stable branches. PiperOrigin-RevId: 668121544 --- pkg/sentry/platform/platform.go | 3 --- pkg/sentry/platform/ptrace/ptrace.go | 3 --- pkg/sentry/platform/systrap/systrap.go | 3 --- runsc/cmd/boot.go | 6 +----- runsc/cmd/chroot.go | 14 ++++---------- runsc/sandbox/sandbox.go | 11 +---------- 6 files changed, 6 insertions(+), 34 deletions(-) diff --git a/pkg/sentry/platform/platform.go b/pkg/sentry/platform/platform.go index c00af4d58..7d58bcdee 100644 --- a/pkg/sentry/platform/platform.go +++ b/pkg/sentry/platform/platform.go @@ -455,9 +455,6 @@ func (f SegmentationFault) Error() string { // Requirements is used to specify platform specific requirements. type Requirements struct { - // RequiresCurrentPIDNS indicates that the sandbox has to be started in the - // current pid namespace. - RequiresCurrentPIDNS bool // RequiresCapSysPtrace indicates that the sandbox has to be started with // the CAP_SYS_PTRACE capability. RequiresCapSysPtrace bool diff --git a/pkg/sentry/platform/ptrace/ptrace.go b/pkg/sentry/platform/ptrace/ptrace.go index b081fff61..48d7793ac 100644 --- a/pkg/sentry/platform/ptrace/ptrace.go +++ b/pkg/sentry/platform/ptrace/ptrace.go @@ -271,11 +271,8 @@ func (*constructor) OpenDevice(_ string) (*fd.FD, error) { // Flags implements platform.Constructor.Flags(). func (*constructor) Requirements() platform.Requirements { - // TODO(b/75837838): Also set a new PID namespace so that we limit - // access to other host processes. return platform.Requirements{ RequiresCapSysPtrace: true, - RequiresCurrentPIDNS: true, } } diff --git a/pkg/sentry/platform/systrap/systrap.go b/pkg/sentry/platform/systrap/systrap.go index c9570d017..b573fb112 100644 --- a/pkg/sentry/platform/systrap/systrap.go +++ b/pkg/sentry/platform/systrap/systrap.go @@ -410,11 +410,8 @@ func (*constructor) OpenDevice(_ string) (*fd.FD, error) { // Requirements implements platform.Constructor.Requirements(). func (*constructor) Requirements() platform.Requirements { - // TODO(b/75837838): Also set a new PID namespace so that we limit - // access to other host processes. return platform.Requirements{ RequiresCapSysPtrace: true, - RequiresCurrentPIDNS: true, } } diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go index 2d4716c7c..49ccfc46e 100644 --- a/runsc/cmd/boot.go +++ b/runsc/cmd/boot.go @@ -141,9 +141,6 @@ type Boot struct { saveFDs intFlags - // pidns is set if the sandbox is in its own pid namespace. - pidns bool - // attached is set to true to kill the sandbox process when the parent process // terminates. This flag is set when the command execve's itself because // parent death signal doesn't propagate through execve when uid/gid changes. @@ -199,7 +196,6 @@ func (b *Boot) SetFlags(f *flag.FlagSet) { f.StringVar(&b.bundleDir, "bundle", "", "required path to the root of the bundle directory") f.BoolVar(&b.applyCaps, "apply-caps", false, "if true, apply capabilities defined in the spec to the process") f.BoolVar(&b.setUpRoot, "setup-root", false, "if true, set up an empty root for the process") - f.BoolVar(&b.pidns, "pidns", false, "if true, the sandbox is in its own PID namespace") f.IntVar(&b.cpuNum, "cpu-num", 0, "number of CPUs to create inside the sandbox") f.IntVar(&b.procMountSyncFD, "proc-mount-sync-fd", -1, "file descriptor that has to be written to when /proc isn't needed anymore and can be unmounted") f.IntVar(&b.syncUsernsFD, "sync-userns-fd", -1, "file descriptor used to synchronize rootless user namespace initialization.") @@ -300,7 +296,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...any) subcomma } if b.setUpRoot { - if err := setUpChroot(b.pidns, spec, conf); err != nil { + if err := setUpChroot(spec, conf); err != nil { util.Fatalf("error setting up chroot: %v", err) } argOverride["setup-root"] = "false" diff --git a/runsc/cmd/chroot.go b/runsc/cmd/chroot.go index 9570d22c9..a561ea6ed 100644 --- a/runsc/cmd/chroot.go +++ b/runsc/cmd/chroot.go @@ -83,7 +83,7 @@ func copyFile(dst, src string) error { // setUpChroot creates an empty directory with runsc mounted at /runsc and proc // mounted at /proc. -func setUpChroot(pidns bool, spec *specs.Spec, conf *config.Config) error { +func setUpChroot(spec *specs.Spec, conf *config.Config) error { // We are a new mount namespace, so we can use /tmp as a directory to // construct a new root. chroot := os.TempDir() @@ -108,15 +108,9 @@ func setUpChroot(pidns bool, spec *specs.Spec, conf *config.Config) error { log.Warningf("Failed to copy /etc/localtime: %v. UTC timezone will be used.", err) } - if pidns { - flags := uint32(unix.MS_NOSUID | unix.MS_NODEV | unix.MS_NOEXEC | unix.MS_RDONLY) - if err := mountInChroot(chroot, "proc", "/proc", "proc", flags); err != nil { - return fmt.Errorf("error mounting proc in chroot: %v", err) - } - } else { - if err := mountInChroot(chroot, "/proc", "/proc", "bind", unix.MS_BIND|unix.MS_RDONLY|unix.MS_REC); err != nil { - return fmt.Errorf("error mounting proc in chroot: %v", err) - } + flags := uint32(unix.MS_NOSUID | unix.MS_NODEV | unix.MS_NOEXEC | unix.MS_RDONLY) + if err := mountInChroot(chroot, "proc", "/proc", "proc", flags); err != nil { + return fmt.Errorf("error mounting proc in chroot: %v", err) } if err := tpuProxyUpdateChroot(chroot, spec, conf); err != nil { diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go index 25915e567..d5fc812bb 100644 --- a/runsc/sandbox/sandbox.go +++ b/runsc/sandbox/sandbox.go @@ -946,16 +946,7 @@ func (s *Sandbox) createSandboxProcess(conf *config.Config, args *Args, startSyn {Type: specs.IPCNamespace}, {Type: specs.MountNamespace}, {Type: specs.UTSNamespace}, - } - - if gPlatform.Requirements().RequiresCurrentPIDNS { - // TODO(b/75837838): Also set a new PID namespace so that we limit - // access to other host processes. - log.Infof("Sandbox will be started in the current PID namespace") - } else { - log.Infof("Sandbox will be started in a new PID namespace") - nss = append(nss, specs.LinuxNamespace{Type: specs.PIDNamespace}) - cmd.Args = append(cmd.Args, "--pidns=true") + {Type: specs.PIDNamespace}, } if specutils.NVProxyEnabled(args.Spec, conf) {