RFE: user notification #147

tych0 · 2019-03-19T16:23:02Z

Here's the user-notification infrastructure, and a test to make sure it really works :)

pcmoore · 2019-03-19T19:36:35Z

Thanks @tych0, although without looking at any of the code, it appears as though you are failing the Travis tests, can you look into that?

tych0 · 2019-03-19T19:52:13Z

Looks like it's failing because the Travis kernels don't have this feature yet. I guess there's some feature check/skip mechanism in the test suite for this?

…

On Tue, Mar 19, 2019, 1:36 PM Paul Moore ***@***.***> wrote: Thanks @tych0 <https://github.com/tych0>, although without looking at any of the code, it appears as though you are failing the Travis tests, can you look into that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#147 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAv61xf7aDoXpT_E-q0-Uozs5znS8Z5Qks5vYTxEgaJpZM4b8kQU> .

coveralls · 2019-03-20T17:54:27Z

Coverage decreased (-2.2%) to 89.873% when pulling 258505c on tych0:user-notification into e7c97e3 on seccomp:master.

tych0 · 2019-03-20T18:50:59Z

I'm confused,

/home/travis/build/seccomp/libseccomp/src/.libs/libseccomp_la-arch-mips64-syscalls.gcda:cannot open data file, assuming not executed
File 'arch-mips64-syscalls.c'
No executable lines
Removing 'libseccomp_la-arch-mips64-syscalls.gcno##arch-mips64-syscalls.c.gcov'

/home/travis/build/seccomp/libseccomp/src/.libs/libseccomp_la-arch-parisc64.gcno:no functions found
/home/travis/build/seccomp/libseccomp/src/.libs/libseccomp_la-arch-aarch64-syscalls.gcda:cannot open data file, assuming not executed
File 'arch-aarch64-syscalls.c'
No executable lines
Removing 'libseccomp_la-arch-aarch64-syscalls.gcno##arch-aarch64-syscalls.c.gcov'

? I don't really know how the coverage stuff works.

pcmoore · 2019-03-20T22:20:15Z

Sorry for the delay, between the day job and the kernel development window opening, it's been a busy week.

Looks like it's failing because the Travis kernels don't have this feature yet. I guess there's some feature check/skip mechanism in the test suite for this?

Yes, libseccomp has the concept of "API levels" which determine what is supported. Normally these are determined automatically at runtime, but you can force a particular level if desired. See seccomp_api_get() and seccomp_apt_set() for some more information.

The live test types allow you to specify a minimum API level required for them to run (you can see your current level with tools/scmp_api_level), see tests/47-live-kill_process.tests for an example.

? I don't really know how the coverage stuff works.

It should "just work" from a Travis perspective. There are some makefile targets for doing it locally via gcov/lcov, but that is currently broken with current gcc, although they are "working on it".

Let's not worry about the coverage too much right now, let's get the tests working first, that's most important.

pcmoore · 2019-03-20T22:22:18Z

tests/50-user-notification.tests

+test type: live
+
+# Testname	API	Result
+50-user-notification	5	ZERO


This is a nit, but as a convention, the "live" tests follow the naming format of "-live-<test_name>" where <test_name> uses underscores not hyphens.

tych0 · 2019-03-20T22:22:43Z

Yeah, I figured out the live stuff today, I think all that should be working now.

include/seccomp.h.in

src/db.c

pcmoore · 2019-04-12T18:57:39Z

I'm currently working my way through these patches, but one nitpick that doesn't lend itself well to the GitHub review mechanism: please make sure the patches have some some of subject prefix (similar to how we do things in kernel land). For example, I would rather "add API support for the SPEC_ALLOW flag" be "api: add support for the SPEC_ALLOW flag". You could probably add an "api:" prefix to the main user notification patch too.

include/seccomp.h.in

pcmoore · 2019-04-12T19:05:27Z

include/seccomp.h.in

+ * SECCOMP_RET_USER_NOTIF was added in kernel 5.0. It will not be defined on
+ * older kernels. This version also added the structures below, so let's define
+ * those if the header doesn't have this definiton.
+ */


