fix(container): make --device-cgroup-rule allow list self-contained

cmd/nerdctl/container/container_run_cgroup_linux_test.go

@@ -498,50 +498,51 @@ func TestParseDevice(t *testing.T) {
 
 func TestParseDeviceCgroupRule(t *testing.T) {
 	t.Parallel()
+	int64Ptr := func(v int64) *int64 { return &v }
 	type testCase struct {
 		s              string
 		expectedType   string
-		expectedMajor  int64
-		expectedMinor  int64
+		expectedMajor  *int64
+		expectedMinor  *int64
 		expectedAccess string
 		err            string
 	}
 	testCases := []testCase{
 		{
 			s:              "c 189:* rmw",
 			expectedType:   "c",
-			expectedMajor:  189,
-			expectedMinor:  -1,
+			expectedMajor:  int64Ptr(189),
+			expectedMinor:  nil,
 			expectedAccess: "rmw",
 		},
 		{
 			s:              "b 8:0 r",
 			expectedType:   "b",
-			expectedMajor:  8,
-			expectedMinor:  0,
+			expectedMajor:  int64Ptr(8),
+			expectedMinor:  int64Ptr(0),
 			expectedAccess: "r",
 		},
 		{
 			s:              "a *:* rwm",
 			expectedType:   "a",
-			expectedMajor:  -1,
-			expectedMinor:  -1,
+			expectedMajor:  nil,
+			expectedMinor:  nil,
 			expectedAccess: "rwm",
 		},
 		{
 			s:              "c *:5 w",
 			expectedType:   "c",
-			expectedMajor:  -1,
-			expectedMinor:  5,
+			expectedMajor:  nil,
+			expectedMinor:  int64Ptr(5),
 			expectedAccess: "w",
 		},
 		{
 			// Docker keeps the numbers given for type "a" rather than
 			// normalizing them, so the rule has to survive unchanged.
 			s:              "a 1:2 rwm",
 			expectedType:   "a",
-			expectedMajor:  1,
-			expectedMinor:  2,
+			expectedMajor:  int64Ptr(1),
+			expectedMinor:  int64Ptr(2),
 			expectedAccess: "rwm",
 		},
 		{
@@ -587,8 +588,8 @@ func TestParseDeviceCgroupRule(t *testing.T) {
 			assert.NilError(t, err)
 			assert.Equal(t, true, rule.Allow)
 			assert.Equal(t, tc.expectedType, rule.Type)
-			assert.Equal(t, tc.expectedMajor, *rule.Major)
-			assert.Equal(t, tc.expectedMinor, *rule.Minor)
+			assert.DeepEqual(t, tc.expectedMajor, rule.Major)
+			assert.DeepEqual(t, tc.expectedMinor, rule.Minor)
 			assert.Equal(t, tc.expectedAccess, rule.Access)
 		} else {
 			assert.ErrorContains(t, err, tc.err)
@@ -612,6 +613,9 @@ func TestRunDeviceCgroupRule(t *testing.T) {
 			Expected: test.Expects(expect.ExitCodeSuccess, nil, nil),
 		},
 		{
+			// Both CLIs reject the missing minor, but with different wording
+			// (nerdctl: "invalid device cgroup rule format", docker: "invalid
+			// device cgroup format"), so only the common prefix is matched.
 			Description: "a malformed rule is rejected",
 			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
 				return helpers.Command("run", "--rm",
@@ -621,7 +625,7 @@ func TestRunDeviceCgroupRule(t *testing.T) {
 			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
 				return &test.Expected{
 					ExitCode: expect.ExitCodeGenericFail,
-					Errors:   []error{fmt.Errorf("invalid device cgroup rule format")},
+					Errors:   []error{fmt.Errorf("invalid device cgroup")},
 				}
 			},
 		},
@@ -636,7 +640,7 @@ func TestRunDeviceCgroupRule(t *testing.T) {
 			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
 				return &test.Expected{
 					ExitCode: expect.ExitCodeGenericFail,
-					Errors:   []error{fmt.Errorf("invalid device cgroup rule format")},
+					Errors:   []error{fmt.Errorf("invalid device cgroup")},
 				}
 			},
 		},
@@ -645,6 +649,28 @@ func TestRunDeviceCgroupRule(t *testing.T) {
 	testCase.Run(t)
 }
 
+func TestRunDeviceCgroupRuleDefaultDevices(t *testing.T) {
+	testCase := nerdtest.Setup()
+
+	testCase.SubTests = []*test.Case{
+		{
+			// The rule only grants access to a non-standard major number (189,
+			// nvidia GPU), so the container's access to /dev/null, /dev/zero and
+			// /dev/urandom must come from the default allow list in the spec.
+			Description: "standard devices remain accessible",
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("run", "--rm",
+					"--device-cgroup-rule", "c 189:* rmw",
+					testutil.AlpineImage, "sh", "-c",
+					"cat /dev/null && head -c 1 /dev/zero > /dev/null && head -c 1 /dev/urandom > /dev/null && echo ok")
+			},
+			Expected: test.Expects(expect.ExitCodeSuccess, nil, expect.Contains("ok")),
+		},
+	}
+
+	testCase.Run(t)
+}
+
 func TestRunCgroupConf(t *testing.T) {
 	testCase := nerdtest.Setup()
 	testCase.Require = require.All(

pkg/cmd/container/run_cgroup_linux.go

@@ -340,19 +340,25 @@ func validateDeviceMode(mode string) error {
 var deviceCgroupRuleRegexp = regexp.MustCompile(`^([acb]) ([0-9]+|\*):([0-9]+|\*) ([rwm]{1,3})$`)
 
 // ParseDeviceCgroupRule parses the given device cgroup rule into an entry for the
-// device allow list of the OCI spec. A "*" major or minor number becomes -1, which
-// the runtime expands to "any".
+// device allow list of the OCI spec. A "*" major or minor number is left unset,
+// which both runc and crun read as "any". It must not be spelled out as -1: runc
+// maps a missing field to -1 itself, but crun writes a present -1 verbatim
+// ("c -1:-1 m") and the kernel rejects that on cgroup v1.
 func ParseDeviceCgroupRule(s string) (specs.LinuxDeviceCgroup, error) {
 	matches := deviceCgroupRuleRegexp.FindStringSubmatch(s)
 	if matches == nil {
 		return specs.LinuxDeviceCgroup{}, fmt.Errorf("invalid device cgroup rule format: %q", s)
 	}
 
-	parseNumber := func(field string) (int64, error) {
+	parseNumber := func(field string) (*int64, error) {
 		if field == "*" {
-			return -1, nil
+			return nil, nil
 		}
-		return strconv.ParseInt(field, 10, 64)
+		value, err := strconv.ParseInt(field, 10, 64)
+		if err != nil {
+			return nil, err
+		}
+		return &value, nil
 	}
 
 	major, err := parseNumber(matches[2])
@@ -367,22 +373,110 @@ func ParseDeviceCgroupRule(s string) (specs.LinuxDeviceCgroup, error) {
 	return specs.LinuxDeviceCgroup{
 		Allow:  true,
 		Type:   matches[1],
-		Major:  &major,
-		Minor:  &minor,
+		Major:  major,
+		Minor:  minor,
 		Access: matches[4],
 	}, nil
 }
 
-// withDeviceCgroupRules appends the given rules to the device allow list.
-// The list starts with an entry denying every device, so a rule only takes
-// effect when it comes after it.
+// defaultDeviceCgroupRules holds the entries that runc and crun add to every
+// container's device allow list (runc: specconv.AllowedDevices; crun keeps the
+// same list for parity with runc).
+//
+// containerd's oci.WithDefaultUnixDevices already lists most of these devices
+// explicitly (/dev/null, /dev/random, /dev/full, /dev/tty, /dev/zero,
+// /dev/urandom, /dev/console, /dev/pts, /dev/ptmx), but the mknod wildcards and
+// /dev/net/tun are in no spec, and a runtime that applies the spec's device list
+// verbatim would deny them. Listing the whole set keeps the allow list
+// self-contained; on runc and crun the duplicate grants are no-ops.
+//
+// Wildcards are deliberately left as nil pointers: runc reads a missing
+// major/minor as -1, whereas crun writes a present -1 literally ("c -1:-1 m"),
+// which the kernel rejects on cgroup v1.
+//
+// Keep in sync with runc's specconv.AllowedDevices (unchanged from v1.1.15
+// through v1.5.2, the current stable release).
+var defaultDeviceCgroupRules = []specs.LinuxDeviceCgroup{
+	// allow mknod for any device
+	{Allow: true, Type: "c", Access: "m"},
+	{Allow: true, Type: "b", Access: "m"},
+	{Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(3), Access: "rwm"},    // /dev/null
+	{Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(8), Access: "rwm"},    // /dev/random
+	{Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(7), Access: "rwm"},    // /dev/full
+	{Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(0), Access: "rwm"},    // /dev/tty
+	{Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(5), Access: "rwm"},    // /dev/zero
+	{Allow: true, Type: "c", Major: int64Ptr(1), Minor: int64Ptr(9), Access: "rwm"},    // /dev/urandom
+	{Allow: true, Type: "c", Major: int64Ptr(136), Access: "rwm"},                      // /dev/pts
+	{Allow: true, Type: "c", Major: int64Ptr(5), Minor: int64Ptr(2), Access: "rwm"},    // /dev/ptmx
+	{Allow: true, Type: "c", Major: int64Ptr(10), Minor: int64Ptr(200), Access: "rwm"}, // /dev/net/tun
+}
+
+func int64Ptr(v int64) *int64 {
+	return &v
+}
+
+// withDeviceCgroupRules adds the given rules to the device allow list.
+//
+// The rules already in the spec stay in front: oci.WithDefaultSpec starts the
+// list with a deny-all rule and oci.WithDefaultUnixDevices appends the standard
+// device grants, and a device cgroup applies its rules in order, so a deny-all
+// rule placed behind a grant cancels it. The defaults and the user rules are
+// therefore appended last, and nothing the runtime adds implicitly is needed to
+// make the list self-contained.
+//
+// A default entry is skipped when the spec already grants that device, as
+// oci.WithDevices does for --device: mapping /dev/net/tun as read-only must not
+// be widened to "rwm" by the fallback grants. The standard devices are already
+// listed by oci.WithDefaultUnixDevices, so only the gaps are filled in.
 func withDeviceCgroupRules(rules []specs.LinuxDeviceCgroup) oci.SpecOpts {
 	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *oci.Spec) error {
-		s.Linux.Resources.Devices = append(s.Linux.Resources.Devices, rules...)
+		if len(rules) == 0 {
+			return nil
+		}
+		existing := s.Linux.Resources.Devices
+		devices := make([]specs.LinuxDeviceCgroup, 0, len(existing)+len(defaultDeviceCgroupRules)+len(rules))
+		devices = append(devices, existing...)
+		for _, d := range defaultDeviceCgroupRules {
+			if deviceCovered(existing, d) {
+				continue
+			}
+			devices = append(devices, d)
+		}
+		devices = append(devices, rules...)
+		s.Linux.Resources.Devices = devices
 		return nil
 	}
 }
 
+// deviceCovered reports whether devices already allows the device that rule
+// grants, in which case appending rule would be redundant. Only allow entries
+// count: the deny-all entry from oci.WithDefaultSpec covers nothing.
+func deviceCovered(devices []specs.LinuxDeviceCgroup, rule specs.LinuxDeviceCgroup) bool {
+	for _, d := range devices {
+		if !d.Allow {
+			continue
+		}
+		if d.Type != rule.Type && d.Type != "a" {
+			continue
+		}
+		if coversNumber(d.Major, rule.Major) && coversNumber(d.Minor, rule.Minor) {
+			return true
+		}
+	}
+	return false
+}
+
+// coversNumber reports whether the number already in the allow list covers the
+// one a default rule grants. A wildcard covers anything, but a wildcard default
+// (the mknod entries) is only covered by another wildcard: a grant for one
+// device does not allow creating device nodes.
+func coversNumber(existing, want *int64) bool {
+	if want == nil {
+		return existing == nil
+	}
+	return existing == nil || *existing == *want
+}
+
 func withUnified(unified map[string]string) oci.SpecOpts {
 	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *oci.Spec) (err error) {
 		if unified == nil {