Let devices cgroup add/remove rules via devices.allow and devices.deny.

PiperOrigin-RevId: 563261158

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
+}

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