> = Specific Comments =
>
> * init/job_process.c: job_process_spawn():
> - No check on return code of install_seccomp_filter(): this gives the
> behaviour that if the job specifies 'seccomp-filter' but the seccomp
> filter
> failed to install, the job is still allowed to run. I'm not sure
> this is desirable?
I'm not sure what would be "desirable" here? If it fails simply
because seccomp is not enabled (user runs a customized kernel) I'm
inclined to pretend nothing happened... (install_seccomp_filter will
produce a waring).
> - I can understand why the seccomp filter it not applied until just
> before the child exec, but why is the parsing of the rules deferred to
> this point (such that each job that uses the seccomp-filter stanza has
> its rules parsed at startup)? Can't we parse the rules fully in
> parse_job() and then simply apply the already-parsed-and-verified rules
> prior to exec()?
We could, but at the moment I'm using (almost) the same code for
Upstart as for a pet project of mine. I could split it up later, but I
would like to focus on other remarks/issues first...
> * init/seccomp_filter.c:
> - macros: Need comment headers.
> - ACT_TYPE:
> - Should use CamelCase for consistency with existing codebase.
> - Need description of elements.
> - actions: I'd prefer a macro to calculate 'len' to avoid
> nasty runtime surprises if someone forgets to change len for a new
> entry (or just drop the len element and use strcmp()).
> - ACT_CMP:
> - Needs a "Returns" comment.
> - Should be lower-case as it's a function.
> - sock_filter:
> - Can you add some details around those magic numbers?
> - Maybe 'filter' would be a clearer name than just 'f'.
I'll fix these.
> - __NR_arch_prctl: this symbol is not defined on 32-bit systems.
That's odd, I have tested that quite a while ago... I will fix this
and test on x86 to make sure. (I cannot test on AArch64, but I will
try to test on ARM (Nexus 7) as well.)
> - install_seccomp_filter:
> - @nnp is always set so can this param be dropped?
Because I am planning to add a separate NoNewPriviledges stanza later.
(I'll probaly move the two prctl's from install_seccomp_filter to
job_process.c)
> - As above: I think the parsing should be performed in parse_job.c,
> not here if possible. Note that by so doing, if the user specifies
> incorrect seccomp-filter syntax, the job will fail to parse and
> register. This is a good thing if it is syntactically incorrect.
Agreed and will split up parsing and installing later...
> = General comments =
>
> * Some of the formatting is not consistent with the standard. See
> 'HACKING' for editor+indent rules.
AFAIK seccomp_filter.[ch] is consistent, but for the other files I've
tried to blend in... ;-)
parse_job.c itself is far from consistent BTW
> * Tests: All new features need to provide tests, so you'll need to
> create a init/tests/test_seccomp_filter.c and also consider adding some
> system
> tests to the
> increasingly-erroneously-named-as-it-does-lots-more-than-the-name-suggests
> test_initctl.c :-)
I've already been working on test_parse_job.c and test_job_process.c,
these test-cases should cover almost everything (assuming touch, true
and echo are present).
I'm thinking of adding a small test application as well that calls one
syscall and then exits with errno, so I can test that part too. I
think that covers everything I can think of without having to run as
root...
> * The new files need the correct Copyright header.
> * nih_warn() should make use of gettext's _() for translations.
> * init/man/init.5: Need to document 'seccomp-filter' stanza.
Will look into this.
> * What happens when a job specifies 'seccomp-filter' is run in user
> mode ('init --user')?
It will run fine, however, since NoNewPriviledges is set (mandatory as
non-root) you might not be able to run setuid'd programs properly
(e.g. sudo, ping)
> * Stateful re-exec: Any change to the internal data structures
> requires updates to the serialisation and deserialisation code.
> Currently, that looks to be quite easy since you've only added a
> string to JobClass. So, you'd need to update job_class_serialise() and
> add a call to state_set_json_string_var_from_obj() for seccomp_filter,
> and upate job_class_deserialise() to add a call to
> state_get_json_string_var_strict(). However, if you can change the
> code to parse the seccomp filters at parse_job() time, I suspect you
> might end up changing the parameters to install_seccomp_filter() so
> the advice above might not be correct. The stateful re-exec code
> is pretty easy to update as all the existing data types used are
> catered for with macros (see init/*.c:*_serialise(),
> init/*.c:*_deserialise()
> and init/state.[ch]). Note though that you'll also need to add a new
> upgrade test as the JSON used to represent internal state on stateful
> re-exec will change with the advent of JobClass->seccomp_filter (see
> test_upgrade() in init/tests/test_state.c).
Will look into this as well...
Thanks for all the remarks. I was going to send new patches including
test-cases and fixing some errors, but I'll fix most of these items
first. I've also added support for SECCOMP_RET_INFO if defined in
<linux/seccomp.h> in that case "info" is recognized as a policy.
SECCOMP_RET_INFO will be added in future kernels and will print the
syscall number in the kernel log (and allow it), which is very handy
to find what syscalls are being used by a program. I intend to add a
modifier to make it the default policy (for unlisted syscalls).
Could you maybe explain what happens if a job is respawned or
restarted, will it parse the job again or will it re-use the existing
job class data?
Kind regards,
David Gaarenstroom
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