We generally don't include commentary like this in the libseccomp header file :)

I think it's both appropriate and a good idea to add a one-liner about user notification first appearing in Linux v5.0 but I would leave it at that.

I probably wasn't clear enough about this previously, but this is what I was thinking for a comment:

/* User notification bits was added in Linux v5.0 */

pcmoore · 2019-04-12T19:08:01Z

I would also include a brief explanation of the user notification feature in the commit description so we have some info in the git log about how this is expected to work.

src/api.c

pcmoore · 2019-04-12T22:33:43Z

src/api.c

+	*req = malloc(sizes.seccomp_notif);
+	if (!*req)
+		return -ENOMEM;
+	memset(*req, 0, sizes.seccomp_notif);


We define a malloc wrapper named zmalloc in src/helper.c which does the allocation/zero'ing for you; please use that instead.

pcmoore · 2019-04-12T22:33:51Z

src/api.c

+		free(req);
+		return -ENOMEM;
+	}
+	memset(*resp, 0, sizes.seccomp_notif_resp);


Same as above.

pcmoore · 2019-04-12T22:34:57Z

src/api.c

@@ -537,3 +551,64 @@ API int seccomp_export_bpf(const scmp_filter_ctx ctx, int fd)

 	return 0;
 }
+
+/* NOTE - function header comment in include/seccomp.h */
+int seccomp_alloc_notifications(struct seccomp_notif **req,


This should probably be prefaced with "API" to ensure the visibility is set correctly.

pcmoore · 2019-04-12T22:37:45Z

src/api.c

+	static struct seccomp_notif_sizes sizes = {};
+
+	if (sizes.seccomp_notif == 0) {
+		int ret = syscall(__NR_seccomp, SECCOMP_GET_NOTIF_SIZES, 0, &sizes);


This should probably be moved to a function in src/system.c and it should make use of _support_seccomp_syscall and sys_chk_seccomp_syscall() to see if it safe/possible to use the seccomp() syscall.

pcmoore · 2019-04-12T22:38:11Z

src/api.c

+		return -1;
+	}
+
+	*req = malloc(sizes.seccomp_notif);


See previous malloc() comments.

pcmoore · 2019-04-12T22:38:52Z

src/api.c

+		return -ENOMEM;
+	memset(*req, 0, sizes.seccomp_notif);
+
+	*resp = malloc(sizes.seccomp_notif_resp);


Another one. I'm going to stop pointing these out and just assume you'll change them all :)

pcmoore · 2019-04-12T22:40:14Z

src/api.c

+int seccomp_alloc_notifications(struct seccomp_notif **req,
+				struct seccomp_notif_resp **resp)
+{
+	static struct seccomp_notif_sizes sizes = {};


This information should probably live in src/system.c. See the other comments in this function.

pcmoore · 2019-04-19T01:33:12Z

src/python/libseccomp.pxd

@@ -59,6 +59,7 @@ cdef extern from "seccomp.h":
        SCMP_FLTATR_API_TSKIP
        SCMP_FLTATR_CTL_LOG
        SCMP_FLTATR_SPEC_ALLOW
+        SCMP_FITATR_NEW_LISTENER


"_FLTATR_" not "_FITATR_"

pcmoore · 2019-04-19T01:39:34Z

tests/13-basic-attrs.c

@@ -123,6 +123,20 @@ int main(int argc, char *argv[])
 		goto out;
 	}

+	expected = 1;
+	rc = seccomp_attr_set(ctx, SCMP_FLTATR_NEW_LISTENER, expected);


See my previous comments about this test as well as the missing Python equivalent.

pcmoore · 2019-04-19T01:40:55Z

tests/50-live-user_notification.tests

+test type: live
+
+# Testname	API	Result
+50-live-user_notification	5	USER_NOTIF


Since you need to respin this patch anyway, you might as well rebase to the current master branch and resolve the test number conflict (we now have a test 50).

You also should have a Python version of this test.

pcmoore · 2019-04-19T01:43:43Z

include/seccomp.h.in

