configs: use different types for .Devices and .Resources.Devices

Making them the same type is simply confusing, but also means that you
could accidentally use one in the wrong context. This eliminates that
problem. This also includes a whole bunch of cleanups for the types
within DeviceRule, so that they can be used more ergonomically.

Signed-off-by: Aleksa Sarai <asarai@suse.de>

libcontainer/cgroups/ebpf/devicefilter/devicefilter.go

@@ -22,7 +22,7 @@ const (
 )
 
 // DeviceFilter returns eBPF device filter program and its license string
-func DeviceFilter(devices []*configs.Device) (asm.Instructions, string, error) {
+func DeviceFilter(devices []*configs.DeviceRule) (asm.Instructions, string, error) {
 	p := &program{}
 	p.init()
 	for i := len(devices) - 1; i >= 0; i-- {
@@ -68,7 +68,7 @@ func (p *program) init() {
 }
 
 // appendDevice needs to be called from the last element of OCI linux.resources.devices to the head element.
-func (p *program) appendDevice(dev *configs.Device) error {
+func (p *program) appendDevice(dev *configs.DeviceRule) error {
 	if p.blockID < 0 {
 		return errors.New("the program is finalized")
 	}

libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go

@@ -20,7 +20,7 @@ func hash(s, comm string) string {
 	return strings.Join(res, "\n")
 }
 
-func testDeviceFilter(t testing.TB, devices []*configs.Device, expectedStr string) {
+func testDeviceFilter(t testing.TB, devices []*configs.DeviceRule, expectedStr string) {
 	insts, _, err := DeviceFilter(devices)
 	if err != nil {
 		t.Fatalf("%s: %v (devices: %+v)", t.Name(), err, devices)
@@ -137,11 +137,15 @@ block-11:
         62: Mov32Imm dst: r0 imm: 0
         63: Exit
 `
-	testDeviceFilter(t, specconv.AllowedDevices, expected)
+	var devices []*configs.DeviceRule
+	for _, device := range specconv.AllowedDevices {
+		devices = append(devices, &device.DeviceRule)
+	}
+	testDeviceFilter(t, devices, expected)
 }
 
 func TestDeviceFilter_Privileged(t *testing.T) {
-	devices := []*configs.Device{
+	devices := []*configs.DeviceRule{
 		{
 			Type:        'a',
 			Major:       -1,
@@ -168,7 +172,7 @@ block-0:
 }
 
 func TestDeviceFilter_PrivilegedExceptSingleDevice(t *testing.T) {
-	devices := []*configs.Device{
+	devices := []*configs.DeviceRule{
 		{
 			Type:        'a',
 			Major:       -1,
@@ -208,7 +212,7 @@ block-1:
 }
 
 func TestDeviceFilter_Weird(t *testing.T) {
-	devices := []*configs.Device{
+	devices := []*configs.DeviceRule{
 		{
 			Type:        'b',
 			Major:       8,

libcontainer/cgroups/fs/devices_test.go

@@ -18,14 +18,12 @@ func TestDevicesSetAllow(t *testing.T) {
 		"devices.deny":  "",
 	})
 
-	helper.CgroupData.config.Resources.Devices = []*configs.Device{
+	helper.CgroupData.config.Resources.Devices = []*configs.DeviceRule{
 		{
-			Path:        "/dev/zero",
-			Type:        'c',
+			Type:        configs.CharDevice,
 			Major:       1,
 			Minor:       5,
-			Permissions: "rwm",
-			FileMode:    0666,
+			Permissions: configs.DevicePermissions("rwm"),
 			Allow:       true,
 		},
 	}

libcontainer/cgroups/fs2/devices.go

@@ -10,12 +10,10 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-func isRWM(cgroupPermissions string) bool {
-	r := false
-	w := false
-	m := false
-	for _, rn := range cgroupPermissions {
-		switch rn {
+func isRWM(perms configs.DevicePermissions) bool {
+	var r, w, m bool
+	for _, perm := range perms {
+		switch perm {
 		case 'r':
 			r = true
 		case 'w':

libcontainer/configs/cgroup_linux.go

@@ -42,7 +42,7 @@ type Cgroup struct {
 
 type Resources struct {
 	// Devices is the set of access rules for devices in the container.
-	Devices []*Device `json:"devices"`
+	Devices []*DeviceRule `json:"devices"`
 
 	// Memory limit (in bytes)
 	Memory int64 `json:"memory"`

libcontainer/configs/device.go

@@ -1,8 +1,12 @@
 package configs
 
 import (
+	"errors"
 	"fmt"
 	"os"
+	"strconv"
+
+	"golang.org/x/sys/unix"
 )
 
 const (
@@ -12,21 +16,11 @@ const (
 // TODO Windows: This can be factored out in the future
 
 type Device struct {
-	// Device type, block, char, etc.
-	Type rune `json:"type"`
+	DeviceRule
 
 	// Path to the device.
 	Path string `json:"path"`
 
-	// Major is the device's major number.
-	Major int64 `json:"major"`
-
-	// Minor is the device's minor number.
-	Minor int64 `json:"minor"`
-
-	// Cgroup permissions format, rwm.
-	Permissions string `json:"permissions"`
-
 	// FileMode permission bits for the device.
 	FileMode os.FileMode `json:"file_mode"`
 
@@ -35,23 +29,154 @@ type Device struct {
 
 	// Gid of the device.
 	Gid uint32 `json:"gid"`
+}
 
-	// Write the file to the allowed list
+// DevicePermissions is a cgroupv1-style string to represent device access. It
+// has to be a string for backward compatibility reasons, hence why it has
+// methods to do set operations.
+type DevicePermissions string
+
+const (
+	deviceRead uint = (1 << iota)
+	deviceWrite
+	deviceMknod
+)
+
+func (p DevicePermissions) toSet() uint {
+	var set uint
+	for _, perm := range p {
+		switch perm {
+		case 'r':
+			set |= deviceRead
+		case 'w':
+			set |= deviceWrite
+		case 'm':
+			set |= deviceMknod
+		}
+	}
+	return set
+}
+
+func fromSet(set uint) DevicePermissions {
+	var perm string
+	if set&deviceRead == deviceRead {
+		perm += "r"
+	}
+	if set&deviceWrite == deviceWrite {
+		perm += "w"
+	}
+	if set&deviceMknod == deviceMknod {
+		perm += "m"
+	}
+	return DevicePermissions(perm)
+}
+
+// Union returns the union of the two sets of DevicePermissions.
+func (p DevicePermissions) Union(o DevicePermissions) DevicePermissions {
+	lhs := p.toSet()
+	rhs := o.toSet()
+	return fromSet(lhs | rhs)
+}
+
+// Difference returns the set difference of the two sets of DevicePermissions.
+// In set notation, A.Difference(B) gives you A\B.
+func (p DevicePermissions) Difference(o DevicePermissions) DevicePermissions {
+	lhs := p.toSet()
+	rhs := o.toSet()
+	return fromSet(lhs &^ rhs)
+}
+
+// Intersection computes the intersection of the two sets of DevicePermissions.
+func (p DevicePermissions) Intersection(o DevicePermissions) DevicePermissions {
+	lhs := p.toSet()
+	rhs := o.toSet()
+	return fromSet(lhs & rhs)
+}
+
+// IsEmpty returns whether the set of permissions in a DevicePermissions is
+// empty.
+func (p DevicePermissions) IsEmpty() bool {
+	return p == DevicePermissions("")
+}
+
+// IsValid returns whether the set of permissions is a subset of valid
+// permissions (namely, {r,w,m}).
+func (p DevicePermissions) IsValid() bool {
+	return p == fromSet(p.toSet())
+}
+
+type DeviceType rune
+
+const (
+	WildcardDevice DeviceType = 'a'
+	BlockDevice    DeviceType = 'b'
+	CharDevice     DeviceType = 'c' // or 'u'
+	FifoDevice     DeviceType = 'p'
+)
+
+func (t DeviceType) IsValid() bool {
+	switch t {
+	case WildcardDevice, BlockDevice, CharDevice, FifoDevice:
+		return true
+	default:
+		return false
+	}
+}
+
+func (t DeviceType) CanMknod() bool {
+	switch t {
+	case BlockDevice, CharDevice, FifoDevice:
+		return true
+	default:
+		return false
+	}
+}
+
+func (t DeviceType) CanCgroup() bool {
+	switch t {
+	case WildcardDevice, BlockDevice, CharDevice:
+		return true
+	default:
+		return false
+	}
+}
+
+type DeviceRule struct {
+	// Type of device ('c' for char, 'b' for block). If set to 'a', this rule
+	// acts as a wildcard and all fields other than Allow are ignored.
+	Type DeviceType `json:"type"`
+
+	// Major is the device's major number.
+	Major int64 `json:"major"`
+
+	// Minor is the device's minor number.
+	Minor int64 `json:"minor"`
+
+	// Permissions is the set of permissions that this rule applies to (in the
+	// cgroupv1 format -- any combination of "rwm").
+	Permissions DevicePermissions `json:"permissions"`
+
+	// Allow specifies whether this rule is allowed.
 	Allow bool `json:"allow"`
 }
 
-func (d *Device) CgroupString() string {
-	return fmt.Sprintf("%c %s:%s %s", d.Type, deviceNumberString(d.Major), deviceNumberString(d.Minor), d.Permissions)
-}
-
-func (d *Device) Mkdev() int {
-	return int((d.Major << 8) | (d.Minor & 0xff) | ((d.Minor & 0xfff00) << 12))
-}
-
-// deviceNumberString converts the device number to a string return result.
-func deviceNumberString(number int64) string {
-	if number == Wildcard {
-		return "*"
+func (d *DeviceRule) CgroupString() string {
+	var (
+		major = strconv.FormatInt(d.Major, 10)
+		minor = strconv.FormatInt(d.Minor, 10)
+	)
+	if d.Major == Wildcard {
+		major = "*"
 	}
-	return fmt.Sprint(number)
+	if d.Minor == Wildcard {
+		minor = "*"
+	}
+	return fmt.Sprintf("%c %s:%s %s", d.Type, major, minor, d.Permissions)
+}
+
+func (d *DeviceRule) Mkdev() (uint64, error) {
+	if d.Major == Wildcard || d.Minor == Wildcard {
+		return 0, errors.New("cannot mkdev() device with wildcards")
+	}
+	return unix.Mkdev(uint32(d.Major), uint32(d.Minor)), nil
 }

libcontainer/devices/devices.go

@@ -31,33 +31,33 @@ func DeviceFromPath(path, permissions string) (*configs.Device, error) {
 	}
 
 	var (
+		devType   configs.DeviceType
+		mode      = stat.Mode
 		devNumber = uint64(stat.Rdev)
 		major     = unix.Major(devNumber)
 		minor     = unix.Minor(devNumber)
 	)
-	if major == 0 {
-		return nil, ErrNotADevice
-	}
-
-	var (
-		devType rune
-		mode    = stat.Mode
-	)
 	switch {
 	case mode&unix.S_IFBLK == unix.S_IFBLK:
-		devType = 'b'
+		devType = configs.BlockDevice
 	case mode&unix.S_IFCHR == unix.S_IFCHR:
-		devType = 'c'
+		devType = configs.CharDevice
+	case mode&unix.S_IFIFO == unix.S_IFIFO:
+		devType = configs.FifoDevice
+	default:
+		return nil, ErrNotADevice
 	}
 	return &configs.Device{
-		Type:        devType,
-		Path:        path,
-		Major:       int64(major),
-		Minor:       int64(minor),
-		Permissions: permissions,
-		FileMode:    os.FileMode(mode),
-		Uid:         stat.Uid,
-		Gid:         stat.Gid,
+		DeviceRule: configs.DeviceRule{
+			Type:        devType,
+			Major:       int64(major),
+			Minor:       int64(minor),
+			Permissions: configs.DevicePermissions(permissions),
+		},
+		Path:     path,
+		FileMode: os.FileMode(mode),
+		Uid:      stat.Uid,
+		Gid:      stat.Gid,
 	}, nil
 }
 

libcontainer/integration/template_test.go

@@ -21,6 +21,10 @@ const defaultMountFlags = unix.MS_NOEXEC | unix.MS_NOSUID | unix.MS_NODEV
 // it uses a network strategy of just setting a loopback interface
 // and the default setup for devices
 func newTemplateConfig(rootfs string) *configs.Config {
+	var allowedDevices []*configs.DeviceRule
+	for _, device := range specconv.AllowedDevices {
+		allowedDevices = append(allowedDevices, &device.DeviceRule)
+	}
 	return &configs.Config{
 		Rootfs: rootfs,
 		Capabilities: &configs.Capabilities{
@@ -116,7 +120,7 @@ func newTemplateConfig(rootfs string) *configs.Config {
 			Path: "integration/test",
 			Resources: &configs.Resources{
 				MemorySwappiness: nil,
-				Devices:          specconv.AllowedDevices,
+				Devices:          allowedDevices,
 			},
 		},
 		MaskPaths: []string{

libcontainer/rootfs_linux.go

@@ -631,16 +631,20 @@ func createDeviceNode(rootfs string, node *configs.Device, bind bool) error {
 func mknodDevice(dest string, node *configs.Device) error {
 	fileMode := node.FileMode
 	switch node.Type {
-	case 'c', 'u':
-		fileMode |= unix.S_IFCHR
-	case 'b':
+	case configs.BlockDevice:
 		fileMode |= unix.S_IFBLK
-	case 'p':
+	case configs.CharDevice:
+		fileMode |= unix.S_IFCHR
+	case configs.FifoDevice:
 		fileMode |= unix.S_IFIFO
 	default:
 		return fmt.Errorf("%c is not a valid device type for device %s", node.Type, node.Path)
 	}
-	if err := unix.Mknod(dest, uint32(fileMode), node.Mkdev()); err != nil {
+	dev, err := node.Mkdev()
+	if err != nil {
+		return err
+	}
+	if err := unix.Mknod(dest, uint32(fileMode), int(dev)); err != nil {
 		return err
 	}
 	return unix.Chown(dest, int(node.Uid), int(node.Gid))

libcontainer/specconv/spec_linux.go

@@ -66,92 +66,129 @@ var mountPropagationMapping = map[string]int{
 var AllowedDevices = []*configs.Device{
 	// allow mknod for any device
 	{
-		Type:        'c',
-		Major:       wildcard,
-		Minor:       wildcard,
-		Permissions: "m",
-		Allow:       true,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       configs.Wildcard,
+			Minor:       configs.Wildcard,
+			Permissions: "m",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'b',
-		Major:       wildcard,
-		Minor:       wildcard,
-		Permissions: "m",
-		Allow:       true,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.BlockDevice,
+			Major:       configs.Wildcard,
+			Minor:       configs.Wildcard,
+			Permissions: "m",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/null",
-		Major:       1,
-		Minor:       3,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/null",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       1,
+			Minor:       3,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/random",
-		Major:       1,
-		Minor:       8,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/random",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       1,
+			Minor:       8,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/full",
-		Major:       1,
-		Minor:       7,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/full",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       1,
+			Minor:       7,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/tty",
-		Major:       5,
-		Minor:       0,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/tty",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       5,
+			Minor:       0,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/zero",
-		Major:       1,
-		Minor:       5,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/zero",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       1,
+			Minor:       5,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Type:        'c',
-		Path:        "/dev/urandom",
-		Major:       1,
-		Minor:       9,
-		Permissions: "rwm",
-		Allow:       true,
+		Path:     "/dev/urandom",
+		FileMode: 0666,
+		Uid:      0,
+		Gid:      0,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       1,
+			Minor:       9,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	// /dev/pts/ - pts namespaces are "coming soon"
 	{
-		Path:        "",
-		Type:        'c',
-		Major:       136,
-		Minor:       wildcard,
-		Permissions: "rwm",
-		Allow:       true,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       136,
+			Minor:       configs.Wildcard,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	{
-		Path:        "",
-		Type:        'c',
-		Major:       5,
-		Minor:       2,
-		Permissions: "rwm",
-		Allow:       true,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       5,
+			Minor:       2,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 	// tuntap
 	{
-		Path:        "",
-		Type:        'c',
-		Major:       10,
-		Minor:       200,
-		Permissions: "rwm",
-		Allow:       true,
+		DeviceRule: configs.DeviceRule{
+			Type:        configs.CharDevice,
+			Major:       10,
+			Minor:       200,
+			Permissions: "rwm",
+			Allow:       true,
+		},
 	},
 }
 
@@ -451,14 +488,13 @@ func CreateCgroupConfig(opts *CreateOpts) (*configs.Cgroup, error) {
 				if err != nil {
 					return nil, err
 				}
-				dd := &configs.Device{
+				c.Resources.Devices = append(c.Resources.Devices, &configs.DeviceRule{
 					Type:        dt,
 					Major:       major,
 					Minor:       minor,
-					Permissions: d.Access,
+					Permissions: configs.DevicePermissions(d.Access),
 					Allow:       d.Allow,
-				}
-				c.Resources.Devices = append(c.Resources.Devices, dd)
+				})
 			}
 			if r.Memory != nil {
 				if r.Memory.Limit != nil {
@@ -583,98 +619,48 @@ func CreateCgroupConfig(opts *CreateOpts) (*configs.Cgroup, error) {
 			}
 		}
 	}
-	// append the default allowed devices to the end of the list
-	c.Resources.Devices = append(c.Resources.Devices, AllowedDevices...)
+	// Append the default allowed devices to the end of the list.
+	// XXX: Really this should be prefixed...
+	for _, device := range AllowedDevices {
+		c.Resources.Devices = append(c.Resources.Devices, &device.DeviceRule)
+	}
 	return c, nil
 }
 
-func stringToCgroupDeviceRune(s string) (rune, error) {
+func stringToCgroupDeviceRune(s string) (configs.DeviceType, error) {
 	switch s {
 	case "a":
-		return 'a', nil
+		return configs.WildcardDevice, nil
 	case "b":
-		return 'b', nil
+		return configs.BlockDevice, nil
 	case "c":
-		return 'c', nil
+		return configs.CharDevice, nil
 	default:
 		return 0, fmt.Errorf("invalid cgroup device type %q", s)
 	}
 }
 
-func stringToDeviceRune(s string) (rune, error) {
+func stringToDeviceRune(s string) (configs.DeviceType, error) {
 	switch s {
 	case "p":
-		return 'p', nil
-	case "u":
-		return 'u', nil
+		return configs.FifoDevice, nil
+	case "u", "c":
+		return configs.CharDevice, nil
 	case "b":
-		return 'b', nil
-	case "c":
-		return 'c', nil
+		return configs.BlockDevice, nil
 	default:
 		return 0, fmt.Errorf("invalid device type %q", s)
 	}
 }
 
 func createDevices(spec *specs.Spec, config *configs.Config) error {
-	// add whitelisted devices
-	config.Devices = []*configs.Device{
-		{
-			Type:     'c',
-			Path:     "/dev/null",
-			Major:    1,
-			Minor:    3,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
-		{
-			Type:     'c',
-			Path:     "/dev/random",
-			Major:    1,
-			Minor:    8,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
-		{
-			Type:     'c',
-			Path:     "/dev/full",
-			Major:    1,
-			Minor:    7,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
-		{
-			Type:     'c',
-			Path:     "/dev/tty",
-			Major:    5,
-			Minor:    0,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
-		{
-			Type:     'c',
-			Path:     "/dev/zero",
-			Major:    1,
-			Minor:    5,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
-		{
-			Type:     'c',
-			Path:     "/dev/urandom",
-			Major:    1,
-			Minor:    9,
-			FileMode: 0666,
-			Uid:      0,
-			Gid:      0,
-		},
+	// Add default set of devices.
+	for _, device := range AllowedDevices {
+		if device.Path != "" {
+			config.Devices = append(config.Devices, device)
+		}
 	}
-	// merge in additional devices from the spec
+	// Merge in additional devices from the spec.
 	if spec.Linux != nil {
 		for _, d := range spec.Linux.Devices {
 			var uid, gid uint32
@@ -694,10 +680,12 @@ func createDevices(spec *specs.Spec, config *configs.Config) error {
 				filemode = *d.FileMode
 			}
 			device := &configs.Device{
-				Type:     dt,
+				DeviceRule: configs.DeviceRule{
+					Type:  dt,
+					Major: d.Major,
+					Minor: d.Minor,
+				},
 				Path:     d.Path,
-				Major:    d.Major,
-				Minor:    d.Minor,
 				FileMode: filemode,
 				Uid:      uid,
 				Gid:      gid,