From b247cd392a833b26751b946c4dec35126c40ec8b Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Fri, 12 Nov 2021 12:11:58 -0800
Subject: [PATCH] runc run: fix ro /dev
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit fb4c27c4b79c7d (went into v1.0.0-rc93) fixed a bug with
read-only tmpfs, but introduced a bug with read-only /dev.

This happens because /dev is a tmpfs mount and is therefore remounted
read-only a bit earlier than before.

To fix,

1. Revert the part of the above commit which remounts all tmpfs mounts
   as read-only in mountToRootfs.

2. Reuse finalizeRootfs (which is already used to remount /dev
   read-only) to also remount all ro tmpfs mounts that were previously
   mounted rw in mountPropagate.

3. Remove the break in finalizeRootfs, as now we have more than one
   mount to care about.

4. Reorder the if statements in finalizeRootfs to perform the fast check
   (for ro flag) first, and compare the strings second. Since /dev is
   most probably also a tmpfs mount, do the m.Device check first.

Add a test case to validate the fix and prevent future regressions;
make sure it fails before the fix:

 ✗ runc run [ro /dev mount]
   (in test file tests/integration/mounts.bats, line 45)
     `[ "$status" -eq 0 ]' failed
   runc spec (status=0):

   runc run test_busybox (status=1):
   time="2021-11-12T12:19:48-08:00" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"devpts\" to rootfs at \"/dev/pts\": mkdir /tmp/bats-run-VJXQk7/runc.0Fj70w/bundle/rootfs/dev/pts: read-only file system"

Fixes: fb4c27c4b79c7d
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/rootfs_linux.go  | 29 ++++++++++++-----------------
 tests/integration/mounts.bats | 10 ++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go
index 9578e4428..f680341e7 100644
--- a/libcontainer/rootfs_linux.go
+++ b/libcontainer/rootfs_linux.go
@@ -164,15 +164,16 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds []int) (err
 // finalizeRootfs sets anything to ro if necessary. You must call
 // prepareRootfs first.
 func finalizeRootfs(config *configs.Config) (err error) {
-	// remount dev as ro if specified
+	// All tmpfs mounts and /dev were previously mounted as rw
+	// by mountPropagate. Remount them read-only as requested.
 	for _, m := range config.Mounts {
-		if utils.CleanPath(m.Destination) == "/dev" {
-			if m.Flags&unix.MS_RDONLY == unix.MS_RDONLY {
-				if err := remountReadonly(m); err != nil {
-					return err
-				}
+		if m.Flags&unix.MS_RDONLY != unix.MS_RDONLY {
+			continue
+		}
+		if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" {
+			if err := remountReadonly(m); err != nil {
+				return err
 			}
-			break
 		}
 	}
 
@@ -452,12 +453,6 @@ func mountToRootfs(m *configs.Mount, c *mountConfig) error {
 				return err
 			}
 		}
-		// Initially mounted rw in mountPropagate, remount to ro if flag set.
-		if m.Flags&unix.MS_RDONLY != 0 {
-			if err := remount(m, rootfs, mountFd); err != nil {
-				return err
-			}
-		}
 		return nil
 	case "bind":
 		if err := prepareBindMount(m, rootfs, mountFd); err != nil {
@@ -1077,10 +1072,10 @@ func mountPropagate(m *configs.Mount, rootfs string, mountLabel string, mountFd
 		flags = m.Flags
 	)
 	// Delay mounting the filesystem read-only if we need to do further
-	// operations on it. We need to set up files in "/dev" and tmpfs mounts may
-	// need to be chmod-ed after mounting. The mount will be remounted ro later
-	// in finalizeRootfs() if necessary.
-	if utils.CleanPath(m.Destination) == "/dev" || m.Device == "tmpfs" {
+	// operations on it. We need to set up files in "/dev", and other tmpfs
+	// mounts may need to be chmod-ed after mounting. These mounts will be
+	// remounted ro later in finalizeRootfs(), if necessary.
+	if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" {
 		flags &= ^unix.MS_RDONLY
 	}
 
diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats
index 5d1ba0974..1ec675aca 100644
--- a/tests/integration/mounts.bats
+++ b/tests/integration/mounts.bats
@@ -38,6 +38,16 @@ function teardown() {
 	[[ "${lines[0]}" == *'ro,'* ]]
 }
 
+# https://github.com/opencontainers/runc/issues/3248
+@test "runc run [ro /dev mount]" {
+	update_config '   .mounts |= map((select(.destination == "/dev") | .options += ["ro"]) // .)
+			| .process.args |= ["grep", "^tmpfs /dev", "/proc/mounts"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == *'ro,'* ]]
+}
+
 # https://github.com/opencontainers/runc/issues/2683
 @test "runc run [tmpfs mount with absolute symlink]" {
 	# in container, /conf -> /real/conf