vc: make host shared path readonly

We need to make sure containers cannot modify host path unless it is explicitly shared to it. Right now we expose an additional top level shared directory to the guest and allow it to be modified. This is less ideal and can be enhanced by following method:
1. create two directories for each sandbox:
  -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts
  -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir)
2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it
3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/

Fixes: #2712
Signed-off-by: Peng Tao <[email protected]>

virtcontainers/agent.go

@@ -224,9 +224,6 @@ type agent interface {
 	// configureFromGrpc will update agent settings based on provided arguments which from Grpc
 	configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error
 
-	// getSharePath will return the agent 9pfs share mount path
-	getSharePath(id string) string
-
 	// reseedRNG will reseed the guest random number generator
 	reseedRNG(data []byte) error
 

virtcontainers/api_test.go

@@ -340,7 +340,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) {
 
 	// TODO: defaultSharedDir is a hyper var = /run/hyper/shared/sandboxes
 	// do we need to unmount sandboxes and containers?
-	err = bindUnmountAllRootfs(ctx, testDir, pImpl)
+	err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl)
 	assert.NoError(err)
 }
 
@@ -474,7 +474,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) {
 	_, err = os.Stat(sandboxDir)
 	assert.NoError(err)
 
-	err = bindUnmountAllRootfs(ctx, testDir, pImpl)
+	err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl)
 	assert.NoError(err)
 }
 

virtcontainers/clh.go

@@ -207,7 +207,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ
 		clh.Logger().WithField("function", "createSandbox").Info("Sandbox already exist, loading from state")
 		clh.virtiofsd = &virtiofsd{
 			PID:        clh.state.VirtiofsdPID,
-			sourcePath: filepath.Join(kataHostSharedDir(), clh.id),
+			sourcePath: filepath.Join(getSharePath(clh.id)),
 			debug:      clh.config.Debug,
 			socketPath: virtiofsdSocketPath,
 		}