+ * loaded. This is only valid after seccomp_load() with
+ * SCMP_FLTATR_NEW_LISTENER set.
+ */
+int seccomp_notif_get_fd(const scmp_filter_ctx ctx);


Can we just call this seccomp_notif_fd(...)? Or do you see us needing to get/put the FD for some reason in the future?

pcmoore · 2019-04-19T01:46:40Z

There were a few carryover items that needed to be addressed, and a few new things, but overall I think you're pretty close. I think the biggest thing is you need to provide proper Python tests (and run them), as there was one typo in the Python bindings that would have been caught. There is also the issue of documenting the API and data types in a manpage. If they are documented elsewhere, that's fine, but we need to reference it in the libseccomp specific manpages.

pcmoore · 2019-04-19T01:47:38Z

One more thing, since the first patch was pretty straight forward, and could stand alone, I went ahead and merged that just now via e7c97e3. Feel free to drop that from future respins.

pcmoore · 2019-04-19T01:48:51Z

include/seccomp.h.in

+ */
+int seccomp_notif_alloc(struct seccomp_notif **req,
+			            struct seccomp_notif_resp **resp);
+


All of these new APIs need to be documented in one or more manpages.

include/seccomp.h.in

pcmoore · 2019-04-19T01:56:44Z

include/seccomp.h.in

@@ -66,6 +67,7 @@ enum scmp_filter_attr {
 	SCMP_FLTATR_API_TSKIP = 5,	/**< allow rules with a -1 syscall */
 	SCMP_FLTATR_CTL_LOG = 6,	/**< log not-allowed actions */
 	SCMP_FLTATR_SPEC_ALLOW = 7, /**< disable SSB mitigation */
+	SCMP_FLTATR_NEW_LISTENER = 8,   /**< returns a listener fd */


See my previous comment regarding attribute names (my apologies once again). Similar to the last one, this should probably be SCMP_FLTATR_CTL_<blah>.

tych0 · 2019-04-23T15:05:38Z

On Thu, Apr 18, 2019 at 06:35:59PM -0700, Paul Moore wrote: pcmoore commented on this pull request. > @@ -336,6 +342,48 @@ struct scmp_arg_cmp { */ #define SCMP_ACT_ALLOW 0x7fff0000U +/* + * User notification bits. It's a little unfortunate that we don't export + * system.h, so we end up having to define all these structures again. + * SECCOMP_RET_USER_NOTIF was added in kernel 5.0. It will not be defined on + * older kernels. This version also added the structures below, so let's define + * those if the header doesn't have this definiton. + */ I probably wasn't clear enough about this previously, but this is what I was thinking for a comment: ``` /* User notification bits was added in Linux v5.0 */ ```

No, you were, it's just that this is copypasta and I only deleted it from one place.

tych0 · 2019-04-23T16:07:54Z

On Thu, Apr 18, 2019 at 04:02:31PM -0700, Paul Moore wrote: pcmoore commented on this pull request. > @@ -108,6 +108,21 @@ int main(int argc, char *argv[]) goto out; } + + expected = 1; + rc = seccomp_attr_set(ctx, SCMP_FLTATR_SPEC_ALLOW, expected); + if (rc == -EOPNOTSUPP) + expected = 0; Since you are forcing the API level to "4" above, this should always be supported regardless of the underlying host kernel. You should be able to just copy/paste the checks used for the other attributes (the "expected" stuff shouldn't be needed).

That doesn't work. seccomp_attr_set() -> db_col_attr_set() -> sys_chk_seccomp_flag() -> checks the actual flag. We could switch the implementation of db_col_attr_set() if you want, but then it wouldn't match the other ones. Tycho

pcmoore · 2019-04-23T20:29:33Z

That doesn't work. seccomp_attr_set() -> db_col_attr_set() -> sys_chk_seccomp_flag() -> checks the actual flag. We could switch the implementation of db_col_attr_set() if you want, but then it wouldn't match the other ones.

If you look at sys_chk_seccomp_flag(...) you'll notice that it checks the _support_* variable associated with the flag before calling into _sys_chk_seccomp_flag_kernel(...). It does this to solve exactly the problem you are describing.

tych0 · 2019-04-23T20:35:04Z

On Tue, Apr 23, 2019 at 01:29:35PM -0700, Paul Moore wrote: > That doesn't work. seccomp_attr_set() -> db_col_attr_set() -> sys_chk_seccomp_flag() -> checks the actual flag. We could switch the implementation of db_col_attr_set() if you want, but then it wouldn't match the other ones. If you look at `sys_chk_seccomp_flag(...)` you'll notice that it checks the `_support_*` variable associated with the flag before calling into `_sys_chk_seccomp_flag_kernel(...)`. It does this to solve exactly the problem you are describing.

Hmm, I must have misread this morning. In any case, it still fails for me without this, so something else is being picky.

pcmoore · 2019-04-23T20:44:14Z

Hmm, I must have misread this morning. In any case, it still fails for me without this, so something else is being picky.

If you end up getting stuck, let me know.

pcmoore · 2019-04-25T17:19:58Z

@tych0 I see there are updates to the PR and wanted to check to see if you are ready for us to review them again, or are they still a work in progress? Either approach is fine, I just want to make sure we aren't holding you up.

tych0 · 2019-04-25T19:27:37Z

On Thu, Apr 25, 2019 at 05:20:00PM +0000, Paul Moore wrote: @tych0 I see there are updates to the PR and wanted to check to see if you are ready for us to review them again, or are they still a work in progress? Either approach is fine, I just want to make sure we aren't holding you up.

I think everything should be addressed except for the man pages, which I only have about half done because I can only write so much troff at a time :). Feel free to take a look if you want, just note that the man pages are absent.

drakenclimber · 2019-04-25T19:39:01Z

I'm out of the office Friday and Monday, but I can look them over on Tuesday.

tych0 · 2019-04-25T23:55:27Z

On Thu, Apr 25, 2019 at 12:39:03PM -0700, Tom Hromatka wrote: I'm out of the office Friday and Monday, but I can look them over on Tuesday.

Cool, thanks. I just pushed the man pages too, so I think it's probably pretty close at this point.

pcmoore · 2019-04-26T01:54:48Z

I can only write so much troff at a time :)

