all: massive src/db.c rework

First, and most importantly, let me state that this is perhaps the worst
possible example of a patch I can think of, and if anyone tries to submit
a PR/patch like this one I will reject it almost immediately.  I'm only
merging this because 1) this patch escalated quickly, 2) splitting it would
require a disproportionate amount of time, and 3) this effort had blocked
other work for too long ... and, well, I'm the maintainer.  Consider this
a bit of "maintainer privilege" if you will.

This patch started simply enough: the goal was to add/augment some tests to
help increase the libseccomp test coverage.  Unfortunately, this particular
test improvement uncovered a rather tricky bug which escalated quite quickly
and soon involved a major rework of how we build the filter tree in src/db.c.
This rework brought about changes throughout the repository, including the
transaction and ABI specific code.

Signed-off-by: Paul Moore <paul@paul-moore.com>

src/arch-s390.c

@@ -194,46 +194,53 @@ int s390_syscall_rewrite(int *syscall)
 
 /**
  * add a new rule to the s390 seccomp filter
- * @param col the filter collection
  * @param db the seccomp filter db
- * @param strict the strict flag
  * @param rule the filter rule
  *
  * This function adds a new syscall filter to the seccomp filter db, making any
  * necessary adjustments for the s390 ABI. Returns zero on success, negative
  * values on failure.
  *
+ * It is important to note that in the case of failure the db may be corrupted,
+ * the caller must use the transaction mechanism if the db integrity is
+ * important.
+ *
  */
-int s390_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
- struct db_api_rule_list *rule)
+int s390_rule_add(struct db_filter *db, struct db_api_rule_list *rule)
 {
- int rc;
+ int rc = 0;
  unsigned int iter;
  int sys = rule->syscall;
  int sys_a, sys_b;
- struct db_api_rule_list *rule_a, *rule_b;
+ struct db_api_rule_list *rule_a, *rule_b, *rule_dup = NULL;
 
  if ((sys <= -100 && sys >= -120) || (sys >= 359 && sys <= 373)) {
  /* (-100 to -120) : multiplexed socket syscalls
  (359 to 373) : direct socket syscalls, Linux 4.3+ */
 
  /* strict check for the multiplexed socket syscalls */
  for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
- if ((rule->args[iter].valid != 0) && (strict))
- return -EINVAL;
+ if ((rule->args[iter].valid != 0) && (rule->strict)) {
+ rc = -EINVAL;
+ goto add_return;
+ }
  }
 
  /* determine both the muxed and direct syscall numbers */
  if (sys > 0) {
  sys_a = _s390_sock_mux(sys);
- if (sys_a == __NR_SCMP_ERROR)
- return __NR_SCMP_ERROR;
+ if (sys_a == __NR_SCMP_ERROR) {
+ rc = __NR_SCMP_ERROR;
+ goto add_return;
+ }
  sys_b = sys;
  } else {
  sys_a = sys;
  sys_b = _s390_sock_demux(sys);
- if (sys_b == __NR_SCMP_ERROR)
- return __NR_SCMP_ERROR;
+ if (sys_b == __NR_SCMP_ERROR) {
+ rc = __NR_SCMP_ERROR;
+ goto add_return;
+ }
  }
 
  /* use rule_a for the multiplexed syscall and use rule_b for
@@ -248,9 +255,12 @@ int s390_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
  } else {
  /* need two rules, dup the first and link together */
  rule_a = rule;
- rule_b = db_rule_dup(rule_a);
- if (rule_b == NULL)
- return -ENOMEM;
+ rule_dup = db_rule_dup(rule_a);
+ rule_b = rule_dup;
+ if (rule_b == NULL) {
+ rc = -ENOMEM;
+ goto add_return;
+ }
  rule_b->prev = rule_a;
  rule_b->next = NULL;
  rule_a->next = rule_b;
@@ -270,26 +280,24 @@ int s390_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
  if (rule_b != NULL)
  rule_b->syscall = sys_b;
 
- /* add the rules as a single transaction */
- rc = db_col_transaction_start(col);
- if (rc < 0)
- return rc;
+ /* we should be protected by a transaction checkpoint */
  if (rule_a != NULL) {
  rc = db_rule_add(db, rule_a);
  if (rc < 0)
- goto fail_transaction;
+ goto add_return;
  }
  if (rule_b != NULL) {
  rc = db_rule_add(db, rule_b);
  if (rc < 0)
- goto fail_transaction;
+ goto add_return;
  }
- db_col_transaction_commit(col);
  } else if (sys <= -200 && sys >= -224) {
  /* multiplexed ipc syscalls */
  for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
- if ((rule->args[iter].valid != 0) && (strict))
- return -EINVAL;
+ if ((rule->args[iter].valid != 0) && (rule->strict)) {
+ rc = -EINVAL;
+ goto add_return;
+ }
  }
  rule->args[0].arg = 0;
  rule->args[0].op = SCMP_CMP_EQ;
@@ -300,18 +308,19 @@ int s390_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 
  rc = db_rule_add(db, rule);
  if (rc < 0)
- return rc;
+ goto add_return;
  } else if (sys >= 0) {
  /* normal syscall processing */
  rc = db_rule_add(db, rule);
  if (rc < 0)
- return rc;
- } else if (strict)
- return -EDOM;
-
- return 0;
+ goto add_return;
+ } else if (rule->strict) {
+ rc = -EDOM;
+ goto add_return;
+ }
 
-fail_transaction:
- db_col_transaction_abort(col);
+add_return:
+ if (rule_dup != NULL)
+ free(rule_dup);
  return rc;
 }

