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 <w.bumiller@proxmox.com>

diff --git a/src/lxc/seccomp.c b/src/lxc/seccomp.c
index 108faa0a8d1382dd42b4f80d7c07e1fc9a66668d..07dfbc6d78e67b42b913280d8226d7748c018f2e 100644 (file)
--- 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;
                }
        }