Remove runc default devices that overlap with spec devices.

Runc has a set of default devices that it includes in Linux containers
(e.g., /dev/null, /dev/random, /dev/tty, etc.)

However if the container's OCI spec includes all or a subset of those same devices,
runc is currently not detecting the redundancy, causing it to create a lib
container config that has redundant device configurations.

This causes a failure in rootless mode, in particular when the /dev/tty device
has a redundant config:

container_linux.go:370: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:70: creating device nodes caused: open /tmp/busyboxtest/rootfs/dev/tty: no such device or address"

The reason this fails in rootless mode only is that in this case runc sets up
/dev/tty not by doing mknod (it's not allowed within a user-ns) but rather by
creating a regular file under /dev/tty and bind-mounting the host's /dev/tty to
the container's /dev/tty. When this operation is done redundantly, it fails the
second time.

This change fixes this problem by ensuring runc checks for redundant devices
between the OCI spec it receives and the default devices it configures. If
a redundant device is detected, the OCI spec takes priority.

The change adds both a unit test and an integration test to verify the
behavior. Without this fix, this new integration test fails as shown above.

Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>

libcontainer/specconv/spec_linux.go

@@ -241,13 +241,17 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 	for _, m := range spec.Mounts {
 		config.Mounts = append(config.Mounts, createLibcontainerMount(cwd, m))
 	}
-	if err := createDevices(spec, config); err != nil {
-		return nil, err
-	}
-	c, err := CreateCgroupConfig(opts)
+
+	defaultDevs, err := createDevices(spec, config)
 	if err != nil {
 		return nil, err
 	}
+
+	c, err := CreateCgroupConfig(opts, defaultDevs)
+	if err != nil {
+		return nil, err
+	}
+
 	config.Cgroups = c
 	// set linux-specific config
 	if spec.Linux != nil {
@@ -410,7 +414,7 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) {
 	return sp, nil
 }
 
-func CreateCgroupConfig(opts *CreateOpts) (*configs.Cgroup, error) {
+func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*configs.Device) (*configs.Cgroup, error) {
 	var (
 		myCgroupPath string
 
@@ -616,9 +620,9 @@ func CreateCgroupConfig(opts *CreateOpts) (*configs.Cgroup, error) {
 			}
 		}
 	}
+
 	// Append the default allowed devices to the end of the list.
-	// XXX: Really this should be prefixed...
-	for _, device := range AllowedDevices {
+	for _, device := range defaultDevs {
 		c.Resources.Devices = append(c.Resources.Devices, &device.DeviceRule)
 	}
 	return c, nil
@@ -650,13 +654,26 @@ func stringToDeviceRune(s string) (configs.DeviceType, error) {
 	}
 }
 
-func createDevices(spec *specs.Spec, config *configs.Config) error {
-	// Add default set of devices.
-	for _, device := range AllowedDevices {
-		if device.Path != "" {
-			config.Devices = append(config.Devices, device)
+func createDevices(spec *specs.Spec, config *configs.Config) ([]*configs.Device, error) {
+	// If a spec device is redundant with a default device, remove that default
+	// device (the spec one takes priority).
+	dedupedAllowDevs := []*configs.Device{}
+
+next:
+	for _, ad := range AllowedDevices {
+		if ad.Path != "" {
+			for _, sd := range spec.Linux.Devices {
+				if sd.Path == ad.Path {
+					continue next
+				}
+			}
+		}
+		dedupedAllowDevs = append(dedupedAllowDevs, ad)
+		if ad.Path != "" {
+			config.Devices = append(config.Devices, ad)
 		}
 	}
+
 	// Merge in additional devices from the spec.
 	if spec.Linux != nil {
 		for _, d := range spec.Linux.Devices {
@@ -671,7 +688,7 @@ func createDevices(spec *specs.Spec, config *configs.Config) error {
 			}
 			dt, err := stringToDeviceRune(d.Type)
 			if err != nil {
-				return err
+				return nil, err
 			}
 			if d.FileMode != nil {
 				filemode = *d.FileMode
@@ -690,7 +707,8 @@ func createDevices(spec *specs.Spec, config *configs.Config) error {
 			config.Devices = append(config.Devices, device)
 		}
 	}
-	return nil
+
+	return dedupedAllowDevs, nil
 }
 
 func setupUserNamespace(spec *specs.Spec, config *configs.Config) error {

libcontainer/specconv/spec_linux_test.go

@@ -259,7 +259,7 @@ func TestLinuxCgroupWithMemoryResource(t *testing.T) {
 		Spec:             spec,
 	}
 
-	cgroup, err := CreateCgroupConfig(opts)
+	cgroup, err := CreateCgroupConfig(opts, nil)
 	if err != nil {
 		t.Errorf("Couldn't create Cgroup config: %v", err)
 	}
@@ -303,7 +303,7 @@ func TestLinuxCgroupSystemd(t *testing.T) {
 		Spec:             spec,
 	}
 
-	cgroup, err := CreateCgroupConfig(opts)
+	cgroup, err := CreateCgroupConfig(opts, nil)
 
 	if err != nil {
 		t.Errorf("Couldn't create Cgroup config: %v", err)
@@ -339,7 +339,7 @@ func TestLinuxCgroupSystemdWithEmptyPath(t *testing.T) {
 		Spec:             spec,
 	}
 
-	cgroup, err := CreateCgroupConfig(opts)
+	cgroup, err := CreateCgroupConfig(opts, nil)
 
 	if err != nil {
 		t.Errorf("Couldn't create Cgroup config: %v", err)
@@ -374,7 +374,7 @@ func TestLinuxCgroupSystemdWithInvalidPath(t *testing.T) {
 		Spec:             spec,
 	}
 
-	_, err := CreateCgroupConfig(opts)
+	_, err := CreateCgroupConfig(opts, nil)
 	if err == nil {
 		t.Error("Expected to produce an error if not using the correct format for cgroup paths belonging to systemd")
 	}
@@ -393,7 +393,7 @@ func TestLinuxCgroupsPathSpecified(t *testing.T) {
 		Spec:             spec,
 	}
 
-	cgroup, err := CreateCgroupConfig(opts)
+	cgroup, err := CreateCgroupConfig(opts, nil)
 	if err != nil {
 		t.Errorf("Couldn't create Cgroup config: %v", err)
 	}
@@ -411,7 +411,7 @@ func TestLinuxCgroupsPathNotSpecified(t *testing.T) {
 		Spec:             spec,
 	}
 
-	cgroup, err := CreateCgroupConfig(opts)
+	cgroup, err := CreateCgroupConfig(opts, nil)
 	if err != nil {
 		t.Errorf("Couldn't create Cgroup config: %v", err)
 	}
@@ -648,3 +648,107 @@ func TestNullProcess(t *testing.T) {
 		t.Errorf("Null process should be forbidden")
 	}
 }
+
+func TestCreateDevices(t *testing.T) {
+	spec := Example()
+
+	// dummy uid/gid for /dev/tty; will enable the test to check if createDevices()
+	// preferred the spec's device over the redundant default device
+	ttyUid := uint32(1000)
+	ttyGid := uint32(1000)
+	fm := os.FileMode(0666)
+
+	spec.Linux = &specs.Linux{
+		Devices: []specs.LinuxDevice{
+			{
+				// This is purposely redundant with one of runc's default devices
+				Path:     "/dev/tty",
+				Type:     "c",
+				Major:    5,
+				Minor:    0,
+				FileMode: &fm,
+				UID:      &ttyUid,
+				GID:      &ttyGid,
+			},
+			{
+				// This is purposely not redundant with one of runc's default devices
+				Path:  "/dev/ram0",
+				Type:  "b",
+				Major: 1,
+				Minor: 0,
+			},
+		},
+	}
+
+	conf := &configs.Config{}
+
+	defaultDevs, err := createDevices(spec, conf)
+	if err != nil {
+		t.Errorf("failed to create devices: %v", err)
+	}
+
+	// Verify the returned default devices has the /dev/tty entry deduplicated
+	found := false
+	for _, d := range defaultDevs {
+		if d.Path == "/dev/tty" {
+			if found {
+				t.Errorf("createDevices failed: returned a duplicated device entry: %v", defaultDevs)
+			}
+			found = true
+		}
+	}
+
+	// Verify that createDevices() placed all default devices in the config
+	for _, allowedDev := range AllowedDevices {
+		if allowedDev.Path == "" {
+			continue
+		}
+
+		found := false
+		for _, configDev := range conf.Devices {
+			if configDev.Path == allowedDev.Path {
+				found = true
+			}
+		}
+		if !found {
+			configDevPaths := []string{}
+			for _, configDev := range conf.Devices {
+				configDevPaths = append(configDevPaths, configDev.Path)
+			}
+			t.Errorf("allowedDevice %s was not found in the config's devices: %v", allowedDev.Path, configDevPaths)
+		}
+	}
+
+	// Verify that createDevices() deduplicated the /dev/tty entry in the config
+	for _, configDev := range conf.Devices {
+		if configDev.Path == "/dev/tty" {
+			wantDev := &configs.Device{
+				Path:     "/dev/tty",
+				FileMode: 0666,
+				Uid:      1000,
+				Gid:      1000,
+				DeviceRule: configs.DeviceRule{
+					Type:  configs.CharDevice,
+					Major: 5,
+					Minor: 0,
+				},
+			}
+
+			if *configDev != *wantDev {
+				t.Errorf("redundant dev was not deduplicated correctly: want %v, got %v", wantDev, configDev)
+			}
+		}
+	}
+
+	// Verify that createDevices() added the entry for /dev/ram0 in the config
+	found = false
+	for _, configDev := range conf.Devices {
+		if configDev.Path == "/dev/ram0" {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("device /dev/ram0 not found in config devices; got %v", conf.Devices)
+	}
+}

tests/integration/dev.bats

@@ -0,0 +1,26 @@
+#!/usr/bin/env bats
+
+load helpers
+
+function setup() {
+	teardown_busybox
+	setup_busybox
+}
+
+function teardown() {
+	teardown_busybox
+}
+
+@test "runc run [redundant default dev]" {
+	update_config ' .linux.devices += [{"path": "/dev/tty", "type": "c", "major": 5, "minor": 0}]
+		      | .process.args |= ["ls", "-l", "/dev/tty"]'
+
+	runc run test_dev
+	[ "$status" -eq 0 ]
+
+	if [[ "$ROOTLESS" -ne 0 ]]; then
+		[[ "${lines[0]}" =~ "crw-rw-rw".+"1".+"nobody".+"nogroup".+"5,".+"0".+"/dev/tty" ]]
+	else
+		[[ "${lines[0]}" =~ "crw-rw-rw".+"1".+"root".+"root".+"5,".+"0".+"/dev/tty" ]]
+	fi
+}