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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jJZYRi194ZGcPbNYYNTaTMaE+nd2+jYaj0nfSoou0shjQ@mail.gmail.com>
Date:   Tue, 7 Feb 2017 16:03:21 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Tyler Hicks <tyhicks@...onical.com>
Cc:     Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...hat.com>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, linux-audit@...hat.com,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@...onical.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...



>
> 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@...onical.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...

> +       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

-- 
Kees Cook
Pixel Security