From a3dec262c5d750c3b7201e19bb504b1ca22c30a9 Mon Sep 17 00:00:00 2001
From: Peng Tao <bergwolf@hyper.sh>
Date: Fri, 29 May 2020 08:35:34 -0700
Subject: [PATCH] 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 <bergwolf@hyper.sh>
---
 virtcontainers/agent.go               |  3 -
 virtcontainers/api_test.go            |  4 +-
 virtcontainers/clh.go                 |  4 +-
 virtcontainers/container.go           |  7 ++-
 virtcontainers/kata_agent.go          | 89 ++++++++++++++++++++++-----
 virtcontainers/kata_agent_test.go     | 13 ----
 virtcontainers/mount.go               | 14 ++---
 virtcontainers/mount_test.go          |  4 +-
 virtcontainers/noop_agent.go          |  5 --
 virtcontainers/noop_agent_test.go     |  7 ---
 virtcontainers/qemu.go                |  2 +-
 virtcontainers/qemu_test.go           |  4 +-
 virtcontainers/virtcontainers_test.go |  2 +
 virtcontainers/vm.go                  |  2 +-
 14 files changed, 96 insertions(+), 64 deletions(-)

diff --git a/virtcontainers/agent.go b/virtcontainers/agent.go
index 2b68d35027..663e38b839 100644
--- a/virtcontainers/agent.go
+++ b/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
 
diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go
index 710b5cc420..5fb8ff4d6d 100644
--- a/virtcontainers/api_test.go
+++ b/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)
 }
 
diff --git a/virtcontainers/clh.go b/virtcontainers/clh.go
index a0d7c7b722..56278e64ec 100644
--- a/virtcontainers/clh.go
+++ b/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,
diff --git a/virtcontainers/container.go b/virtcontainers/container.go
index 4ba7ee2e3a..9e2d1e94b4 100644
--- a/virtcontainers/container.go
+++ b/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
 	}
 
diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go
index 160093b106..7ff0310025 100644
--- a/virtcontainers/kata_agent.go
+++ b/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))
 	}
 }
 
diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go
index 52c1da7629..55444d46b6 100644
--- a/virtcontainers/kata_agent_test.go
+++ b/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)
 
diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go
index dd6177bef7..f7822a2968 100644
--- a/virtcontainers/mount.go
+++ b/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,7 +343,7 @@ 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
 		}
@@ -351,7 +351,7 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo
 		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()
diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go
index 5a39133be7..20b0157a40 100644
--- a/virtcontainers/mount_test.go
+++ b/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")
diff --git a/virtcontainers/noop_agent.go b/virtcontainers/noop_agent.go
index 02c4cf0a17..73f6974823 100644
--- a/virtcontainers/noop_agent.go
+++ b/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
diff --git a/virtcontainers/noop_agent_test.go b/virtcontainers/noop_agent_test.go
index 8e60f44deb..f6172263d9 100644
--- a/virtcontainers/noop_agent_test.go
+++ b/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{}
diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go
index 51b68b0544..35478e3b0b 100644
--- a/virtcontainers/qemu.go
+++ b/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,
diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go
index f490d5d7de..6d8c01caa7 100644
--- a/virtcontainers/qemu_test.go
+++ b/virtcontainers/qemu_test.go
@@ -555,12 +555,12 @@ func TestQemuVirtiofsdArgs(t *testing.T) {
 		kataHostSharedDir = savedKataHostSharedDir
 	}()
 
-	result := "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -o no_posix_lock -d"
+	result := "--fd=123 -o source=test-share-dir/foo/shared -o cache=none --syslog -o no_posix_lock -d"
 	args := q.virtiofsdArgs(123)
 	assert.Equal(strings.Join(args, " "), result)
 
 	q.config.Debug = false
-	result = "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -o no_posix_lock -f"
+	result = "--fd=123 -o source=test-share-dir/foo/shared -o cache=none --syslog -o no_posix_lock -f"
 	args = q.virtiofsdArgs(123)
 	assert.Equal(strings.Join(args, " "), result)
 }
diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go
index 383310fbc7..3d10476210 100644
--- a/virtcontainers/virtcontainers_test.go
+++ b/virtcontainers/virtcontainers_test.go
@@ -13,6 +13,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"syscall"
 	"testing"
 
 	"github.com/kata-containers/runtime/virtcontainers/persist"
@@ -59,6 +60,7 @@ func cleanUp() {
 	globalSandboxList.removeSandbox(testSandboxID)
 	os.RemoveAll(fs.MockRunStoragePath())
 	os.RemoveAll(fs.MockRunVMStoragePath())
+	syscall.Unmount(getSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow)
 	os.RemoveAll(testDir)
 	os.MkdirAll(testDir, DirMode)
 
diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go
index cfed806914..fcda1e977e 100644
--- a/virtcontainers/vm.go
+++ b/virtcontainers/vm.go
@@ -412,7 +412,7 @@ func (v *VM) assignSandbox(s *Sandbox) error {
 
 	vmSharePath := buildVMSharePath(v.id, v.store.RunVMStoragePath())
 	vmSockDir := filepath.Join(v.store.RunVMStoragePath(), v.id)
-	sbSharePath := s.agent.getSharePath(s.id)
+	sbSharePath := getMountPath(s.id)
 	sbSockDir := filepath.Join(v.store.RunVMStoragePath(), s.id)
 
 	v.logger().WithFields(logrus.Fields{