@@ -307,7 +307,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ
 
 	clh.virtiofsd = &virtiofsd{
 		path:       clh.config.VirtioFSDaemon,
-		sourcePath: filepath.Join(kataHostSharedDir(), clh.id),
+		sourcePath: filepath.Join(getSharePath(clh.id)),
 		socketPath: virtiofsdSocketPath,
 		extraArgs:  clh.config.VirtioFSExtraArgs,
 		debug:      clh.config.Debug,

virtcontainers/container.go

@@ -480,7 +480,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s
 		}
 	} else {
 		// These mounts are created in the shared dir
-		mountDest := filepath.Join(hostSharedDir, c.sandbox.id, filename)
+		mountDest := filepath.Join(hostSharedDir, filename)
 		if err := bindMount(c.ctx, m.Source, mountDest, false, "private"); err != nil {
 			return "", false, err
 		}
@@ -850,7 +850,7 @@ func (c *Container) rollbackFailingContainerCreation() {
 	if err := c.unmountHostMounts(); err != nil {
 		c.Logger().WithError(err).Error("rollback failed unmountHostMounts()")
 	}
-	if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil {
+	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil {
 		c.Logger().WithError(err).Error("rollback failed bindUnmountContainerRootfs()")
 	}
 }
@@ -875,6 +875,7 @@ func (c *Container) create() (err error) {
 	// of rolling back all the actions previously performed.
 	defer func() {
 		if err != nil {
+			c.Logger().WithError(err).Error("container create failed")
 			c.rollbackFailingContainerCreation()
 		}
 	}()
@@ -1119,7 +1120,7 @@ func (c *Container) stop(force bool) error {
 		return err
 	}
 
-	if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil && !force {
+	if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil && !force {
 		return err
 	}
 

virtcontainers/kata_agent.go

@@ -140,6 +140,26 @@ var kataHostSharedDir = func() string {
 	return defaultKataHostSharedDir
 }
 
+// Shared path handling:
+// 1. create two directories for each sandbox:
+// -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts
+// -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir)
+//
+// 2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it
+//
+// 3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/
+func getSharePath(id string) string {
+	return filepath.Join(kataHostSharedDir(), id, "shared")
+}
+
+func getMountPath(id string) string {
+	return filepath.Join(kataHostSharedDir(), id, "mounts")
+}
+
+func getSandboxPath(id string) string {
+	return filepath.Join(kataHostSharedDir(), id)
+}
+
 // The function is declared this way for mocking in unit tests
 var kataGuestSharedDir = func() string {
 	if rootless.IsRootless() {
@@ -158,6 +178,10 @@ var kataGuestSandboxDir = func() string {
 	return defaultKataGuestSandboxDir
 }
 
+var kataGuestSandboxStorageDir = func() string {
+	return filepath.Join(defaultKataGuestSandboxDir, "storage")
+}
+
 func ephemeralPath() string {
 	if rootless.IsRootless() {
 		return filepath.Join(kataGuestSandboxDir(), kataEphemeralDevType)
@@ -223,10 +247,6 @@ func (k *kataAgent) Logger() *logrus.Entry {
 	return virtLog.WithField("subsystem", "kata_agent")
 }
 
-func (k *kataAgent) getSharePath(id string) string {
-	return filepath.Join(kataHostSharedDir(), id)
-}
-
 func (k *kataAgent) longLiveConn() bool {
 	return k.keepConn
 }
@@ -356,7 +376,7 @@ func (k *kataAgent) capabilities() types.Capabilities {
 	return caps
 }
 
-func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error {
+func (k *kataAgent) internalConfigure(h hypervisor, id string, builtin bool, config interface{}) error {
 	var err error
 	if config != nil {
 		switch c := config.(type) {
@@ -378,7 +398,7 @@ func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builti
 }
 
 func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error {
-	err := k.internalConfigure(h, id, sharePath, builtin, config)
+	err := k.internalConfigure(h, id, builtin, config)
 	if err != nil {
 		return err
 	}
@@ -424,14 +444,36 @@ func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool,
 }
 
 func (k *kataAgent) configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error {
-	return k.internalConfigure(h, id, "", builtin, config)
+	return k.internalConfigure(h, id, builtin, config)
+}
+
+func (k *kataAgent) setupSharedPath(sandbox *Sandbox) error {
+	// create shared path structure
+	sharePath := getSharePath(sandbox.id)
+	mountPath := getMountPath(sandbox.id)
+	if err := os.MkdirAll(sharePath, DirMode); err != nil {
+		return err
+	}
+	if err := os.MkdirAll(mountPath, DirMode); err != nil {
+		return err
+	}
+
+	// slave mount so that future mountpoints under mountPath are shown in sharePath as well
+	if err := bindMount(context.Background(), mountPath, sharePath, true, "slave"); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (k *kataAgent) createSandbox(sandbox *Sandbox) error {
 	span, _ := k.trace("createSandbox")
 	defer span.Finish()
 
-	return k.configure(sandbox.hypervisor, sandbox.id, k.getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig)
+	if err := k.setupSharedPath(sandbox); err != nil {
+		return err
+	}
+	return k.configure(sandbox.hypervisor, sandbox.id, getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig)
 }
 
 func cmdToKataProcess(cmd types.Cmd) (process *grpc.Process, err error) {
@@ -1030,7 +1072,7 @@ func (k *kataAgent) replaceOCIMountsForStorages(spec *specs.Spec, volumeStorages
 			// Create a temporary location to mount the Storage. Mounting to the correct location
 			// will be handled by the OCI mount structure.
 			filename := fmt.Sprintf("%s-%s", uuid.Generate().String(), filepath.Base(m.Destination))
-			path := filepath.Join(kataGuestSharedDir(), filename)
+			path := filepath.Join(kataGuestSandboxStorageDir(), filename)
 
 			k.Logger().Debugf("Replacing OCI mount source (%s) with %s", m.Source, path)
 			ociMounts[index].Source = path
@@ -1245,7 +1287,7 @@ func (k *kataAgent) rollbackFailingContainerCreation(c *Container) {
 			k.Logger().WithError(err2).Error("rollback failed unmountHostMounts()")
 		}
 
-		if err2 := bindUnmountContainerRootfs(k.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err2 != nil {
+		if err2 := bindUnmountContainerRootfs(k.ctx, getMountPath(c.sandbox.id), c.id); err2 != nil {
 			k.Logger().WithError(err2).Error("rollback failed bindUnmountContainerRootfs()")
 		}
 	}
@@ -1302,6 +1344,13 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat
 			rootfs.Options = []string{"nouuid"}
 		}
 
+		// Ensure container mount destination exists
+		// TODO: remove dependency on shared fs path. shared fs is just one kind of storage sources.
+		// we should not always use shared fs path for all kinds of storage. Stead, all storage
+		// should be bind mounted to a tmpfs path for containers to use.
+		if err := os.MkdirAll(filepath.Join(getMountPath(c.sandbox.id), c.id, c.rootfsSuffix), DirMode); err != nil {
+			return nil, err
+		}
 		return rootfs, nil
 	}
 
@@ -1313,7 +1362,7 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat
 	// (kataGuestSharedDir) is already mounted in the
 	// guest. We only need to mount the rootfs from
 	// the host and it will show up in the guest.
-	if err := bindMountContainerRootfs(k.ctx, kataHostSharedDir(), sandbox.id, c.id, c.rootFs.Target, false); err != nil {
+	if err := bindMountContainerRootfs(k.ctx, getMountPath(sandbox.id), c.id, c.rootFs.Target, false); err != nil {
 		return nil, err
 	}
 
@@ -1346,6 +1395,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,
 	// takes care of rolling back actions previously performed.
 	defer func() {
 		if err != nil {
+			k.Logger().WithError(err).Error("createContainer failed")
 			k.rollbackFailingContainerCreation(c)
 		}
 	}()
@@ -1366,7 +1416,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,
 	}
 
 	// Handle container mounts
-	newMounts, ignoredMounts, err := c.mountSharedDirMounts(kataHostSharedDir(), kataGuestSharedDir())
+	newMounts, ignoredMounts, err := c.mountSharedDirMounts(getMountPath(sandbox.id), kataGuestSharedDir())
 	if err != nil {
 		return nil, err
 	}
@@ -2363,13 +2413,20 @@ func (k *kataAgent) markDead() {
 }
 
 func (k *kataAgent) cleanup(s *Sandbox) {
-	path := k.getSharePath(s.id)
+	// Unmount shared path
+	path := getSharePath(s.id)
 	k.Logger().WithField("path", path).Infof("cleanup agent")
+	if err := syscall.Unmount(path, syscall.MNT_DETACH|UmountNoFollow); err != nil {
+		k.Logger().WithError(err).Errorf("failed to unmount vm share path %s", path)
+	}
+
+	// Unmount mount path
+	path = getMountPath(s.id)
 	if err := bindUnmountAllRootfs(k.ctx, path, s); err != nil {
-		k.Logger().WithError(err).Errorf("failed to unmount container share path %s", path)
+		k.Logger().WithError(err).Errorf("failed to unmount vm mount path %s", path)
 	}
-	if err := os.RemoveAll(path); err != nil {
-		k.Logger().WithError(err).Errorf("failed to cleanup vm share path %s", path)
+	if err := os.RemoveAll(getSandboxPath(s.id)); err != nil {
+		k.Logger().WithError(err).Errorf("failed to cleanup vm path %s", getSandboxPath(s.id))
 	}
 }
 

virtcontainers/kata_agent_test.go

@@ -785,19 +785,6 @@ func TestHandlePidNamespace(t *testing.T) {
 	assert.False(testIsPidNamespacePresent(g))
 }
 
-func TestAgentPathAPI(t *testing.T) {
-	assert := assert.New(t)
-
-	k1 := &kataAgent{}
-	k2 := &kataAgent{}
-	id := "foobar"
-
-	// getSharePath
-	path1 := k1.getSharePath(id)
-	path2 := k2.getSharePath(id)
-	assert.Equal(path1, path2)
-}
-
 func TestAgentConfigure(t *testing.T) {
 	assert := assert.New(t)
 

virtcontainers/mount.go

@@ -275,11 +275,11 @@ func remount(ctx context.Context, mountflags uintptr, src string) error {
 
 // bindMountContainerRootfs bind mounts a container rootfs into a 9pfs shared
 // directory between the guest and the host.
-func bindMountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID, cRootFs string, readonly bool) error {
+func bindMountContainerRootfs(ctx context.Context, shareDir, cid, cRootFs string, readonly bool) error {
 	span, _ := trace(ctx, "bindMountContainerRootfs")
 	defer span.Finish()
 
-	rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
+	rootfsDest := filepath.Join(shareDir, cid, rootfsDir)
 
 	return bindMount(ctx, cRootFs, rootfsDest, readonly, "private")
 }
@@ -315,12 +315,12 @@ func isSymlink(path string) bool {
 	return stat.Mode()&os.ModeSymlink != 0
 }
 
-func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID string) error {
+func bindUnmountContainerRootfs(ctx context.Context, sharedDir, cID string) error {
 	span, _ := trace(ctx, "bindUnmountContainerRootfs")
 	defer span.Finish()
 
-	rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
-	if isSymlink(filepath.Join(sharedDir, sandboxID, cID)) || isSymlink(rootfsDest) {
+	rootfsDest := filepath.Join(sharedDir, cID, rootfsDir)
+	if isSymlink(filepath.Join(sharedDir, cID)) || isSymlink(rootfsDest) {
 		logrus.Warnf("container dir %s is a symlink, malicious guest?", cID)
 		return nil
 	}
@@ -343,15 +343,15 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo
 
 	var errors *merr.Error
 	for _, c := range sandbox.containers {
-		if isSymlink(filepath.Join(sharedDir, sandbox.id, c.id)) {
+		if isSymlink(filepath.Join(sharedDir, c.id)) {
 			logrus.Warnf("container dir %s is a symlink, malicious guest?", c.id)
 			continue
 		}
 		c.unmountHostMounts()
 		if c.state.Fstype == "" {
 			// even if error found, don't break out of loop until all mounts attempted
 			// to be unmounted, and collect all errors
-			errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, sandbox.id, c.id))
+			errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, c.id))
 		}
 	}
 	return errors.ErrorOrNil()

virtcontainers/mount_test.go

@@ -386,7 +386,7 @@ func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) {
 		assert.NoError(os.Remove(testPath))
 	}
 
-	err := bindUnmountContainerRootfs(context.Background(), testMnt, sID, cID)
+	err := bindUnmountContainerRootfs(context.Background(), filepath.Join(testMnt, sID), cID)
 	assert.NoError(err)
 }
 
@@ -407,7 +407,7 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) {
 	assert.NoError(err)
 	defer os.RemoveAll(filepath.Join(testDir, sID))
 
-	bindUnmountContainerRootfs(context.Background(), testDir, sID, cID)
+	bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), cID)
 
 	if _, err := os.Stat(testPath); err == nil {
 		t.Fatal("empty rootfs dest should be removed")

virtcontainers/noop_agent.go

@@ -185,11 +185,6 @@ func (n *noopAgent) configureFromGrpc(h hypervisor, id string, builtin bool, con
 	return nil
 }
 
-// getSharePath is the Noop agent share path getter. It does nothing.
-func (n *noopAgent) getSharePath(id string) string {
-	return ""
-}
-
 // reseedRNG is the Noop agent RND reseeder. It does nothing.
 func (n *noopAgent) reseedRNG(data []byte) error {
 	return nil

virtcontainers/noop_agent_test.go

@@ -156,13 +156,6 @@ func TestNoopAgentConfigure(t *testing.T) {
 	assert.NoError(err)
 }
 
-func TestNoopAgentGetSharePath(t *testing.T) {
-	n := &noopAgent{}
-	path := n.getSharePath("")
-	assert := assert.New(t)
-	assert.Empty(path)
-}
-
 func TestNoopAgentStartProxy(t *testing.T) {
 	assert := assert.New(t)
 	n := &noopAgent{}

virtcontainers/qemu.go

@@ -618,7 +618,7 @@ func (q *qemu) virtiofsdArgs(fd uintptr) []string {
 	// The daemon will terminate when the vhost-user socket
 	// connection with QEMU closes.  Therefore we do not keep track
 	// of this child process after returning from this function.
-	sourcePath := filepath.Join(kataHostSharedDir(), q.id)
+	sourcePath := filepath.Join(getSharePath(q.id))
 	args := []string{
 		fmt.Sprintf("--fd=%v", fd),
 		"-o", "source=" + sourcePath,