Me too. I love projects that have manpages, but I hate writing them :)

FWIW, we do have an issue open (#83) to look into alternate source formats for our manpages to make life easier.

pcmoore · 2019-04-26T02:03:47Z

I think something may be wrong @tych0 - it doesn't look like I'm seeing all of your updates. As an example, I know you were working on adding the Python tests, but I'm not seeing any additions/changes to the Python tests in this PR.

???

pcmoore · 2019-04-30T00:30:08Z

The SPEC_ALLOW patch looked good to me so I went ahead and merged it via 11667b4 with a follow patch in a15a87c to rename SCMP_FLTATR_SPEC_ALLOW to SCMP_FLTATR_CTL_SSB as discussed in the review.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>

Kernel 5.0 includes the new user notification return code. Here's all the infrastructure to handle that. The idea behind the user notification return code is that the filter stops the syscall, and forwards it to a "listener fd" that is created when installing a filter. Then then some userspace task can listen and process events accordingly by taking some (or no) action in userspace, and then returning a value from the command. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

Here's some basic documentation for the seccomp_notif_* family of functions. I put them all on the same page because that seems to make the most sense; they'll all be used together. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

pcmoore · 2019-05-02T00:06:42Z

I happened to be in the same room as @tych0 this week, so I offered to pick up this patch to add the missing bits so he could focus on other things. I'm going to close this PR as a result, in favor of a new PR with the updated patches in just a moment.

pcmoore changed the title ~~User notification~~ RFE: user notification Mar 19, 2019

pcmoore added enhancement priority/low pending/review labels Mar 19, 2019

pcmoore added the pending/revision label Mar 19, 2019

tych0 force-pushed the user-notification branch from 902c493 to 7f36c76 Compare March 20, 2019 17:33

tych0 force-pushed the user-notification branch from 7f36c76 to a0c6ae5 Compare March 20, 2019 20:41

pcmoore reviewed Mar 20, 2019

View reviewed changes

pcmoore removed the pending/revision label Apr 12, 2019