On Thu, 23.01.14 01:34, Ronny Chevalier (chevalier.ro...@gmail.com) wrote: > --- > Hi, > > This patch ports the syscall filter to libseccomp. It can be disable with > --disable-seccomp and is enabled by default if libseccomp is present. > > Maybe I should add a warning when parsing SyscallFilter in a .service > if seccomp has been disabled ?
There's "config_warn_commpat()" for cases like this, but it is currently only used for the optional sysv-hookup. I am pretty sure we should reuse this scheme here... > > Now the SyscallFilter property is a duplicate of the string in the .service > file instead of a uint array. > > const sd_bus_vtable bus_exec_vtable[] = { > @@ -420,7 +416,7 @@ const sd_bus_vtable bus_exec_vtable[] = { > SD_BUS_PROPERTY("UtmpIdentifier", "s", NULL, offsetof(ExecContext, > utmp_id), SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_PROPERTY("IgnoreSIGPIPE", "b", bus_property_get_bool, > offsetof(ExecContext, ignore_sigpipe), SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_PROPERTY("NoNewPrivileges", "b", bus_property_get_bool, > offsetof(ExecContext, no_new_privileges), SD_BUS_VTABLE_PROPERTY_CONST), > - SD_BUS_PROPERTY("SystemCallFilter", "au", > property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST), > + SD_BUS_PROPERTY("SystemCallFilter", "s", > property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_VTABLE_END Soooo, strictly speaking this is API breakage. However, given that the old API was soo naive and pretty much unusable and arch specific, and I hence doubt that people wrote code against this specific property I am tempted to just let go this through... > @@ -1936,49 +1928,61 @@ int config_parse_syscall_filter(const char *unit, > const char *rvalue, > void *data, > void *userdata) { > +#ifdef HAVE_SECCOMP > +#define SECCOMP_RULE_ADD(ctx, action, id, name) \ > + do {\ > + r = seccomp_rule_add(ctx, action, id, 0);\ > + if (r < 0)\ > + log_syntax(unit, LOG_ERR, filename, line, -r,\ > + "Failed to add syscall filter, ignoring: > %s", name);\ > + } while(0) I'd prefer if this could become a normal function rather than a macro... There's no benefit of this being a macro... > > ExecContext *c = data; > Unit *u = userdata; > bool invert = false; > + uint32_t action = SCMP_ACT_ALLOW; > char *w; > size_t l; > char *state; > + int r; > > assert(filename); > assert(lvalue); > assert(rvalue); > assert(u); > > + No spurious double whitespace lines, please.... > if (isempty(rvalue)) { > /* Empty assignment resets the list */ > - free(c->syscall_filter); > + seccomp_release(c->syscall_filter); > c->syscall_filter = NULL; > + free(c->syscall_filter_string = NULL); ^^^^^^^ This certainly looks wrong? > + c->syscall_filter_string = NULL; > return 0; > } > + c->syscall_filter_string = strdup(rvalue); I really don't like this I must say. Handing the unnormalized original configuration string back to the clients via the bus API sounds wrong. Doesn't libseccomp provide a way to enumerate the contents of the defined filter again? I'd really prefer if we could find a way that specifiying a filter of "read write" and of "write read" would actually result in the same string exposed via the bus. Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel