all: only request the userspace notification fd once

It turns out that requesting the seccomp userspace notifcation fd
more than once is a bad thing which causes the kernel to complain
(rightfully so for a variety of reasons).  Unfortunately as we were
always requesting the notification fd whenever possible this results
in problems at filter load time.

Our solution is to move the notification fd out of the filter context
and into the global task context, using a newly created task_state
structure.  This allows us to store, and retrieve the notification
outside the scope of an individual filter context.  It also provides
some implementation improvements by giving us a convenient place to
stash all of the API level related support variables.  We also extend
the seccomp_reset() API call to reset this internal global state when
passed a NULL filter context.

There is one potential case which we don't currently handle well:
threads.  At the moment libseccomp is thread ignorant, and that works
well as the only global state up to this point was the currently
supported API level information which was common to all threads in a
process.  Unfortunately, it appears that the notification fd need not
be common to all threads in a process, yet this patch treats it as if
it is common.  I suspect this is a very unusual use case so I decided
to keep this patch simple and ignore this case, but in the future if
we need to support this properly we should be able to do so without
API changes by keeping an internal list of notification fds indexed
by gettid(2).

This fixes the GitHub issue below:
* https://github.com/seccomp/libseccomp/issues/273

Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Acked-by: Tom Hromatka <tom.hromatka@oracle.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>

5 files changed, 148 insertions, 82 deletions

diff --git a/src/api.c b/src/api.c
index 00975ad..5cec088 100644
--- a/src/api.c
+++ b/src/api.c
@@ -301,10 +301,18 @@ API int seccomp_reset(scmp_filter_ctx ctx, uint32_t def_action)
 {
 	struct db_filter_col *col = (struct db_filter_col *)ctx;
 
-	/* use a NULL filter collection here since we are resetting it */
-	if (ctx == NULL || db_col_action_valid(NULL, def_action) < 0)
+	/* a NULL filter context indicates we are resetting the global state */
+	if (ctx == NULL) {
+		/* reset the global state and redetermine the api level */
+		sys_reset_state();
+		_seccomp_api_update();
+		return _rc_filter(0);
+	}
+	/* ensure the default action is valid */
+	if (db_col_action_valid(NULL, def_action) < 0)
 		return _rc_filter(-EINVAL);
 
+	/* reset the filter */
 	return _rc_filter(db_col_reset(col, def_action));
 }
 
@@ -675,16 +683,17 @@ API int seccomp_notify_id_valid(int fd, uint64_t id)
 /* NOTE - function header comment in include/seccomp.h */
 API int seccomp_notify_fd(const scmp_filter_ctx ctx)
 {
-	struct db_filter_col *col;
+	/* NOTE: for historical reasons, and possibly future use, we require a
+	 * valid filter context even though we don't actual use it here; the
+	 * api update is also not strictly necessary, but keep it for now */
 
 	/* force a runtime api level detection */
 	_seccomp_api_update();
 
 	if (_ctx_valid(ctx))
 		return _rc_filter(-EINVAL);
-	col = (struct db_filter_col *)ctx;
 
-	return _rc_filter(col->notify_fd);
+	return _rc_filter(sys_notify_fd());
 }
 
 /* NOTE - function header comment in include/seccomp.h */
diff --git a/src/db.c b/src/db.c
index 4a87ea3..836171a 100644
--- a/src/db.c
+++ b/src/db.c
@@ -1057,7 +1057,6 @@ int db_col_reset(struct db_filter_col *col, uint32_t def_action)
 	if (col->filters)
 		free(col->filters);
 	col->filters = NULL;
-	col->notify_fd = -1;
 
 	/* set the endianess to undefined */
 	col->endian = 0;
diff --git a/src/db.h b/src/db.h
index b96b104..765c607 100644
--- a/src/db.h
+++ b/src/db.h
@@ -160,8 +160,7 @@ struct db_filter_col {
 	/* transaction snapshots */
 	struct db_filter_snap *snapshots;
 
-	/* notification fd that was returned from seccomp() */
-	int notify_fd;
+	/* userspace notification */
 	bool notify_used;
 };
 
diff --git a/src/system.c b/src/system.c
index 6cdfc16..3b43b2a 100644
--- a/src/system.c
+++ b/src/system.c
@@ -40,16 +40,61 @@
  *       our next release we may have to enable the allowlist */
 #define SYSCALL_ALLOWLIST_ENABLE	0
 
