platform: Pass device fd into platform constructor.

We were previously openining the platform device (i.e. /dev/kvm) inside the
platfrom constructor (i.e. kvm.New).  This requires that we have RW access to
the platform device when constructing the platform.

However, now that the runsc sandbox process runs as user "nobody", it is not
able to open the platform device.

This CL changes the kvm constructor to take the platform device FD, rather than
opening the device file itself. The device file is opened outside of the
sandbox and passed to the sandbox process.

PiperOrigin-RevId: 212505804
Change-Id: I427e1d9de5eb84c84f19d513356e1bb148a52910

9 files changed, 96 insertions, 59 deletions

diff --git a/pkg/sentry/platform/kvm/kvm.go b/pkg/sentry/platform/kvm/kvm.go
index 2dc3239a5..19bc2d515 100644
--- a/pkg/sentry/platform/kvm/kvm.go
+++ b/pkg/sentry/platform/kvm/kvm.go
@@ -17,6 +17,7 @@ package kvm
 
 import (
 	"fmt"
+	"os"
 	"sync"
 	"syscall"
 
@@ -44,25 +45,29 @@ var (
 	globalErr  error
 )
 
+// OpenDevice opens the KVM device at /dev/kvm and returns the File.
+func OpenDevice() (*os.File, error) {
+	f, err := os.OpenFile("/dev/kvm", syscall.O_RDWR, 0)
+	if err != nil {
+		return nil, fmt.Errorf("error opening /dev/kvm: %v", err)
+	}
+	return f, nil
+}
+
 // New returns a new KVM-based implementation of the platform interface.
-func New() (*KVM, error) {
+func New(deviceFile *os.File) (*KVM, error) {
 	// Allocate physical memory for the vCPUs.
 	fm, err := filemem.New("kvm-memory")
 	if err != nil {
 		return nil, err
 	}
 
-	// Try opening KVM.
-	fd, err := syscall.Open("/dev/kvm", syscall.O_RDWR, 0)
-	if err != nil {
-		return nil, fmt.Errorf("opening /dev/kvm: %v", err)
-	}
-	defer syscall.Close(fd)
+	fd := deviceFile.Fd()
 
 	// Ensure global initialization is done.
 	globalOnce.Do(func() {
 		physicalInit()
-		globalErr = updateSystemValues(fd)
+		globalErr = updateSystemValues(int(fd))
 		ring0.Init(cpuid.HostFeatureSet())
 	})
 	if globalErr != nil {
@@ -70,10 +75,12 @@ func New() (*KVM, error) {
 	}
 
 	// Create a new VM fd.
-	vm, _, errno := syscall.RawSyscall(syscall.SYS_IOCTL, uintptr(fd), _KVM_CREATE_VM, 0)
+	vm, _, errno := syscall.RawSyscall(syscall.SYS_IOCTL, fd, _KVM_CREATE_VM, 0)
 	if errno != 0 {
 		return nil, fmt.Errorf("creating VM: %v", errno)
 	}
+	// We are done with the device file.
+	deviceFile.Close()
 
 	// Create a VM context.
 	machine, err := newMachine(int(vm))
diff --git a/pkg/sentry/platform/kvm/kvm_test.go b/pkg/sentry/platform/kvm/kvm_test.go
index 180bf7bb0..52448839f 100644
--- a/pkg/sentry/platform/kvm/kvm_test.go
+++ b/pkg/sentry/platform/kvm/kvm_test.go
@@ -39,7 +39,11 @@ type testHarness interface {
 
 func kvmTest(t testHarness, setup func(*KVM), fn func(*vCPU) bool) {
 	// Create the machine.
-	k, err := New()
+	deviceFile, err := OpenDevice()
+	if err != nil {
+		t.Fatalf("error opening device file: %v", err)
+	}
+	k, err := New(deviceFile)
 	if err != nil {
 		t.Fatalf("error creating KVM instance: %v", err)
 	}
diff --git a/runsc/boot/controller.go b/runsc/boot/controller.go
index fd5b7cc9e..257f275f9 100644
--- a/runsc/boot/controller.go
+++ b/runsc/boot/controller.go
@@ -17,6 +17,7 @@ package boot
 import (
 	"errors"
 	"fmt"
+	"os"
 	"path"
 
 	specs "github.com/opencontainers/runtime-spec/specs-go"
@@ -287,7 +288,8 @@ func (cm *containerManager) WaitForLoader(_, _ *struct{}) error {
 
 // RestoreOpts contains options related to restoring a container's file system.
 type RestoreOpts struct {
-	// FilePayload contains the state file to be restored.
+	// FilePayload contains the state file to be restored, followed by the
+	// platform device file if necessary.
 	urpc.FilePayload
 
 	// SandboxID contains the ID of the sandbox.
@@ -300,16 +302,28 @@ type RestoreOpts struct {
 // signal to start.
 func (cm *containerManager) Restore(o *RestoreOpts, _ *struct{}) error {
 	log.Debugf("containerManager.Restore")
-	if len(o.FilePayload.Files) != 1 {
-		return fmt.Errorf("exactly one file must be provided")
+
+	var specFile, deviceFile *os.File
+	switch numFiles := len(o.FilePayload.Files); numFiles {
+	case 2:
+		// The device file is donated to the platform, so don't Close
+		// it here.
+		deviceFile = o.FilePayload.Files[1]
+		fallthrough
+	case 1:
+		specFile = o.FilePayload.Files[0]
+		defer specFile.Close()
+	case 0:
+		return fmt.Errorf("at least one file must be passed to Restore")
+	default:
+		return fmt.Errorf("at most two files may be passed to Restore")
 	}
-	defer o.FilePayload.Files[0].Close()
 
 	// Destroy the old kernel and create a new kernel.
 	cm.l.k.Pause()
 	cm.l.k.Destroy()
 
-	p, err := createPlatform(cm.l.conf)
+	p, err := createPlatform(cm.l.conf, int(deviceFile.Fd()))
 	if err != nil {
 		return fmt.Errorf("error creating platform: %v", err)
 	}
diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go
index 994b3d2e2..30d22b9c6 100644
--- a/runsc/boot/loader.go
+++ b/runsc/boot/loader.go
@@ -122,9 +122,9 @@ func init() {
 
 // New initializes a new kernel loader configured by spec.
 // New also handles setting up a kernel for restoring a container.
-func New(spec *specs.Spec, conf *Config, controllerFD int, ioFDs []int, console bool) (*Loader, error) {
+func New(spec *specs.Spec, conf *Config, controllerFD, deviceFD int, ioFDs []int, console bool) (*Loader, error) {
 	// Create kernel and platform.
-	p, err := createPlatform(conf)
+	p, err := createPlatform(conf, deviceFD)
 	if err != nil {
 		return nil, fmt.Errorf("error creating platform: %v", err)
 	}
@@ -301,14 +301,17 @@ func (l *Loader) Destroy() {
 	l.watchdog.Stop()
 }
 
-func createPlatform(conf *Config) (platform.Platform, error) {
+func createPlatform(conf *Config, deviceFD int) (platform.Platform, error) {
 	switch conf.Platform {
 	case PlatformPtrace:
 		log.Infof("Platform: ptrace")
 		return ptrace.New()
 	case PlatformKVM:
 		log.Infof("Platform: kvm")
-		return kvm.New()
+		if deviceFD < 0 {
+			return nil, fmt.Errorf("kvm device fd must be provided")
+		}
+		return kvm.New(os.NewFile(uintptr(deviceFD), "kvm device"))
 	default:
 		return nil, fmt.Errorf("invalid platform %v", conf.Platform)
 	}
diff --git a/runsc/boot/loader_test.go b/runsc/boot/loader_test.go
index d6bfe9ff1..9398292ff 100644
--- a/runsc/boot/loader_test.go
+++ b/runsc/boot/loader_test.go
@@ -101,7 +101,7 @@ func createLoader() (*Loader, func(), error) {
 		return nil, nil, err
 	}
 
-	l, err := New(spec, conf, fd, []int{sandEnd}, false)
+	l, err := New(spec, conf, fd, -1 /* device fd */, []int{sandEnd}, false)
 	if err != nil {
 		cleanup()
 		return nil, nil, err
diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go
index d8c7b9cd3..035147cf1 100644
--- a/runsc/cmd/boot.go
+++ b/runsc/cmd/boot.go
@@ -42,6 +42,9 @@ type Boot struct {
 	// control server that is donated to this process.
 	controllerFD int
 
+	// deviceFD is the file descriptor for the platform device file.
+	deviceFD int
+
 	// ioFDs is the list of FDs used to connect to FS gofers.
 	ioFDs intFlags
 
@@ -74,6 +77,7 @@ func (b *Boot) SetFlags(f *flag.FlagSet) {
 	f.StringVar(&b.bundleDir, "bundle", "", "required path to the root of the bundle directory")
 	f.IntVar(&b.specFD, "spec-fd", -1, "required fd with the container spec")
 	f.IntVar(&b.controllerFD, "controller-fd", -1, "required FD of a stream socket for the control server that must be donated to this process")
+	f.IntVar(&b.deviceFD, "device-fd", -1, "FD for the platform device file")
 	f.Var(&b.ioFDs, "io-fds", "list of FDs to connect 9P clients. They must follow this order: root first, then mounts as defined in the spec")
 	f.BoolVar(&b.console, "console", false, "set to true if the sandbox should allow terminal ioctl(2) syscalls")
 	f.BoolVar(&b.applyCaps, "apply-caps", false, "if true, apply capabilities defined in the spec to the process")
@@ -134,7 +138,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{})
 	}
 
 	// Create the loader.
-	l, err := boot.New(spec, conf, b.controllerFD, b.ioFDs.GetArray(), b.console)
+	l, err := boot.New(spec, conf, b.controllerFD, b.deviceFD, b.ioFDs.GetArray(), b.console)
 	if err != nil {
 		Fatalf("error creating loader: %v", err)
 	}
diff --git a/runsc/sandbox/BUILD b/runsc/sandbox/BUILD
index 8ebd14c4e..5cf8f0cda 100644
--- a/runsc/sandbox/BUILD
+++ b/runsc/sandbox/BUILD
@@ -18,6 +18,7 @@ go_library(
         "//pkg/control/server",
         "//pkg/log",
         "//pkg/sentry/control",
+        "//pkg/sentry/platform/kvm",
         "//pkg/urpc",
         "//runsc/boot",
         "//runsc/console",
diff --git a/runsc/sandbox/chroot.go b/runsc/sandbox/chroot.go
index f35d9c72d..749bf3782 100644
--- a/runsc/sandbox/chroot.go
+++ b/runsc/sandbox/chroot.go
@@ -22,7 +22,6 @@ import (
 	"syscall"
 
 	"gvisor.googlesource.com/gvisor/pkg/log"
-	"gvisor.googlesource.com/gvisor/runsc/boot"
 	"gvisor.googlesource.com/gvisor/runsc/specutils"
 )
 
@@ -39,18 +38,12 @@ func mountInChroot(chroot, src, dst, typ string, flags uint32) error {
 	if err := specutils.Mount(src, chrootDst, typ, flags); err != nil {
 		return fmt.Errorf("error mounting %q at %q: %v", src, chrootDst, err)
 	}
-
-	// Make sure the mount is accessible to all users, since we will be
-	// running as nobody inside the chroot.
-	if err := os.Chmod(chrootDst, 0777); err != nil {
-		return fmt.Errorf("Chmod(%q) failed: %v", chroot, err)
-	}
 	return nil
 }
 
-// setUpChroot creates an empty directory with runsc mounted at /runsc, proc
-// mounted at /proc, and any dev files needed for the platform.
-func setUpChroot(platform boot.PlatformType) (string, error) {
+// setUpChroot creates an empty directory with runsc mounted at /runsc and proc
+// mounted at /proc.
+func setUpChroot() (string, error) {
 	// Create the chroot directory and make it accessible to all users.
 	chroot, err := ioutil.TempDir("", "runsc-sandbox-chroot-")
 	if err != nil {
@@ -75,18 +68,6 @@ func setUpChroot(platform boot.PlatformType) (string, error) {
 		return "", fmt.Errorf("error mounting runsc in chroot: %v", err)
 	}
 
-	// Mount dev files needed for platform.
-	var devMount string
-	switch platform {
-	case boot.PlatformKVM:
-		devMount = "/dev/kvm"
-	}
-	if devMount != "" {
-		if err := mountInChroot(chroot, devMount, devMount, "bind", syscall.MS_BIND); err != nil {
-			return "", fmt.Errorf("error mounting platform device in chroot: %v", err)
-		}
-	}
-
 	return chroot, nil
 }
 
@@ -105,21 +86,6 @@ func tearDownChroot(chroot string) error {
 		return fmt.Errorf("error unmounting %q: %v", exe, err)
 	}
 
-	// Unmount platform dev files.
-	devFiles := []string{"dev/kvm"}
-	for _, f := range devFiles {
-		devPath := filepath.Join(chroot, f)
-		if _, err := os.Stat(devPath); err != nil {
-			if os.IsNotExist(err) {
-				continue
-			}
-			return fmt.Errorf("Stat(%q) failed: %v", devPath, err)
-		}
-		if err := syscall.Unmount(devPath, 0); err != nil {
-			return fmt.Errorf("error unmounting %q: %v", devPath, err)
-		}
-	}
-
 	// Remove chroot directory.
 	if err := os.RemoveAll(chroot); err != nil {
 		return fmt.Errorf("error removing %q: %v", chroot, err)
diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go
index f272496a1..195deda1e 100644
--- a/runsc/sandbox/sandbox.go
+++ b/runsc/sandbox/sandbox.go
@@ -29,6 +29,7 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/control/server"
 	"gvisor.googlesource.com/gvisor/pkg/log"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/control"
+	"gvisor.googlesource.com/gvisor/pkg/sentry/platform/kvm"
 	"gvisor.googlesource.com/gvisor/pkg/urpc"
 	"gvisor.googlesource.com/gvisor/runsc/boot"
 	"gvisor.googlesource.com/gvisor/runsc/console"
@@ -140,6 +141,14 @@ func (s *Sandbox) Restore(cid string, spec *specs.Spec, conf *boot.Config, f str
 		SandboxID: s.ID,
 	}
 
+	// If the platform needs a device fd we must pass it in.
+	if deviceFile, err := deviceFileForPlatform(conf.Platform); err != nil {
+		return err
+	} else if deviceFile != nil {
+		defer deviceFile.Close()
+		opt.FilePayload.Files = append(opt.FilePayload.Files, deviceFile)
+	}
+
 	conn, err := s.sandboxConnect()
 	if err != nil {
 		return err
@@ -315,6 +324,16 @@ func (s *Sandbox) createSandboxProcess(spec *specs.Spec, conf *boot.Config, bund
 		nextFD++
 	}
 
+	// If the platform needs a device fd we must pass it in.
+	if deviceFile, err := deviceFileForPlatform(conf.Platform); err != nil {
+		return err
+	} else if deviceFile != nil {
+		defer deviceFile.Close()
+		cmd.ExtraFiles = append(cmd.ExtraFiles, deviceFile)
+		cmd.Args = append(cmd.Args, "--device-fd="+strconv.Itoa(nextFD))
+		nextFD++
+	}
+
 	// Sandbox stdio defaults to current process stdio.
 	cmd.Stdin = os.Stdin
 	cmd.Stdout = os.Stdout
@@ -428,7 +447,7 @@ func (s *Sandbox) createSandboxProcess(spec *specs.Spec, conf *boot.Config, bund
 			log.Warningf("Running sandbox in test mode without chroot. This is only safe in tests!")
 		} else if specutils.HasCapSysAdmin() {
 			log.Infof("Sandbox will be started in minimal chroot")
-			chroot, err := setUpChroot(conf.Platform)
+			chroot, err := setUpChroot()
 			if err != nil {
 				return fmt.Errorf("error setting up chroot: %v", err)
 			}
@@ -660,3 +679,22 @@ func signalProcess(pid int, sig syscall.Signal) error {
 	}
 	return nil
 }
+
+// deviceFileForPlatform opens the device file for the given platform. If the
+// platform does not need a device file, then nil is returned.
+func deviceFileForPlatform(p boot.PlatformType) (*os.File, error) {
+	var (
+		f   *os.File
+		err error
+	)
+	switch p {
+	case boot.PlatformKVM:
+		f, err = kvm.OpenDevice()
+	default:
+		return nil, nil
+	}
+	if err != nil {
+		return nil, fmt.Errorf("error opening device file for platform %q: %v", p, err)
+	}
+	return f, err
+}