Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4912 bytes --]

On 02/07/2017 06:03 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch creates a read-only sysctl containing an ordered list of
>> seccomp actions that the kernel supports. The ordering, from left to
>> right, is the lowest action value (kill) to the highest action value
>> (allow). Currently, a read of the sysctl file would return "kill trap
>> errno trace allow". The contents of this sysctl file can be useful for
>> userspace code as well as the system administrator.
>>
>> The path to the sysctl is:
>>
>>   /proc/sys/kernel/seccomp/actions_avail
>>
>> libseccomp and other userspace code can easily determine which actions
>> the current kernel supports. The set of actions supported by the current
>> kernel may be different than the set of action macros found in kernel
>> headers that were installed where the userspace code was built.
> 
> This is certainly good: having a discoverable way to detect filter
> capabilities. I do wonder if it'd still be easier to just expose the
> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
> all part of uapi, so we can't escape their values...

I was very torn on whether to use a numeric or string representation
here. The reason I decided on string representation is because I think
these sysctls are mostly aimed for admins and numeric representations
wouldn't be easy to use. I considered added a utility to libseccomp but,
since the kernel code to do a string representation was so simple, I
went with doing it in the kernel.

Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
the uapi.

> 
> 
> 
>>
>> In addition, this sysctl will allow system administrators to know which
>> actions are supported by the kernel and make it easier to configure
>> exactly what seccomp logs through the audit subsystem. Support for this
>> level of logging configuration will come in a future patch.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  kernel/seccomp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index f7ce79a..919ad9f 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -16,10 +16,12 @@
>>  #include <linux/atomic.h>
>>  #include <linux/audit.h>
>>  #include <linux/compat.h>
>> +#include <linux/kmemleak.h>
>>  #include <linux/sched.h>
>>  #include <linux/seccomp.h>
>>  #include <linux/slab.h>
>>  #include <linux/syscalls.h>
>> +#include <linux/sysctl.h>
>>
>>  #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
>>  #include <asm/syscall.h>
>> @@ -905,3 +907,51 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>         return ret;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_SYSCTL
>> +
>> +#define SECCOMP_RET_KILL_NAME          "kill"
>> +#define SECCOMP_RET_TRAP_NAME          "trap"
>> +#define SECCOMP_RET_ERRNO_NAME         "errno"
>> +#define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_ALLOW_NAME         "allow"
>> +
>> +static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>> +                                     SECCOMP_RET_TRAP_NAME     " "
>> +                                     SECCOMP_RET_ERRNO_NAME    " "
>> +                                     SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_ALLOW_NAME;
>> +
>> +static struct ctl_path seccomp_sysctl_path[] = {
>> +       { .procname = "kernel", },
>> +       { .procname = "seccomp", },
>> +       { }
>> +};
>> +
>> +static struct ctl_table seccomp_sysctl_table[] = {
>> +       {
>> +               .procname       = "actions_avail",
>> +               .data           = &seccomp_actions_avail,
>> +               .maxlen         = sizeof(seccomp_actions_avail),
>> +               .mode           = 0444,
>> +               .proc_handler   = proc_dostring,
>> +       },
>> +       { }
>> +};
>> +
>> +static int __init seccomp_sysctl_init(void)
>> +{
>> +       struct ctl_table_header *hdr;
>> +
>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>> +       kmemleak_not_leak(hdr);
> 
> Will kmemleak complain about this if hdr is saved to a global (or not
> saved at all)? Also, something should be reported in the failure
> case...

I have to admit to blindly following the example set by sysctl_init() in
kernel/sysctl.c. I can test what kmemleak will/won't complain about and
report back (tomorrow at the earliest).

Tyler

> 
>> +       return 0;
>> +}
>> +
>> +#else /* CONFIG_SYSCTL */
>> +
>> +static __init int seccomp_sysctl_init(void) { return 0; }
>> +
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +device_initcall(seccomp_sysctl_init)
>> --
>> 2.7.4
>>
> 
> -Kees
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]