bpf: make sure the kernel do not submit an invalid program if no pattern matched

It turns out that the kernel verifier would reject a program we would build
if there was a whitelist, but no entries in the whitelist matched.
The program would approximately like this:
   0: (61) r2 = *(u32 *)(r1 +0)
   1: (54) w2 &= 65535
   2: (61) r3 = *(u32 *)(r1 +0)
   3: (74) w3 >>= 16
   4: (61) r4 = *(u32 *)(r1 +4)
   5: (61) r5 = *(u32 *)(r1 +8)
  48: (b7) r0 = 0
  49: (05) goto pc+1
  50: (b7) r0 = 1
  51: (95) exit
and insn 50 is unreachable, which is illegal. We would then either keep a
previous version of the program or allow everything. Make sure we build a
valid program that simply rejects everything.

diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c
index 827c3ab94927ee41bef379020c3549931ef9690e..984603003e9b21c94f4635e18511ebaba41dfe08 100644 (file)
--- a/src/core/bpf-devices.c
+++ b/src/core/bpf-devices.c
@@ -395,7 +395,7 @@ int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char *
                 return whitelist_device_pattern(prog, path, type, &maj, NULL, acc);
 
         _cleanup_fclose_ FILE *f = NULL;
-        bool good = false;
+        bool good = false, any = false;
 
         f = fopen("/proc/devices", "re");
         if (!f)
@@ -448,9 +448,14 @@ int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char *
                 if (fnmatch(name, w, 0) != 0)
                         continue;
 
+                any = true;
                 (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc);
         }
 
