Skip to content

Commit

Permalink
merge branch 'pr-2522' into master
Browse files Browse the repository at this point in the history
Cesar Talledo (2):
  Remove runc default devices that overlap with spec devices.
  Skip redundant setup for /dev/ptmx when specified explicitly in the OCI spec.

LGTMs: @AkihiroSuda @cyphar
Closes #2522
  • Loading branch information
cyphar committed Aug 19, 2020
2 parents a5847db + 9a699e1 commit 2265daa
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 18 deletions.
7 changes: 7 additions & 0 deletions libcontainer/rootfs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/system"
"github.com/opencontainers/runc/libcontainer/utils"
libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/selinux/go-selinux/label"
Expand Down Expand Up @@ -590,6 +591,12 @@ func createDevices(config *configs.Config) error {
useBindMount := system.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER)
oldMask := unix.Umask(0000)
for _, node := range config.Devices {

// The /dev/ptmx device is setup by setupPtmx()
if utils.CleanPath(node.Path) == "/dev/ptmx" {
continue
}

// containers running in a user namespace are not allowed to mknod
// devices so we can just bind mount it from the host.
if err := createDeviceNode(config.Rootfs, node, useBindMount); err != nil {
Expand Down
42 changes: 30 additions & 12 deletions libcontainer/specconv/spec_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

defaultDevs, err := createDevices(spec, config)
if err != nil {
return nil, err
}
c, err := CreateCgroupConfig(opts)

c, err := CreateCgroupConfig(opts, defaultDevs)
if err != nil {
return nil, err
}

config.Cgroups = c
// set linux-specific config
if spec.Linux != nil {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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 {
Expand Down
116 changes: 110 additions & 6 deletions libcontainer/specconv/spec_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
35 changes: 35 additions & 0 deletions tests/integration/dev.bats
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#!/usr/bin/env bats

load helpers

function setup() {
teardown_busybox
setup_busybox
}

function teardown() {
teardown_busybox
}

@test "runc run [redundant default /dev/tty]" {
update_config ' .linux.devices += [{"path": "/dev/tty", "type": "c", "major": 5, "minor": 0}]
| .process.args |= ["ls", "-lL", "/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
}

@test "runc run [redundant default /dev/ptmx]" {
update_config ' .linux.devices += [{"path": "/dev/ptmx", "type": "c", "major": 5, "minor": 2}]
| .process.args |= ["ls", "-lL", "/dev/ptmx"]'

runc run test_dev
[ "$status" -eq 0 ]
[[ "${lines[0]}" =~ "crw-rw-rw".+"1".+"root".+"root".+"5,".+"2".+"/dev/ptmx" ]]
}

0 comments on commit 2265daa

Please sign in to comment.