filter: fix filter

An additional notification is need either when we jump from an
instruction without arguments then at the end of the argument checks.

4 files changed, 133 insertions, 56 deletions

diff --git a/cooker/filter.c b/cooker/filter.c
index 717e525..350d6bc 100644
--- a/cooker/filter.c
+++ b/cooker/filter.c
@@ -172,43 +172,63 @@ static unsigned get_n_args_syscall(const struct syscall_entry *table)
 	return n;
 }
 
-static unsigned int get_total_args_instr(const struct syscall_entry table[],
-					 unsigned int n_syscall)
+static unsigned int get_n_args_syscall_instr(const struct syscall_entry *table)
 {
-	unsigned i, k, j, n = 0, total_instr = 0;
-	const struct syscall_entry *t = &table[0];
 	const struct bpf_call *entry;
-
-	for (j = 0; j < n_syscall; j++) {
-		t = &table[j];
-		for (i = 0; i < t->count; i++) {
-			entry = t->entry + i;
-			n = 0;
-			/* For every argument there are 2 instructions, one to
-			 * load the value and the second to evaluate the
-			 * argument
-			 */
-			for (k = 0; k < 6; k++) {
-				if (entry->check_arg[k])
-					n += 2;
+	bool has_arg = false;
+	unsigned n = 0, total_instr = 0;
+
+	for (unsigned int i = 0; i < table->count; i++) {
+		entry = table->entry + i;
+		n = 0;
+
+		/* For every argument there are 2 instructions, one to
+		* load the value and the second to evaluate the
+		* argument
+		*/
+		for (unsigned int k = 0; k < 6; k++) {
+			if (entry->check_arg[k]) {
+				n += 2;
 			}
-			total_instr += n;
-			/* If there is at least an arguments then there is an additional
-		 * instruction for the notification
-		 */
-			if (n > 0)
-				total_instr++;
+		}
+		total_instr += n;
+		if (n > 0) {
+			has_arg = true;
+			total_instr++;
 		}
 	}
+	if (has_arg)
+		total_instr++;
+
 	return total_instr;
 }
 
+static unsigned int get_total_args_instr(const struct syscall_entry table[],
+					 unsigned int n_syscall)
+{
+	unsigned i, n = 0;
+
+	for (i = 0; i < n_syscall; i++) {
+			n += get_n_args_syscall_instr(&table[i]);
+	}
+	return n;
+}
+
 static bool check_args_syscall_entry(const struct bpf_call *entry){
 	return entry->check_arg[0] || entry->check_arg[1] ||
 	       entry->check_arg[2] || entry->check_arg[3] ||
 	       entry->check_arg[4] || entry->check_arg[5];
 }
 
+static bool check_args_syscall(const struct syscall_entry *table)
+{
+	for (unsigned int i = 0; i < table->count; i++) {
+		if (check_args_syscall_entry(table->entry + i))
+			return true;
+	}
+	return false;
+}
+
 unsigned int create_bpf_program_log(struct sock_filter filter[])
 {
 	filter[0] = (struct sock_filter)BPF_STMT(
@@ -232,20 +252,22 @@ unsigned int create_bfp_program(struct syscall_entry table[],
 	unsigned int n_args, n_nodes;
 	unsigned int notify, accept;
 	unsigned int i, j, k, size;
-	unsigned int next_offset;
+	unsigned int next_offset, offset;
 	unsigned int next_args_off;
 	int nodes[MAX_JUMPS];
 
 	create_lookup_nodes(nodes, n_syscall);
 
+	/* First 3 checks */
 	size = 3;
 	/* No nodes if there is a single syscall */
 	n_nodes = (1 << count_shift_right(n_syscall - 1)) - 1;
 
 	n_args = get_total_args_instr(table, n_syscall);
 
-	accept = 2 + n_nodes + 2 * n_syscall + n_args + 1;
-	notify = 2 + n_nodes + 2 * n_syscall + n_args + 2;
+	/* pre-check instruction + load syscall number (4 instructions) */
+	accept = 3 + n_nodes + n_syscall + n_args;
+	notify = accept + 1;
 
 	/* Pre */
 	/* cppcheck-suppress badBitmaskCheck */
@@ -274,8 +296,15 @@ unsigned int create_bfp_program(struct syscall_entry table[],
 	next_offset = n_syscall - 1;
 	/* Insert leaves */
 	for (i = 0; i < n_syscall; i++) {
-		filter[size++] = (struct sock_filter)EQ(
-			table[i].nr, next_offset, accept - size);
+		/* If the syscall doesn't have any arguments, jump directly to
+		 * the notification
+		 */
+		if (check_args_syscall(&table[i]))
+			offset = next_offset;
+		else
+			offset = notify - size - 1;
+		filter[size++] = (struct sock_filter)EQ(table[i].nr, offset,
+							accept - size);
 		next_offset += get_n_args_syscall(&table[i]);
 	}
 
@@ -285,11 +314,12 @@ unsigned int create_bfp_program(struct syscall_entry table[],
 	 * check the following entry of the syscall and its arguments.
 	 */
 	for (i = 0; i < n_syscall; i++) {
+		bool has_arg = false;
 		for (j = 0; j < (table[i]).count; j++) {
 			unsigned n_checks = 0;
 			entry = table[i].entry + j;
 			next_args_off = get_n_args_syscall_entry(entry);
-			for (k = 0; k < 6; k++)
+			for (k = 0; k < 6; k++) {
 				if (entry->check_arg[k]) {
 					filter[size++] = (struct sock_filter)
 						LOAD((offsetof(
@@ -299,18 +329,19 @@ unsigned int create_bfp_program(struct syscall_entry table[],
 						(table[i].entry + j)->args[k],
 						0, next_args_off - n_checks);
 					n_checks++;
+					has_arg = true;
 				}
-			/* If there is at least an argument, it needs to notify
-			 * if all the arguments checks have passed.
-			 */
-			if (check_args_syscall_entry(entry))
+			}
+			if (check_args_syscall_entry(table[i].entry))
 				filter[size++] = (struct sock_filter)JUMPA(
 					notify - size);
 		}
 		/* At this point none of the checks was positive, it jumps to
 		 * the default behavior
 		 */
-		filter[size++] = (struct sock_filter)JUMPA(accept - size);
+		if (has_arg)
+			filter[size++] =
+				(struct sock_filter)JUMPA(accept - size);
 	}
 
 	/* Seccomp accept and notify instruction */
diff --git a/tests/unit/test_filter.c b/tests/unit/test_filter.c
index 5ff9d65..9583b56 100644
--- a/tests/unit/test_filter.c
+++ b/tests/unit/test_filter.c
@@ -6,21 +6,9 @@
 #define _GNU_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
-#include <string.h>
-#include <sched.h>
-#include <unistd.h>
-#include <limits.h>
-#include <fcntl.h>
-#include <sys/prctl.h>
 #include <sys/syscall.h>
-#include <sys/ioctl.h>
-#include <sys/wait.h>
-#include <linux/audit.h>
-#include <linux/filter.h>
-#include <linux/seccomp.h>
 #include <sys/mman.h>
-#include <sys/un.h>
-#include <sys/socket.h>
+#include <sys/resource.h>
 
 #include <check.h>
 
@@ -28,52 +16,94 @@
 #include "common.h"
 #include "testutil.h"
 #include "filter.h"
+#include "disasm.h"
 
 static int generate_install_filter(struct args_target *at)
 {
-	struct bpf_call calls[] = { {} };
+	unsigned int i;
+	struct bpf_call calls[1];
 	struct syscall_entry table[] = {
 		{ .count = 1, .nr = at->nr, .entry = &calls[0] }
 	};
 	struct sock_filter filter[30];
 	unsigned int size;
 
+	for (i = 0; i < 6; i++) {
+		if (at->args[i] != NULL) {
+			calls[0].args[i] = (int)at->args[i];
+			calls[0].check_arg[i] = true;
+		} else {
+			calls[0].check_arg[i] = false;
+		}
+	}
 	size = create_bfp_program(table, filter, 1);
+	//bpf_disasm_all(filter, size);
 	return install_filter(filter, size);
 }
 
-void setup_build_filter()
+START_TEST(no_args)
 {
 	at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE,
 		  MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 	at->check_fd = false;
 	at->nr = __NR_getpid;
-	at->args[0] = NULL;
 	at->install_filter = generate_install_filter;
 	setup();
+	mock_syscall_target();
 }
+END_TEST
 
-START_TEST(filter)
+START_TEST(with_getsid)
 {
-	continue_target();
+	int id = 12345;
+	at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE,
+		  MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+	at->check_fd = false;
+	at->nr = __NR_getsid;
+	at->args[0] = &id;
+	at->install_filter = generate_install_filter;
+	setup();
+	mock_syscall_target();
 }
 END_TEST
 
+START_TEST(with_getpriority)
+{
+	int which = 12345;
+	id_t who = PRIO_PROCESS;
+	at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE,
+		  MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+	at->check_fd = false;
+	at->nr = __NR_getpriority;
+	at->args[0] = &which;
+	at->args[1] = &who;
+	at->install_filter = generate_install_filter;
+	setup();
+	mock_syscall_target();
+}
+END_TEST
 
 Suite *op_call_suite(void)
 {
 	Suite *s;
 	int timeout = 30;
-	TCase *simple;
+	TCase *simple, *args32;
 
 	s = suite_create("Test filter with target");
 
-	simple = tcase_create("simple");
-	tcase_add_checked_fixture(simple, setup_build_filter, teardown);
+	simple = tcase_create("no args");
+	tcase_add_checked_fixture(simple, NULL, teardown);
 	tcase_set_timeout(simple, timeout);
-	tcase_add_test(simple, filter);
+	tcase_add_test(simple, no_args);
 	suite_add_tcase(s, simple);
 
+	args32 = tcase_create("with args 32 bit");
+	tcase_add_checked_fixture(args32, NULL, teardown);
+	tcase_set_timeout(args32, timeout);
+	tcase_add_test(args32, with_getsid);
+	tcase_add_test(args32, with_getpriority);
+	suite_add_tcase(s, args32);
+
 	return s;
 }
 
diff --git a/tests/unit/testutil.h b/tests/unit/testutil.h
index d4f83af..dd4f1e9 100644
--- a/tests/unit/testutil.h
+++ b/tests/unit/testutil.h
@@ -42,5 +42,6 @@ void setup();
 void teardown();
 int install_notification_filter(struct args_target *at);
 void continue_target();
+void mock_syscall_target();
 
 #endif /* TESTUTIL_H */
diff --git a/tests/unit/util.c b/tests/unit/util.c
index c6fc3fb..5a1c5aa 100644
--- a/tests/unit/util.c
+++ b/tests/unit/util.c
@@ -170,6 +170,21 @@ void continue_target()
 	ck_assert_msg(ret == 0, strerror(errno));
 }
 
+void mock_syscall_target()
+{
+	struct seccomp_notif_resp resp;
+	int ret;
+
+	ret = ioctl(notifyfd, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id);
+	ck_assert_msg(ret == 0, strerror(errno));
+	resp.id = req.id;
+	resp.flags = 0;
+	resp.error = 0;
+	resp.val = 0;
+	ret = ioctl(notifyfd, SECCOMP_IOCTL_NOTIF_SEND, &resp);
+	ck_assert_msg(ret == 0, strerror(errno));
+}
+
 void setup()
 {
 	int ret;