-static int _nr_seccomp = -1;
-static int _support_seccomp_syscall = -1;
-static int _support_seccomp_flag_tsync = -1;
-static int _support_seccomp_flag_log = -1;
-static int _support_seccomp_action_log = -1;
-static int _support_seccomp_kill_process = -1;
-static int _support_seccomp_flag_spec_allow = -1;
-static int _support_seccomp_flag_new_listener = -1;
-static int _support_seccomp_user_notif = -1;
-static int _support_seccomp_flag_tsync_esrch = -1;
+/* task global state */
+struct task_state {
+	/* seccomp(2) syscall */
+	int nr_seccomp;
+
+	/* userspace notification fd */
+	int notify_fd;
+
+	/* runtime support flags */
+	int sup_syscall;
+	int sup_flag_tsync;
+	int sup_flag_log;
+	int sup_action_log;
+	int sup_kill_process;
+	int sup_flag_spec_allow;
+	int sup_flag_new_listener;
+	int sup_user_notif;
+	int sup_flag_tsync_esrch;
+};
+static struct task_state state = {
+	.nr_seccomp = -1,
+
+	.notify_fd = -1,
+
+	.sup_syscall = -1,
+	.sup_flag_tsync = -1,
+	.sup_flag_log = -1,
+	.sup_action_log = -1,
+	.sup_kill_process = -1,
+	.sup_flag_spec_allow = -1,
+	.sup_flag_new_listener = -1,
+	.sup_user_notif = -1,
+	.sup_flag_tsync_esrch = -1,
+};
+
+/**
+ * Reset the task state
+ *
+ * This function fully resets the library's global "system task state".
+ *
+ */
+void sys_reset_state(void)
+{
+	state.nr_seccomp = -1;
+	state.notify_fd = -1;
+	state.sup_syscall = -1;
+	state.sup_flag_tsync = -1;
+	state.sup_flag_log = -1;
+	state.sup_action_log = -1;
+	state.sup_kill_process = -1;
+	state.sup_flag_spec_allow = -1;
+	state.sup_flag_new_listener = -1;
+	state.sup_user_notif = -1;
+	state.sup_flag_tsync_esrch = -1;
+}
 
 /**
  * Check to see if the seccomp() syscall is supported
@@ -68,8 +113,8 @@ int sys_chk_seccomp_syscall(void)
 	/* NOTE: it is reasonably safe to assume that we should be able to call
 	 *       seccomp() when the caller first starts, but we can't rely on
 	 *       it later so we need to cache our findings for use later */
-	if (_support_seccomp_syscall >= 0)
-		return _support_seccomp_syscall;
+	if (state.sup_syscall >= 0)
+		return state.sup_syscall;
 
 #if SYSCALL_ALLOWLIST_ENABLE
 	/* architecture allowlist */
@@ -100,11 +145,11 @@ int sys_chk_seccomp_syscall(void)
 		goto supported;
 
 unsupported:
-	_support_seccomp_syscall = 0;
+	state.sup_syscall = 0;
 	return 0;
 supported:
-	_nr_seccomp = nr_seccomp;
-	_support_seccomp_syscall = 1;
+	state.nr_seccomp = nr_seccomp;
+	state.sup_syscall = 1;
 	return 1;
 }
 
