From 2899aac46a8d8d5cf1aa5cbf04f755ef7b9f9643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 08:55:54 +0100 Subject: [PATCH 01/25] core: constify bpf program arrays In cases where the programs were modified after being initially declared, reorder operations so that the declaration is already in final form. --- src/core/bpf-devices.c | 90 +++++++++++++++++++---------------------- src/core/bpf-firewall.c | 10 ++--- src/test/test-bpf.c | 2 +- 3 files changed, 48 insertions(+), 54 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 9750c4c682..693ff124c5 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -30,15 +30,6 @@ static int bpf_access_type(const char *acc) { } int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc) { - struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 6), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ - BPF_ALU32_IMM(BPF_AND, BPF_REG_1, 0), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 3), /* compare access type */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 2), /* compare major */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_5, minor, 1), /* compare minor */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ - }; int r, access; assert(prog); @@ -48,7 +39,15 @@ int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor if (access <= 0) return -EINVAL; - insn[2].imm = access; + const struct bpf_insn insn[] = { + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 6), /* compare device type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 3), /* compare access type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 2), /* compare major */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_5, minor, 1), /* compare minor */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + }; r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) @@ -58,14 +57,6 @@ int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor } int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char *acc) { - struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ - BPF_ALU32_IMM(BPF_AND, BPF_REG_1, 0), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 2), /* compare access type */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 1), /* compare major */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ - }; int r, access; assert(prog); @@ -75,7 +66,14 @@ int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char if (access <= 0) return -EINVAL; - insn[2].imm = access; + const struct bpf_insn insn[] = { + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 2), /* compare access type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 1), /* compare major */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + }; r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) @@ -85,13 +83,6 @@ int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char } int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc) { - struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ - BPF_ALU32_IMM(BPF_AND, BPF_REG_1, 0), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 1), /* compare access type */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ - }; int r, access; assert(prog); @@ -101,7 +92,13 @@ int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc) { if (access <= 0) return -EINVAL; - insn[2].imm = access; + const struct bpf_insn insn[] = { + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 1), /* compare access type */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + }; r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) @@ -111,7 +108,7 @@ int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc) { } int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist) { - struct bpf_insn pre_insn[] = { + const struct bpf_insn pre_insn[] = { /* load device type to r2 */ BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_1, offsetof(struct bpf_cgroup_dev_ctx, access_type)), @@ -154,19 +151,6 @@ int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whi } int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist) { - struct bpf_insn post_insn[] = { - /* return DENY */ - BPF_MOV64_IMM(BPF_REG_0, 0), - BPF_JMP_A(1), - - }; - - struct bpf_insn exit_insn[] = { - /* else return ALLOW */ - BPF_MOV64_IMM(BPF_REG_0, 1), - BPF_EXIT_INSN() - }; - _cleanup_free_ char *path = NULL; int r; @@ -176,23 +160,33 @@ int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy return 0; } - if (policy != CGROUP_STRICT || whitelist) { - size_t off; + const bool deny_everything = policy == CGROUP_STRICT && !whitelist; + const struct bpf_insn post_insn[] = { + /* return DENY */ + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_JMP_A(1), + }; + + const struct bpf_insn exit_insn[] = { + /* finally return DENY if deny_everything else ALLOW */ + BPF_MOV64_IMM(BPF_REG_0, deny_everything ? 0 : 1), + BPF_EXIT_INSN() + }; + + if (!deny_everything) { r = bpf_program_add_instructions(prog, post_insn, ELEMENTSOF(post_insn)); if (r < 0) return log_error_errno(r, "Extending device control BPF program failed: %m"); /* Fixup PASS_JUMP_OFF jump offsets. */ - for (off = 0; off < prog->n_instructions; off++) { + for (size_t off = 0; off < prog->n_instructions; off++) { struct bpf_insn *ins = &prog->instructions[off]; if (ins->code == (BPF_JMP | BPF_JA) && ins->off == PASS_JUMP_OFF) ins->off = prog->n_instructions - off - 1; } - } else - /* Explicitly forbid everything. */ - exit_insn[0].imm = 0; + } r = bpf_program_add_instructions(prog, exit_insn, ELEMENTSOF(exit_insn)); if (r < 0) @@ -216,7 +210,7 @@ int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy } int bpf_devices_supported(void) { - struct bpf_insn trivial[] = { + const struct bpf_insn trivial[] = { BPF_MOV64_IMM(BPF_REG_0, 1), BPF_EXIT_INSN() }; diff --git a/src/core/bpf-firewall.c b/src/core/bpf-firewall.c index 424162f445..96c1a28b4f 100644 --- a/src/core/bpf-firewall.c +++ b/src/core/bpf-firewall.c @@ -132,7 +132,7 @@ static int add_instructions_for_ip_any( assert(p); - struct bpf_insn insn[] = { + const struct bpf_insn insn[] = { BPF_ALU32_IMM(BPF_OR, BPF_REG_8, verdict), }; @@ -150,7 +150,7 @@ static int bpf_firewall_compile_bpf( bool ip_allow_any, bool ip_deny_any) { - struct bpf_insn pre_insn[] = { + const struct bpf_insn pre_insn[] = { /* * When the eBPF program is entered, R1 contains the address of the skb. * However, R1-R5 are scratch registers that are not preserved when calling @@ -186,7 +186,7 @@ static int bpf_firewall_compile_bpf( * This means that if both ACCESS_DENIED and ACCESS_ALLOWED are set, the packet * is allowed to pass. */ - struct bpf_insn post_insn[] = { + const struct bpf_insn post_insn[] = { BPF_MOV64_IMM(BPF_REG_0, 1), BPF_JMP_IMM(BPF_JNE, BPF_REG_8, ACCESS_DENIED, 1), BPF_MOV64_IMM(BPF_REG_0, 0), @@ -321,7 +321,7 @@ static int bpf_firewall_compile_bpf( * Exit from the eBPF program, R0 contains the verdict. * 0 means the packet is denied, 1 means the packet may pass. */ - struct bpf_insn insn[] = { + const struct bpf_insn insn[] = { BPF_EXIT_INSN() }; @@ -795,7 +795,7 @@ int bpf_firewall_reset_accounting(int map_fd) { static int bpf_firewall_unsupported_reason = 0; int bpf_firewall_supported(void) { - struct bpf_insn trivial[] = { + const struct bpf_insn trivial[] = { BPF_MOV64_IMM(BPF_REG_0, 1), BPF_EXIT_INSN() }; diff --git a/src/test/test-bpf.c b/src/test/test-bpf.c index d333466ab8..49dc0b7e89 100644 --- a/src/test/test-bpf.c +++ b/src/test/test-bpf.c @@ -41,7 +41,7 @@ static bool can_memlock(void) { } int main(int argc, char *argv[]) { - struct bpf_insn exit_insn[] = { + const struct bpf_insn exit_insn[] = { BPF_MOV64_IMM(BPF_REG_0, 0), /* drop */ BPF_EXIT_INSN() }; From fa6613fc539cfcfb5b64ac31c8a3e2857a8cad56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 11:13:20 +0100 Subject: [PATCH 02/25] bpf: refactor how we create device major:minor whitelists No functional change intended except for minor adjustments to error messages. --- src/core/bpf-devices.h | 6 +- src/core/cgroup.c | 140 ++++++++++++++++------------------------- 2 files changed, 56 insertions(+), 90 deletions(-) diff --git a/src/core/bpf-devices.h b/src/core/bpf-devices.h index 8d3de3bd94..504b0902c2 100644 --- a/src/core/bpf-devices.h +++ b/src/core/bpf-devices.h @@ -9,9 +9,9 @@ struct BPFProgram; int bpf_devices_supported(void); -int cgroup_bpf_whitelist_device(BPFProgram *p, int type, int major, int minor, const char *acc); -int cgroup_bpf_whitelist_major(BPFProgram *p, int type, int major, const char *acc); +int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc); +int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char *acc); int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc); int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist); -int cgroup_apply_device_bpf(Unit *u, BPFProgram *p, CGroupDevicePolicy policy, bool whitelist); +int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index ec8a262314..d024f88e6a 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -667,13 +667,52 @@ static int lookup_block_device(const char *p, dev_t *ret) { return 0; } +static int whitelist_device_pattern(BPFProgram *prog, const char *path, char type, const unsigned *maj, const unsigned *min, const char *acc) { + assert(IN_SET(type, 'b', 'c')); + + if (cg_all_unified() > 0) { + if (!prog) + return 0; + + const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; + if (maj && min) + return cgroup_bpf_whitelist_device(prog, bpf_type, *maj, *min, acc); + else if (maj) + return cgroup_bpf_whitelist_major(prog, bpf_type, *maj, acc); + else + return cgroup_bpf_whitelist_class(prog, bpf_type, acc); + + } else { + char buf[2+DECIMAL_STR_MAX(unsigned)*2+2+4]; + int r; + + if (maj && min) + xsprintf(buf, "%c %u:%u %s", type, *maj, *min, acc); + else if (maj) + xsprintf(buf, "%c %u:* %s", type, *maj, acc); + else + xsprintf(buf, "%c *:* %s", type, acc); + + /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore + * EINVAL here. */ + + r = cg_set_attribute("devices", path, "devices.allow", buf); + if (r < 0) + log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, + r, "Failed to set devices.allow on %s: %m", path); + + return r; + } +} + static int whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc) { - dev_t rdev; mode_t mode; + dev_t rdev; int r; assert(path); assert(acc); + assert(strlen(acc) <= 3); /* Some special handling for /dev/block/%u:%u, /dev/char/%u:%u, /run/systemd/inaccessible/chr and * /run/systemd/inaccessible/blk paths. Instead of stat()ing these we parse out the major/minor directly. This @@ -687,44 +726,19 @@ static int whitelist_device(BPFProgram *prog, const char *path, const char *node if (stat(node, &st) < 0) return log_warning_errno(errno, "Couldn't stat device %s: %m", node); - if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) { - log_warning("%s is not a device.", node); - return -ENODEV; - } - rdev = (dev_t) st.st_rdev; + if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) + return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), "%s is not a device.", node); + mode = st.st_mode; + rdev = (dev_t) st.st_rdev; } - if (cg_all_unified() > 0) { - if (!prog) - return 0; - - return cgroup_bpf_whitelist_device(prog, S_ISCHR(mode) ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK, - major(rdev), minor(rdev), acc); - - } else { - char buf[2+DECIMAL_STR_MAX(dev_t)*2+2+4]; - - sprintf(buf, - "%c %u:%u %s", - S_ISCHR(mode) ? 'c' : 'b', - major(rdev), minor(rdev), - acc); - - /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore EINVAL here. */ - - r = cg_set_attribute("devices", path, "devices.allow", buf); - if (r < 0) - return log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, - r, "Failed to set devices.allow on %s: %m", path); - - return 0; - } + unsigned maj = major(rdev), min = minor(rdev); + return whitelist_device_pattern(prog, path, S_ISCHR(mode) ? 'c' : 'b', &maj, &min, acc); } static int whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc) { _cleanup_fclose_ FILE *f = NULL; - char buf[2+DECIMAL_STR_MAX(unsigned)+3+4]; bool good = false; unsigned maj; int r; @@ -735,49 +749,22 @@ static int whitelist_major(BPFProgram *prog, const char *path, const char *name, if (streq(name, "*")) { /* If the name is a wildcard, then apply this list to all devices of this type */ + (void) whitelist_device_pattern(prog, path, type, NULL, NULL, acc); - if (cg_all_unified() > 0) { - if (!prog) - return 0; - - (void) cgroup_bpf_whitelist_class(prog, type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK, acc); - } else { - xsprintf(buf, "%c *:* %s", type, acc); - - r = cg_set_attribute("devices", path, "devices.allow", buf); - if (r < 0) - log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES) ? LOG_DEBUG : LOG_WARNING, r, - "Failed to set devices.allow on %s: %m", path); + if (cg_all_unified() <= 0) return 0; - } } if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) { - /* The name is numeric and suitable as major. In that case, let's take is major, and create the entry - * directly */ - - if (cg_all_unified() > 0) { - if (!prog) - return 0; - - (void) cgroup_bpf_whitelist_major(prog, - type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK, - maj, acc); - } else { - xsprintf(buf, "%c %u:* %s", type, maj, acc); - - r = cg_set_attribute("devices", path, "devices.allow", buf); - if (r < 0) - log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES) ? LOG_DEBUG : LOG_WARNING, r, - "Failed to set devices.allow on %s: %m", path); - } - + /* The name is numeric and suitable as major. In that case, let's take its major, and create + * the entry directly. */ + (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc); return 0; } f = fopen("/proc/devices", "re"); if (!f) - return log_warning_errno(errno, "Cannot open /proc/devices to resolve %s (%c): %m", name, type); + return log_warning_errno(errno, "Cannot open /proc/devices to resolve %s: %m", name); for (;;) { _cleanup_free_ char *line = NULL; @@ -826,28 +813,7 @@ static int whitelist_major(BPFProgram *prog, const char *path, const char *name, if (fnmatch(name, w, 0) != 0) continue; - if (cg_all_unified() > 0) { - if (!prog) - continue; - - (void) cgroup_bpf_whitelist_major(prog, - type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK, - maj, acc); - } else { - sprintf(buf, - "%c %u:* %s", - type, - maj, - acc); - - /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore EINVAL - * here. */ - - r = cg_set_attribute("devices", path, "devices.allow", buf); - if (r < 0) - log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, - r, "Failed to set devices.allow on %s: %m", path); - } + (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc); } return 0; From d49c180826e56771a1cdb31eb01fe258a4b74076 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 11:17:08 +0100 Subject: [PATCH 03/25] bpf: do not bother adding device patterns after whitelisting the full class This seems to have been unintentional. --- src/core/cgroup.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index d024f88e6a..61d5a4dfca 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -750,9 +750,7 @@ static int whitelist_major(BPFProgram *prog, const char *path, const char *name, if (streq(name, "*")) { /* If the name is a wildcard, then apply this list to all devices of this type */ (void) whitelist_device_pattern(prog, path, type, NULL, NULL, acc); - - if (cg_all_unified() <= 0) - return 0; + return 0; } if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) { From 672cbcbc20a79b32a69a65f73b319e3c5111d09e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 11:24:17 +0100 Subject: [PATCH 04/25] bpf: return normally from whitelist_major() All callers do (void) anyway, so we can just use normal return here. --- src/core/cgroup.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 61d5a4dfca..747543a995 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -738,8 +738,6 @@ static int whitelist_device(BPFProgram *prog, const char *path, const char *node } static int whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc) { - _cleanup_fclose_ FILE *f = NULL; - bool good = false; unsigned maj; int r; @@ -747,18 +745,17 @@ static int whitelist_major(BPFProgram *prog, const char *path, const char *name, assert(acc); assert(IN_SET(type, 'b', 'c')); - if (streq(name, "*")) { + if (streq(name, "*")) /* If the name is a wildcard, then apply this list to all devices of this type */ - (void) whitelist_device_pattern(prog, path, type, NULL, NULL, acc); - return 0; - } + return whitelist_device_pattern(prog, path, type, NULL, NULL, acc); - if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) { + if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) /* The name is numeric and suitable as major. In that case, let's take its major, and create * the entry directly. */ - (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc); - return 0; - } + return whitelist_device_pattern(prog, path, type, &maj, NULL, acc); + + _cleanup_fclose_ FILE *f = NULL; + bool good = false; f = fopen("/proc/devices", "re"); if (!f) From d3472f9b50d76545f93f90662d1abd815e0a4c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 11:58:55 +0100 Subject: [PATCH 05/25] test-bpf: rename to test-bpf-firewall Let's make the name better reflect its scope. --- src/test/meson.build | 2 +- src/test/{test-bpf.c => test-bpf-firewall.c} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename src/test/{test-bpf.c => test-bpf-firewall.c} (98%) diff --git a/src/test/meson.build b/src/test/meson.build index 6179607a43..16050f0a7e 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -428,7 +428,7 @@ tests += [ [libbasic], []], - [['src/test/test-bpf.c', + [['src/test/test-bpf-firewall.c', 'src/test/test-helper.c'], [libcore, libshared], diff --git a/src/test/test-bpf.c b/src/test/test-bpf-firewall.c similarity index 98% rename from src/test/test-bpf.c rename to src/test/test-bpf-firewall.c index 49dc0b7e89..3155cd24cf 100644 --- a/src/test/test-bpf.c +++ b/src/test/test-bpf-firewall.c @@ -61,7 +61,7 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); if (detect_container() > 0) - return log_tests_skipped("test-bpf fails inside LXC and Docker containers: https://github.com/systemd/systemd/issues/9666"); + return log_tests_skipped("test-bpf-firewall fails inside LXC and Docker containers: https://github.com/systemd/systemd/issues/9666"); assert_se(getrlimit(RLIMIT_MEMLOCK, &rl) >= 0); rl.rlim_cur = rl.rlim_max = MAX3(rl.rlim_cur, rl.rlim_max, CAN_MEMLOCK_SIZE); From ee19c8073345877c6cb9e6fa760660b82b71558d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 12:03:23 +0100 Subject: [PATCH 06/25] test-bpf-firewall: do not mlock() a large amount of memory 64MB is not that much, but let's not be greedy, esp. because we may run many things in parallel. Also, rlim_cur should never be higher than rlim_max, so let's simplify our code. --- src/test/test-bpf-firewall.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/test-bpf-firewall.c b/src/test/test-bpf-firewall.c index 3155cd24cf..9fd1b429db 100644 --- a/src/test/test-bpf-firewall.c +++ b/src/test/test-bpf-firewall.c @@ -16,8 +16,8 @@ #include "unit.h" #include "virt.h" -/* We use the same limit here that PID 1 bumps RLIMIT_MEMLOCK to if it can */ -#define CAN_MEMLOCK_SIZE (64U*1024U*1024U) +/* We use the small but non-trivial limit here */ +#define CAN_MEMLOCK_SIZE (512 * 1024U) static bool can_memlock(void) { void *p; @@ -64,7 +64,7 @@ int main(int argc, char *argv[]) { return log_tests_skipped("test-bpf-firewall fails inside LXC and Docker containers: https://github.com/systemd/systemd/issues/9666"); assert_se(getrlimit(RLIMIT_MEMLOCK, &rl) >= 0); - rl.rlim_cur = rl.rlim_max = MAX3(rl.rlim_cur, rl.rlim_max, CAN_MEMLOCK_SIZE); + rl.rlim_cur = rl.rlim_max = MAX(rl.rlim_max, CAN_MEMLOCK_SIZE); (void) setrlimit(RLIMIT_MEMLOCK, &rl); if (!can_memlock()) From 77abd02985415fc90db03511b6bcc4479f61f81f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 14:58:28 +0100 Subject: [PATCH 07/25] tests: move memlock helper to shared code --- src/shared/tests.c | 19 +++++++++++++++++++ src/shared/tests.h | 4 ++++ src/test/test-bpf-firewall.c | 25 ------------------------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/shared/tests.c b/src/shared/tests.c index 11ea12ed69..fc53546be0 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -149,3 +150,21 @@ bool have_namespaces(void) { assert_not_reached("unexpected exit code"); } + +bool can_memlock(void) { + /* Let's see if we can mlock() a larger blob of memory. BPF programs are charged against + * RLIMIT_MEMLOCK, hence let's first make sure we can lock memory at all, and skip the test if we + * cannot. Why not check RLIMIT_MEMLOCK explicitly? Because in container environments the + * RLIMIT_MEMLOCK value we see might not match the RLIMIT_MEMLOCK value actually in effect. */ + + void *p = mmap(NULL, CAN_MEMLOCK_SIZE, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0); + if (p == MAP_FAILED) + return false; + + bool b = mlock(p, CAN_MEMLOCK_SIZE) >= 0; + if (b) + assert_se(munlock(p, CAN_MEMLOCK_SIZE) >= 0); + + assert_se(munmap(p, CAN_MEMLOCK_SIZE) >= 0); + return b; +} diff --git a/src/shared/tests.h b/src/shared/tests.h index 718196f134..8416b107f3 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -12,3 +12,7 @@ int log_tests_skipped(const char *message); int log_tests_skipped_errno(int r, const char *message); bool have_namespaces(void); + +/* We use the small but non-trivial limit here */ +#define CAN_MEMLOCK_SIZE (512 * 1024U) +bool can_memlock(void); diff --git a/src/test/test-bpf-firewall.c b/src/test/test-bpf-firewall.c index 9fd1b429db..0942c61e0f 100644 --- a/src/test/test-bpf-firewall.c +++ b/src/test/test-bpf-firewall.c @@ -2,7 +2,6 @@ #include #include -#include #include #include "bpf-firewall.h" @@ -16,30 +15,6 @@ #include "unit.h" #include "virt.h" -/* We use the small but non-trivial limit here */ -#define CAN_MEMLOCK_SIZE (512 * 1024U) - -static bool can_memlock(void) { - void *p; - bool b; - - /* Let's see if we can mlock() a larger blob of memory. BPF programs are charged against - * RLIMIT_MEMLOCK, hence let's first make sure we can lock memory at all, and skip the test if we - * cannot. Why not check RLIMIT_MEMLOCK explicitly? Because in container environments the - * RLIMIT_MEMLOCK value we see might not match the RLIMIT_MEMLOCK value actually in effect. */ - - p = mmap(NULL, CAN_MEMLOCK_SIZE, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0); - if (p == MAP_FAILED) - return false; - - b = mlock(p, CAN_MEMLOCK_SIZE) >= 0; - if (b) - assert_se(munlock(p, CAN_MEMLOCK_SIZE) >= 0); - - assert_se(munmap(p, CAN_MEMLOCK_SIZE) >= 0); - return b; -} - int main(int argc, char *argv[]) { const struct bpf_insn exit_insn[] = { BPF_MOV64_IMM(BPF_REG_0, 0), /* drop */ From 084870f9c0c03d5766d272828254f8d0a7aacf66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 15:12:23 +0100 Subject: [PATCH 08/25] =?UTF-8?q?core:=20rename=20CGROUP=5FAUTO/STRICT/CLO?= =?UTF-8?q?SED=20to=20CGROUP=5FDEVICE=5FPOLICY=5F=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old names were very generic, and when used without context it wasn't at all clear that they are about the devices policy. --- src/core/bpf-devices.c | 6 +++--- src/core/cgroup.c | 14 +++++++------- src/core/cgroup.h | 11 +++++------ src/core/unit.c | 6 +++--- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 693ff124c5..3b8e0ac7f8 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -132,14 +132,14 @@ int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whi assert(ret); - if (policy == CGROUP_AUTO && !whitelist) + if (policy == CGROUP_DEVICE_POLICY_AUTO && !whitelist) return 0; r = bpf_program_new(BPF_PROG_TYPE_CGROUP_DEVICE, &prog); if (r < 0) return log_error_errno(r, "Loading device control BPF program failed: %m"); - if (policy == CGROUP_CLOSED || whitelist) { + if (policy == CGROUP_DEVICE_POLICY_CLOSED || whitelist) { r = bpf_program_add_instructions(prog, pre_insn, ELEMENTSOF(pre_insn)); if (r < 0) return log_error_errno(r, "Extending device control BPF program failed: %m"); @@ -160,7 +160,7 @@ int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy return 0; } - const bool deny_everything = policy == CGROUP_STRICT && !whitelist; + const bool deny_everything = policy == CGROUP_DEVICE_POLICY_STRICT && !whitelist; const struct bpf_insn post_insn[] = { /* return DENY */ diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 747543a995..48a320fc9d 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1392,7 +1392,7 @@ static void cgroup_context_apply( /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore EINVAL * here. */ - if (c->device_allow || c->device_policy != CGROUP_AUTO) + if (c->device_allow || c->device_policy != CGROUP_DEVICE_POLICY_AUTO) r = cg_set_attribute("devices", path, "devices.deny", "a"); else r = cg_set_attribute("devices", path, "devices.allow", "a"); @@ -1401,8 +1401,8 @@ static void cgroup_context_apply( "Failed to reset devices.allow/devices.deny: %m"); } - if (c->device_policy == CGROUP_CLOSED || - (c->device_policy == CGROUP_AUTO && c->device_allow)) { + if (c->device_policy == CGROUP_DEVICE_POLICY_CLOSED || + (c->device_policy == CGROUP_DEVICE_POLICY_AUTO && c->device_allow)) { static const char auto_devices[] = "/dev/null\0" "rwm\0" "/dev/zero\0" "rwm\0" @@ -1570,7 +1570,7 @@ static CGroupMask unit_get_cgroup_mask(Unit *u) { mask |= CGROUP_MASK_MEMORY; if (c->device_allow || - c->device_policy != CGROUP_AUTO) + c->device_policy != CGROUP_DEVICE_POLICY_AUTO) mask |= CGROUP_MASK_DEVICES | CGROUP_MASK_BPF_DEVICES; if (c->tasks_accounting || @@ -3708,9 +3708,9 @@ int compare_job_priority(const void *a, const void *b) { } static const char* const cgroup_device_policy_table[_CGROUP_DEVICE_POLICY_MAX] = { - [CGROUP_AUTO] = "auto", - [CGROUP_CLOSED] = "closed", - [CGROUP_STRICT] = "strict", + [CGROUP_DEVICE_POLICY_AUTO] = "auto", + [CGROUP_DEVICE_POLICY_CLOSED] = "closed", + [CGROUP_DEVICE_POLICY_STRICT] = "strict", }; int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name) { diff --git a/src/core/cgroup.h b/src/core/cgroup.h index a66c702125..3d4bb4142d 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -18,16 +18,15 @@ typedef struct CGroupBlockIODeviceWeight CGroupBlockIODeviceWeight; typedef struct CGroupBlockIODeviceBandwidth CGroupBlockIODeviceBandwidth; typedef enum CGroupDevicePolicy { - - /* When devices listed, will allow those, plus built-in ones, - if none are listed will allow everything. */ - CGROUP_AUTO, + /* When devices listed, will allow those, plus built-in ones, if none are listed will allow + * everything. */ + CGROUP_DEVICE_POLICY_AUTO, /* Everything forbidden, except built-in ones and listed ones. */ - CGROUP_CLOSED, + CGROUP_DEVICE_POLICY_CLOSED, /* Everything forbidden, except for the listed devices */ - CGROUP_STRICT, + CGROUP_DEVICE_POLICY_STRICT, _CGROUP_DEVICE_POLICY_MAX, _CGROUP_DEVICE_POLICY_INVALID = -1 diff --git a/src/core/unit.c b/src/core/unit.c index 5f2ca44701..137a110cc2 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4303,11 +4303,11 @@ int unit_patch_contexts(Unit *u) { if (cc && ec) { if (ec->private_devices && - cc->device_policy == CGROUP_AUTO) - cc->device_policy = CGROUP_CLOSED; + cc->device_policy == CGROUP_DEVICE_POLICY_AUTO) + cc->device_policy = CGROUP_DEVICE_POLICY_CLOSED; if (ec->root_image && - (cc->device_policy != CGROUP_AUTO || cc->device_allow)) { + (cc->device_policy != CGROUP_DEVICE_POLICY_AUTO || cc->device_allow)) { /* When RootImage= is specified, the following devices are touched. */ r = cgroup_add_device_allow(cc, "/dev/loop-control", "rw"); From 124e05b3b60c253d83ba5e122aca34be719391ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 15:51:28 +0100 Subject: [PATCH 09/25] core: move bpf devices implementation to bpf-devices.[ch] and rename The naming of the functions was a complete mess: the most specific functions which don't know anything about cgroups had "cgroup_" prefix, while more general functions which took a node path and a cgroup for reporting had no prefix. Let's use "bpf_devices_" for the latter group, and "bpf_prog_*" for the rest. The main goal of this move is to split the implementation from the calling code and add unit tests in a later patch. --- src/core/bpf-devices.c | 165 ++++++++++++++++++++++++++++++++++++++-- src/core/bpf-devices.h | 12 ++- src/core/cgroup.c | 167 +++-------------------------------------- 3 files changed, 174 insertions(+), 170 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 3b8e0ac7f8..41b751b83a 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -1,8 +1,16 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ + +#include #include #include "bpf-devices.h" #include "bpf-program.h" +#include "fd-util.h" +#include "fileio.h" +#include "parse-util.h" +#include "stat-util.h" +#include "stdio-util.h" +#include "string-util.h" #define PASS_JUMP_OFF 4096 @@ -29,7 +37,7 @@ static int bpf_access_type(const char *acc) { return r; } -int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc) { +static int bpf_prog_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc) { int r, access; assert(prog); @@ -56,7 +64,7 @@ int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor return r; } -int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char *acc) { +static int bpf_prog_whitelist_major(BPFProgram *prog, int type, int major, const char *acc) { int r, access; assert(prog); @@ -82,7 +90,7 @@ int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char return r; } -int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc) { +static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) { int r, access; assert(prog); @@ -107,7 +115,7 @@ int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc) { return r; } -int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist) { +int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist) { const struct bpf_insn pre_insn[] = { /* load device type to r2 */ BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_1, @@ -150,7 +158,7 @@ int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whi return 0; } -int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist) { +int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist) { _cleanup_free_ char *path = NULL; int r; @@ -262,3 +270,150 @@ int bpf_devices_supported(void) { return supported = 1; } + +static int whitelist_device_pattern(BPFProgram *prog, const char *path, char type, const unsigned *maj, const unsigned *min, const char *acc) { + assert(IN_SET(type, 'b', 'c')); + + if (cg_all_unified() > 0) { + if (!prog) + return 0; + + const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; + if (maj && min) + return bpf_prog_whitelist_device(prog, bpf_type, *maj, *min, acc); + else if (maj) + return bpf_prog_whitelist_major(prog, bpf_type, *maj, acc); + else + return bpf_prog_whitelist_class(prog, bpf_type, acc); + + } else { + char buf[2+DECIMAL_STR_MAX(unsigned)*2+2+4]; + int r; + + if (maj && min) + xsprintf(buf, "%c %u:%u %s", type, *maj, *min, acc); + else if (maj) + xsprintf(buf, "%c %u:* %s", type, *maj, acc); + else + xsprintf(buf, "%c *:* %s", type, acc); + + /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore + * EINVAL here. */ + + r = cg_set_attribute("devices", path, "devices.allow", buf); + if (r < 0) + log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, + r, "Failed to set devices.allow on %s: %m", path); + + return r; + } +} + +int bpf_devices_whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc) { + mode_t mode; + dev_t rdev; + int r; + + assert(path); + assert(acc); + assert(strlen(acc) <= 3); + + /* Some special handling for /dev/block/%u:%u, /dev/char/%u:%u, /run/systemd/inaccessible/chr and + * /run/systemd/inaccessible/blk paths. Instead of stat()ing these we parse out the major/minor directly. This + * means clients can use these path without the device node actually around */ + r = device_path_parse_major_minor(node, &mode, &rdev); + if (r < 0) { + if (r != -ENODEV) + return log_warning_errno(r, "Couldn't parse major/minor from device path '%s': %m", node); + + struct stat st; + if (stat(node, &st) < 0) + return log_warning_errno(errno, "Couldn't stat device %s: %m", node); + + if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) + return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), "%s is not a device.", node); + + mode = st.st_mode; + rdev = (dev_t) st.st_rdev; + } + + unsigned maj = major(rdev), min = minor(rdev); + return whitelist_device_pattern(prog, path, S_ISCHR(mode) ? 'c' : 'b', &maj, &min, acc); +} + +int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc) { + unsigned maj; + int r; + + assert(path); + assert(acc); + assert(IN_SET(type, 'b', 'c')); + + if (streq(name, "*")) + /* If the name is a wildcard, then apply this list to all devices of this type */ + return whitelist_device_pattern(prog, path, type, NULL, NULL, acc); + + if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) + /* The name is numeric and suitable as major. In that case, let's take its major, and create + * the entry directly. */ + return whitelist_device_pattern(prog, path, type, &maj, NULL, acc); + + _cleanup_fclose_ FILE *f = NULL; + bool good = false; + + f = fopen("/proc/devices", "re"); + if (!f) + return log_warning_errno(errno, "Cannot open /proc/devices to resolve %s: %m", name); + + for (;;) { + _cleanup_free_ char *line = NULL; + char *w, *p; + + r = read_line(f, LONG_LINE_MAX, &line); + if (r < 0) + return log_warning_errno(r, "Failed to read /proc/devices: %m"); + if (r == 0) + break; + + if (type == 'c' && streq(line, "Character devices:")) { + good = true; + continue; + } + + if (type == 'b' && streq(line, "Block devices:")) { + good = true; + continue; + } + + if (isempty(line)) { + good = false; + continue; + } + + if (!good) + continue; + + p = strstrip(line); + + w = strpbrk(p, WHITESPACE); + if (!w) + continue; + *w = 0; + + r = safe_atou(p, &maj); + if (r < 0) + continue; + if (maj <= 0) + continue; + + w++; + w += strspn(w, WHITESPACE); + + if (fnmatch(name, w, 0) != 0) + continue; + + (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc); + } + + return 0; +} diff --git a/src/core/bpf-devices.h b/src/core/bpf-devices.h index 504b0902c2..744d5f8fa0 100644 --- a/src/core/bpf-devices.h +++ b/src/core/bpf-devices.h @@ -7,11 +7,9 @@ struct BPFProgram; +int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist); +int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist); + int bpf_devices_supported(void); - -int cgroup_bpf_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc); -int cgroup_bpf_whitelist_major(BPFProgram *prog, int type, int major, const char *acc); -int cgroup_bpf_whitelist_class(BPFProgram *prog, int type, const char *acc); - -int cgroup_init_device_bpf(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist); -int cgroup_apply_device_bpf(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist); +int bpf_devices_whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc); +int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 48a320fc9d..9857a68770 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include "sd-messages.h" @@ -667,153 +666,6 @@ static int lookup_block_device(const char *p, dev_t *ret) { return 0; } -static int whitelist_device_pattern(BPFProgram *prog, const char *path, char type, const unsigned *maj, const unsigned *min, const char *acc) { - assert(IN_SET(type, 'b', 'c')); - - if (cg_all_unified() > 0) { - if (!prog) - return 0; - - const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; - if (maj && min) - return cgroup_bpf_whitelist_device(prog, bpf_type, *maj, *min, acc); - else if (maj) - return cgroup_bpf_whitelist_major(prog, bpf_type, *maj, acc); - else - return cgroup_bpf_whitelist_class(prog, bpf_type, acc); - - } else { - char buf[2+DECIMAL_STR_MAX(unsigned)*2+2+4]; - int r; - - if (maj && min) - xsprintf(buf, "%c %u:%u %s", type, *maj, *min, acc); - else if (maj) - xsprintf(buf, "%c %u:* %s", type, *maj, acc); - else - xsprintf(buf, "%c *:* %s", type, acc); - - /* Changing the devices list of a populated cgroup might result in EINVAL, hence ignore - * EINVAL here. */ - - r = cg_set_attribute("devices", path, "devices.allow", buf); - if (r < 0) - log_full_errno(IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, - r, "Failed to set devices.allow on %s: %m", path); - - return r; - } -} - -static int whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc) { - mode_t mode; - dev_t rdev; - int r; - - assert(path); - assert(acc); - assert(strlen(acc) <= 3); - - /* Some special handling for /dev/block/%u:%u, /dev/char/%u:%u, /run/systemd/inaccessible/chr and - * /run/systemd/inaccessible/blk paths. Instead of stat()ing these we parse out the major/minor directly. This - * means clients can use these path without the device node actually around */ - r = device_path_parse_major_minor(node, &mode, &rdev); - if (r < 0) { - if (r != -ENODEV) - return log_warning_errno(r, "Couldn't parse major/minor from device path '%s': %m", node); - - struct stat st; - if (stat(node, &st) < 0) - return log_warning_errno(errno, "Couldn't stat device %s: %m", node); - - if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) - return log_warning_errno(SYNTHETIC_ERRNO(ENODEV), "%s is not a device.", node); - - mode = st.st_mode; - rdev = (dev_t) st.st_rdev; - } - - unsigned maj = major(rdev), min = minor(rdev); - return whitelist_device_pattern(prog, path, S_ISCHR(mode) ? 'c' : 'b', &maj, &min, acc); -} - -static int whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc) { - unsigned maj; - int r; - - assert(path); - assert(acc); - assert(IN_SET(type, 'b', 'c')); - - if (streq(name, "*")) - /* If the name is a wildcard, then apply this list to all devices of this type */ - return whitelist_device_pattern(prog, path, type, NULL, NULL, acc); - - if (safe_atou(name, &maj) >= 0 && DEVICE_MAJOR_VALID(maj)) - /* The name is numeric and suitable as major. In that case, let's take its major, and create - * the entry directly. */ - return whitelist_device_pattern(prog, path, type, &maj, NULL, acc); - - _cleanup_fclose_ FILE *f = NULL; - bool good = false; - - f = fopen("/proc/devices", "re"); - if (!f) - return log_warning_errno(errno, "Cannot open /proc/devices to resolve %s: %m", name); - - for (;;) { - _cleanup_free_ char *line = NULL; - char *w, *p; - - r = read_line(f, LONG_LINE_MAX, &line); - if (r < 0) - return log_warning_errno(r, "Failed to read /proc/devices: %m"); - if (r == 0) - break; - - if (type == 'c' && streq(line, "Character devices:")) { - good = true; - continue; - } - - if (type == 'b' && streq(line, "Block devices:")) { - good = true; - continue; - } - - if (isempty(line)) { - good = false; - continue; - } - - if (!good) - continue; - - p = strstrip(line); - - w = strpbrk(p, WHITESPACE); - if (!w) - continue; - *w = 0; - - r = safe_atou(p, &maj); - if (r < 0) - continue; - if (maj <= 0) - continue; - - w++; - w += strspn(w, WHITESPACE); - - if (fnmatch(name, w, 0) != 0) - continue; - - (void) whitelist_device_pattern(prog, path, type, &maj, NULL, acc); - } - - return 0; -} - static bool cgroup_context_has_cpu_weight(CGroupContext *c) { return c->cpu_weight != CGROUP_WEIGHT_INVALID || c->startup_cpu_weight != CGROUP_WEIGHT_INVALID; @@ -1385,7 +1237,7 @@ static void cgroup_context_apply( CGroupDeviceAllow *a; if (cg_all_unified() > 0) { - r = cgroup_init_device_bpf(&prog, c->device_policy, c->device_allow); + r = bpf_devices_cgroup_init(&prog, c->device_policy, c->device_allow); if (r < 0) log_unit_warning_errno(u, r, "Failed to initialize device control bpf program: %m"); } else { @@ -1415,13 +1267,12 @@ static void cgroup_context_apply( "/run/systemd/inaccessible/chr\0" "rwm\0" "/run/systemd/inaccessible/blk\0" "rwm\0"; - const char *x, *y; - - NULSTR_FOREACH_PAIR(x, y, auto_devices) - (void) whitelist_device(prog, path, x, y); + const char *node, *acc; + NULSTR_FOREACH_PAIR(node, acc, auto_devices) + (void) bpf_devices_whitelist_device(prog, path, node, acc); /* PTS (/dev/pts) devices may not be duplicated, but accessed */ - (void) whitelist_major(prog, path, "pts", 'c', "rw"); + (void) bpf_devices_whitelist_major(prog, path, "pts", 'c', "rw"); } LIST_FOREACH(device_allow, a, c->device_allow) { @@ -1441,16 +1292,16 @@ static void cgroup_context_apply( acc[k++] = 0; if (path_startswith(a->path, "/dev/")) - (void) whitelist_device(prog, path, a->path, acc); + (void) bpf_devices_whitelist_device(prog, path, a->path, acc); else if ((val = startswith(a->path, "block-"))) - (void) whitelist_major(prog, path, val, 'b', acc); + (void) bpf_devices_whitelist_major(prog, path, val, 'b', acc); else if ((val = startswith(a->path, "char-"))) - (void) whitelist_major(prog, path, val, 'c', acc); + (void) bpf_devices_whitelist_major(prog, path, val, 'c', acc); else log_unit_debug(u, "Ignoring device '%s' while writing cgroup attribute.", a->path); } - r = cgroup_apply_device_bpf(u, prog, c->device_policy, c->device_allow); + r = bpf_devices_apply_policy(u, prog, c->device_policy, c->device_allow); if (r < 0) { static bool warned = false; From a9aac7d8dd8939d48df5dba391fae43bf34afcdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:09:28 +0100 Subject: [PATCH 10/25] core: also split out helper to handle static device nodes --- src/core/bpf-devices.c | 30 ++++++++++++++++++++++++++++++ src/core/bpf-devices.h | 1 + src/core/cgroup.c | 23 ++--------------------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 41b751b83a..dd38a61981 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -7,6 +7,7 @@ #include "bpf-program.h" #include "fd-util.h" #include "fileio.h" +#include "nulstr-util.h" #include "parse-util.h" #include "stat-util.h" #include "stdio-util.h" @@ -417,3 +418,32 @@ int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char * return 0; } + +int bpf_devices_whitelist_static(BPFProgram *prog, const char *path) { + static const char auto_devices[] = + "/dev/null\0" "rwm\0" + "/dev/zero\0" "rwm\0" + "/dev/full\0" "rwm\0" + "/dev/random\0" "rwm\0" + "/dev/urandom\0" "rwm\0" + "/dev/tty\0" "rwm\0" + "/dev/ptmx\0" "rwm\0" + /* Allow /run/systemd/inaccessible/{chr,blk} devices for mapping InaccessiblePaths */ + "/run/systemd/inaccessible/chr\0" "rwm\0" + "/run/systemd/inaccessible/blk\0" "rwm\0"; + int r = 0, k; + + const char *node, *acc; + NULSTR_FOREACH_PAIR(node, acc, auto_devices) { + k = bpf_devices_whitelist_device(prog, path, node, acc); + if (r >= 0 && k < 0) + r = k; + } + + /* PTS (/dev/pts) devices may not be duplicated, but accessed */ + k = bpf_devices_whitelist_major(prog, path, "pts", 'c', "rw"); + if (r >= 0 && k < 0) + r = k; + + return r; +} diff --git a/src/core/bpf-devices.h b/src/core/bpf-devices.h index 744d5f8fa0..cd5f074bba 100644 --- a/src/core/bpf-devices.h +++ b/src/core/bpf-devices.h @@ -13,3 +13,4 @@ int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy polic int bpf_devices_supported(void); int bpf_devices_whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc); int bpf_devices_whitelist_major(BPFProgram *prog, const char *path, const char *name, char type, const char *acc); +int bpf_devices_whitelist_static(BPFProgram *prog, const char *path); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 9857a68770..475e7df23f 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -16,7 +16,6 @@ #include "fd-util.h" #include "fileio.h" #include "fs-util.h" -#include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" #include "process-util.h" @@ -1254,26 +1253,8 @@ static void cgroup_context_apply( } if (c->device_policy == CGROUP_DEVICE_POLICY_CLOSED || - (c->device_policy == CGROUP_DEVICE_POLICY_AUTO && c->device_allow)) { - static const char auto_devices[] = - "/dev/null\0" "rwm\0" - "/dev/zero\0" "rwm\0" - "/dev/full\0" "rwm\0" - "/dev/random\0" "rwm\0" - "/dev/urandom\0" "rwm\0" - "/dev/tty\0" "rwm\0" - "/dev/ptmx\0" "rwm\0" - /* Allow /run/systemd/inaccessible/{chr,blk} devices for mapping InaccessiblePaths */ - "/run/systemd/inaccessible/chr\0" "rwm\0" - "/run/systemd/inaccessible/blk\0" "rwm\0"; - - const char *node, *acc; - NULSTR_FOREACH_PAIR(node, acc, auto_devices) - (void) bpf_devices_whitelist_device(prog, path, node, acc); - - /* PTS (/dev/pts) devices may not be duplicated, but accessed */ - (void) bpf_devices_whitelist_major(prog, path, "pts", 'c', "rw"); - } + (c->device_policy == CGROUP_DEVICE_POLICY_AUTO && c->device_allow)) + (void) bpf_devices_whitelist_static(prog, path); LIST_FOREACH(device_allow, a, c->device_allow) { char acc[4], *val; From 8b139557febfc1c62c37214378a911d4f515aac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:19:43 +0100 Subject: [PATCH 11/25] core: split out one more function --- src/core/cgroup.c | 137 +++++++++++++++++++++++++--------------------- 1 file changed, 74 insertions(+), 63 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 475e7df23f..10273b4a80 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -955,6 +955,78 @@ static void cgroup_apply_firewall(Unit *u) { (void) bpf_firewall_install(u); } +static int cgroup_apply_devices(Unit *u) { + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + const char *path; + CGroupContext *c; + CGroupDeviceAllow *a; + int r; + + assert_se(c = unit_get_cgroup_context(u)); + assert_se(path = u->cgroup_path); + + if (cg_all_unified() > 0) { + r = bpf_devices_cgroup_init(&prog, c->device_policy, c->device_allow); + if (r < 0) + return log_unit_warning_errno(u, r, "Failed to initialize device control bpf program: %m"); + + } else { + /* 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) + r = cg_set_attribute("devices", path, "devices.deny", "a"); + else + r = cg_set_attribute("devices", path, "devices.allow", "a"); + if (r < 0) + log_unit_full(u, IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, r, + "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)) + (void) bpf_devices_whitelist_static(prog, path); + + LIST_FOREACH(device_allow, a, c->device_allow) { + char acc[4], *val; + unsigned k = 0; + + if (a->r) + acc[k++] = 'r'; + if (a->w) + 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); + else if ((val = startswith(a->path, "block-"))) + (void) 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 + log_unit_debug(u, "Ignoring device '%s' while writing cgroup attribute.", a->path); + } + + r = bpf_devices_apply_policy(u, prog, c->device_policy, c->device_allow); + if (r < 0) { + static bool warned = false; + + log_full_errno(warned ? LOG_DEBUG : LOG_WARNING, r, + "Unit %s configures device ACL, but the local system doesn't seem to support the BPF-based device controller.\n" + "Proceeding WITHOUT applying ACL (all devices will be accessible)!\n" + "(This warning is only shown for the first loaded unit using device ACL.)", u->id); + + warned = true; + } + return r; +} + static void cgroup_context_apply( Unit *u, CGroupMask apply_mask, @@ -1231,69 +1303,8 @@ static void cgroup_context_apply( /* On cgroup v2 we can apply BPF everywhere. On cgroup v1 we apply it everywhere except for the root of * containers, where we leave this to the manager */ if ((apply_mask & (CGROUP_MASK_DEVICES | CGROUP_MASK_BPF_DEVICES)) && - (is_host_root || cg_all_unified() > 0 || !is_local_root)) { - _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; - CGroupDeviceAllow *a; - - if (cg_all_unified() > 0) { - r = bpf_devices_cgroup_init(&prog, c->device_policy, c->device_allow); - if (r < 0) - log_unit_warning_errno(u, r, "Failed to initialize device control bpf program: %m"); - } else { - /* 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) - r = cg_set_attribute("devices", path, "devices.deny", "a"); - else - r = cg_set_attribute("devices", path, "devices.allow", "a"); - if (r < 0) - log_unit_full(u, IN_SET(r, -ENOENT, -EROFS, -EINVAL, -EACCES, -EPERM) ? LOG_DEBUG : LOG_WARNING, r, - "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)) - (void) bpf_devices_whitelist_static(prog, path); - - LIST_FOREACH(device_allow, a, c->device_allow) { - char acc[4], *val; - unsigned k = 0; - - if (a->r) - acc[k++] = 'r'; - if (a->w) - 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); - else if ((val = startswith(a->path, "block-"))) - (void) 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 - log_unit_debug(u, "Ignoring device '%s' while writing cgroup attribute.", a->path); - } - - r = bpf_devices_apply_policy(u, prog, c->device_policy, c->device_allow); - if (r < 0) { - static bool warned = false; - - log_full_errno(warned ? LOG_DEBUG : LOG_WARNING, r, - "Unit %s configures device ACL, but the local system doesn't seem to support the BPF-based device controller.\n" - "Proceeding WITHOUT applying ACL (all devices will be accessible)!\n" - "(This warning is only shown for the first loaded unit using device ACL.)", u->id); - - warned = true; - } - } + (is_host_root || cg_all_unified() > 0 || !is_local_root)) + (void) cgroup_apply_devices(u); if (apply_mask & CGROUP_MASK_PIDS) { From 5eecb1030ffd55296112a3308beca7beca5903b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:37:15 +0100 Subject: [PATCH 12/25] tests: make manager_skip_test() not a macro and move to tests.h There is nothing magic in it. --- src/shared/tests.h | 13 +++++++++++++ src/test/test-engine.c | 2 +- src/test/test-execute.c | 2 +- src/test/test-helper.h | 14 ++------------ src/test/test-load-fragment.c | 4 ++-- src/test/test-path.c | 2 +- src/test/test-sched-prio.c | 2 +- src/test/test-unit-name.c | 2 +- 8 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/shared/tests.h b/src/shared/tests.h index 8416b107f3..72f6f63651 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -3,6 +3,19 @@ #include +#include "macro.h" + +static inline bool manager_errno_skip_test(int r) { + return IN_SET(abs(r), + EPERM, + EACCES, + EADDRINUSE, + EHOSTDOWN, + ENOENT, + ENOMEDIUM /* cannot determine cgroup */ + ); +} + char* setup_fake_runtime_dir(void); const char* get_testdata_dir(void); const char* get_catalog_dir(void); diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 265152fb76..c292ad1c7c 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -30,7 +30,7 @@ int main(int argc, char *argv[]) { assert_se(set_unit_path(get_testdata_dir()) >= 0); assert_se(runtime_dir = setup_fake_runtime_dir()); r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_BASIC, &m); - if (MANAGER_SKIP_TEST(r)) + if (manager_errno_skip_test(r)) return log_tests_skipped_errno(r, "manager_new"); assert_se(r >= 0); assert_se(manager_startup(m, NULL, NULL) >= 0); diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 92fb35fd87..57a4b1c282 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -781,7 +781,7 @@ static int run_tests(UnitFileScope scope, const test_entry tests[], char **patte assert_se(tests); r = manager_new(scope, MANAGER_TEST_RUN_BASIC, &m); - if (MANAGER_SKIP_TEST(r)) + if (manager_errno_skip_test(r)) return log_tests_skipped_errno(r, "manager_new"); assert_se(r >= 0); assert_se(manager_startup(m, NULL, NULL) >= 0); diff --git a/src/test/test-helper.h b/src/test/test-helper.h index 77af40d555..81e19ee207 100644 --- a/src/test/test-helper.h +++ b/src/test/test-helper.h @@ -5,9 +5,9 @@ Copyright © 2013 Holger Hans Peter Freyther ***/ -#include "sd-daemon.h" +#include -#include "macro.h" +#include "sd-daemon.h" #define TEST_REQ_RUNNING_SYSTEMD(x) \ if (sd_booted() > 0) { \ @@ -16,16 +16,6 @@ printf("systemd not booted skipping '%s'\n", #x); \ } -#define MANAGER_SKIP_TEST(r) \ - IN_SET(r, \ - -EPERM, \ - -EACCES, \ - -EADDRINUSE, \ - -EHOSTDOWN, \ - -ENOENT, \ - -ENOMEDIUM /* cannot determine cgroup */ \ - ) - int enter_cgroup_subroot(void); bool is_run_on_travis_ci(void); diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index 216f0e4d94..cb808bfce0 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -97,7 +97,7 @@ static void test_config_parse_exec(void) { _cleanup_(unit_freep) Unit *u = NULL; r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m); - if (MANAGER_SKIP_TEST(r)) { + if (manager_errno_skip_test(r)) { log_notice_errno(r, "Skipping test: manager_new: %m"); return; } @@ -442,7 +442,7 @@ static void test_config_parse_log_extra_fields(void) { ExecContext c = {}; r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m); - if (MANAGER_SKIP_TEST(r)) { + if (manager_errno_skip_test(r)) { log_notice_errno(r, "Skipping test: manager_new: %m"); return; } diff --git a/src/test/test-path.c b/src/test/test-path.c index 07a0e413ee..9157c6a13b 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -37,7 +37,7 @@ static int setup_test(Manager **m) { return log_tests_skipped("cgroupfs not available"); r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_BASIC, &tmp); - if (MANAGER_SKIP_TEST(r)) + if (manager_errno_skip_test(r)) return log_tests_skipped_errno(r, "manager_new"); assert_se(r >= 0); assert_se(manager_startup(tmp, NULL, NULL) >= 0); diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c index 1aa178182b..d179ae21a6 100644 --- a/src/test/test-sched-prio.c +++ b/src/test/test-sched-prio.c @@ -29,7 +29,7 @@ int main(int argc, char *argv[]) { assert_se(set_unit_path(get_testdata_dir()) >= 0); assert_se(runtime_dir = setup_fake_runtime_dir()); r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_BASIC, &m); - if (MANAGER_SKIP_TEST(r)) + if (manager_errno_skip_test(r)) return log_tests_skipped_errno(r, "manager_new"); assert_se(r >= 0); assert_se(manager_startup(m, NULL, NULL) >= 0); diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index c9bbce0d2e..52c75406ae 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -209,7 +209,7 @@ static int test_unit_printf(void) { assert_se(get_shell(&shell) >= 0); r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m); - if (MANAGER_SKIP_TEST(r)) + if (manager_errno_skip_test(r)) return log_tests_skipped_errno(r, "manager_new"); assert_se(r == 0); From 2025f0f602b2ce7f90cdaaef6b219d1dbd1515b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:40:11 +0100 Subject: [PATCH 13/25] tests: make is_run_on_travis_ci() static This is a pretty specific hack, so let's just define it close to the one place where it is used. --- src/test/test-execute.c | 5 +++++ src/test/test-helper.c | 5 ----- src/test/test-helper.h | 4 ---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 57a4b1c282..957cc2e384 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -37,6 +37,11 @@ static int cld_dumped_to_killed(int code) { return code == CLD_DUMPED ? CLD_KILLED : code; } +_unused_ static bool is_run_on_travis_ci(void) { + /* https://docs.travis-ci.com/user/environment-variables#default-environment-variables */ + return streq_ptr(getenv("TRAVIS"), "true"); +} + static void wait_for_service_finish(Manager *m, Unit *unit) { Service *service = NULL; usec_t ts; diff --git a/src/test/test-helper.c b/src/test/test-helper.c index dc8c80a14b..9e9de0132f 100644 --- a/src/test/test-helper.c +++ b/src/test/test-helper.c @@ -28,8 +28,3 @@ int enter_cgroup_subroot(void) { return cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); } - -/* https://docs.travis-ci.com/user/environment-variables#default-environment-variables */ -bool is_run_on_travis_ci(void) { - return streq_ptr(getenv("TRAVIS"), "true"); -} diff --git a/src/test/test-helper.h b/src/test/test-helper.h index 81e19ee207..6e886ae3de 100644 --- a/src/test/test-helper.h +++ b/src/test/test-helper.h @@ -5,8 +5,6 @@ Copyright © 2013 Holger Hans Peter Freyther ***/ -#include - #include "sd-daemon.h" #define TEST_REQ_RUNNING_SYSTEMD(x) \ @@ -17,5 +15,3 @@ } int enter_cgroup_subroot(void); - -bool is_run_on_travis_ci(void); From 48e98ba5c351e43d086b474c81efe293d751c0e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:55:28 +0100 Subject: [PATCH 14/25] tests: get rid of test-helper.[ch] completely I don't think there's any particular reason to keep those functions in a separate file. --- src/shared/tests.c | 26 +++++++++++++++++++++++++ src/shared/tests.h | 10 ++++++++++ src/test/meson.build | 30 ++++++++++------------------- src/test/test-bpf-firewall.c | 1 - src/test/test-cgroup-mask.c | 1 - src/test/test-cgroup-unit-default.c | 1 - src/test/test-cgroup-util.c | 1 - src/test/test-engine.c | 1 - src/test/test-execute.c | 1 - src/test/test-helper.c | 30 ----------------------------- src/test/test-helper.h | 17 ---------------- src/test/test-load-fragment.c | 1 - src/test/test-path.c | 1 - src/test/test-process-util.c | 1 - src/test/test-sched-prio.c | 1 - src/test/test-unit-name.c | 1 - src/test/test-watch-pid.c | 1 - 17 files changed, 46 insertions(+), 79 deletions(-) delete mode 100644 src/test/test-helper.c delete mode 100644 src/test/test-helper.h diff --git a/src/shared/tests.c b/src/shared/tests.c index fc53546be0..a4ffd3ec02 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -15,11 +15,14 @@ #undef basename #include "alloc-util.h" +#include "cgroup-setup.h" +#include "cgroup-util.h" #include "env-file.h" #include "env-util.h" #include "fs-util.h" #include "log.h" #include "path-util.h" +#include "random-util.h" #include "strv.h" #include "tests.h" @@ -168,3 +171,26 @@ bool can_memlock(void) { assert_se(munmap(p, CAN_MEMLOCK_SIZE) >= 0); return b; } + +int enter_cgroup_subroot(void) { + _cleanup_free_ char *cgroup_root = NULL, *cgroup_subroot = NULL; + CGroupMask supported; + int r; + + r = cg_pid_get_path(NULL, 0, &cgroup_root); + if (r == -ENOMEDIUM) + return log_warning_errno(r, "cg_pid_get_path(NULL, 0, ...) failed: %m"); + assert(r >= 0); + + assert_se(asprintf(&cgroup_subroot, "%s/%" PRIx64, cgroup_root, random_u64()) >= 0); + assert_se(cg_mask_supported(&supported) >= 0); + + /* If this fails, then we don't mind as the later cgroup operations will fail too, and it's fine if + * we handle any errors at that point. */ + + r = cg_create_everywhere(supported, _CGROUP_MASK_ALL, cgroup_subroot); + if (r < 0) + return r; + + return cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); +} diff --git a/src/shared/tests.h b/src/shared/tests.h index 72f6f63651..3e4368460d 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -3,6 +3,8 @@ #include +#include "sd-daemon.h" + #include "macro.h" static inline bool manager_errno_skip_test(int r) { @@ -17,6 +19,7 @@ static inline bool manager_errno_skip_test(int r) { } char* setup_fake_runtime_dir(void); +int enter_cgroup_subroot(void); const char* get_testdata_dir(void); const char* get_catalog_dir(void); bool slow_tests_enabled(void); @@ -29,3 +32,10 @@ bool have_namespaces(void); /* We use the small but non-trivial limit here */ #define CAN_MEMLOCK_SIZE (512 * 1024U) bool can_memlock(void); + +#define TEST_REQ_RUNNING_SYSTEMD(x) \ + if (sd_booted() > 0) { \ + x; \ + } else { \ + printf("systemd not booted skipping '%s'\n", #x); \ + } diff --git a/src/test/meson.build b/src/test/meson.build index 16050f0a7e..10ac3c973f 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -51,8 +51,7 @@ tests += [ [], []], - [['src/test/test-engine.c', - 'src/test/test-helper.c'], + [['src/test/test-engine.c'], [libcore, libudev, libshared], @@ -141,8 +140,7 @@ tests += [ [], []], - [['src/test/test-unit-name.c', - 'src/test/test-helper.c'], + [['src/test/test-unit-name.c'], [libcore, libshared], [threads, @@ -152,8 +150,7 @@ tests += [ libmount, libblkid]], - [['src/test/test-load-fragment.c', - 'src/test/test-helper.c'], + [['src/test/test-load-fragment.c'], [libcore, libshared], [threads, @@ -428,8 +425,7 @@ tests += [ [libbasic], []], - [['src/test/test-bpf-firewall.c', - 'src/test/test-helper.c'], + [['src/test/test-bpf-firewall.c'], [libcore, libshared], [libmount, @@ -439,8 +435,7 @@ tests += [ libselinux, libblkid]], - [['src/test/test-watch-pid.c', - 'src/test/test-helper.c'], + [['src/test/test-watch-pid.c'], [libcore, libshared], [libmount, @@ -590,14 +585,12 @@ tests += [ libshared], []], - [['src/test/test-cgroup-unit-default.c', - 'src/test/test-helper.c'], + [['src/test/test-cgroup-unit-default.c'], [libcore, libshared], []], - [['src/test/test-cgroup-mask.c', - 'src/test/test-helper.c'], + [['src/test/test-cgroup-mask.c'], [libcore, libshared], [threads, @@ -644,8 +637,7 @@ tests += [ [], '', 'manual'], - [['src/test/test-path.c', - 'src/test/test-helper.c'], + [['src/test/test-path.c'], [libcore, libshared], [threads, @@ -655,8 +647,7 @@ tests += [ libmount, libblkid]], - [['src/test/test-execute.c', - 'src/test/test-helper.c'], + [['src/test/test-execute.c'], [libcore, libshared], [threads, @@ -685,8 +676,7 @@ tests += [ [], []], - [['src/test/test-sched-prio.c', - 'src/test/test-helper.c'], + [['src/test/test-sched-prio.c'], [libcore, libshared], [threads, diff --git a/src/test/test-bpf-firewall.c b/src/test/test-bpf-firewall.c index 0942c61e0f..13f81d48d0 100644 --- a/src/test/test-bpf-firewall.c +++ b/src/test/test-bpf-firewall.c @@ -10,7 +10,6 @@ #include "manager.h" #include "rm-rf.h" #include "service.h" -#include "test-helper.h" #include "tests.h" #include "unit.h" #include "virt.h" diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index ef3755de41..28a3721255 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -8,7 +8,6 @@ #include "manager.h" #include "rm-rf.h" #include "string-util.h" -#include "test-helper.h" #include "tests.h" #include "unit.h" diff --git a/src/test/test-cgroup-unit-default.c b/src/test/test-cgroup-unit-default.c index 4fb629217f..c4af2a3cdf 100644 --- a/src/test/test-cgroup-unit-default.c +++ b/src/test/test-cgroup-unit-default.c @@ -5,7 +5,6 @@ #include "cgroup.h" #include "manager.h" #include "rm-rf.h" -#include "test-helper.h" #include "tests.h" #include "unit.h" diff --git a/src/test/test-cgroup-util.c b/src/test/test-cgroup-util.c index e4fbb9edce..83b5156c11 100644 --- a/src/test/test-cgroup-util.c +++ b/src/test/test-cgroup-util.c @@ -13,7 +13,6 @@ #include "stat-util.h" #include "string-util.h" #include "strv.h" -#include "test-helper.h" #include "tests.h" #include "user-util.h" #include "util.h" diff --git a/src/test/test-engine.c b/src/test/test-engine.c index c292ad1c7c..a32e98af11 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -7,7 +7,6 @@ #include "manager.h" #include "rm-rf.h" #include "strv.h" -#include "test-helper.h" #include "tests.h" #include "service.h" diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 957cc2e384..d846414750 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -20,7 +20,6 @@ #endif #include "service.h" #include "stat-util.h" -#include "test-helper.h" #include "tests.h" #include "unit.h" #include "user-util.h" diff --git a/src/test/test-helper.c b/src/test/test-helper.c deleted file mode 100644 index 9e9de0132f..0000000000 --- a/src/test/test-helper.c +++ /dev/null @@ -1,30 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1+ */ - -#include "test-helper.h" -#include "random-util.h" -#include "alloc-util.h" -#include "cgroup-setup.h" -#include "string-util.h" - -int enter_cgroup_subroot(void) { - _cleanup_free_ char *cgroup_root = NULL, *cgroup_subroot = NULL; - CGroupMask supported; - int r; - - r = cg_pid_get_path(NULL, 0, &cgroup_root); - if (r == -ENOMEDIUM) - return log_warning_errno(r, "cg_pid_get_path(NULL, 0, ...) failed: %m"); - assert(r >= 0); - - assert_se(asprintf(&cgroup_subroot, "%s/%" PRIx64, cgroup_root, random_u64()) >= 0); - assert_se(cg_mask_supported(&supported) >= 0); - - /* If this fails, then we don't mind as the later cgroup operations will fail too, and it's fine if we handle - * any errors at that point. */ - - r = cg_create_everywhere(supported, _CGROUP_MASK_ALL, cgroup_subroot); - if (r < 0) - return r; - - return cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); -} diff --git a/src/test/test-helper.h b/src/test/test-helper.h deleted file mode 100644 index 6e886ae3de..0000000000 --- a/src/test/test-helper.h +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1+ */ -#pragma once - -/*** - Copyright © 2013 Holger Hans Peter Freyther -***/ - -#include "sd-daemon.h" - -#define TEST_REQ_RUNNING_SYSTEMD(x) \ - if (sd_booted() > 0) { \ - x; \ - } else { \ - printf("systemd not booted skipping '%s'\n", #x); \ - } - -int enter_cgroup_subroot(void); diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index cb808bfce0..8127afdd76 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -23,7 +23,6 @@ #include "specifier.h" #include "string-util.h" #include "strv.h" -#include "test-helper.h" #include "tests.h" #include "tmpfile-util.h" #include "user-util.h" diff --git a/src/test/test-path.c b/src/test/test-path.c index 9157c6a13b..2537dc6972 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -16,7 +16,6 @@ #include "rm-rf.h" #include "string-util.h" #include "strv.h" -#include "test-helper.h" #include "tests.h" #include "unit.h" #include "util.h" diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 762a203577..d0f126b236 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -25,7 +25,6 @@ #include "stdio-util.h" #include "string-util.h" #include "terminal-util.h" -#include "test-helper.h" #include "tests.h" #include "util.h" #include "virt.h" diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c index d179ae21a6..386cd93cbf 100644 --- a/src/test/test-sched-prio.c +++ b/src/test/test-sched-prio.c @@ -9,7 +9,6 @@ #include "macro.h" #include "manager.h" #include "rm-rf.h" -#include "test-helper.h" #include "tests.h" int main(int argc, char *argv[]) { diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index 52c75406ae..d63647bad2 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -15,7 +15,6 @@ #include "special.h" #include "specifier.h" #include "string-util.h" -#include "test-helper.h" #include "tests.h" #include "unit-def.h" #include "unit-name.h" diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c index 04d7e1fbd8..a26a0c9d9b 100644 --- a/src/test/test-watch-pid.c +++ b/src/test/test-watch-pid.c @@ -4,7 +4,6 @@ #include "manager.h" #include "rm-rf.h" #include "service.h" -#include "test-helper.h" #include "tests.h" int main(int argc, char *argv[]) { From 64ad9e088d1c93a62d67ff571ced2ad30955626c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 16:57:25 +0100 Subject: [PATCH 15/25] tests: modify enter_cgroup_subroot() to return the new path --- src/shared/tests.c | 10 ++++++++-- src/shared/tests.h | 2 +- src/test/test-bpf-firewall.c | 2 +- src/test/test-cgroup-mask.c | 2 +- src/test/test-cgroup-unit-default.c | 2 +- src/test/test-engine.c | 2 +- src/test/test-execute.c | 2 +- src/test/test-load-fragment.c | 2 +- src/test/test-path.c | 2 +- src/test/test-sched-prio.c | 2 +- src/test/test-unit-name.c | 2 +- src/test/test-watch-pid.c | 2 +- 12 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/shared/tests.c b/src/shared/tests.c index a4ffd3ec02..96b5b805a9 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -172,7 +172,7 @@ bool can_memlock(void) { return b; } -int enter_cgroup_subroot(void) { +int enter_cgroup_subroot(char **ret_cgroup) { _cleanup_free_ char *cgroup_root = NULL, *cgroup_subroot = NULL; CGroupMask supported; int r; @@ -192,5 +192,11 @@ int enter_cgroup_subroot(void) { if (r < 0) return r; - return cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); + r = cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); + if (r < 0) + return r; + + if (ret_cgroup) + *ret_cgroup = TAKE_PTR(cgroup_subroot); + return 0; } diff --git a/src/shared/tests.h b/src/shared/tests.h index 3e4368460d..5a6fd53f36 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -19,7 +19,7 @@ static inline bool manager_errno_skip_test(int r) { } char* setup_fake_runtime_dir(void); -int enter_cgroup_subroot(void); +int enter_cgroup_subroot(char **ret_cgroup); const char* get_testdata_dir(void); const char* get_catalog_dir(void); bool slow_tests_enabled(void); diff --git a/src/test/test-bpf-firewall.c b/src/test/test-bpf-firewall.c index 13f81d48d0..31e0c0b916 100644 --- a/src/test/test-bpf-firewall.c +++ b/src/test/test-bpf-firewall.c @@ -44,7 +44,7 @@ int main(int argc, char *argv[]) { if (!can_memlock()) return log_tests_skipped("Can't use mlock(), skipping."); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index 28a3721255..7fad58f690 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -33,7 +33,7 @@ static int test_cgroup_mask(void) { int r; CGroupMask cpu_accounting_mask = get_cpu_accounting_mask(); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-cgroup-unit-default.c b/src/test/test-cgroup-unit-default.c index c4af2a3cdf..1286f11e4e 100644 --- a/src/test/test-cgroup-unit-default.c +++ b/src/test/test-cgroup-unit-default.c @@ -18,7 +18,7 @@ static int test_default_memory_low(void) { uint64_t dml_tree_default; int r; - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-engine.c b/src/test/test-engine.c index a32e98af11..b8351141fe 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -21,7 +21,7 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-execute.c b/src/test/test-execute.c index d846414750..9d6aeed776 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -869,7 +869,7 @@ int main(int argc, char *argv[]) { if (getuid() != 0) return log_tests_skipped("not root"); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index 8127afdd76..8d0a4ad2e2 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -779,7 +779,7 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-path.c b/src/test/test-path.c index 2537dc6972..6ad222b5f9 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -31,7 +31,7 @@ static int setup_test(Manager **m) { assert_se(m); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c index 386cd93cbf..cd45537847 100644 --- a/src/test/test-sched-prio.c +++ b/src/test/test-sched-prio.c @@ -20,7 +20,7 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index d63647bad2..38e37dacb2 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -820,7 +820,7 @@ int main(int argc, char* argv[]) { test_setup_logging(LOG_INFO); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c index a26a0c9d9b..bad289767d 100644 --- a/src/test/test-watch-pid.c +++ b/src/test/test-watch-pid.c @@ -16,7 +16,7 @@ int main(int argc, char *argv[]) { if (getuid() != 0) return log_tests_skipped("not root"); - r = enter_cgroup_subroot(); + r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) return log_tests_skipped("cgroupfs not available"); From 0848715cab537f83f51a51f32c9ce2507d23928c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Nov 2019 20:33:10 +0100 Subject: [PATCH 16/25] bpf: make bpf_devices_apply_policy() independent of any unit code --- src/core/bpf-devices.c | 26 +++++++++++++++----------- src/core/bpf-devices.h | 11 ++++++++--- src/core/cgroup.c | 2 +- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index dd38a61981..49c8456597 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -159,13 +159,18 @@ int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool wh return 0; } -int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist) { - _cleanup_free_ char *path = NULL; +int bpf_devices_apply_policy( + BPFProgram *prog, + CGroupDevicePolicy policy, + bool whitelist, + const char *cgroup_path, + BPFProgram **prog_installed) { int r; + /* This will assign *keep_program if everything goes well. */ if (!prog) { /* Remove existing program. */ - u->bpf_device_control_installed = bpf_program_unref(u->bpf_device_control_installed); + *prog_installed = bpf_program_unref(*prog_installed); return 0; } @@ -201,20 +206,19 @@ int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy polic if (r < 0) return log_error_errno(r, "Extending device control BPF program failed: %m"); - r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, NULL, &path); + _cleanup_free_ char *controller_path = NULL; + r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, NULL, &controller_path); if (r < 0) return log_error_errno(r, "Failed to determine cgroup path: %m"); - r = bpf_program_cgroup_attach(prog, BPF_CGROUP_DEVICE, path, BPF_F_ALLOW_MULTI); + r = bpf_program_cgroup_attach(prog, BPF_CGROUP_DEVICE, controller_path, BPF_F_ALLOW_MULTI); if (r < 0) - return log_error_errno(r, "Attaching device control BPF program to cgroup %s failed: %m", path); + return log_error_errno(r, "Attaching device control BPF program to cgroup %s failed: %m", + cgroup_path); /* Unref the old BPF program (which will implicitly detach it) right before attaching the new program. */ - u->bpf_device_control_installed = bpf_program_unref(u->bpf_device_control_installed); - - /* Remember that this BPF program is installed now. */ - u->bpf_device_control_installed = bpf_program_ref(prog); - + bpf_program_unref(*prog_installed); + *prog_installed = bpf_program_ref(prog); return 0; } diff --git a/src/core/bpf-devices.h b/src/core/bpf-devices.h index cd5f074bba..4a5f4b1fb1 100644 --- a/src/core/bpf-devices.h +++ b/src/core/bpf-devices.h @@ -3,12 +3,17 @@ #include -#include "unit.h" +#include "cgroup.h" -struct BPFProgram; +typedef struct BPFProgram BPFProgram; int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist); -int bpf_devices_apply_policy(Unit *u, BPFProgram *prog, CGroupDevicePolicy policy, bool whitelist); +int bpf_devices_apply_policy( + BPFProgram *prog, + CGroupDevicePolicy policy, + bool whitelist, + const char *cgroup_path, + BPFProgram **prog_installed); int bpf_devices_supported(void); int bpf_devices_whitelist_device(BPFProgram *prog, const char *path, const char *node, const char *acc); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 10273b4a80..5f9a6b617b 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1013,7 +1013,7 @@ static int cgroup_apply_devices(Unit *u) { log_unit_debug(u, "Ignoring device '%s' while writing cgroup attribute.", a->path); } - r = bpf_devices_apply_policy(u, prog, c->device_policy, c->device_allow); + r = bpf_devices_apply_policy(prog, c->device_policy, c->device_allow, path, &u->bpf_device_control_installed); if (r < 0) { static bool warned = false; From 4140351189c272666d4180d6927c83622e39aa54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 9 Nov 2019 11:13:02 +0100 Subject: [PATCH 17/25] test-bpf-firewall: fix message --- src/test/test-bpf-firewall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-bpf-firewall.c b/src/test/test-bpf-firewall.c index 31e0c0b916..fbaa349b45 100644 --- a/src/test/test-bpf-firewall.c +++ b/src/test/test-bpf-firewall.c @@ -42,7 +42,7 @@ int main(int argc, char *argv[]) { (void) setrlimit(RLIMIT_MEMLOCK, &rl); if (!can_memlock()) - return log_tests_skipped("Can't use mlock(), skipping."); + return log_tests_skipped("Can't use mlock()"); r = enter_cgroup_subroot(NULL); if (r == -ENOMEDIUM) From 786cce009978a7f7ffd7cdff45a3339c373077d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 9 Nov 2019 12:34:30 +0100 Subject: [PATCH 18/25] bpf: add trace logging Very helpful when trying to figure out what exactly is going on. --- src/core/bpf-devices.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 49c8456597..c94b9353a3 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -44,6 +44,8 @@ static int bpf_prog_whitelist_device(BPFProgram *prog, int type, int major, int assert(prog); assert(acc); + log_trace("%s: %c %d:%d %s", __func__, type, major, minor, acc); + access = bpf_access_type(acc); if (access <= 0) return -EINVAL; @@ -71,6 +73,8 @@ static int bpf_prog_whitelist_major(BPFProgram *prog, int type, int major, const assert(prog); assert(acc); + log_trace("%s: %c %d:* %s", __func__, type, major, acc); + access = bpf_access_type(acc); if (access <= 0) return -EINVAL; @@ -97,6 +101,8 @@ static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) assert(prog); assert(acc); + log_trace("%s: %c *:* %s", __func__, type, acc); + access = bpf_access_type(acc); if (access <= 0) return -EINVAL; @@ -323,6 +329,8 @@ int bpf_devices_whitelist_device(BPFProgram *prog, const char *path, const char assert(acc); assert(strlen(acc) <= 3); + log_trace("%s: %s %s", __func__, node, acc); + /* Some special handling for /dev/block/%u:%u, /dev/char/%u:%u, /run/systemd/inaccessible/chr and * /run/systemd/inaccessible/blk paths. Instead of stat()ing these we parse out the major/minor directly. This * means clients can use these path without the device node actually around */ From 415fe5ec7d38db5e8e0b97c68e462734f543ad17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 10 Nov 2019 13:13:31 +0100 Subject: [PATCH 19/25] bpf: fix device type filter On big endian arches, we were taking the wrong half-word, so the check was giving bogus results. https://bugzilla.redhat.com/show_bug.cgi?id=1769148. --- src/core/bpf-devices.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index c94b9353a3..d83fc44938 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -125,8 +125,9 @@ static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist) { const struct bpf_insn pre_insn[] = { /* load device type to r2 */ - BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_1, + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, offsetof(struct bpf_cgroup_dev_ctx, access_type)), + BPF_ALU32_IMM(BPF_AND, BPF_REG_2, 0xFFFF), /* load access type to r3 */ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, From a72a5326a47733f057d1bea1ddb319b779165f8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 10 Nov 2019 13:32:36 +0100 Subject: [PATCH 20/25] bpf: fix off-by-one in class whitelisting We would jump one insn too many, landing in the middle of the subsequent block. --- src/core/bpf-devices.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index d83fc44938..60cc2f6b4e 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -108,7 +108,7 @@ static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) return -EINVAL; const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 4), /* compare device type */ BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 1), /* compare access type */ From 7973f564687f7e6dc5bcfee09aec3cb19ed34f1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 9 Nov 2019 11:50:25 +0100 Subject: [PATCH 21/25] test-bpf-devices: new test for the devices bpf code --- src/core/bpf-devices.c | 18 +-- src/test/meson.build | 10 ++ src/test/test-bpf-devices.c | 268 ++++++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 8 deletions(-) create mode 100644 src/test/test-bpf-devices.c diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 60cc2f6b4e..2683db8e71 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -172,14 +172,14 @@ int bpf_devices_apply_policy( bool whitelist, const char *cgroup_path, BPFProgram **prog_installed) { + + _cleanup_free_ char *controller_path = NULL; int r; /* This will assign *keep_program if everything goes well. */ - if (!prog) { - /* Remove existing program. */ - *prog_installed = bpf_program_unref(*prog_installed); - return 0; - } + + if (!prog) + goto finish; const bool deny_everything = policy == CGROUP_DEVICE_POLICY_STRICT && !whitelist; @@ -213,7 +213,6 @@ int bpf_devices_apply_policy( if (r < 0) return log_error_errno(r, "Extending device control BPF program failed: %m"); - _cleanup_free_ char *controller_path = NULL; r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, NULL, &controller_path); if (r < 0) return log_error_errno(r, "Failed to determine cgroup path: %m"); @@ -223,9 +222,12 @@ int bpf_devices_apply_policy( return log_error_errno(r, "Attaching device control BPF program to cgroup %s failed: %m", cgroup_path); + finish: /* Unref the old BPF program (which will implicitly detach it) right before attaching the new program. */ - bpf_program_unref(*prog_installed); - *prog_installed = bpf_program_ref(prog); + if (prog_installed) { + bpf_program_unref(*prog_installed); + *prog_installed = bpf_program_ref(prog); + } return 0; } diff --git a/src/test/meson.build b/src/test/meson.build index 10ac3c973f..b8ef163438 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -425,6 +425,16 @@ tests += [ [libbasic], []], + [['src/test/test-bpf-devices.c'], + [libcore, + libshared], + [libmount, + threads, + librt, + libseccomp, + libselinux, + libblkid]], + [['src/test/test-bpf-firewall.c'], [libcore, libshared], diff --git a/src/test/test-bpf-devices.c b/src/test/test-bpf-devices.c new file mode 100644 index 0000000000..aef9d359d4 --- /dev/null +++ b/src/test/test-bpf-devices.c @@ -0,0 +1,268 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include +#include +#include + +#include "alloc-util.h" +#include "bpf-devices.h" +#include "bpf-program.h" +#include "cgroup-setup.h" +#include "errno-list.h" +#include "fd-util.h" +#include "fs-util.h" +#include "path-util.h" +#include "tests.h" + +static void test_policy_closed(const char *cgroup_path, BPFProgram **installed_prog) { + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + unsigned wrong = 0; + int r; + + log_info("/* %s */", __func__); + + r = bpf_devices_cgroup_init(&prog, CGROUP_DEVICE_POLICY_CLOSED, true); + assert_se(r >= 0); + + r = bpf_devices_whitelist_static(prog, cgroup_path); + assert_se(r >= 0); + + r = bpf_devices_apply_policy(prog, CGROUP_DEVICE_POLICY_CLOSED, true, cgroup_path, installed_prog); + assert_se(r >= 0); + + const char *s; + FOREACH_STRING(s, "/dev/null", + "/dev/zero", + "/dev/full", + "/dev/random", + "/dev/urandom", + "/dev/tty", + "/dev/ptmx") { + _cleanup_close_ int fd, fd2; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd < 0 && errno == EPERM; + /* We ignore errors other than EPERM, e.g. ENOENT or ENXIO */ + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 < 0 && errno == EPERM; + } + assert_se(wrong == 0); +} + +static void test_policy_strict(const char *cgroup_path, BPFProgram **installed_prog) { + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + unsigned wrong = 0; + int r; + + log_info("/* %s */", __func__); + + r = bpf_devices_cgroup_init(&prog, CGROUP_DEVICE_POLICY_STRICT, true); + assert_se(r >= 0); + + r = bpf_devices_whitelist_device(prog, cgroup_path, "/dev/null", "rw"); + assert_se(r >= 0); + + r = bpf_devices_whitelist_device(prog, cgroup_path, "/dev/random", "r"); + assert_se(r >= 0); + + r = bpf_devices_whitelist_device(prog, cgroup_path, "/dev/zero", "w"); + assert_se(r >= 0); + + r = bpf_devices_apply_policy(prog, CGROUP_DEVICE_POLICY_STRICT, true, cgroup_path, installed_prog); + assert_se(r >= 0); + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/null"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd < 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 < 0; + } + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/random"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd < 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 >= 0; + } + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/zero"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd >= 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 < 0; + } + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/full"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd >= 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 >= 0; + } + + assert_se(wrong == 0); +} + +static void test_policy_whitelist_major(const char *pattern, const char *cgroup_path, BPFProgram **installed_prog) { + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + unsigned wrong = 0; + int r; + + log_info("/* %s(%s) */", __func__, pattern); + + r = bpf_devices_cgroup_init(&prog, CGROUP_DEVICE_POLICY_STRICT, true); + assert_se(r >= 0); + + r = bpf_devices_whitelist_major(prog, cgroup_path, pattern, 'c', "rw"); + assert_se(r >= 0); + + r = bpf_devices_apply_policy(prog, CGROUP_DEVICE_POLICY_STRICT, true, cgroup_path, installed_prog); + assert_se(r >= 0); + + /* /dev/null, /dev/full have major==1, /dev/tty has major==5 */ + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/null"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd < 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 < 0; + } + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/full"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd < 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 < 0; + } + + { + _cleanup_close_ int fd, fd2; + const char *s = "/dev/tty"; + + fd = open(s, O_CLOEXEC|O_RDONLY|O_NOCTTY); + log_debug("open(%s, \"r\") = %d/%s", s, fd, fd < 0 ? errno_to_name(errno) : "-"); + wrong += fd >= 0; + + fd2 = open(s, O_CLOEXEC|O_WRONLY|O_NOCTTY); + log_debug("open(%s, \"w\") = %d/%s", s, fd2, fd2 < 0 ? errno_to_name(errno) : "-"); + wrong += fd2 >= 0; + } + + assert_se(wrong == 0); +} + +static void test_policy_whitelist_major_star(char type, const char *cgroup_path, BPFProgram **installed_prog) { + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + unsigned wrong = 0; + int r; + + log_info("/* %s(type=%c) */", __func__, type); + + r = bpf_devices_cgroup_init(&prog, CGROUP_DEVICE_POLICY_STRICT, true); + assert_se(r >= 0); + + r = bpf_devices_whitelist_major(prog, cgroup_path, "*", type, "rw"); + assert_se(r >= 0); + + r = bpf_devices_apply_policy(prog, CGROUP_DEVICE_POLICY_STRICT, true, 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) : "-"); + if (type == 'c') + wrong += fd < 0; + else + 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; + CGroupMask supported; + struct rlimit rl; + int r; + + test_setup_logging(LOG_DEBUG); + + assert_se(getrlimit(RLIMIT_MEMLOCK, &rl) >= 0); + rl.rlim_cur = rl.rlim_max = MAX(rl.rlim_max, CAN_MEMLOCK_SIZE); + (void) setrlimit(RLIMIT_MEMLOCK, &rl); + + if (!can_memlock()) + return log_tests_skipped("Can't use mlock()"); + + r = enter_cgroup_subroot(&cgroup); + if (r == -ENOMEDIUM) + return log_tests_skipped("cgroupfs not available"); + + r = bpf_devices_supported(); + if (!r) + return log_tests_skipped("BPF device filter not supported"); + assert_se(r == 1); + + r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, cgroup, NULL, &controller_path); + assert_se(r >= 0); + + _cleanup_(bpf_program_unrefp) BPFProgram *prog = NULL; + + test_policy_closed(cgroup, &prog); + test_policy_strict(cgroup, &prog); + + test_policy_whitelist_major("mem", cgroup, &prog); + test_policy_whitelist_major("1", cgroup, &prog); + + test_policy_whitelist_major_star('c', cgroup, &prog); + test_policy_whitelist_major_star('b', cgroup, &prog); + + assert_se(parent = dirname_malloc(cgroup)); + + assert_se(cg_mask_supported(&supported) >= 0); + r = cg_attach_everywhere(supported, parent, 0, NULL, NULL); + assert_se(r >= 0); + + return 0; +} From 8ad08622d6f818f65e56b1e588221a4d546ab9bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 9 Nov 2019 12:32:44 +0100 Subject: [PATCH 22/25] bpf: convert 'c'/'b' to bpf_type at the very end This makes the code a bit longer, but easier to read I think, because the cgroup v1 and v2 code paths are more similar. And whent he type is a char, any backtrace is easier to interpret. --- src/core/bpf-devices.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 2683db8e71..932c73033a 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -38,7 +38,7 @@ static int bpf_access_type(const char *acc) { return r; } -static int bpf_prog_whitelist_device(BPFProgram *prog, int type, int major, int minor, const char *acc) { +static int bpf_prog_whitelist_device(BPFProgram *prog, char type, int major, int minor, const char *acc) { int r, access; assert(prog); @@ -50,8 +50,11 @@ static int bpf_prog_whitelist_device(BPFProgram *prog, int type, int major, int if (access <= 0) return -EINVAL; + assert(IN_SET(type, 'b', 'c')); + const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; + const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 6), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 6), /* compare device type */ BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 3), /* compare access type */ @@ -67,7 +70,7 @@ static int bpf_prog_whitelist_device(BPFProgram *prog, int type, int major, int return r; } -static int bpf_prog_whitelist_major(BPFProgram *prog, int type, int major, const char *acc) { +static int bpf_prog_whitelist_major(BPFProgram *prog, char type, int major, const char *acc) { int r, access; assert(prog); @@ -79,8 +82,11 @@ static int bpf_prog_whitelist_major(BPFProgram *prog, int type, int major, const if (access <= 0) return -EINVAL; + assert(IN_SET(type, 'b', 'c')); + const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; + const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 5), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 5), /* compare device type */ BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 2), /* compare access type */ @@ -95,7 +101,7 @@ static int bpf_prog_whitelist_major(BPFProgram *prog, int type, int major, const return r; } -static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) { +static int bpf_prog_whitelist_class(BPFProgram *prog, char type, const char *acc) { int r, access; assert(prog); @@ -107,8 +113,11 @@ static int bpf_prog_whitelist_class(BPFProgram *prog, int type, const char *acc) if (access <= 0) return -EINVAL; + assert(IN_SET(type, 'b', 'c')); + const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; + const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, type, 4), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 4), /* compare device type */ BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 1), /* compare access type */ @@ -292,13 +301,12 @@ static int whitelist_device_pattern(BPFProgram *prog, const char *path, char typ if (!prog) return 0; - const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; if (maj && min) - return bpf_prog_whitelist_device(prog, bpf_type, *maj, *min, acc); + return bpf_prog_whitelist_device(prog, type, *maj, *min, acc); else if (maj) - return bpf_prog_whitelist_major(prog, bpf_type, *maj, acc); + return bpf_prog_whitelist_major(prog, type, *maj, acc); else - return bpf_prog_whitelist_class(prog, bpf_type, acc); + return bpf_prog_whitelist_class(prog, type, acc); } else { char buf[2+DECIMAL_STR_MAX(unsigned)*2+2+4]; From 00486578281bccbe36a6a447b3597d6bf8967933 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 10 Nov 2019 13:19:51 +0100 Subject: [PATCH 23/25] bpf: optimize device type access away most of the time Most of the time, we specify the allowed access mode as "rwm", so the check always trivially passes. In that case, skip the check. The repeating part changes from: 5: (55) if r2 != 0x2 goto pc+6 6: (bc) w1 = w3 7: (54) w1 &= 7 8: (5d) if r1 != r3 goto pc+3 9: (55) if r4 != 0x1 goto pc+2 10: (55) if r5 != 0x3 goto pc+1 11: (05) goto pc+8 to 6: (55) if r2 != 0x2 goto pc+3 7: (55) if r4 != 0x1 goto pc+2 8: (55) if r5 != 0x3 goto pc+1 9: (05) goto pc+40 --- src/core/bpf-devices.c | 48 ++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 932c73033a..827c3ab949 100644 --- a/src/core/bpf-devices.c +++ b/src/core/bpf-devices.c @@ -54,16 +54,20 @@ static int bpf_prog_whitelist_device(BPFProgram *prog, char type, int major, int const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 6), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 3), /* compare access type */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 2), /* compare major */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_5, minor, 1), /* compare minor */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 4), /* compare access type */ + + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 3), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 2), /* compare major */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_5, minor, 1), /* compare minor */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ }; - r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); + if (FLAGS_SET(access, BPF_DEVCG_ACC_READ | BPF_DEVCG_ACC_WRITE | BPF_DEVCG_ACC_MKNOD)) + r = bpf_program_add_instructions(prog, insn + 3, ELEMENTSOF(insn) - 3); + else + r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) log_error_errno(r, "Extending device control BPF program failed: %m"); @@ -86,15 +90,19 @@ static int bpf_prog_whitelist_major(BPFProgram *prog, char type, int major, cons const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 5), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 2), /* compare access type */ - BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 1), /* compare major */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 3), /* compare access type */ + + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 2), /* compare device type */ + BPF_JMP_IMM(BPF_JNE, BPF_REG_4, major, 1), /* compare major */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ }; - r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); + if (FLAGS_SET(access, BPF_DEVCG_ACC_READ | BPF_DEVCG_ACC_WRITE | BPF_DEVCG_ACC_MKNOD)) + r = bpf_program_add_instructions(prog, insn + 3, ELEMENTSOF(insn) - 3); + else + r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) log_error_errno(r, "Extending device control BPF program failed: %m"); @@ -117,14 +125,18 @@ static int bpf_prog_whitelist_class(BPFProgram *prog, char type, const char *acc const int bpf_type = type == 'c' ? BPF_DEVCG_DEV_CHAR : BPF_DEVCG_DEV_BLOCK; const struct bpf_insn insn[] = { - BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 4), /* compare device type */ - BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), /* calculate access type */ + BPF_MOV32_REG(BPF_REG_1, BPF_REG_3), BPF_ALU32_IMM(BPF_AND, BPF_REG_1, access), - BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 1), /* compare access type */ - BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ + BPF_JMP_REG(BPF_JNE, BPF_REG_1, BPF_REG_3, 2), /* compare access type */ + + BPF_JMP_IMM(BPF_JNE, BPF_REG_2, bpf_type, 1), /* compare device type */ + BPF_JMP_A(PASS_JUMP_OFF), /* jump to PASS */ }; - r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); + if (FLAGS_SET(access, BPF_DEVCG_ACC_READ | BPF_DEVCG_ACC_WRITE | BPF_DEVCG_ACC_MKNOD)) + r = bpf_program_add_instructions(prog, insn + 3, ELEMENTSOF(insn) - 3); + else + r = bpf_program_add_instructions(prog, insn, ELEMENTSOF(insn)); if (r < 0) log_error_errno(r, "Extending device control BPF program failed: %m"); From 45669ae264c2d16db006c49ea264c38bd2080a22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 10 Nov 2019 23:08:21 +0100 Subject: [PATCH 24/25] 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. --- src/core/bpf-devices.c | 7 ++++++- src/core/cgroup.c | 39 ++++++++++++++++++++++++++----------- src/test/test-bpf-devices.c | 34 ++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 12 deletions(-) diff --git a/src/core/bpf-devices.c b/src/core/bpf-devices.c index 827c3ab949..984603003e 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 5f9a6b617b..ca86cdc3c4 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; } - r = bpf_devices_apply_policy(prog, c->device_policy, c->device_allow, path, &u->bpf_device_control_installed); + 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, 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 aef9d359d4..aaa2c3ce3a 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); From 26444635eabf481dfa05d18cc552cb90a6885a54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 11 Nov 2019 21:04:25 +0100 Subject: [PATCH 25/25] test-bpf-devices: skip test on !unified The code in cgroup.c has support for all hierarchies, but the test, as written, will only work on unified. Since the test is really about bpf code, and not the legacy devices controller, let's just skip the test. --- src/test/test-bpf-devices.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/test-bpf-devices.c b/src/test/test-bpf-devices.c index aaa2c3ce3a..1322af4812 100644 --- a/src/test/test-bpf-devices.c +++ b/src/test/test-bpf-devices.c @@ -263,6 +263,10 @@ int main(int argc, char *argv[]) { rl.rlim_cur = rl.rlim_max = MAX(rl.rlim_max, CAN_MEMLOCK_SIZE); (void) setrlimit(RLIMIT_MEMLOCK, &rl); + r = cg_all_unified(); + if (r <= 0) + return log_tests_skipped("We don't seem to be running with unified cgroup hierarchy"); + if (!can_memlock()) return log_tests_skipped("Can't use mlock()");