From: Wolfgang Bumiller Date: Thu, 23 Jul 2015 09:10:18 +0000 (+0200) Subject: seccomp: simplify and fix rule parsing X-Git-Tag: lxc-1.1.3~20 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=04c3432004cc805e3bb6e1159769adf416c9b913;p=thirdparty%2Flxc.git seccomp: simplify and fix rule parsing 1) Two checks on amd64 for whether compat_ctx has already been generated were redundant, as compat_ctx is generally generated before entering the parsing loop. 2) With introduction of reject_force_umount the check for whether the syscall has the same id on both native and compat archs results in false behavior as this is an internal keyword and thus produces a -1 on seccomp_syscall_resolve_name_arch(). The result was that it was added to the native architecture twice and never to the 32 bit architecture, causing it to have no effect on 32 bit containers on 64 bit hosts. 3) I do not see a reason to care about whether the syscalls have the same number on the two architectures. On the one hand this check was there to avoid adding it to two archs (and effectively leaving one arch unprotected), while on the other hand it seemed to be okay to add it to the same arch *twice*. The entire architecture checking branches are now reduced to three simple cases: 'native', 'non-native' and 'all'. With 'all' adding to both architectures regardless of the syscall ID. Also note that libseccomp had a bug in its architecture checking, so architecture related filters weren't working as expected before version 2.2.2, which may have contributed to the confusion in the original architecture-related code. Signed-off-by: Wolfgang Bumiller --- diff --git a/src/lxc/seccomp.c b/src/lxc/seccomp.c index 108faa0a8..07dfbc6d7 100644 --- a/src/lxc/seccomp.c +++ b/src/lxc/seccomp.c @@ -259,6 +259,7 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf) uint32_t default_policy_action = -1, default_rule_action = -1, action; enum lxc_hostarch_t native_arch = get_hostarch(), cur_rule_arch = native_arch; + uint32_t compat_arch = SCMP_ARCH_NATIVE; if (strncmp(line, "blacklist", 9) == 0) blacklist = true; @@ -288,6 +289,7 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf) if (native_arch == lxc_seccomp_arch_amd64) { cur_rule_arch = lxc_seccomp_arch_all; + compat_arch = SCMP_ARCH_X86; compat_ctx = get_new_ctx(lxc_seccomp_arch_i386, default_policy_action); if (!compat_ctx) @@ -324,14 +326,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf) continue; } cur_rule_arch = lxc_seccomp_arch_i386; - if (native_arch == lxc_seccomp_arch_amd64) { - if (compat_ctx) - continue; - compat_ctx = get_new_ctx(lxc_seccomp_arch_i386, - default_policy_action); - if (!compat_ctx) - goto bad; - } } else if (strcmp(line, "[X86_64]") == 0 || strcmp(line, "[x86_64]") == 0) { if (native_arch != lxc_seccomp_arch_amd64) { @@ -342,14 +336,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf) } else if (strcmp(line, "[all]") == 0 || strcmp(line, "[ALL]") == 0) { cur_rule_arch = lxc_seccomp_arch_all; - if (native_arch == lxc_seccomp_arch_amd64 && !compat_ctx) { - if (compat_ctx) - continue; - compat_ctx = get_new_ctx(lxc_seccomp_arch_i386, - default_policy_action); - if (!compat_ctx) - goto bad; - } } #ifdef SCMP_ARCH_ARM else if (strcmp(line, "[arm]") == 0 || @@ -408,41 +394,24 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf) goto bad_rule; } - /* - * TODO generalize - if !is_compat_only(native_arch, cur_rule_arch) - * - * in other words, the rule is 32-bit only, on 64-bit host; don't run - * the rule against the native arch. - */ - if (!(cur_rule_arch == lxc_seccomp_arch_i386 && - native_arch == lxc_seccomp_arch_amd64)) { - INFO("Adding non-compat rule for %s action %d", line, action); + if (cur_rule_arch == native_arch || + cur_rule_arch == lxc_seccomp_arch_native || + compat_arch == SCMP_ARCH_NATIVE) { + INFO("Adding native rule for %s action %d", line, action); if (!do_resolve_add_rule(SCMP_ARCH_NATIVE, line, conf->seccomp_ctx, action)) goto bad_rule; } - - /* - * TODO generalize - if need_compat(native_arch, cur_rule_arch) - */ - if (native_arch == lxc_seccomp_arch_amd64 && - cur_rule_arch != lxc_seccomp_arch_amd64) { - int nr1, nr2; + else if (cur_rule_arch != lxc_seccomp_arch_all) { + INFO("Adding compat-only rule for %s action %d", line, action); + if (!do_resolve_add_rule(compat_arch, line, compat_ctx, action)) + goto bad_rule; + } + else { + INFO("Adding native rule for %s action %d", line, action); + if (!do_resolve_add_rule(SCMP_ARCH_NATIVE, line, conf->seccomp_ctx, action)) + goto bad_rule; INFO("Adding compat rule for %s action %d", line, action); - nr1 = seccomp_syscall_resolve_name_arch(SCMP_ARCH_X86, line); - nr2 = seccomp_syscall_resolve_name_arch(SCMP_ARCH_NATIVE, line); - if (nr1 == nr2) { - /* If the syscall # is the same for 32- and 64-bit, then we cannot - * apply it to the compat_ctx. So apply it to the noncompat ctx. - * We may already have done so, but that's ok - */ - INFO("Adding non-compat rule bc nr1 == nr2 (%d, %d)", nr1, nr2); - if (!do_resolve_add_rule(SCMP_ARCH_NATIVE, line, conf->seccomp_ctx, action)) - goto bad_rule; - continue; - } - INFO("Really adding compat rule bc nr1 == nr2 (%d, %d)", nr1, nr2); - if (!do_resolve_add_rule(SCMP_ARCH_X86, line, - compat_ctx, action)) + if (!do_resolve_add_rule(compat_arch, line, compat_ctx, action)) goto bad_rule; } }