@@ -118,7 +163,7 @@ supported:
  */
 void sys_set_seccomp_syscall(bool enable)
 {
-	_support_seccomp_syscall = (enable ? 1 : 0);
+	state.sup_syscall = (enable ? 1 : 0);
 }
 
 /**
@@ -132,16 +177,16 @@ void sys_set_seccomp_syscall(bool enable)
 int sys_chk_seccomp_action(uint32_t action)
 {
 	if (action == SCMP_ACT_KILL_PROCESS) {
-		if (_support_seccomp_kill_process < 0) {
+		if (state.sup_kill_process < 0) {
 			if (sys_chk_seccomp_syscall() == 1 &&
-			    syscall(_nr_seccomp, SECCOMP_GET_ACTION_AVAIL, 0,
-				    &action) == 0)
-				_support_seccomp_kill_process = 1;
+			    syscall(state.nr_seccomp,
+				    SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0)
+				state.sup_kill_process = 1;
 			else
-				_support_seccomp_kill_process = 0;
+				state.sup_kill_process = 0;
 		}
 
-		return _support_seccomp_kill_process;
+		return state.sup_kill_process;
 	} else if (action == SCMP_ACT_KILL_THREAD) {
 		return 1;
 	} else if (action == SCMP_ACT_TRAP) {
@@ -152,30 +197,30 @@ int sys_chk_seccomp_action(uint32_t action)
 	} else if (action == SCMP_ACT_TRACE(action & 0x0000ffff)) {
 		return 1;
 	} else if (action == SCMP_ACT_LOG) {
-		if (_support_seccomp_action_log < 0) {
+		if (state.sup_action_log < 0) {
 			if (sys_chk_seccomp_syscall() == 1 &&
-			    syscall(_nr_seccomp, SECCOMP_GET_ACTION_AVAIL, 0,
-				    &action) == 0)
-				_support_seccomp_action_log = 1;
+			    syscall(state.nr_seccomp,
+				    SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0)
+				state.sup_action_log = 1;
 			else
-				_support_seccomp_action_log = 0;
+				state.sup_action_log = 0;
 		}
 
-		return _support_seccomp_action_log;
+		return state.sup_action_log;
 	} else if (action == SCMP_ACT_ALLOW) {
 		return 1;
 	} else if (action == SCMP_ACT_NOTIFY) {
-		if (_support_seccomp_user_notif < 0) {
+		if (state.sup_user_notif < 0) {
 			struct seccomp_notif_sizes sizes;
 			if (sys_chk_seccomp_syscall() == 1 &&
-			    syscall(_nr_seccomp, SECCOMP_GET_NOTIF_SIZES, 0,
-				    &sizes) == 0)
-				_support_seccomp_user_notif = 1;
+			    syscall(state.nr_seccomp,
+				    SECCOMP_GET_NOTIF_SIZES, 0, &sizes) == 0)
+				state.sup_user_notif = 1;
 			else
-				_support_seccomp_user_notif = 0;
+				state.sup_user_notif = 0;
 		}
 
-		return _support_seccomp_user_notif;
+		return state.sup_user_notif;
 	}
 
 	return 0;
@@ -193,13 +238,13 @@ void sys_set_seccomp_action(uint32_t action, bool enable)
 {
 	switch (action) {
 	case SCMP_ACT_LOG:
-		_support_seccomp_action_log = (enable ? 1 : 0);
+		state.sup_action_log = (enable ? 1 : 0);
 		break;
 	case SCMP_ACT_KILL_PROCESS:
-		_support_seccomp_kill_process = (enable ? 1 : 0);
+		state.sup_kill_process = (enable ? 1 : 0);
 		break;
 	case SCMP_ACT_NOTIFY:
-		_support_seccomp_user_notif = (enable ? 1 : 0);
+		state.sup_user_notif = (enable ? 1 : 0);
 		break;
 	}
 }
@@ -212,13 +257,14 @@ void sys_set_seccomp_action(uint32_t action, bool enable)
  * Return one if the flag is supported, zero otherwise.
  *
  */
