diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 6fd98f027..0ef707acd 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/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 { diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 1a506adc1..52954399e 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/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) + } +} diff --git a/tests/integration/dev.bats b/tests/integration/dev.bats new file mode 100644 index 000000000..2ce97615a --- /dev/null +++ b/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 +}