From: Zbigniew Jędrzejewski-Szmek Date: Sun, 10 Nov 2019 22:08:21 +0000 (+0100) Subject: bpf: make sure the kernel do not submit an invalid program if no pattern matched X-Git-Tag: v244-rc1~62^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=45669ae264c2d16db006c49ea264c38bd2080a22;p=thirdparty%2Fsystemd.git 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 827c3ab9492..984603003e9 100644 --- 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 5f9a6b617b0..ca86cdc3c4b 100644 --- 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 aef9d359d47..aaa2c3ce3a4 100644 --- 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);