-static int _sys_chk_seccomp_flag_kernel(int flag)
+static int _sys_chk_flag_kernel(int flag)
 {
 	/* this is an invalid seccomp(2) call because the last argument
 	 * is NULL, but depending on the errno value of EFAULT we can
 	 * guess if the filter flag is supported or not */
 	if (sys_chk_seccomp_syscall() == 1 &&
-	    syscall(_nr_seccomp, SECCOMP_SET_MODE_FILTER, flag, NULL) == -1 &&
+	    syscall(state.nr_seccomp,
+		    SECCOMP_SET_MODE_FILTER, flag, NULL) == -1 &&
 	    errno == EFAULT)
 		return 1;
 
@@ -238,29 +284,25 @@ int sys_chk_seccomp_flag(int flag)
 {
 	switch (flag) {
 	case SECCOMP_FILTER_FLAG_TSYNC:
-		if (_support_seccomp_flag_tsync < 0)
-			_support_seccomp_flag_tsync = _sys_chk_seccomp_flag_kernel(flag);
-
-		return _support_seccomp_flag_tsync;
+		if (state.sup_flag_tsync < 0)
+			state.sup_flag_tsync = _sys_chk_flag_kernel(flag);
+		return state.sup_flag_tsync;
 	case SECCOMP_FILTER_FLAG_LOG:
-		if (_support_seccomp_flag_log < 0)
-			_support_seccomp_flag_log = _sys_chk_seccomp_flag_kernel(flag);
-
-		return _support_seccomp_flag_log;
+		if (state.sup_flag_log < 0)
+			state.sup_flag_log = _sys_chk_flag_kernel(flag);
+		return state.sup_flag_log;
 	case SECCOMP_FILTER_FLAG_SPEC_ALLOW:
-		if (_support_seccomp_flag_spec_allow < 0)
-			_support_seccomp_flag_spec_allow = _sys_chk_seccomp_flag_kernel(flag);
-
-		return _support_seccomp_flag_spec_allow;
+		if (state.sup_flag_spec_allow < 0)
+			state.sup_flag_spec_allow = _sys_chk_flag_kernel(flag);
+		return state.sup_flag_spec_allow;
 	case SECCOMP_FILTER_FLAG_NEW_LISTENER:
-		if (_support_seccomp_flag_new_listener < 0)
-			_support_seccomp_flag_new_listener = _sys_chk_seccomp_flag_kernel(flag);
-
-		return _support_seccomp_flag_new_listener;
+		if (state.sup_flag_new_listener < 0)
+			state.sup_flag_new_listener = _sys_chk_flag_kernel(flag);
+		return state.sup_flag_new_listener;
 	case SECCOMP_FILTER_FLAG_TSYNC_ESRCH:
-		if (_support_seccomp_flag_tsync_esrch < 0)
-			_support_seccomp_flag_tsync_esrch = _sys_chk_seccomp_flag_kernel(flag);
-		return _support_seccomp_flag_tsync_esrch;
+		if (state.sup_flag_tsync_esrch < 0)
+			state.sup_flag_tsync_esrch = _sys_chk_flag_kernel(flag);
+		return state.sup_flag_tsync_esrch;
 	}
 
 	return -EOPNOTSUPP;
@@ -279,19 +321,19 @@ void sys_set_seccomp_flag(int flag, bool enable)
 {
 	switch (flag) {
 	case SECCOMP_FILTER_FLAG_TSYNC:
-		_support_seccomp_flag_tsync = (enable ? 1 : 0);
+		state.sup_flag_tsync = (enable ? 1 : 0);
 		break;
 	case SECCOMP_FILTER_FLAG_LOG:
-		_support_seccomp_flag_log = (enable ? 1 : 0);
+		state.sup_flag_log = (enable ? 1 : 0);
 		break;
 	case SECCOMP_FILTER_FLAG_SPEC_ALLOW:
-		_support_seccomp_flag_spec_allow = (enable ? 1 : 0);
+		state.sup_flag_spec_allow = (enable ? 1 : 0);
 		break;
 	case SECCOMP_FILTER_FLAG_NEW_LISTENER:
-		_support_seccomp_flag_new_listener = (enable ? 1 : 0);
+		state.sup_flag_new_listener = (enable ? 1 : 0);
 		break;
 	case SECCOMP_FILTER_FLAG_TSYNC_ESRCH:
-		_support_seccomp_flag_tsync_esrch = (enable ? 1 : 0);
+		state.sup_flag_tsync_esrch = (enable ? 1 : 0);
 		break;
 	}
 }
@@ -324,7 +366,7 @@ int sys_filter_load(struct db_filter_col *col, bool rawrc)
 			goto filter_load_out;
 	}
 
-	tsync_notify = (_support_seccomp_flag_tsync_esrch > 0);
+	tsync_notify = state.sup_flag_tsync_esrch > 0 && state.notify_fd == -1;
 
 	/* load the filter into the kernel */
 	if (sys_chk_seccomp_syscall() == 1) {
@@ -333,28 +375,29 @@ int sys_filter_load(struct db_filter_col *col, bool rawrc)
 			if (col->attr.tsync_enable)
 				flgs |= SECCOMP_FILTER_FLAG_TSYNC | \
 					SECCOMP_FILTER_FLAG_TSYNC_ESRCH;
-			if (_support_seccomp_user_notif > 0)
+			if (state.sup_user_notif > 0)
 				flgs |= SECCOMP_FILTER_FLAG_NEW_LISTENER;
 		} else if (col->attr.tsync_enable)
 			flgs |= SECCOMP_FILTER_FLAG_TSYNC;
-		else if (_support_seccomp_user_notif > 0)
+		else if (state.sup_user_notif > 0 && state.notify_fd == -1)
 			flgs |= SECCOMP_FILTER_FLAG_NEW_LISTENER;
 		if (col->attr.log_enable)
 			flgs |= SECCOMP_FILTER_FLAG_LOG;
 		if (col->attr.spec_allow)
 			flgs |= SECCOMP_FILTER_FLAG_SPEC_ALLOW;
-		rc = syscall(_nr_seccomp, SECCOMP_SET_MODE_FILTER, flgs, prgm);
+		rc = syscall(state.nr_seccomp,
+			     SECCOMP_SET_MODE_FILTER, flgs, prgm);
 		if (tsync_notify && rc > 0) {
 			/* return 0 on NEW_LISTENER success, but save the fd */
-			col->notify_fd = rc;
+			state.notify_fd = rc;
 			rc = 0;
 		} else if (rc > 0 && col->attr.tsync_enable) {
 			/* always return -ESRCH if we fail to sync threads */
 			errno = ESRCH;
 			rc = -errno;
-		} else if (rc > 0 && _support_seccomp_user_notif > 0) {
+		} else if (rc > 0 && state.sup_user_notif > 0) {
 			/* return 0 on NEW_LISTENER success, but save the fd */
-			col->notify_fd = rc;
+			state.notify_fd = rc;
 			rc = 0;
 		}
 	} else
