diff --git a/pkg/sentry/fsimpl/cgroupfs/devices.go b/pkg/sentry/fsimpl/cgroupfs/devices.go index 3c1d9d6a5..efec8b6f8 100644 --- a/pkg/sentry/fsimpl/cgroupfs/devices.go +++ b/pkg/sentry/fsimpl/cgroupfs/devices.go @@ -17,8 +17,12 @@ package cgroupfs import ( "bytes" "fmt" + "strconv" + "strings" "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/errors/linuxerr" + "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/sentry/fsimpl/kernfs" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" @@ -28,25 +32,98 @@ import ( ) const ( - allowedDevices = "devices.allow" - deniedDevices = "devices.deny" - controlledDevices = "devices.list" - wildcardDevice = 'a' + canRead = 1 << iota + canWrite + canMknod ) +const ( + allowedDevices = "devices.allow" + controlledDevices = "devices.list" + deniedDevices = "devices.deny" + wildcardDeviceNumber = -1 +) +const ( + blockDevice deviceType = "b" + charDevice deviceType = "c" + wildcardDevice deviceType = "a" +) + +// type denotes a device's type. +type deviceType string + +func (d deviceType) valid() bool { + switch d { + case wildcardDevice, charDevice, blockDevice: + return true + default: + return false + } +} // permission represents a device access, read, write, and mknod. type permission string +func (p permission) valid() bool { + for _, c := range p { + switch c { + case 'r', 'w', 'm': + continue + default: + return false + } + } + return true +} + +// toBinary converts permission to its binary representation. +func (p permission) toBinary() int { + var perm int + for _, c := range p { + switch c { + case 'r': + perm |= canRead + case 'w': + perm |= canWrite + case 'm': + perm |= canMknod + } + } + return perm +} + +// union returns a permission which unions p and perm. +func (p permission) union(perm permission) permission { + return fromBinary(p.toBinary() | perm.toBinary()) +} + +// difference returns a permission which consists of accesses in p and not in perm. +func (p permission) difference(perm permission) permission { + return fromBinary(p.toBinary() & ^perm.toBinary()) +} + +// fromBinary converts permission to its string representation. +func fromBinary(i int) permission { + var perm permission + if i&canRead == canRead { + perm += "r" + } + if i&canWrite == canWrite { + perm += "w" + } + if i&canMknod == canMknod { + perm += "m" + } + return perm +} + // +stateify savable -type deviceRule struct { +type deviceID struct { // Device type, when the type is all, the following fields are ignored. - controllerType rune + controllerType deviceType // The device's major number. - major *int64 + major int64 // The device's minor number. - minor *int64 - // Cgroup access permission. - access permission + minor int64 } // +stateify savable @@ -59,8 +136,8 @@ type devicesController struct { mu sync.Mutex `state:"nosave"` // Allow or deny the device rules below. - allow bool - deviceRules []deviceRule + defaultAllow bool + deviceRules map[deviceID]permission } // +stateify savable @@ -75,7 +152,7 @@ func (d *allowedDevicesData) Generate(ctx context.Context, buf *bytes.Buffer) er // Write implements vfs.WritableDynamicBytesSource.Write. func (d *allowedDevicesData) Write(ctx context.Context, _ *vfs.FileDescription, src usermem.IOSequence, offset int64) (int64, error) { - return write(ctx, src, offset, d.c, true) + return d.c.write(ctx, src, offset, true) } // +stateify savable @@ -90,7 +167,7 @@ func (d *deniedDevicesData) Generate(ctx context.Context, buf *bytes.Buffer) err // Write implements vfs.WritableDynamicBytesSource.Write. func (d *deniedDevicesData) Write(ctx context.Context, _ *vfs.FileDescription, src usermem.IOSequence, offset int64) (int64, error) { - return write(ctx, src, offset, d.c, true) + return d.c.write(ctx, src, offset, false) } // +stateify savable @@ -102,33 +179,138 @@ type controlledDevicesData struct { // // The corresponding devices.list shows devices for which access control is set. func (d *controlledDevicesData) Generate(ctx context.Context, buf *bytes.Buffer) error { - return generate(ctx, buf, d.c) + return d.c.generate(ctx, buf) } -func generate(ctx context.Context, buf *bytes.Buffer, c *devicesController) error { - c.mu.Lock() - defer c.mu.Unlock() - if c.allow && len(c.deviceRules) > 0 { - for _, rule := range c.deviceRules { - if rule.controllerType == wildcardDevice { - buf.WriteString(deviceRuleString(deviceRule{controllerType: wildcardDevice, access: "rwm"})) - return nil - } - buf.WriteString(deviceRuleString(rule)) - // It lists one rule per line. - buf.WriteRune('\n') +func (c *devicesController) addRule(id deviceID, newPermission permission) error { + existingPermission := c.deviceRules[id] + c.deviceRules[id] = existingPermission.union(newPermission) + return nil +} + +func (c *devicesController) removeRule(id deviceID, p permission) error { + // cgroupv1 ignores silently requests to remove a partially-matching wildcard rule, + // which are {majorDevice:wildcardDevice}, {wildcardDevice:minorDevice}, and {wildcardDevice:wildcardDevice} + for _, wildcardDeviceID := range []deviceID{ + {controllerType: id.controllerType, major: id.major, minor: wildcardDeviceNumber}, + {controllerType: id.controllerType, major: wildcardDeviceNumber, minor: id.minor}, + {controllerType: id.controllerType, major: wildcardDeviceNumber, minor: wildcardDeviceNumber}, + } { + // If there is a exact match, the permission needs to be updated. + if id == wildcardDeviceID { + continue + } + if _, exist := c.deviceRules[wildcardDeviceID]; exist { + return nil + } + } + if existingPermission, exist := c.deviceRules[id]; exist { + if newPermission := existingPermission.difference(p); len(newPermission) == 0 { + delete(c.deviceRules, id) + } else { + c.deviceRules[id] = newPermission } - } else { - // When all-all rule presents at devices.list, it actually indicates that - // the cgroup is in black-list mode. - buf.WriteString(deviceRuleString(deviceRule{controllerType: wildcardDevice, access: "rwm"})) } return nil } -func write(ctx context.Context, src usermem.IOSequence, offset int64, c *devicesController, allow bool) (int64, error) { - // TODO(b/289099718): add functions to add and remove rules when writing to device controller data. - return 0, nil +func (c *devicesController) applyRule(id deviceID, p permission, allow bool) error { + if !id.controllerType.valid() { + return linuxerr.EINVAL + } + // If the device type is all, it will reset the rules for all. + if id.controllerType == wildcardDevice { + c.defaultAllow = allow + c.deviceRules = make(map[deviceID]permission) + return nil + } + if !p.valid() { + return linuxerr.EINVAL + } + if len(c.deviceRules) == 0 { + c.defaultAllow = allow + c.deviceRules = make(map[deviceID]permission) + } + if allow == c.defaultAllow { + return c.addRule(id, p) + } + return c.removeRule(id, p) +} + +func (c *devicesController) generate(ctx context.Context, buf *bytes.Buffer) error { + c.mu.Lock() + defer c.mu.Unlock() + switch { + case c.defaultAllow && len(c.deviceRules) > 0: + for id, p := range c.deviceRules { + buf.WriteString(deviceRuleString(id, p)) + // It lists one rule per line. + buf.WriteRune('\n') + } + case c.defaultAllow && len(c.deviceRules) == 0: + buf.WriteString(deviceRuleString(deviceID{controllerType: wildcardDevice, major: wildcardDeviceNumber, minor: wildcardDeviceNumber}, "rwm")) + case !c.defaultAllow && len(c.deviceRules) == 0: + buf.WriteString("") + default: + // When allow-all rule presents at devices.list, it actually indicates that + // the cgroup is in black-list mode. + buf.WriteString(deviceRuleString(deviceID{controllerType: wildcardDevice, major: wildcardDeviceNumber, minor: wildcardDeviceNumber}, "rwm")) + } + return nil +} + +func (c *devicesController) write(ctx context.Context, src usermem.IOSequence, offset int64, allow bool) (int64, error) { + c.mu.Lock() + defer c.mu.Unlock() + if src.NumBytes() > hostarch.PageSize { + return 0, linuxerr.EINVAL + } + buf := copyScratchBufferFromContext(ctx, hostarch.PageSize) + n, err := src.CopyIn(ctx, buf) + if err != nil { + return 0, err + } + rule := string(buf[:n]) + fields := strings.FieldsFunc(rule, func(r rune) bool { + return r == ' ' || r == ':' + }) + switch { + case len(fields) != 1 && len(fields) != 4: + return 0, linuxerr.EINVAL + case len(fields) == 4: + controllerType := deviceType(fields[0]) + perm := permission(fields[3]) + if i := strings.IndexFunc(fields[3], func(r rune) bool { return r == '\n' }); i != -1 { + perm = perm[:i] + } + if len(perm) > 3 { + perm = perm[:3] + } + majorDevice, err := toDeviceNumber(fields[1]) + if err != nil { + return 0, err + } + minorDevice, err := toDeviceNumber(fields[2]) + if err != nil { + return 0, err + } + id := deviceID{ + controllerType: controllerType, + major: majorDevice, + minor: minorDevice, + } + if err := c.applyRule(id, perm, allow); err != nil { + return 0, err + } + case len(fields) == 1: + if deviceType(fields[0]) != wildcardDevice { + return 0, linuxerr.EINVAL + } + if err := c.applyRule(deviceID{controllerType: wildcardDevice}, permission(""), allow); err != nil { + return 0, err + } + } + return int64(n), nil } var _ controller = (*devicesController)(nil) @@ -136,8 +318,8 @@ var _ controller = (*devicesController)(nil) func newDevicesController(fs *filesystem) *devicesController { // The root device cgroup starts with rwm to all. c := &devicesController{ - allow: true, - deviceRules: []deviceRule{}, + defaultAllow: true, + deviceRules: make(map[deviceID]permission), } c.controllerCommon.init(kernel.CgroupControllerDevices, fs) return c @@ -147,11 +329,13 @@ func newDevicesController(fs *filesystem) *devicesController { func (c *devicesController) Clone() controller { c.mu.Lock() defer c.mu.Unlock() - newRules := make([]deviceRule, len(c.deviceRules)) - copy(newRules, c.deviceRules) + newRules := make(map[deviceID]permission) + for id, p := range c.deviceRules { + newRules[id] = p + } new := &devicesController{ - allow: c.allow, - deviceRules: newRules, + defaultAllow: c.defaultAllow, + deviceRules: newRules, } new.controllerCommon.cloneFromParent(c) return new @@ -164,14 +348,25 @@ func (c *devicesController) AddControlFiles(ctx context.Context, creds *auth.Cre contents[controlledDevices] = c.fs.newControllerFile(ctx, creds, &controlledDevicesData{c: c}, true) } -func deviceRuleString(rule deviceRule) string { - return fmt.Sprintf("%c %s:%s %s", rule.controllerType, deviceNumber(rule.major), deviceNumber(rule.minor), rule.access) +func deviceRuleString(id deviceID, p permission) string { + return fmt.Sprintf("%s %s:%s %s", id.controllerType, deviceNumber(id.major), deviceNumber(id.minor), p) } // deviceNumber converts a device number to string. -func deviceNumber(number *int64) string { - if number == nil { +func deviceNumber(number int64) string { + if number == wildcardDeviceNumber { return "*" } return fmt.Sprint(number) } + +func toDeviceNumber(s string) (int64, error) { + if s == "*" { + return wildcardDeviceNumber, nil + } + val, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, err + } + return val, nil +} diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc index d21faceac..4fca07c7e 100644 --- a/test/syscalls/linux/cgroup.cc +++ b/test/syscalls/linux/cgroup.cc @@ -1442,6 +1442,84 @@ TEST(DevicesCgroup, ControlFilesExist) { IsPosixErrorOkAndHolds("a *:* rwm")); } +TEST(DevicesCgroup, DenyAll) { + SKIP_IF(!CgroupsAvailable()); + + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("devices")); + + ASSERT_NO_ERRNO(c.WriteControlFile("devices.allow", "b *:* rw\n")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("b *:* rw\n")); + + ASSERT_NO_ERRNO(c.WriteControlFile("devices.deny", "a")); + EXPECT_THAT(c.ReadControlFile("devices.list"), IsPosixErrorOkAndHolds("")); +} + +TEST(DevicesCgroup, AddDeviceRule) { + SKIP_IF(!CgroupsAvailable()); + + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("devices")); + + ASSERT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("a *:* rwm")); + // Gives character devices with major device number 7 read and write + // permission. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.allow", "c 7:* rw\n")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("c 7:* rw\n")); + + // Diasllows all devices. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.deny", "a")); + EXPECT_THAT(c.ReadControlFile("devices.list"), IsPosixErrorOkAndHolds("")); + + // Adds one more rule. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.allow", "b *:* rw\n")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("b *:* rw\n")); +} + +TEST(DevicesCgroup, RemoveDeviceRule) { + SKIP_IF(!CgroupsAvailable()); + + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("devices")); + // The root group starts with allowing rwm to all. + ASSERT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("a *:* rwm")); + // Gives character devices with the major device number 7 read and write + // permission. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.allow", "c 7:* rw")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("c 7:* rw\n")); + + // Removes the write permission from the character devices with the major + // device number 7. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.deny", "c 7:* w")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("c 7:* r\n")); +} + +TEST(DevicesCgroup, IgnorePartialMatchRule) { + SKIP_IF(!CgroupsAvailable()); + + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("devices")); + + // Gives character devices with the major device number 7 read and write + // permission. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.allow", "c 7:* rw")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("c 7:* rw\n")); + + // Expect no change to the allow list since minor device matches partially a + // exsting rule for character devices 7:*. + ASSERT_NO_ERRNO(c.WriteControlFile("devices.deny", "c 7:0 w")); + EXPECT_THAT(c.ReadControlFile("devices.list"), + IsPosixErrorOkAndHolds("c 7:* rw\n")); +} + } // namespace } // namespace testing } // namespace gvisor