src/arch-s390.h

@@ -24,7 +24,6 @@ const struct arch_syscall_def *s390_syscall_iterate(unsigned int spot);
 
 int s390_syscall_rewrite(int *syscall);
 
-int s390_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
- struct db_api_rule_list *rule);
+int s390_rule_add(struct db_filter *db, struct db_api_rule_list *rule);
 
 #endif

src/arch-s390x.c

@@ -194,46 +194,53 @@ int s390x_syscall_rewrite(int *syscall)
 
 /**
  * add a new rule to the s390x seccomp filter
- * @param col the filter collection
  * @param db the seccomp filter db
- * @param strict the strict flag
  * @param rule the filter rule
  *
  * This function adds a new syscall filter to the seccomp filter db, making any
  * necessary adjustments for the s390x ABI. Returns zero on success, negative
  * values on failure.
  *
+ * It is important to note that in the case of failure the db may be corrupted,
+ * the caller must use the transaction mechanism if the db integrity is
+ * important.
+ *
  */
-int s390x_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
- struct db_api_rule_list *rule)
+int s390x_rule_add(struct db_filter *db, struct db_api_rule_list *rule)
 {
- int rc;
+ int rc = 0;
  unsigned int iter;
  int sys = rule->syscall;
  int sys_a, sys_b;
- struct db_api_rule_list *rule_a, *rule_b;
+ struct db_api_rule_list *rule_a, *rule_b, *rule_dup = NULL;
 
  if ((sys <= -100 && sys >= -120) || (sys >= 359 && sys <= 373)) {
  /* (-100 to -120) : multiplexed socket syscalls
  (359 to 373) : direct socket syscalls, Linux 4.3+ */
 
  /* strict check for the multiplexed socket syscalls */
  for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
- if ((rule->args[iter].valid != 0) && (strict))
- return -EINVAL;
+ if ((rule->args[iter].valid != 0) && (rule->strict)) {
+ rc = -EINVAL;
+ goto add_return;
+ }
  }
 
  /* determine both the muxed and direct syscall numbers */
  if (sys > 0) {
  sys_a = _s390x_sock_mux(sys);
- if (sys_a == __NR_SCMP_ERROR)
- return __NR_SCMP_ERROR;
+ if (sys_a == __NR_SCMP_ERROR) {
+ rc = __NR_SCMP_ERROR;
+ goto add_return;
+ }
  sys_b = sys;
  } else {
  sys_a = sys;
  sys_b = _s390x_sock_demux(sys);
- if (sys_b == __NR_SCMP_ERROR)
- return __NR_SCMP_ERROR;
+ if (sys_b == __NR_SCMP_ERROR) {
+ rc = __NR_SCMP_ERROR;
+ goto add_return;
+ }
  }
 
  /* use rule_a for the multiplexed syscall and use rule_b for
@@ -248,9 +255,12 @@ int s390x_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
  } else {
  /* need two rules, dup the first and link together */
  rule_a = rule;
- rule_b = db_rule_dup(rule_a);
- if (rule_b == NULL)
- return -ENOMEM;
+ rule_dup = db_rule_dup(rule_a);
+ rule_b = rule_dup;
+ if (rule_b == NULL) {
+ rc = -ENOMEM;
+ goto add_return;
+ }
  rule_b->prev = rule_a;
  rule_b->next = NULL;
  rule_a->next = rule_b;
@@ -270,26 +280,24 @@ int s390x_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
  if (rule_b != NULL)
  rule_b->syscall = sys_b;
 
- /* add the rules as a single transaction */
- rc = db_col_transaction_start(col);
- if (rc < 0)
- return rc;
+ /* we should be protected by a transaction checkpoint */
  if (rule_a != NULL) {
  rc = db_rule_add(db, rule_a);
  if (rc < 0)
- goto fail_transaction;
+ goto add_return;
  }
  if (rule_b != NULL) {
  rc = db_rule_add(db, rule_b);
  if (rc < 0)
- goto fail_transaction;
+ goto add_return;
  }
- db_col_transaction_commit(col);
  } else if (sys <= -200 && sys >= -224) {
  /* multiplexed ipc syscalls */
  for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
- if ((rule->args[iter].valid != 0) && (strict))
- return -EINVAL;
+ if ((rule->args[iter].valid != 0) && (rule->strict)) {
+ rc = -EINVAL;
+ goto add_return;
+ }
  }
  rule->args[0].arg = 0;
  rule->args[0].op = SCMP_CMP_EQ;
@@ -300,18 +308,19 @@ int s390x_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 
  rc = db_rule_add(db, rule);
  if (rc < 0)
- return rc;
+ goto add_return;
  } else if (sys >= 0) {
  /* normal syscall processing */
  rc = db_rule_add(db, rule);
  if (rc < 0)
- return rc;
- } else if (strict)
- return -EDOM;
-
- return 0;
+ goto add_return;
+ } else if (rule->strict) {
+ rc = -EDOM;
+ goto add_return;
+ }
 
-fail_transaction:
- db_col_transaction_abort(col);
+add_return:
+ if (rule_dup != NULL)
+ free(rule_dup);
  return rc;
 }

src/arch-s390x.h

@@ -27,7 +27,6 @@ const struct arch_syscall_def *s390x_syscall_iterate(unsigned int spot);
 
 int s390x_syscall_rewrite(int *syscall);
 
-int s390x_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
- struct db_api_rule_list *rule);
+int s390x_rule_add(struct db_filter *db, struct db_api_rule_list *rule);
 
 #endif