+        if (!any)
+                return log_debug_errno(SYNTHETIC_ERRNO(ENOENT),
+                                       "Device whitelist pattern \"%s\" did not match anything.", name);
+
         return 0;
 }
 

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 5f9a6b617b09065bd5cf9c27f800df7bed4e4d20..ca86cdc3c4b3928ac080fc3f497c5a8867ecee75 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -960,13 +960,16 @@ static int cgroup_apply_devices(Unit *u) {
         const char *path;
         CGroupContext *c;
         CGroupDeviceAllow *a;
+        CGroupDevicePolicy policy;
         int r;
 
         assert_se(c = unit_get_cgroup_context(u));
         assert_se(path = u->cgroup_path);
 
+        policy = c->device_policy;
+
         if (cg_all_unified() > 0) {
-                r = bpf_devices_cgroup_init(&prog, c->device_policy, c->device_allow);
+                r = bpf_devices_cgroup_init(&prog, policy, c->device_allow);
                 if (r < 0)
                         return log_unit_warning_errno(u, r, "Failed to initialize device control bpf program: %m");
 
@@ -974,7 +977,7 @@ static int cgroup_apply_devices(Unit *u) {
                 /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore
                  * EINVAL here. */
 
-                if (c->device_allow || c->device_policy != CGROUP_DEVICE_POLICY_AUTO)
+                if (c->device_allow || policy != CGROUP_DEVICE_POLICY_AUTO)
                         r = cg_set_attribute("devices", path, "devices.deny", "a");
                 else
                         r = cg_set_attribute("devices", path, "devices.allow", "a");
@@ -983,10 +986,12 @@ static int cgroup_apply_devices(Unit *u) {
                                       "Failed to reset devices.allow/devices.deny: %m");
         }
 
-        if (c->device_policy == CGROUP_DEVICE_POLICY_CLOSED ||
-            (c->device_policy == CGROUP_DEVICE_POLICY_AUTO && c->device_allow))
+        bool whitelist_static = policy == CGROUP_DEVICE_POLICY_CLOSED ||
+                (policy == CGROUP_DEVICE_POLICY_AUTO && c->device_allow);
+        if (whitelist_static)
                 (void) bpf_devices_whitelist_static(prog, path);
 
+        bool any = whitelist_static;
         LIST_FOREACH(device_allow, a, c->device_allow) {
                 char acc[4], *val;
                 unsigned k = 0;
@@ -997,23 +1002,35 @@ static int cgroup_apply_devices(Unit *u) {
                         acc[k++] = 'w';
                 if (a->m)
                         acc[k++] = 'm';
-
                 if (k == 0)
                         continue;
-
                 acc[k++] = 0;
 
                 if (path_startswith(a->path, "/dev/"))
-                        (void) bpf_devices_whitelist_device(prog, path, a->path, acc);
+                        r = bpf_devices_whitelist_device(prog, path, a->path, acc);
                 else if ((val = startswith(a->path, "block-")))
-                        (void) bpf_devices_whitelist_major(prog, path, val, 'b', acc);
+                        r = bpf_devices_whitelist_major(prog, path, val, 'b', acc);
                 else if ((val = startswith(a->path, "char-")))
-                        (void) bpf_devices_whitelist_major(prog, path, val, 'c', acc);
-                else
+                        r = bpf_devices_whitelist_major(prog, path, val, 'c', acc);
+                else {
                         log_unit_debug(u, "Ignoring device '%s' while writing cgroup attribute.", a->path);
+                        continue;
+                }
+
+                if (r >= 0)
+                        any = true;
+        }
+
+        if (prog && !any) {
+                log_unit_warning_errno(u, SYNTHETIC_ERRNO(ENODEV), "No devices matched by device filter.");
+
+                /* The kernel verifier would reject a program we would build with the normal intro and outro
+                   but no whitelisting rules (outro would contain an unreachable instruction for successful
+                   return). */
+                policy = CGROUP_DEVICE_POLICY_STRICT;
         }
 
-        r = bpf_devices_apply_policy(prog, c->device_policy, c->device_allow, path, &u->bpf_device_control_installed);
+        r = bpf_devices_apply_policy(prog, policy, any, path, &u->bpf_device_control_installed);
         if (r < 0) {
                 static bool warned = false;
 

diff --git a/src/test/test-bpf-devices.c b/src/test/test-bpf-devices.c
index aef9d359d470fbd1c08367eddb2b57c124beb27c..aaa2c3ce3a4392c0b2c05db6503b1675d3e764b5 100644 (file)
--- a/src/test/test-bpf-devices.c
+++ b/src/test/test-bpf-devices.c
@@ -219,6 +219,37 @@ static void test_policy_whitelist_major_star(char type, const char *cgroup_path,
         assert_se(wrong == 0);
 }
 
+static void test_policy_empty(bool add_mismatched, const char *cgroup_path, BPFProgram **installed_prog) {
+        _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL;
+        unsigned wrong = 0;
+        int r;
+
+        log_info("/* %s(add_mismatched=%s) */", __func__, yes_no(add_mismatched));
+
+        r = bpf_devices_cgroup_init(&prog, CGROUP_DEVICE_POLICY_STRICT, add_mismatched);
+        assert_se(r >= 0);
+
+        if (add_mismatched) {
+                r = bpf_devices_whitelist_major(prog, cgroup_path, "foobarxxx", 'c', "rw");
+                assert_se(r < 0);
+        }
+
+        r = bpf_devices_apply_policy(prog, CGROUP_DEVICE_POLICY_STRICT, false, cgroup_path, installed_prog);
+        assert_se(r >= 0);
+
+        {
+                _cleanup_close_ int fd;
+                const char *s = "/dev/null";
+
+                fd = open(s, O_CLOEXEC|O_RDWR|O_NOCTTY);
+                log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-");
+                wrong += fd >= 0;
+        }
+
+        assert_se(wrong == 0);
+}
+
+
 int main(int argc, char *argv[]) {
         _cleanup_free_ char *cgroup = NULL, *parent = NULL;
         _cleanup_(rmdir_and_freep) char *controller_path = NULL;
@@ -258,6 +289,9 @@ int main(int argc, char *argv[]) {
         test_policy_whitelist_major_star('c', cgroup, &prog);
         test_policy_whitelist_major_star('b', cgroup, &prog);
 
+        test_policy_empty(false, cgroup, &prog);
+        test_policy_empty(true, cgroup, &prog);
+
         assert_se(parent = dirname_malloc(cgroup));
 
         assert_se(cg_mask_supported(&supported) >= 0);