From 24388be71e1aef7facd0d78dda22e696c1694272 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 7 May 2020 13:59:36 +1000 Subject: [PATCH] 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 --- .../cgroups/ebpf/devicefilter/devicefilter.go | 4 +- .../ebpf/devicefilter/devicefilter_test.go | 14 +- libcontainer/cgroups/fs/devices_test.go | 8 +- libcontainer/cgroups/fs2/devices.go | 10 +- libcontainer/configs/cgroup_linux.go | 2 +- libcontainer/configs/device.go | 175 ++++++++++-- libcontainer/devices/devices.go | 36 +-- libcontainer/integration/template_test.go | 6 +- libcontainer/rootfs_linux.go | 14 +- libcontainer/specconv/spec_linux.go | 270 +++++++++--------- 10 files changed, 330 insertions(+), 209 deletions(-) diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go index 5a1cdd91..b593eb43 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go +++ b/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") } diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go index af432e52..d81e90f8 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go +++ b/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, diff --git a/libcontainer/cgroups/fs/devices_test.go b/libcontainer/cgroups/fs/devices_test.go index 90b9f084..77e29124 100644 --- a/libcontainer/cgroups/fs/devices_test.go +++ b/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, }, } diff --git a/libcontainer/cgroups/fs2/devices.go b/libcontainer/cgroups/fs2/devices.go index 96d6daeb..ee180bda 100644 --- a/libcontainer/cgroups/fs2/devices.go +++ b/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': diff --git a/libcontainer/configs/cgroup_linux.go b/libcontainer/configs/cgroup_linux.go index 1d2dca43..6ab03b76 100644 --- a/libcontainer/configs/cgroup_linux.go +++ b/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"` diff --git a/libcontainer/configs/device.go b/libcontainer/configs/device.go index 8701bb21..24c5bbfa 100644 --- a/libcontainer/configs/device.go +++ b/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 } diff --git a/libcontainer/devices/devices.go b/libcontainer/devices/devices.go index 5dabe06c..702f913e 100644 --- a/libcontainer/devices/devices.go +++ b/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 } diff --git a/libcontainer/integration/template_test.go b/libcontainer/integration/template_test.go index 3f3a0811..30503a98 100644 --- a/libcontainer/integration/template_test.go +++ b/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{ diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 40a5686d..1052b965 100644 --- a/libcontainer/rootfs_linux.go +++ b/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)) diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 881b42db..6df2e2af 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/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,