@@ -371,6 +414,19 @@ filter_load_out:
 }
 
 /**
+ * Return the userspace notification fd
+ *
+ * This function returns the userspace notification fd from
+ * SECCOMP_FILTER_FLAG_NEW_LISTENER.  If the notification fd has not yet been
+ * set, or an error has occurred, -1 is returned.
+ *
+ */
+int sys_notify_fd(void)
+{
+	return state.notify_fd;
+}
+
+/**
  * Allocate a pair of notification request/response structures
  * @param req the request location
  * @param resp the response location
@@ -386,7 +442,7 @@ int sys_notify_alloc(struct seccomp_notif **req,
 	int rc;
 	static struct seccomp_notif_sizes sizes = { 0, 0, 0 };
 
-	if (_support_seccomp_syscall <= 0)
+	if (state.sup_syscall <= 0)
 		return -EOPNOTSUPP;
 
 	if (sizes.seccomp_notif == 0 && sizes.seccomp_notif_resp == 0) {
@@ -427,7 +483,7 @@ int sys_notify_alloc(struct seccomp_notif **req,
  */
 int sys_notify_receive(int fd, struct seccomp_notif *req)
 {
-	if (_support_seccomp_user_notif <= 0)
+	if (state.sup_user_notif <= 0)
 		return -EOPNOTSUPP;
 
 	if (ioctl(fd, SECCOMP_IOCTL_NOTIF_RECV, req) < 0)
@@ -448,7 +504,7 @@ int sys_notify_receive(int fd, struct seccomp_notif *req)
  */
 int sys_notify_respond(int fd, struct seccomp_notif_resp *resp)
 {
-	if (_support_seccomp_user_notif <= 0)
+	if (state.sup_user_notif <= 0)
 		return -EOPNOTSUPP;
 
 	if (ioctl(fd, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0)
@@ -467,7 +523,7 @@ int sys_notify_respond(int fd, struct seccomp_notif_resp *resp)
  */
 int sys_notify_id_valid(int fd, uint64_t id)
 {
-	if (_support_seccomp_user_notif <= 0)
+	if (state.sup_user_notif <= 0)
 		return -EOPNOTSUPP;
 
 	if (ioctl(fd, SECCOMP_IOCTL_NOTIF_ID_VALID, &id) < 0)
diff --git a/src/system.h b/src/system.h
index 133f9b1..096f3ca 100644
--- a/src/system.h
+++ b/src/system.h
@@ -182,6 +182,8 @@ struct seccomp_notif_resp {
 #define SECCOMP_IOCTL_NOTIF_ID_VALID    SECCOMP_IOR(2, __u64)
 #endif /* SECCOMP_RET_USER_NOTIF */
 
+void sys_reset_state(void);
+
 int sys_chk_seccomp_syscall(void);
 void sys_set_seccomp_syscall(bool enable);
 
@@ -193,6 +195,7 @@ void sys_set_seccomp_flag(int flag, bool enable);
 
 int sys_filter_load(struct db_filter_col *col, bool rawrc);
 
+int sys_notify_fd(void);
 int sys_notify_alloc(struct seccomp_notif **req,
 		     struct seccomp_notif_resp **resp);
 int sys_notify_receive(int fd, struct seccomp_notif *req);