* Re: [RFC PATCH v1 0/5] Add support for O_MAYEXEC
From: Mickaël Salaün @ 2018-12-12 17:01 UTC (permalink / raw)
To: Jordan Glover, Mickaël Salaün
Cc: linux-kernel@vger.kernel.org, Al Viro, James Morris,
Jonathan Corbet, Kees Cook, Matthew Garrett, Michael Kerrisk,
Mimi Zohar, Philippe Trébuchet, Shuah Khan,
Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org
In-Reply-To: <ML-sxPRwNNExkZHkXghYN7e3VrhuZXYLjFv-aniX8qbjcde2GfHMBaI4B3A7IoUEfycQcWCPuxATkYHOz--FqMjH1KRwxMxBCb6Q-1IsKTM=@protonmail.ch>
Le 12/12/2018 à 17:29, Jordan Glover a écrit :
> On Wednesday, December 12, 2018 9:17 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
>> Hi,
>>
>> The goal of this patch series is to control script interpretation. A
>> new O_MAYEXEC flag used by sys_open() is added to enable userland script
>> interpreter to delegate to the kernel (and thus the system security
>> policy) the permission to interpret scripts or other files containing
>> what can be seen as commands.
>>
>> The security policy is the responsibility of an LSM. A basic
>> system-wide policy is implemented with Yama and configurable through a
>> sysctl.
>>
>> The initial idea come from CLIP OS and the original implementation has
>> been used for more than 10 years:
>> https://github.com/clipos-archive/clipos4_doc
>>
>> An introduction to O_MAYEXEC was given at the Linux Security Summit
>> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
>> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
>> The "write xor execute" principle was explained at Kernel Recipes 2018 -
>> CLIP OS: a defense-in-depth OS:
>> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
>>
>> This patch series can be applied on top of v4.20-rc6. This can be
>> tested with CONFIG_SECURITY_YAMA. I would really appreciate
>> constructive comments on this RFC.
>>
>> Regards,
>>
>
> Are various interpreters upstreams interested in adding support
> for O_MAYEXEC if it land in kernel? Did you contacted them about this?
I think the first step is to be OK on the kernel side. We will then be
able to help upstream interpreters implement this feature. It should be
OK because the behavior doesn't change by default, i.e. if the sysadmin
doesn't configure (and test) the whole system. Some examples of modified
interpreters can be found at
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
.
Mickaël
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Rich Felker @ 2018-12-12 16:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: tg, Linus Torvalds, X86 ML, LKML, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, Mike Frysinger,
H. J. Lu, x32, Arnd Bergmann, Will Deacon, Catalin Marinas
In-Reply-To: <CALCETrUYn=S=hmJ0tKdm2LoSgkWchY2_65sH7hJZp7wfS30giw@mail.gmail.com>
On Wed, Dec 12, 2018 at 08:39:53AM -0800, Andy Lutomirski wrote:
> > On Dec 11, 2018, at 6:33 PM, Thorsten Glaser <tg@mirbsd.de> wrote:
> >
> > Andy Lutomirski dixit:
> >
>
> >
> >> IMO the real right solution would be to push the whole problem to
> >> userspace: get an ILP32 system working with almost or entirely LP64
> >
> > Is this a reflex of Linux kernel developers? ;-)
> >
> > I doubt that userspace is the right place for this, remember
> > the recent glibc vs. syscalls debate. It would also need to
> > multiply across various libcs.
> >
> >> How hard would it be to have __attribute__((ilp64)), with an optional
> >> warning if any embedded structs are not ilp64? This plus a wrapper to
> >
> > You mean LP64. Impossible, because LP64 vs. ILP32 is not the only
> > difference between amd64 and x32.
>
> I mean LP64. And I'm not suggesting that ILP32 is the only difference
> between x32 and x86_64, nor am I suggesting that a technique like this
> would implement x32 -- I'm suggesting it would implement something
> better than x32.
>
> The kernel, as a practical matter, supports two ABIs on 64-bit builds:
> LP64 and ILP32. ILP32 is what the kernel calls "compat". ("compat"
> comes with other baggage -- it generally has a 32-bit signal frame,
> syscall arguments are mostly limited to 32 bits, etc.) Allowing a
> user program that runs in 64-bit mode to issue compat syscalls is not
> really a big deal. x86_64 has allowed this forever using int $0x80 --
> it's just slow. Adding a faster mechanism would be straightforward.
> As I understand it, the arm64 ilp32 proposal involves using a genuine
> ILP32 model for user code, so the syscalls will all (except for signal
> handling) go through the compat path.
>
> x32 is not this at all. The kernel ABI part of x32 isn't ILP32. It's
> IP32, 32-bit size_t, and *64-bit* long. The core kernel doesn't
> really support this. The only good things I can think of about it are
> that (a) it proves that somewhat odd ABIs are possible, at least in
> principle, and (b) three users have come out of the woodwork to say
> that they use it.
>
> I'm proposing another alternative. Given that x32 already proves that
> the user bitness model doesn't have to match the kernel model (in x32,
> user "long" is 32-bit but the kernel ABI "long" is 64-bit), I'm
> proposing extending this to just make the kernel ABI be LP64. So
> __kernel_size_t would be 64-bit and pointers in kernel data structures
> would be 64-bit. In other words, most or all of the kernel ABI would
> just match x86_64.
>
> As far as I can tell, the only thing that really needs unusual
> toolchain features here is that C doesn't have an extra-wide pointer
> type. The kernel headers would need a way to say "this pointer is
> still logically a pointer, and user code may assume that it's 32 bits,
> but it has 8-byte alignment."
None of this works on the userspace/C side, nor should any attempt be
made to make it work. Types fundamentally cannot have alignments
larger than their size. If you want to make the alignment of some
pointers 8, you have to make their size 8, and then you just have LP64
again if you did it for all pointers.
If on the other hand you tried to make just some pointers "wide
pointers", you'd also be completely breaking the specified API
contracts of standard interfaces. For example in struct iovec's
iov_base, &foo->iov_base is no longer a valid pointer to an object of
type void* that you can pass to interfaces expecting void**. Sloppy
misunderstandings like what you're making now are exactly why x32 is
already broken and buggy (&foo->tv_nsec already has wrong type for
struct timespec foo).
If you wanted to do this and have it work, libc would have to
completely wrap and convert all the types from userspace structures
with the right types matching the specified API contracts to
kernel-interface structures that can be passed to the LP64 syscalls.
This could be done, but it's a big task, and I don't think anyone
wants to do it, especially if it would be yet another new thing that
doesn't exactly match the already-existing x32. The cost-benefit ratio
is just too high.
> There's an added benefit to my proposal over x32. With my proposal,
> an ILP32 program could plausibly call into a true 64-bit C library
> using a straightforward FFI translation.
Unless it's a thin, "pure" library that doesn't need anything from
libc, or needs sufficiently little that it could be satisfied by some
shims, this would necessarily require having two libcs in the same
process, which is not going to work.
Rich
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Andy Lutomirski @ 2018-12-12 16:39 UTC (permalink / raw)
To: tg
Cc: Andrew Lutomirski, Linus Torvalds, X86 ML, LKML, Linux API,
H. Peter Anvin, Peter Zijlstra, Borislav Petkov, Florian Weimer,
Mike Frysinger, H. J. Lu, Rich Felker, x32, Arnd Bergmann,
Will Deacon, Catalin Marinas
In-Reply-To: <Pine.BSM.4.64L.1812120231410.21176@herc.mirbsd.org>
> On Dec 11, 2018, at 6:33 PM, Thorsten Glaser <tg@mirbsd.de> wrote:
>
> Andy Lutomirski dixit:
>
>
>> IMO the real right solution would be to push the whole problem to
>> userspace: get an ILP32 system working with almost or entirely LP64
>
> Is this a reflex of Linux kernel developers? ;-)
>
> I doubt that userspace is the right place for this, remember
> the recent glibc vs. syscalls debate. It would also need to
> multiply across various libcs.
>
>> How hard would it be to have __attribute__((ilp64)), with an optional
>> warning if any embedded structs are not ilp64? This plus a wrapper to
>
> You mean LP64. Impossible, because LP64 vs. ILP32 is not the only
> difference between amd64 and x32.
I mean LP64. And I'm not suggesting that ILP32 is the only difference
between x32 and x86_64, nor am I suggesting that a technique like this
would implement x32 -- I'm suggesting it would implement something
better than x32.
The kernel, as a practical matter, supports two ABIs on 64-bit builds:
LP64 and ILP32. ILP32 is what the kernel calls "compat". ("compat"
comes with other baggage -- it generally has a 32-bit signal frame,
syscall arguments are mostly limited to 32 bits, etc.) Allowing a
user program that runs in 64-bit mode to issue compat syscalls is not
really a big deal. x86_64 has allowed this forever using int $0x80 --
it's just slow. Adding a faster mechanism would be straightforward.
As I understand it, the arm64 ilp32 proposal involves using a genuine
ILP32 model for user code, so the syscalls will all (except for signal
handling) go through the compat path.
x32 is not this at all. The kernel ABI part of x32 isn't ILP32. It's
IP32, 32-bit size_t, and *64-bit* long. The core kernel doesn't
really support this. The only good things I can think of about it are
that (a) it proves that somewhat odd ABIs are possible, at least in
principle, and (b) three users have come out of the woodwork to say
that they use it.
I'm proposing another alternative. Given that x32 already proves that
the user bitness model doesn't have to match the kernel model (in x32,
user "long" is 32-bit but the kernel ABI "long" is 64-bit), I'm
proposing extending this to just make the kernel ABI be LP64. So
__kernel_size_t would be 64-bit and pointers in kernel data structures
would be 64-bit. In other words, most or all of the kernel ABI would
just match x86_64.
As far as I can tell, the only thing that really needs unusual
toolchain features here is that C doesn't have an extra-wide pointer
type. The kernel headers would need a way to say "this pointer is
still logically a pointer, and user code may assume that it's 32 bits,
but it has 8-byte alignment."
So I guess I'm proposing that people who really like x32 and are
willing to invest effort it in instead invest some effort into getting
something similar to work using the normal x86_64 syscalls. And I'm
hinting to the ARM folks on the thread that they consider this
approach for arm64.
There's an added benefit to my proposal over x32. With my proposal,
an ILP32 program could plausibly call into a true 64-bit C library
using a straightforward FFI translation.
>
> bye,
> //mirabilos
> --
> I believe no one can invent an algorithm. One just happens to hit upon it
> when God enlightens him. Or only God invents algorithms, we merely copy them.
> If you don't believe in God, just consider God as Nature if you won't deny
> existence. -- Coywolf Qi Hunt
^ permalink raw reply
* Re: [RFC PATCH v1 0/5] Add support for O_MAYEXEC
From: Jordan Glover @ 2018-12-12 16:29 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel@vger.kernel.org, Al Viro, James Morris,
Jonathan Corbet, Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
On Wednesday, December 12, 2018 9:17 AM, Mickaël Salaün <mic@digikod.net> wrote:
> Hi,
>
> The goal of this patch series is to control script interpretation. A
> new O_MAYEXEC flag used by sys_open() is added to enable userland script
> interpreter to delegate to the kernel (and thus the system security
> policy) the permission to interpret scripts or other files containing
> what can be seen as commands.
>
> The security policy is the responsibility of an LSM. A basic
> system-wide policy is implemented with Yama and configurable through a
> sysctl.
>
> The initial idea come from CLIP OS and the original implementation has
> been used for more than 10 years:
> https://github.com/clipos-archive/clipos4_doc
>
> An introduction to O_MAYEXEC was given at the Linux Security Summit
> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
> The "write xor execute" principle was explained at Kernel Recipes 2018 -
> CLIP OS: a defense-in-depth OS:
> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
>
> This patch series can be applied on top of v4.20-rc6. This can be
> tested with CONFIG_SECURITY_YAMA. I would really appreciate
> constructive comments on this RFC.
>
> Regards,
>
Are various interpreters upstreams interested in adding support
for O_MAYEXEC if it land in kernel? Did you contacted them about this?
Jordan
^ permalink raw reply
* Re: [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Jan Kara @ 2018-12-12 14:43 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Al Viro, James Morris, Jonathan Corbet, Kees Cook,
Matthew Garrett, Michael Kerrisk, Mickaël Salaün,
Mimi Zohar, Philippe Trébuchet, Shuah Khan,
Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel,
sgrubb
In-Reply-To: <20181212081712.32347-2-mic@digikod.net>
On Wed 12-12-18 09:17:08, Mickaël Salaün wrote:
> When the O_MAYEXEC flag is passed, sys_open() may be subject to
> additional restrictions depending on a security policy implemented by an
> LSM through the inode_permission hook.
>
> The underlying idea is to be able to restrict scripts interpretation
> according to a policy defined by the system administrator. For this to
> be possible, script interpreters must use the O_MAYEXEC flag
> appropriately. To be fully effective, these interpreters also need to
> handle the other ways to execute code (for which the kernel can't help):
> command line parameters (e.g., option -e for Perl), module loading
> (e.g., option -m for Python), stdin, file sourcing, environment
> variables, configuration files... According to the threat model, it may
> be acceptable to allow some script interpreters (e.g. Bash) to interpret
> commands from stdin, may it be a TTY or a pipe, because it may not be
> enough to (directly) perform syscalls.
>
> A simple security policy implementation is available in a following
> patch for Yama.
>
> This is an updated subset of the patch initially written by Vincent
> Strubel for CLIP OS:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than 10 years with customized script
> interpreters. Some examples can be found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
...
> diff --git a/fs/open.c b/fs/open.c
> index 0285ce7dbd51..75479b79a58f 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
> if (flags & O_APPEND)
> acc_mode |= MAY_APPEND;
>
> + /* Check execution permissions on open. */
> + if (flags & O_MAYEXEC)
> + acc_mode |= MAY_OPENEXEC;
> +
> op->acc_mode = acc_mode;
>
> op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
I don't feel experienced enough in security to tell whether we want this
functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
on the resulting struct file? That way also security_file_open() can be
used to arbitrate such executable opens and in particular
fanotify permission event FAN_OPEN_EXEC will get properly generated which I
guess is desirable (support for it is sitting in my tree waiting for the
merge window) - adding some audit people involved in FAN_OPEN_EXEC to
CC. Just an idea...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH v1 3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
From: Mickaël Salaün @ 2018-12-12 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Shuah Khan, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-4-mic@digikod.net>
Le 12/12/2018 à 09:17, Mickaël Salaün a écrit :
> Enable to either propagate the mount options from the underlying VFS
> mount to prevent execution, or to propagate the file execute permission.
> This may allow a script interpreter to check execution permissions
> before reading commands from a file.
>
> The main goal is to be able to protect the kernel by restricting
> arbitrary syscalls that an attacker could perform with a crafted binary
> or certain script languages. It also improves multilevel isolation
> by reducing the ability of an attacker to use side channels with
> specific code. These restrictions can natively be enforced for ELF
> binaries (with the noexec mount option) but require this kernel
> extension to properly handle scripts (e.g., Python, Perl).
>
> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
> behavior. A following patch adds documentation.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> ---
> security/yama/Kconfig | 3 +-
> security/yama/yama_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/security/yama/Kconfig b/security/yama/Kconfig
> index 96b27405558a..9457619fabd5 100644
> --- a/security/yama/Kconfig
> +++ b/security/yama/Kconfig
> @@ -5,7 +5,8 @@ config SECURITY_YAMA
> help
> This selects Yama, which extends DAC support with additional
> system-wide security settings beyond regular Linux discretionary
> - access controls. Currently available is ptrace scope restriction.
> + access controls. Currently available are ptrace scope restriction and
> + enforcement of the O_MAYEXEC open flag.
> Like capabilities, this security module stacks with other LSMs.
> Further information can be found in
> Documentation/admin-guide/LSM/Yama.rst.
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index ffda91a4a1aa..120664e94ee5 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -1,10 +1,12 @@
> /*
> * Yama Linux Security Module
> *
> - * Author: Kees Cook <keescook@chromium.org>
> + * Authors: Kees Cook <keescook@chromium.org>
> + * Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
> *
> * Copyright (C) 2010 Canonical, Ltd.
> * Copyright (C) 2011 The Chromium OS Authors.
> + * Copyright (C) 2018 ANSSI
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2, as
> @@ -28,7 +30,14 @@
> #define YAMA_SCOPE_CAPABILITY 2
> #define YAMA_SCOPE_NO_ATTACH 3
>
> +#define YAMA_OMAYEXEC_ENFORCE_NONE 0
> +#define YAMA_OMAYEXEC_ENFORCE_MOUNT (1 << 0)
> +#define YAMA_OMAYEXEC_ENFORCE_FILE (1 << 1)
> +#define _YAMA_OMAYEXEC_LAST YAMA_OMAYEXEC_ENFORCE_FILE
> +#define _YAMA_OMAYEXEC_MASK ((_YAMA_OMAYEXEC_LAST << 1) - 1)
> +
> static int ptrace_scope = YAMA_SCOPE_RELATIONAL;
> +static int open_mayexec_enforce = YAMA_OMAYEXEC_ENFORCE_NONE;
>
> /* describe a ptrace relationship for potential exception */
> struct ptrace_relation {
> @@ -423,7 +432,40 @@ int yama_ptrace_traceme(struct task_struct *parent)
> return rc;
> }
>
> +/**
> + * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
> + * @inode: inode structure to check
> + * @mask: permission mask
> + *
> + * Return 0 if access is permitted, -EACCES otherwise.
> + */
> +int yama_inode_permission(struct inode *inode, int mask)
> +{
> + if (!(mask & MAY_OPENEXEC))
> + return 0;
> + /*
> + * Match regular files and directories to make it easier to
> + * modify script interpreters.
> + */
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return 0;
I forgot to mention that these checks do not handle fifos. This is
relevant in a threat model targeting persistent attacks (and with
additional protections/restrictions).
> +
> + if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
> + !(mask & MAY_EXECMOUNT))
> + return -EACCES;
> +
> + /*
> + * May prefer acl_permission_check() instead of generic_permission(),
> + * to not be bypassable with CAP_DAC_READ_SEARCH.
> + */
> + if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
> + return generic_permission(inode, MAY_EXEC);
> +
> + return 0;
> +}
> +
> static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> + LSM_HOOK_INIT(inode_permission, yama_inode_permission),
> LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
> LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
> LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
> return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
> }
>
> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int error;
> +
> + if (write) {
> + struct ctl_table table_copy;
> + int tmp_mayexec_enforce;
> +
> + if (!capable(CAP_MAC_ADMIN))
> + return -EPERM;
> + tmp_mayexec_enforce = *((int *)table->data);
> + table_copy = *table;
> + /* do not erase open_mayexec_enforce */
> + table_copy.data = &tmp_mayexec_enforce;
> + error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> + if ((tmp_mayexec_enforce | _YAMA_OMAYEXEC_MASK) !=
> + _YAMA_OMAYEXEC_MASK)
> + return -EINVAL;
> + *((int *)table->data) = tmp_mayexec_enforce;
> + } else {
> + error = proc_dointvec(table, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> + }
> + return 0;
> +}
> +
> static int zero;
> static int max_scope = YAMA_SCOPE_NO_ATTACH;
>
> @@ -466,6 +539,13 @@ static struct ctl_table yama_sysctl_table[] = {
> .extra1 = &zero,
> .extra2 = &max_scope,
> },
> + {
> + .procname = "open_mayexec_enforce",
> + .data = &open_mayexec_enforce,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = yama_dointvec_bitmask_macadmin,
> + },
> { }
> };
> static void __init yama_init_sysctl(void)
>
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: kbuild test robot @ 2018-12-12 9:28 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: kbuild-all, Oleg Nesterov, Andy Lutomirski, Elvira Khabirova,
Eugene Syromyatnikov, Kees Cook, Jann Horn, linux-api,
strace-devel, linux-kernel
In-Reply-To: <20181210043126.GX6131@altlinux.org>
[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]
Hi Elvira,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6]
[cannot apply to next-20181211]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 6.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=6.4.0 make.cross ARCH=nds32
All errors (new ones prefixed by >>):
kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer' [-Werror=implicit-function-declaration]
.frame_pointer = frame_pointer(regs)
^~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/frame_pointer +942 kernel/ptrace.c
931
932 static int
933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
934 void __user *datavp)
935 {
936 struct pt_regs *regs = task_pt_regs(child);
937 struct ptrace_syscall_info info = {
938 .op = PTRACE_SYSCALL_INFO_NONE,
939 .arch = syscall_get_arch(child),
940 .instruction_pointer = instruction_pointer(regs),
941 .stack_pointer = user_stack_pointer(regs),
> 942 .frame_pointer = frame_pointer(regs)
943 };
944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
945 unsigned long write_size;
946
947 /*
948 * This does not need lock_task_sighand() to access
949 * child->last_siginfo because ptrace_freeze_traced()
950 * called earlier by ptrace_check_attach() ensures that
951 * the tracee cannot go away and clear its last_siginfo.
952 */
953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
954 case SIGTRAP | 0x80:
955 switch (child->ptrace_message) {
956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
957 actual_size = ptrace_get_syscall_info_entry(child, regs,
958 &info);
959 break;
960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
961 actual_size = ptrace_get_syscall_info_exit(child, regs,
962 &info);
963 break;
964 }
965 break;
966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
968 &info);
969 break;
970 }
971
972 write_size = min(actual_size, user_size);
973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
974 }
975
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48501 bytes --]
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Arnd Bergmann @ 2018-12-12 9:04 UTC (permalink / raw)
To: tg
Cc: Andy Lutomirski, Linus Torvalds, the arch/x86 maintainers,
Linux Kernel Mailing List, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, vapier, H.J. Lu,
Rich Felker, x32, Will Deacon, Catalin Marinas
In-Reply-To: <Pine.BSM.4.64L.1812120231410.21176@herc.mirbsd.org>
On Wed, Dec 12, 2018 at 3:38 AM Thorsten Glaser <tg@mirbsd.de> wrote:
>
> Andy Lutomirski dixit:
>
> >That’s the thing, though: the whole generic kernel compat
> >infrastructure assumes there are at most two ABIs: native and, if
> >enabled and relevant, compat. x32 breaks this entirely.
>
> MIPS had o32, n32, n64 since like forever.
o32 and n32 are practically the same, the only difference on the
syscall ABI that I can see are the actual syscall numbers, and
the 'struct sigcontext' definition.
> ARM has old ABI, EABI and now 64-bit.
arm64 intentionally did not attempt to support OABI user space
because of this, and as I said the ilp32 ABI follows what MIPS
n32 does using the same data structures as aarch32 (corresponding
to mips o32).
> >How hard would it be to have __attribute__((ilp64)), with an optional
> >warning if any embedded structs are not ilp64? This plus a wrapper to
>
> You mean LP64. Impossible, because LP64 vs. ILP32 is not the only
> difference between amd64 and x32.
I think the above is what Intel's compiler does, and similar to what they
do for mixing big-endian and little-endian code (!). Generally possible yes,
but a lot of work, as well as error-prone and not particular appealing for
the GNU toolchain IMHO.
Arnd
^ permalink raw reply
* [RFC PATCH v1 5/5] doc: Add documentation for Yama's open_mayexec_enforce
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
Documentation/admin-guide/LSM/Yama.rst | 41 ++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/Documentation/admin-guide/LSM/Yama.rst b/Documentation/admin-guide/LSM/Yama.rst
index d0a060de3973..a72c86a24b35 100644
--- a/Documentation/admin-guide/LSM/Yama.rst
+++ b/Documentation/admin-guide/LSM/Yama.rst
@@ -72,3 +72,44 @@ The sysctl settings (writable only with ``CAP_SYS_PTRACE``) are:
``PTRACE_TRACEME``. Once set, this sysctl value cannot be changed.
The original children-only logic was based on the restrictions in grsecurity.
+
+open_mayexec_enforce
+====================
+
+The ``O_MAYEXEC`` flag can be passed to :manpage:`open(2)` to only open files
+(or directories) that are executable. If the file is not identified as
+executable, then the syscall returns -EACCES. This may allow a script
+interpreter to check executable permission before reading commands from a file.
+One interesting use case is to enforce a "write xor execute" policy through
+interpreters.
+
+Thanks to this flag, Yama enables to enforce the ``noexec`` mount option (i.e.
+the underlying mount point of the file is mounted with MNT_NOEXEC or its
+underlying superblock is SB_I_NOEXEC) not only on ELF binaries but also on
+scripts. This may be possible thanks to script interpreters using the
+``O_MAYEXEC`` flag. The executable permission is then checked before reading
+commands from a file, and thus can enforce the ``noexec`` at the interpreter
+level by propagating this security policy to the scripts. To be fully
+effective, these interpreters also need to handle the other ways to execute
+code (for which the kernel can't help): command line parameters (e.g., option
+``-e`` for Perl), module loading (e.g., option ``-m`` for Python), stdin, file
+sourcing, environment variables, configuration files... According to the
+threat model, it may be acceptable to allow some script interpreters (e.g.
+Bash) to interpret commands from stdin, may it be a TTY or a pipe, because it
+may not be enough to (directly) perform syscalls.
+
+Yama implements two complementary security policies to propagate the ``noexec``
+mount option or the executable file permission. These policies are handled by
+the ``kernel.yama.open_mayexec_enforce`` sysctl (writable only with
+``CAP_MAC_ADMIN``) as a bitmask:
+
+1 - mount restriction:
+ check that the mount options for the underlying VFS mount do not prevent
+ execution.
+
+2 - file permission restriction:
+ check that the to-be-opened file is marked as executable for the current
+ process (e.g., POSIX permissions).
+
+Code samples can be found in tools/testing/selftests/yama/test_omayexec.c and
+https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC .
--
2.20.0.rc2
^ permalink raw reply related
* [RFC PATCH v1 4/5] selftest/yama: Add tests for O_MAYEXEC enforcing
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
Test propagation of noexec mount points or file executability through
files open with or without O_MAYEXEC.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
Cc: Shuah Khan <shuah@kernel.org>
---
MAINTAINERS | 1 +
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/yama/.gitignore | 1 +
tools/testing/selftests/yama/Makefile | 19 ++
tools/testing/selftests/yama/config | 2 +
tools/testing/selftests/yama/test_omayexec.c | 276 +++++++++++++++++++
6 files changed, 300 insertions(+)
create mode 100644 tools/testing/selftests/yama/.gitignore
create mode 100644 tools/testing/selftests/yama/Makefile
create mode 100644 tools/testing/selftests/yama/config
create mode 100644 tools/testing/selftests/yama/test_omayexec.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8119141a926f..a1d01a81b283 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16591,6 +16591,7 @@ M: Kees Cook <keescook@chromium.org>
T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
S: Supported
F: security/yama/
+F: tools/testing/selftests/yama/
F: Documentation/admin-guide/LSM/Yama.rst
YEALINK PHONE DRIVER
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index f0017c831e57..608f31167aa6 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -46,6 +46,7 @@ endif
TARGETS += user
TARGETS += vm
TARGETS += x86
+TARGETS += yama
TARGETS += zram
#Please keep the TARGETS list alphabetically sorted
# Run "make quicktest=1 run_tests" or
diff --git a/tools/testing/selftests/yama/.gitignore b/tools/testing/selftests/yama/.gitignore
new file mode 100644
index 000000000000..6e8d5cfb48d0
--- /dev/null
+++ b/tools/testing/selftests/yama/.gitignore
@@ -0,0 +1 @@
+/test_omayexec
diff --git a/tools/testing/selftests/yama/Makefile b/tools/testing/selftests/yama/Makefile
new file mode 100644
index 000000000000..d411f1615b60
--- /dev/null
+++ b/tools/testing/selftests/yama/Makefile
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+
+all:
+
+include ../lib.mk
+
+.PHONY: all clean
+
+BINARIES := test_omayexec
+CFLAGS += -Wl,-no-as-needed -Wall -Werror
+LDFLAGS += -lcap
+
+test_omayexec: test_omayexec.c ../kselftest_harness.h
+ $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
+TEST_PROGS += $(BINARIES)
+EXTRA_CLEAN := $(BINARIES)
+
+all: $(BINARIES)
diff --git a/tools/testing/selftests/yama/config b/tools/testing/selftests/yama/config
new file mode 100644
index 000000000000..9d375bfc465b
--- /dev/null
+++ b/tools/testing/selftests/yama/config
@@ -0,0 +1,2 @@
+CONFIG_SECURITY=y
+CONFIG_SECURITY_YAMA=y
diff --git a/tools/testing/selftests/yama/test_omayexec.c b/tools/testing/selftests/yama/test_omayexec.c
new file mode 100644
index 000000000000..7d41097f0e89
--- /dev/null
+++ b/tools/testing/selftests/yama/test_omayexec.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Yama tests - O_MAYEXEC
+ *
+ * Copyright © 2018 ANSSI
+ *
+ * Author: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
+ */
+
+#include <errno.h>
+#include <fcntl.h> /* O_CLOEXEC */
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h> /* strlen */
+#include <sys/capability.h>
+#include <sys/mount.h>
+#include <sys/stat.h> /* mkdir */
+#include <unistd.h> /* unlink, rmdir */
+
+#include "../kselftest_harness.h"
+
+#ifndef O_MAYEXEC
+#define O_MAYEXEC 040000000
+#endif
+
+#define SYSCTL_MAYEXEC "/proc/sys/kernel/yama/open_mayexec_enforce"
+
+#define BIN_DIR "./test-mount"
+#define BIN_PATH BIN_DIR "/file"
+#define DIR_PATH BIN_DIR "/directory"
+
+#define ALLOWED 1
+#define DENIED 0
+
+static void test_omx(struct __test_metadata *_metadata,
+ const char *const path, const int exec_allowed)
+{
+ int fd;
+
+ /* without O_MAYEXEC */
+ fd = open(path, O_RDONLY | O_CLOEXEC);
+ ASSERT_NE(-1, fd);
+ EXPECT_FALSE(close(fd));
+
+ /* with O_MAYEXEC */
+ fd = open(path, O_RDONLY | O_CLOEXEC | O_MAYEXEC);
+ if (exec_allowed) {
+ /* open should succeed */
+ ASSERT_NE(-1, fd);
+ EXPECT_FALSE(close(fd));
+ } else {
+ /* open should return EACCES */
+ ASSERT_EQ(-1, fd);
+ ASSERT_EQ(EACCES, errno);
+ }
+}
+
+static void ignore_dac(struct __test_metadata *_metadata, int override)
+{
+ cap_t caps;
+ const cap_value_t cap_val[2] = {
+ CAP_DAC_OVERRIDE,
+ CAP_DAC_READ_SEARCH,
+ };
+
+ caps = cap_get_proc();
+ ASSERT_TRUE(!!caps);
+ ASSERT_FALSE(cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_val,
+ override ? CAP_SET : CAP_CLEAR));
+ ASSERT_FALSE(cap_set_proc(caps));
+ EXPECT_FALSE(cap_free(caps));
+}
+
+static void test_dir_file(struct __test_metadata *_metadata,
+ const char *const dir_path, const char *const file_path,
+ const int exec_allowed, const int only_file_perm)
+{
+ if (only_file_perm) {
+ /* test as root */
+ ignore_dac(_metadata, 1);
+ /* always allowed because of generic_permission() use */
+ test_omx(_metadata, dir_path, ALLOWED);
+ }
+
+ /* without bypass */
+ ignore_dac(_metadata, 0);
+ test_omx(_metadata, dir_path, exec_allowed);
+ test_omx(_metadata, file_path, exec_allowed);
+}
+
+static void sysctl_write(struct __test_metadata *_metadata,
+ const char *path, const char *value)
+{
+ int fd;
+ size_t len_value;
+ ssize_t len_wrote;
+
+ fd = open(path, O_WRONLY | O_CLOEXEC);
+ ASSERT_NE(-1, fd);
+ len_value = strlen(value);
+ len_wrote = write(fd, value, len_value);
+ ASSERT_EQ(len_wrote, len_value);
+ EXPECT_FALSE(close(fd));
+}
+
+static void create_workspace(struct __test_metadata *_metadata,
+ int mount_exec, int file_exec)
+{
+ int fd;
+
+ /*
+ * Cleanup previous workspace if any error previously happened (don't
+ * check errors).
+ */
+ umount(BIN_DIR);
+ rmdir(BIN_DIR);
+
+ /* create a clean mount point */
+ ASSERT_FALSE(mkdir(BIN_DIR, 00700));
+ ASSERT_FALSE(mount("test", BIN_DIR, "tmpfs",
+ MS_MGC_VAL | (mount_exec ? 0 : MS_NOEXEC),
+ "mode=0700,size=4k"));
+
+ /* create a test file */
+ fd = open(BIN_PATH, O_CREAT | O_RDONLY | O_CLOEXEC,
+ file_exec ? 00500 : 00400);
+ ASSERT_NE(-1, fd);
+ EXPECT_NE(-1, close(fd));
+
+ /* create a test directory */
+ ASSERT_FALSE(mkdir(DIR_PATH, file_exec ? 00500 : 00400));
+}
+
+static void delete_workspace(struct __test_metadata *_metadata)
+{
+ ignore_dac(_metadata, 1);
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "0");
+
+ /* no need to unlink BIN_PATH nor DIR_PATH */
+ ASSERT_FALSE(umount(BIN_DIR));
+ ASSERT_FALSE(rmdir(BIN_DIR));
+}
+
+FIXTURE_DATA(mount_exec_file_exec) { };
+
+FIXTURE_SETUP(mount_exec_file_exec)
+{
+ create_workspace(_metadata, 1, 1);
+}
+
+FIXTURE_TEARDOWN(mount_exec_file_exec)
+{
+ delete_workspace(_metadata);
+}
+
+TEST_F(mount_exec_file_exec, mount)
+{
+ /* enforce mount exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED, 0);
+}
+
+TEST_F(mount_exec_file_exec, file)
+{
+ /* enforce file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED, 0);
+}
+
+TEST_F(mount_exec_file_exec, mount_file)
+{
+ /* enforce mount and file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED, 0);
+}
+
+FIXTURE_DATA(mount_exec_file_noexec) { };
+
+FIXTURE_SETUP(mount_exec_file_noexec)
+{
+ create_workspace(_metadata, 1, 0);
+}
+
+FIXTURE_TEARDOWN(mount_exec_file_noexec)
+{
+ delete_workspace(_metadata);
+}
+
+TEST_F(mount_exec_file_noexec, mount)
+{
+ /* enforce mount exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED, 0);
+}
+
+TEST_F(mount_exec_file_noexec, file)
+{
+ /* enforce file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 1);
+}
+
+TEST_F(mount_exec_file_noexec, mount_file)
+{
+ /* enforce mount and file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 1);
+}
+
+FIXTURE_DATA(mount_noexec_file_exec) { };
+
+FIXTURE_SETUP(mount_noexec_file_exec)
+{
+ create_workspace(_metadata, 0, 1);
+}
+
+FIXTURE_TEARDOWN(mount_noexec_file_exec)
+{
+ delete_workspace(_metadata);
+}
+
+TEST_F(mount_noexec_file_exec, mount)
+{
+ /* enforce mount exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 0);
+}
+
+TEST_F(mount_noexec_file_exec, file)
+{
+ /* enforce file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED, 0);
+}
+
+TEST_F(mount_noexec_file_exec, mount_file)
+{
+ /* enforce mount and file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 0);
+}
+
+FIXTURE_DATA(mount_noexec_file_noexec) { };
+
+FIXTURE_SETUP(mount_noexec_file_noexec)
+{
+ create_workspace(_metadata, 0, 0);
+}
+
+FIXTURE_TEARDOWN(mount_noexec_file_noexec)
+{
+ delete_workspace(_metadata);
+}
+
+TEST_F(mount_noexec_file_noexec, mount)
+{
+ /* enforce mount exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 0);
+}
+
+TEST_F(mount_noexec_file_noexec, file)
+{
+ /* enforce file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 1);
+}
+
+TEST_F(mount_noexec_file_noexec, mount_file)
+{
+ /* enforce mount and file exec check */
+ sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+ test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED, 0);
+}
+
+TEST_HARNESS_MAIN
--
2.20.0.rc2
^ permalink raw reply related
* [RFC PATCH v1 3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
Enable to either propagate the mount options from the underlying VFS
mount to prevent execution, or to propagate the file execute permission.
This may allow a script interpreter to check execution permissions
before reading commands from a file.
The main goal is to be able to protect the kernel by restricting
arbitrary syscalls that an attacker could perform with a crafted binary
or certain script languages. It also improves multilevel isolation
by reducing the ability of an attacker to use side channels with
specific code. These restrictions can natively be enforced for ELF
binaries (with the noexec mount option) but require this kernel
extension to properly handle scripts (e.g., Python, Perl).
Add a new sysctl kernel.yama.open_mayexec_enforce to control this
behavior. A following patch adds documentation.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
security/yama/Kconfig | 3 +-
security/yama/yama_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/security/yama/Kconfig b/security/yama/Kconfig
index 96b27405558a..9457619fabd5 100644
--- a/security/yama/Kconfig
+++ b/security/yama/Kconfig
@@ -5,7 +5,8 @@ config SECURITY_YAMA
help
This selects Yama, which extends DAC support with additional
system-wide security settings beyond regular Linux discretionary
- access controls. Currently available is ptrace scope restriction.
+ access controls. Currently available are ptrace scope restriction and
+ enforcement of the O_MAYEXEC open flag.
Like capabilities, this security module stacks with other LSMs.
Further information can be found in
Documentation/admin-guide/LSM/Yama.rst.
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index ffda91a4a1aa..120664e94ee5 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -1,10 +1,12 @@
/*
* Yama Linux Security Module
*
- * Author: Kees Cook <keescook@chromium.org>
+ * Authors: Kees Cook <keescook@chromium.org>
+ * Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
*
* Copyright (C) 2010 Canonical, Ltd.
* Copyright (C) 2011 The Chromium OS Authors.
+ * Copyright (C) 2018 ANSSI
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2, as
@@ -28,7 +30,14 @@
#define YAMA_SCOPE_CAPABILITY 2
#define YAMA_SCOPE_NO_ATTACH 3
+#define YAMA_OMAYEXEC_ENFORCE_NONE 0
+#define YAMA_OMAYEXEC_ENFORCE_MOUNT (1 << 0)
+#define YAMA_OMAYEXEC_ENFORCE_FILE (1 << 1)
+#define _YAMA_OMAYEXEC_LAST YAMA_OMAYEXEC_ENFORCE_FILE
+#define _YAMA_OMAYEXEC_MASK ((_YAMA_OMAYEXEC_LAST << 1) - 1)
+
static int ptrace_scope = YAMA_SCOPE_RELATIONAL;
+static int open_mayexec_enforce = YAMA_OMAYEXEC_ENFORCE_NONE;
/* describe a ptrace relationship for potential exception */
struct ptrace_relation {
@@ -423,7 +432,40 @@ int yama_ptrace_traceme(struct task_struct *parent)
return rc;
}
+/**
+ * yama_inode_permission - check O_MAYEXEC permission before accessing an inode
+ * @inode: inode structure to check
+ * @mask: permission mask
+ *
+ * Return 0 if access is permitted, -EACCES otherwise.
+ */
+int yama_inode_permission(struct inode *inode, int mask)
+{
+ if (!(mask & MAY_OPENEXEC))
+ return 0;
+ /*
+ * Match regular files and directories to make it easier to
+ * modify script interpreters.
+ */
+ if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+ return 0;
+
+ if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
+ !(mask & MAY_EXECMOUNT))
+ return -EACCES;
+
+ /*
+ * May prefer acl_permission_check() instead of generic_permission(),
+ * to not be bypassable with CAP_DAC_READ_SEARCH.
+ */
+ if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
+ return generic_permission(inode, MAY_EXEC);
+
+ return 0;
+}
+
static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
+ LSM_HOOK_INIT(inode_permission, yama_inode_permission),
LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
LSM_HOOK_INIT(task_prctl, yama_task_prctl),
@@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
}
+static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int error;
+
+ if (write) {
+ struct ctl_table table_copy;
+ int tmp_mayexec_enforce;
+
+ if (!capable(CAP_MAC_ADMIN))
+ return -EPERM;
+ tmp_mayexec_enforce = *((int *)table->data);
+ table_copy = *table;
+ /* do not erase open_mayexec_enforce */
+ table_copy.data = &tmp_mayexec_enforce;
+ error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+ if ((tmp_mayexec_enforce | _YAMA_OMAYEXEC_MASK) !=
+ _YAMA_OMAYEXEC_MASK)
+ return -EINVAL;
+ *((int *)table->data) = tmp_mayexec_enforce;
+ } else {
+ error = proc_dointvec(table, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+ }
+ return 0;
+}
+
static int zero;
static int max_scope = YAMA_SCOPE_NO_ATTACH;
@@ -466,6 +539,13 @@ static struct ctl_table yama_sysctl_table[] = {
.extra1 = &zero,
.extra2 = &max_scope,
},
+ {
+ .procname = "open_mayexec_enforce",
+ .data = &open_mayexec_enforce,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = yama_dointvec_bitmask_macadmin,
+ },
{ }
};
static void __init yama_init_sysctl(void)
--
2.20.0.rc2
^ permalink raw reply related
* [RFC PATCH v1 2/5] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount propertie
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
An LSM doesn't get path information related to an access request to open
an inode. This new (internal) MAY_EXECMOUNT flag enables an LSM to
check if the underlying mount point of an inode is marked as executable.
This is useful to implement a security policy taking advantage of the
noexec mount option.
This flag is set according to path_noexec(), which checks if a mount
point is mounted with MNT_NOEXEC or if the underlying superblock is
SB_I_NOEXEC.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
fs/namei.c | 2 ++
include/linux/fs.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/fs/namei.c b/fs/namei.c
index 0cab6494978c..de4f33b3f464 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2970,6 +2970,8 @@ static int may_open(const struct path *path, int acc_mode, int flag)
break;
}
+ /* Pass the mount point executability. */
+ acc_mode |= path_noexec(path) ? 0 : MAY_EXECMOUNT;
error = inode_permission(inode, MAY_OPEN | acc_mode);
if (error)
return error;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 584c9329ad78..083a31b8068e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -96,6 +96,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_NOT_BLOCK 0x00000080
/* the inode is opened with O_MAYEXEC */
#define MAY_OPENEXEC 0x00000100
+/* the mount point is marked as executable */
+#define MAY_EXECMOUNT 0x00000200
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
--
2.20.0.rc2
^ permalink raw reply related
* [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <20181212081712.32347-1-mic@digikod.net>
When the O_MAYEXEC flag is passed, sys_open() may be subject to
additional restrictions depending on a security policy implemented by an
LSM through the inode_permission hook.
The underlying idea is to be able to restrict scripts interpretation
according to a policy defined by the system administrator. For this to
be possible, script interpreters must use the O_MAYEXEC flag
appropriately. To be fully effective, these interpreters also need to
handle the other ways to execute code (for which the kernel can't help):
command line parameters (e.g., option -e for Perl), module loading
(e.g., option -m for Python), stdin, file sourcing, environment
variables, configuration files... According to the threat model, it may
be acceptable to allow some script interpreters (e.g. Bash) to interpret
commands from stdin, may it be a TTY or a pipe, because it may not be
enough to (directly) perform syscalls.
A simple security policy implementation is available in a following
patch for Yama.
This is an updated subset of the patch initially written by Vincent
Strubel for CLIP OS:
https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
This patch has been used for more than 10 years with customized script
interpreters. Some examples can be found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
---
fs/fcntl.c | 2 +-
fs/open.c | 4 ++++
include/linux/fcntl.h | 2 +-
include/linux/fs.h | 2 ++
include/uapi/asm-generic/fcntl.h | 3 +++
5 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..6c85c4d0c006 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..75479b79a58f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -974,6 +974,10 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
if (flags & O_APPEND)
acc_mode |= MAY_APPEND;
+ /* Check execution permissions on open. */
+ if (flags & O_MAYEXEC)
+ acc_mode |= MAY_OPENEXEC;
+
op->acc_mode = acc_mode;
op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..1fc00cabe9ab 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,7 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_MAYEXEC)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c95c0807471f..584c9329ad78 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -94,6 +94,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_CHDIR 0x00000040
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
+/* the inode is opened with O_MAYEXEC */
+#define MAY_OPENEXEC 0x00000100
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..cbb9425d6e7c 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,9 @@
#define O_NDELAY O_NONBLOCK
#endif
+/* command execution from file is intended, check exec permissions */
+#define O_MAYEXEC 040000000
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
--
2.20.0.rc2
^ permalink raw reply related
* [RFC PATCH v1 0/5] Add support for O_MAYEXEC
From: Mickaël Salaün @ 2018-12-12 8:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Al Viro, James Morris, Jonathan Corbet,
Kees Cook, Matthew Garrett, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Shuah Khan, Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
Hi,
The goal of this patch series is to control script interpretation. A
new O_MAYEXEC flag used by sys_open() is added to enable userland script
interpreter to delegate to the kernel (and thus the system security
policy) the permission to interpret scripts or other files containing
what can be seen as commands.
The security policy is the responsibility of an LSM. A basic
system-wide policy is implemented with Yama and configurable through a
sysctl.
The initial idea come from CLIP OS and the original implementation has
been used for more than 10 years:
https://github.com/clipos-archive/clipos4_doc
An introduction to O_MAYEXEC was given at the Linux Security Summit
Europe 2018 - Linux Kernel Security Contributions by ANSSI:
https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
The "write xor execute" principle was explained at Kernel Recipes 2018 -
CLIP OS: a defense-in-depth OS:
https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
This patch series can be applied on top of v4.20-rc6. This can be
tested with CONFIG_SECURITY_YAMA. I would really appreciate
constructive comments on this RFC.
Regards,
Mickaël Salaün (5):
fs: Add support for an O_MAYEXEC flag on sys_open()
fs: Add a MAY_EXECMOUNT flag to infer the noexec mount propertie
Yama: Enforces noexec mounts or file executability through O_MAYEXEC
selftest/yama: Add tests for O_MAYEXEC enforcing
doc: Add documentation for Yama's open_mayexec_enforce
Documentation/admin-guide/LSM/Yama.rst | 41 +++
MAINTAINERS | 1 +
fs/fcntl.c | 2 +-
fs/namei.c | 2 +
fs/open.c | 4 +
include/linux/fcntl.h | 2 +-
include/linux/fs.h | 4 +
include/uapi/asm-generic/fcntl.h | 3 +
security/yama/Kconfig | 3 +-
security/yama/yama_lsm.c | 82 +++++-
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/yama/.gitignore | 1 +
tools/testing/selftests/yama/Makefile | 19 ++
tools/testing/selftests/yama/config | 2 +
tools/testing/selftests/yama/test_omayexec.c | 276 +++++++++++++++++++
15 files changed, 439 insertions(+), 4 deletions(-)
create mode 100644 tools/testing/selftests/yama/.gitignore
create mode 100644 tools/testing/selftests/yama/Makefile
create mode 100644 tools/testing/selftests/yama/config
create mode 100644 tools/testing/selftests/yama/test_omayexec.c
--
2.20.0.rc2
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Thorsten Glaser @ 2018-12-12 2:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, X86 ML, LKML, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, Mike Frysinger,
H. J. Lu, Rich Felker, x32, Arnd Bergmann, Will Deacon,
Catalin Marinas
In-Reply-To: <CALCETrXf0rmadycxmpGxd41qP9X+PAjyGHTwbGhKyp6oMKMRrA@mail.gmail.com>
Andy Lutomirski dixit:
>That’s the thing, though: the whole generic kernel compat
>infrastructure assumes there are at most two ABIs: native and, if
>enabled and relevant, compat. x32 breaks this entirely.
MIPS had o32, n32, n64 since like forever.
ARM has old ABI, EABI and now 64-bit.
Other architectures are yet to come.
>IMO the real right solution would be to push the whole problem to
>userspace: get an ILP32 system working with almost or entirely LP64
Is this a reflex of Linux kernel developers? ;-)
I doubt that userspace is the right place for this, remember
the recent glibc vs. syscalls debate. It would also need to
multiply across various libcs.
>How hard would it be to have __attribute__((ilp64)), with an optional
>warning if any embedded structs are not ilp64? This plus a wrapper to
You mean LP64. Impossible, because LP64 vs. ILP32 is not the only
difference between amd64 and x32.
bye,
//mirabilos
--
I believe no one can invent an algorithm. One just happens to hit upon it
when God enlightens him. Or only God invents algorithms, we merely copy them.
If you don't believe in God, just consider God as Nature if you won't deny
existence. -- Coywolf Qi Hunt
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Andy Lutomirski @ 2018-12-12 2:24 UTC (permalink / raw)
To: tg
Cc: Andrew Lutomirski, Linus Torvalds, X86 ML, LKML, Linux API,
H. Peter Anvin, Peter Zijlstra, Borislav Petkov, Florian Weimer,
Mike Frysinger, H. J. Lu, Rich Felker, x32, Arnd Bergmann,
Will Deacon, Catalin Marinas
In-Reply-To: <Pine.BSM.4.64L.1812112327500.21176@herc.mirbsd.org>
> On Dec 11, 2018, at 3:35 PM, Thorsten Glaser <tg@mirbsd.de> wrote:
>
> Andy Lutomirski dixit:
>
>> What happens if someone adds a struct like:
>>
>> struct nasty_on_x32 {
>> __kernel_long_t a;
>> void * __user b;
>> };
>>
>> On x86_64, that's two 8-byte fields. On x86_32, it's two four-byte
>> fields. On x32, it's an 8-byte field and a 4-byte field. Now what?
>
> Yes, that’s indeed ugly. I understand. But don’t we already have
> this problem with architectures which support multiple ABIs at the
> same time? An amd64 kernel with i386 userspace comes to mind, or
> the multiple MIPS ABIs.
That’s the thing, though: the whole generic kernel compat
infrastructure assumes there are at most two ABIs: native and, if
enabled and relevant, compat. x32 breaks this entirely.
>
>> I'm sure we could have some magic gcc plugin or other nifty tool that
>> gives us:
>>
>> copy_from_user(struct struct_name, kernel_ptr, user_ptr);
>
> Something like that might be useful. Generate call stubs, which
> then call the syscall implementation with the actual user-space
> struct contents as arguments. Hm, that might be too generic to
> be useful. Generate macros that can read from or write specific
> structures to userspace?
>
> I think something like this could solve other more general problems
> as well, so it might be “nice to have anyway”. Of course it’s work,
> and I’m not involved enough in Linux kernel programming to be able
> to usefully help with it (doing too much elsewhere already).
>
>> actually do this work. Instead we get ad hoc fixes for each syscall,
>> along the lines of preadv64v2(), which get done when somebody notices
>
> Yes, that’s absolutely ugly and ridiculous and all kinds of bad.
>
> On the other hand, from my current experience, someone (Arnd?) noticed
> all the currently existing baddies for x32 already and fixed them.
>
> New syscalls are indeed an issue, but perhaps something generating
> copyinout stubs could help. This might allow other architectures
> that could do with a new ABI but have until now feared the overhead
> as well. (IIRC, m68k could do with a new ABI that reserves a register
> for TLS, but Geert would know. At the same time, time_t and off_t could
> be bumped to 64 bit. Something like that. If changing sizes of types
> shared between kernel and user spaces is not something feared…)
Magic autogenerated stubs would be great. Difficult, too, given
unions, multiplexers, cmsg, etc.
I suppose I will see how bad it would be to split out the x32 syscall
table and at least isolate the mess to some extent.
IMO the real right solution would be to push the whole problem to
userspace: get an ILP32 system working with almost or entirely LP64
syscalls. POSIX support might have to be a bit flexible, but still.
How hard would it be to have __attribute__((ilp64)), with an optional
warning if any embedded structs are not ilp64? This plus a wrapper to
make sure that mmap puts everything below 4GB ought to do the trick.
Or something like what arm64 is proposing where the kernel ABI has
32-bit long doesn’t seem too horrible.
^ permalink raw reply
* Re: [PATCH v10 4/4] samples: add an example of seccomp user trap
From: Kees Cook @ 2018-12-12 0:46 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Tycho Andersen, Andy Lutomirski, Oleg Nesterov, Eric W. Biederman,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
Jann Horn, LKML, Linux Containers, Linux API
In-Reply-To: <20181211222449.GA937@mail.hallyn.com>
On Tue, Dec 11, 2018 at 2:24 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Sun, Dec 09, 2018 at 11:24:14AM -0700, Tycho Andersen wrote:
> > The idea here is just to give a demonstration of how one could safely use
> > the SECCOMP_RET_USER_NOTIF feature to do mount policies. This particular
> > policy is (as noted in the comment) not very interesting, but it serves to
> > illustrate how one might apply a policy dodging the various TOCTOU issues.
> >
> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Andy Lutomirski <luto@amacapital.net>
> > CC: Oleg Nesterov <oleg@redhat.com>
> > CC: Eric W. Biederman <ebiederm@xmission.com>
> > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > CC: Christian Brauner <christian@brauner.io>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > ---
> > v5: new in v5
> > v7: updates for v7 API changes
> > v8: * add some more comments about what's happening in main() (Kees)
> > * move from ptrace API to SECCOMP_FILTER_FLAG_NEW_LISTENER
> > v9: * s/mknod/mount in error message
> > * switch to the SECCOMP_GET_NOTIF_SIZES API
> > * add a note about getting ENOENT from SECCOMP_IOCTL_NOTIF_SEND
> > ---
> > samples/seccomp/.gitignore | 1 +
> > samples/seccomp/Makefile | 7 +-
> > samples/seccomp/user-trap.c | 375 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 382 insertions(+), 1 deletion(-)
> >
> > diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
> > index 78fb78184291..d1e2e817d556 100644
> > --- a/samples/seccomp/.gitignore
> > +++ b/samples/seccomp/.gitignore
> > @@ -1,3 +1,4 @@
> > bpf-direct
> > bpf-fancy
> > dropper
> > +user-trap
> > diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
> > index cf34ff6b4065..4920903c8009 100644
> > --- a/samples/seccomp/Makefile
> > +++ b/samples/seccomp/Makefile
> > @@ -1,6 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0
> > ifndef CROSS_COMPILE
> > -hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
> > +hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct user-trap
> >
> > HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
> > HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
> > @@ -16,6 +16,10 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
> > HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
> > bpf-direct-objs := bpf-direct.o
> >
> > +HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
> > +HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
> > +user-trap-objs := user-trap.o
> > +
> > # Try to match the kernel target.
> > ifndef CONFIG_64BIT
> >
> > @@ -33,6 +37,7 @@ HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
> > HOSTLDLIBS_bpf-direct += $(MFLAG)
> > HOSTLDLIBS_bpf-fancy += $(MFLAG)
> > HOSTLDLIBS_dropper += $(MFLAG)
> > +HOSTLDLIBS_user-trap += $(MFLAG)
> > endif
> > always := $(hostprogs-m)
> > endif
> > diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> > new file mode 100644
> > index 000000000000..61267cb59c8e
> > --- /dev/null
> > +++ b/samples/seccomp/user-trap.c
> > @@ -0,0 +1,375 @@
> > +#include <signal.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <stddef.h>
> > +#include <sys/sysmacros.h>
> > +#include <sys/types.h>
> > +#include <sys/wait.h>
> > +#include <sys/socket.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <sys/syscall.h>
> > +#include <sys/user.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/ptrace.h>
> > +#include <sys/mount.h>
> > +#include <linux/limits.h>
> > +#include <linux/filter.h>
> > +#include <linux/seccomp.h>
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
> > +
> > +static int seccomp(unsigned int op, unsigned int flags, void *args)
> > +{
> > + errno = 0;
> > + return syscall(__NR_seccomp, op, flags, args);
> > +}
> > +
> > +static int send_fd(int sock, int fd)
> > +{
> > + struct msghdr msg = {};
> > + struct cmsghdr *cmsg;
> > + char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
> > + struct iovec io = {
> > + .iov_base = &c,
> > + .iov_len = 1,
> > + };
> > +
> > + msg.msg_iov = &io;
> > + msg.msg_iovlen = 1;
> > + msg.msg_control = buf;
> > + msg.msg_controllen = sizeof(buf);
> > + cmsg = CMSG_FIRSTHDR(&msg);
> > + cmsg->cmsg_level = SOL_SOCKET;
> > + cmsg->cmsg_type = SCM_RIGHTS;
> > + cmsg->cmsg_len = CMSG_LEN(sizeof(int));
> > + *((int *)CMSG_DATA(cmsg)) = fd;
> > + msg.msg_controllen = cmsg->cmsg_len;
> > +
> > + if (sendmsg(sock, &msg, 0) < 0) {
> > + perror("sendmsg");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int recv_fd(int sock)
> > +{
> > + struct msghdr msg = {};
> > + struct cmsghdr *cmsg;
> > + char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
> > + struct iovec io = {
> > + .iov_base = &c,
> > + .iov_len = 1,
> > + };
> > +
> > + msg.msg_iov = &io;
> > + msg.msg_iovlen = 1;
> > + msg.msg_control = buf;
> > + msg.msg_controllen = sizeof(buf);
> > +
> > + if (recvmsg(sock, &msg, 0) < 0) {
> > + perror("recvmsg");
> > + return -1;
> > + }
> > +
> > + cmsg = CMSG_FIRSTHDR(&msg);
> > +
> > + return *((int *)CMSG_DATA(cmsg));
> > +}
> > +
> > +static int user_trap_syscall(int nr, unsigned int flags)
> > +{
> > + struct sock_filter filter[] = {
> > + BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
> > + offsetof(struct seccomp_data, nr)),
> > + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
> > + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
> > + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
> > + };
> > +
> > + struct sock_fprog prog = {
> > + .len = (unsigned short)ARRAY_SIZE(filter),
> > + .filter = filter,
> > + };
> > +
> > + return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
> > +}
> > +
> > +static int handle_req(struct seccomp_notif *req,
> > + struct seccomp_notif_resp *resp, int listener)
> > +{
> > + char path[PATH_MAX], source[PATH_MAX], target[PATH_MAX];
> > + int ret = -1, mem;
> > +
> > + resp->id = req->id;
> > + resp->error = -EPERM;
> > + resp->val = 0;
> > +
> > + if (req->data.nr != __NR_mount) {
> > + fprintf(stderr, "huh? trapped something besides mount? %d\n", req->data.nr);
> > + return -1;
> > + }
> > +
> > + /* Only allow bind mounts. */
> > + if (!(req->data.args[3] & MS_BIND))
> > + return 0;
> > +
> > + /*
> > + * Ok, let's read the task's memory to see where they wanted their
> > + * mount to go.
> > + */
> > + snprintf(path, sizeof(path), "/proc/%d/mem", req->pid);
> > + mem = open(path, O_RDONLY);
> > + if (mem < 0) {
> > + perror("open mem");
> > + return -1;
> > + }
> > +
> > + /*
> > + * Now we avoid a TOCTOU: we referred to a pid by its pid, but since
> > + * the pid that made the syscall may have died, we need to confirm that
> > + * the pid is still valid after we open its /proc/pid/mem file. We can
> > + * ask the listener fd this as follows.
> > + *
> > + * Note that this check should occur *after* any task-specific
> > + * resources are opened, to make sure that the task has not died and
> > + * we're not wrongly reading someone else's state in order to make
> > + * decisions.
> > + */
> > + if (ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req->id) < 0) {
> > + fprintf(stderr, "task died before we could map its memory\n");
> > + goto out;
> > + }
> > +
> > + /*
> > + * Phew, we've got the right /proc/pid/mem. Now we can read it. Note
> > + * that to avoid another TOCTOU, we should read all of the pointer args
> > + * before we decide to allow the syscall.
> > + */
> > + if (lseek(mem, req->data.args[0], SEEK_SET) < 0) {
> > + perror("seek");
> > + goto out;
> > + }
> > +
> > + ret = read(mem, source, sizeof(source));
> > + if (ret < 0) {
> > + perror("read");
> > + goto out;
> > + }
> > +
> > + if (lseek(mem, req->data.args[1], SEEK_SET) < 0) {
> > + perror("seek");
> > + goto out;
> > + }
> > +
> > + ret = read(mem, target, sizeof(target));
> > + if (ret < 0) {
> > + perror("read");
> > + goto out;
> > + }
> > +
> > + /*
> > + * Our policy is to only allow bind mounts inside /tmp. This isn't very
> > + * interesting, because we could do unprivlieged bind mounts with user
> > + * namespaces already, but you get the idea.
> > + */
> > + if (!strncmp(source, "/tmp", 4) && !strncmp(target, "/tmp", 4)) {
>
> Of course the host could have a /tmproot or somesuch, so you might want to
> check the first 5 characters :)
I manually bumped this to "/tmp/", 5 in both tests and applied it for -next
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v10 3/4] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-12-12 0:46 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W. Biederman,
Serge E. Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, Jann Horn, LKML, Linux Containers, Linux API
In-Reply-To: <20181209182414.30862-4-tycho@tycho.ws>
On Sun, Dec 9, 2018 at 10:24 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> This patch introduces a means for syscalls matched in seccomp to notify
> some other task that a particular filter has been triggered.
>
> The motivation for this is primarily for use with containers. For example,
> if a container does an init_module(), we obviously don't want to load this
> untrusted code, which may be compiled for the wrong version of the kernel
> anyway. Instead, we could parse the module image, figure out which module
> the container is trying to load and load it on the host.
>
> As another example, containers cannot mount() in general since various
> filesystems assume a trusted image. However, if an orchestrator knows that
> e.g. a particular block device has not been exposed to a container for
> writing, it want to allow the container to mount that block device (that
> is, handle the mount for it).
>
> This patch adds functionality that is already possible via at least two
> other means that I know about, both of which involve ptrace(): first, one
> could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> Unfortunately this is slow, so a faster version would be to install a
> filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> Since ptrace allows only one tracer, if the container runtime is that
> tracer, users inside the container (or outside) trying to debug it will not
> be able to use ptrace, which is annoying. It also means that older
> distributions based on Upstart cannot boot inside containers using ptrace,
> since upstart itself uses ptrace to monitor services while starting.
>
> The actual implementation of this is fairly small, although getting the
> synchronization right was/is slightly complex.
>
> Finally, it's worth noting that the classic seccomp TOCTOU of reading
> memory data from the task still applies here, but can be avoided with
> careful design of the userspace handler: if the userspace handler reads all
> of the task memory that is necessary before applying its security policy,
> the tracee's subsequent memory edits will not be read by the tracer.
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> CC: Christian Brauner <christian@brauner.io>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
This takes care of everything I mentioned (and has incorporated LOTS
of people's suggestions), so I think it's ready for -next. I've
applied this and am doing local testing now.
Thanks for keeping with this!
-Kees
> ---
> v2: * make id a u64; the idea here being that it will never overflow,
> because 64 is huge (one syscall every nanosecond => wrap every 584
> years) (Andy)
> * prevent nesting of user notifications: if someone is already attached
> the tree in one place, nobody else can attach to the tree (Andy)
> * notify the listener of signals the tracee receives as well (Andy)
> * implement poll
> v3: * lockdep fix (Oleg)
> * drop unnecessary WARN()s (Christian)
> * rearrange error returns to be more rpetty (Christian)
> * fix build in !CONFIG_SECCOMP_USER_NOTIFICATION case
> v4: * fix implementation of poll to use poll_wait() (Jann)
> * change listener's fd flags to be 0 (Jann)
> * hoist filter initialization out of ifdefs to its own function
> init_user_notification()
> * add some more testing around poll() and closing the listener while a
> syscall is in action
> * s/GET_LISTENER/NEW_LISTENER, since you can't _get_ a listener, but it
> creates a new one (Matthew)
> * correctly handle pid namespaces, add some testcases (Matthew)
> * use EINPROGRESS instead of EINVAL when a notification response is
> written twice (Matthew)
> * fix comment typo from older version (SEND vs READ) (Matthew)
> * whitespace and logic simplification (Tobin)
> * add some Documentation/ bits on userspace trapping
> v5: * fix documentation typos (Jann)
> * add signalled field to struct seccomp_notif (Jann)
> * switch to using ioctls instead of read()/write() for struct passing
> (Jann)
> * add an ioctl to ensure an id is still valid
> v6: * docs typo fixes, update docs for ioctl() change (Christian)
> v7: * switch struct seccomp_knotif's id member to a u64 (derp :)
> * use notify_lock in IS_ID_VALID query to avoid racing
> * s/signalled/signaled (Tyler)
> * fix docs to reflect that ids are not globally unique (Tyler)
> * add a test to check -ERESTARTSYS behavior (Tyler)
> * drop CONFIG_SECCOMP_USER_NOTIFICATION (Tyler)
> * reorder USER_NOTIF in seccomp return codes list (Tyler)
> * return size instead of sizeof(struct user_notif) (Tyler)
> * ENOENT instead of EINVAL when invalid id is passed (Tyler)
> * drop CONFIG_SECCOMP_USER_NOTIFICATION guards (Tyler)
> * s/IS_ID_VALID/ID_VALID and switch ioctl to be "well behaved" (Tyler)
> * add a new struct notification to minimize the additions to
> struct seccomp_filter, also pack the necessary additions a bit more
> cleverly (Tyler)
> * switch to keeping track of the task itself instead of the pid (we'll
> use this for implementing PUT_FD)
> v8: * in recv, don't copy_to_user() while holding notify lock, in case
> userfaultfd blocks and causes all syscalls to block (Kees)
> * switch ioctl character to something more fun ! (Kees)
> * switch ioctl defines to use their own SECCOMP_IO* macros (Kees)
> * rename seccomp ioctls to be SECCOMP_IOCTL_* (Kees)
> * move comment of notify_lock to the right place (Jann)
> * drop comment abount reference count bounding in __get_seccomp_filter (Jann)
> * add lockdep_assert_held() in seccomp_next_notify_id() (Kees)
> * in seccomp_do_user_notification(), always increment semaphore before
> releasing lock, to prevent use after free of ->notif (Kees)
> * add another wake_up_poll() when a signal is received (Jann)
> * make all listener fds O_CLOEXEC (Jann/Kees)
> * use memset() instead of = {} initialization for structures (Kees)
> * move casting of buf pointer to ioctl, instead of in handler functions (Kees)
> * fix ENOENT testing in seccomp_notify_send() (Jann)
> * use ENOENT instead of -1 (EPERM) for ID_VALID ioctl (Jann)
> * use ()s around "nested" bit operations (Kees)
> * init struct notification members in the order they're declared (Jann)
> * rearrange things so no forward declaration of init_listener() is
> required (Kees)
> * switch to a flags based future-proofing mechanism for struct
> seccomp_notif and seccomp_notif_resp, thus avoiding version issues
> with structure length (Kees)
> * fix a memory leak in init_listener() in a failure case
> * fix a use-after-free of filter->notif in do_user_notification() when
> the listener fd is closed after a signal is sent
> * add a comment about semaphore state in the interrupt case in
> do_user_notification() + seccomp_notify_recv()
> v9: * add SECCOMP_GET_NOTIF_SIZES to handle when struct seccomp_data
> changes in size
> * don't do locking all the way up the seccomp tree (Oleg)
> * rearrange the tests so that one test tests one thing
> * avoid an unkillable sleep by dropping the signaled flag (Oleg)
> ---
> Documentation/ioctl/ioctl-number.txt | 1 +
> .../userspace-api/seccomp_filter.rst | 84 ++++
> include/linux/seccomp.h | 7 +-
> include/uapi/linux/seccomp.h | 40 +-
> kernel/seccomp.c | 448 +++++++++++++++++-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 447 ++++++++++++++++-
> 6 files changed, 1017 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index af6f6ba1fe80..c9558146ac58 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -79,6 +79,7 @@ Code Seq#(hex) Include File Comments
> 0x1b all InfiniBand Subsystem <http://infiniband.sourceforge.net/>
> 0x20 all drivers/cdrom/cm206.h
> 0x22 all scsi/sg.h
> +'!' 00-1F uapi/linux/seccomp.h
> '#' 00-3F IEEE 1394 Subsystem Block for the entire subsystem
> '$' 00-0F linux/perf_counter.h, linux/perf_event.h
> '%' 00-0F include/uapi/linux/stm.h
> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> index 82a468bc7560..b1b846d8a094 100644
> --- a/Documentation/userspace-api/seccomp_filter.rst
> +++ b/Documentation/userspace-api/seccomp_filter.rst
> @@ -122,6 +122,11 @@ In precedence order, they are:
> Results in the lower 16-bits of the return value being passed
> to userland as the errno without executing the system call.
>
> +``SECCOMP_RET_USER_NOTIF``:
> + Results in a ``struct seccomp_notif`` message sent on the userspace
> + notification fd, if it is attached, or ``-ENOSYS`` if it is not. See below
> + on discussion of how to handle user notifications.
> +
> ``SECCOMP_RET_TRACE``:
> When returned, this value will cause the kernel to attempt to
> notify a ``ptrace()``-based tracer prior to executing the system
> @@ -183,6 +188,85 @@ The ``samples/seccomp/`` directory contains both an x86-specific example
> and a more generic example of a higher level macro interface for BPF
> program generation.
>
> +Userspace Notification
> +======================
> +
> +The ``SECCOMP_RET_USER_NOTIF`` return code lets seccomp filters pass a
> +particular syscall to userspace to be handled. This may be useful for
> +applications like container managers, which wish to intercept particular
> +syscalls (``mount()``, ``finit_module()``, etc.) and change their behavior.
> +
> +To acquire a notification FD, use the ``SECCOMP_FILTER_FLAG_NEW_LISTENER``
> +argument to the ``seccomp()`` syscall:
> +
> +.. code-block:: c
> +
> + fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
> +
> +which (on success) will return a listener fd for the filter, which can then be
> +passed around via ``SCM_RIGHTS`` or similar. Note that filter fds correspond to
> +a particular filter, and not a particular task. So if this task then forks,
> +notifications from both tasks will appear on the same filter fd. Reads and
> +writes to/from a filter fd are also synchronized, so a filter fd can safely
> +have many readers.
> +
> +The interface for a seccomp notification fd consists of two structures:
> +
> +.. code-block:: c
> +
> + struct seccomp_notif_sizes {
> + __u16 seccomp_notif;
> + __u16 seccomp_notif_resp;
> + __u16 seccomp_data;
> + };
> +
> + struct seccomp_notif {
> + __u64 id;
> + __u32 pid;
> + __u32 flags;
> + struct seccomp_data data;
> + };
> +
> + struct seccomp_notif_resp {
> + __u64 id;
> + __s64 val;
> + __s32 error;
> + __u32 flags;
> + };
> +
> +The ``struct seccomp_notif_sizes`` structure can be used to determine the size
> +of the various structures used in seccomp notifications. The size of ``struct
> +seccomp_data`` may change in the future, so code should use:
> +
> +.. code-block:: c
> +
> + struct seccomp_notif_sizes sizes;
> + seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes);
> +
> +to determine the size of the various structures to allocate. See
> +samples/seccomp/user-trap.c for an example.
> +
> +Users can read via ``ioctl(SECCOMP_IOCTL_NOTIF_RECV)`` (or ``poll()``) on a
> +seccomp notification fd to receive a ``struct seccomp_notif``, which contains
> +five members: the input length of the structure, a unique-per-filter ``id``,
> +the ``pid`` of the task which triggered this request (which may be 0 if the
> +task is in a pid ns not visible from the listener's pid namespace), a ``flags``
> +member which for now only has ``SECCOMP_NOTIF_FLAG_SIGNALED``, representing
> +whether or not the notification is a result of a non-fatal signal, and the
> +``data`` passed to seccomp. Userspace can then make a decision based on this
> +information about what to do, and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a
> +response, indicating what should be returned to userspace. The ``id`` member of
> +``struct seccomp_notif_resp`` should be the same ``id`` as in ``struct
> +seccomp_notif``.
> +
> +It is worth noting that ``struct seccomp_data`` contains the values of register
> +arguments to the syscall, but does not contain pointers to memory. The task's
> +memory is accessible to suitably privileged traces via ``ptrace()`` or
> +``/proc/pid/mem``. However, care should be taken to avoid the TOCTOU mentioned
> +above in this document: all arguments being read from the tracee's memory
> +should be read into the tracer's memory before any policy decisions are made.
> +This allows for an atomic decision on syscall arguments.
> +
> Sysctls
> =======
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index b5103c019cf4..84868d37b35d 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -4,9 +4,10 @@
>
> #include <uapi/linux/seccomp.h>
>
> -#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \
> - SECCOMP_FILTER_FLAG_LOG | \
> - SECCOMP_FILTER_FLAG_SPEC_ALLOW)
> +#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \
> + SECCOMP_FILTER_FLAG_LOG | \
> + SECCOMP_FILTER_FLAG_SPEC_ALLOW | \
> + SECCOMP_FILTER_FLAG_NEW_LISTENER)
>
> #ifdef CONFIG_SECCOMP
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 9efc0e73d50b..90734aa5aa36 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -15,11 +15,13 @@
> #define SECCOMP_SET_MODE_STRICT 0
> #define SECCOMP_SET_MODE_FILTER 1
> #define SECCOMP_GET_ACTION_AVAIL 2
> +#define SECCOMP_GET_NOTIF_SIZES 3
>
> /* Valid flags for SECCOMP_SET_MODE_FILTER */
> -#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> -#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
> -#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
> +#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> +#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
> +#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
> +#define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
>
> /*
> * All BPF programs must return a 32-bit value.
> @@ -35,6 +37,7 @@
> #define SECCOMP_RET_KILL SECCOMP_RET_KILL_THREAD
> #define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SIGSYS */
> #define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */
> +#define SECCOMP_RET_USER_NOTIF 0x7fc00000U /* notifies userspace */
> #define SECCOMP_RET_TRACE 0x7ff00000U /* pass to a tracer or disallow */
> #define SECCOMP_RET_LOG 0x7ffc0000U /* allow after logging */
> #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
> @@ -60,4 +63,35 @@ struct seccomp_data {
> __u64 args[6];
> };
>
> +struct seccomp_notif_sizes {
> + __u16 seccomp_notif;
> + __u16 seccomp_notif_resp;
> + __u16 seccomp_data;
> +};
> +
> +struct seccomp_notif {
> + __u64 id;
> + __u32 pid;
> + __u32 flags;
> + struct seccomp_data data;
> +};
> +
> +struct seccomp_notif_resp {
> + __u64 id;
> + __s64 val;
> + __s32 error;
> + __u32 flags;
> +};
> +
> +#define SECCOMP_IOC_MAGIC '!'
> +#define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
> +#define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
> +#define SECCOMP_IOW(nr, type) _IOW(SECCOMP_IOC_MAGIC, nr, type)
> +#define SECCOMP_IOWR(nr, type) _IOWR(SECCOMP_IOC_MAGIC, nr, type)
> +
> +/* Flags for seccomp notification fd ioctl. */
> +#define SECCOMP_IOCTL_NOTIF_RECV SECCOMP_IOWR(0, struct seccomp_notif)
> +#define SECCOMP_IOCTL_NOTIF_SEND SECCOMP_IOWR(1, \
> + struct seccomp_notif_resp)
> +#define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64)
> #endif /* _UAPI_LINUX_SECCOMP_H */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 393e029f778a..15b6be97fc09 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -33,12 +33,74 @@
> #endif
>
> #ifdef CONFIG_SECCOMP_FILTER
> +#include <linux/file.h>
> #include <linux/filter.h>
> #include <linux/pid.h>
> #include <linux/ptrace.h>
> #include <linux/security.h>
> #include <linux/tracehook.h>
> #include <linux/uaccess.h>
> +#include <linux/anon_inodes.h>
> +
> +enum notify_state {
> + SECCOMP_NOTIFY_INIT,
> + SECCOMP_NOTIFY_SENT,
> + SECCOMP_NOTIFY_REPLIED,
> +};
> +
> +struct seccomp_knotif {
> + /* The struct pid of the task whose filter triggered the notification */
> + struct task_struct *task;
> +
> + /* The "cookie" for this request; this is unique for this filter. */
> + u64 id;
> +
> + /*
> + * The seccomp data. This pointer is valid the entire time this
> + * notification is active, since it comes from __seccomp_filter which
> + * eclipses the entire lifecycle here.
> + */
> + const struct seccomp_data *data;
> +
> + /*
> + * Notification states. When SECCOMP_RET_USER_NOTIF is returned, a
> + * struct seccomp_knotif is created and starts out in INIT. Once the
> + * handler reads the notification off of an FD, it transitions to SENT.
> + * If a signal is received the state transitions back to INIT and
> + * another message is sent. When the userspace handler replies, state
> + * transitions to REPLIED.
> + */
> + enum notify_state state;
> +
> + /* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
> + int error;
> + long val;
> +
> + /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
> + struct completion ready;
> +
> + struct list_head list;
> +};
> +
> +/**
> + * struct notification - container for seccomp userspace notifications. Since
> + * most seccomp filters will not have notification listeners attached and this
> + * structure is fairly large, we store the notification-specific stuff in a
> + * separate structure.
> + *
> + * @request: A semaphore that users of this notification can wait on for
> + * changes. Actual reads and writes are still controlled with
> + * filter->notify_lock.
> + * @next_id: The id of the next request.
> + * @notifications: A list of struct seccomp_knotif elements.
> + * @wqh: A wait queue for poll.
> + */
> +struct notification {
> + struct semaphore request;
> + u64 next_id;
> + struct list_head notifications;
> + wait_queue_head_t wqh;
> +};
>
> /**
> * struct seccomp_filter - container for seccomp BPF programs
> @@ -50,6 +112,8 @@
> * @log: true if all actions except for SECCOMP_RET_ALLOW should be logged
> * @prev: points to a previously installed, or inherited, filter
> * @prog: the BPF program to evaluate
> + * @notif: the struct that holds all notification related information
> + * @notify_lock: A lock for all notification-related accesses.
> *
> * seccomp_filter objects are organized in a tree linked via the @prev
> * pointer. For any task, it appears to be a singly-linked list starting
> @@ -66,6 +130,8 @@ struct seccomp_filter {
> bool log;
> struct seccomp_filter *prev;
> struct bpf_prog *prog;
> + struct notification *notif;
> + struct mutex notify_lock;
> };
>
> /* Limit any path through the tree to 256KB worth of instructions. */
> @@ -386,6 +452,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
> if (!sfilter)
> return ERR_PTR(-ENOMEM);
>
> + mutex_init(&sfilter->notify_lock);
> ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
> seccomp_check_filter, save_orig);
> if (ret < 0) {
> @@ -479,7 +546,6 @@ static long seccomp_attach_filter(unsigned int flags,
>
> static void __get_seccomp_filter(struct seccomp_filter *filter)
> {
> - /* Reference count is bounded by the number of total processes. */
> refcount_inc(&filter->usage);
> }
>
> @@ -550,11 +616,13 @@ static void seccomp_send_sigsys(int syscall, int reason)
> #define SECCOMP_LOG_TRACE (1 << 4)
> #define SECCOMP_LOG_LOG (1 << 5)
> #define SECCOMP_LOG_ALLOW (1 << 6)
> +#define SECCOMP_LOG_USER_NOTIF (1 << 7)
>
> static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_PROCESS |
> SECCOMP_LOG_KILL_THREAD |
> SECCOMP_LOG_TRAP |
> SECCOMP_LOG_ERRNO |
> + SECCOMP_LOG_USER_NOTIF |
> SECCOMP_LOG_TRACE |
> SECCOMP_LOG_LOG;
>
> @@ -575,6 +643,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> case SECCOMP_RET_TRACE:
> log = requested && seccomp_actions_logged & SECCOMP_LOG_TRACE;
> break;
> + case SECCOMP_RET_USER_NOTIF:
> + log = requested && seccomp_actions_logged & SECCOMP_LOG_USER_NOTIF;
> + break;
> case SECCOMP_RET_LOG:
> log = seccomp_actions_logged & SECCOMP_LOG_LOG;
> break;
> @@ -646,6 +717,68 @@ void secure_computing_strict(int this_syscall)
> #else
>
> #ifdef CONFIG_SECCOMP_FILTER
> +static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
> +{
> + /*
> + * Note: overflow is ok here, the id just needs to be unique per
> + * filter.
> + */
> + lockdep_assert_held(&filter->notify_lock);
> + return filter->notif->next_id++;
> +}
> +
> +static void seccomp_do_user_notification(int this_syscall,
> + struct seccomp_filter *match,
> + const struct seccomp_data *sd)
> +{
> + int err;
> + long ret = 0;
> + struct seccomp_knotif n = {};
> +
> + mutex_lock(&match->notify_lock);
> + err = -ENOSYS;
> + if (!match->notif)
> + goto out;
> +
> + n.task = current;
> + n.state = SECCOMP_NOTIFY_INIT;
> + n.data = sd;
> + n.id = seccomp_next_notify_id(match);
> + init_completion(&n.ready);
> + list_add(&n.list, &match->notif->notifications);
> +
> + up(&match->notif->request);
> + wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
> + mutex_unlock(&match->notify_lock);
> +
> + /*
> + * This is where we wait for a reply from userspace.
> + */
> + err = wait_for_completion_interruptible(&n.ready);
> + mutex_lock(&match->notify_lock);
> + if (err == 0) {
> + ret = n.val;
> + err = n.error;
> + }
> +
> + /*
> + * Note that it's possible the listener died in between the time when
> + * we were notified of a respons (or a signal) and when we were able to
> + * re-acquire the lock, so only delete from the list if the
> + * notification actually exists.
> + *
> + * Also note that this test is only valid because there's no way to
> + * *reattach* to a notifier right now. If one is added, we'll need to
> + * keep track of the notif itself and make sure they match here.
> + */
> + if (match->notif)
> + list_del(&n.list);
> +out:
> + mutex_unlock(&match->notify_lock);
> + syscall_set_return_value(current, task_pt_regs(current),
> + err, ret);
> +}
> +
> static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> const bool recheck_after_trace)
> {
> @@ -728,6 +861,10 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>
> return 0;
>
> + case SECCOMP_RET_USER_NOTIF:
> + seccomp_do_user_notification(this_syscall, match, sd);
> + goto skip;
> +
> case SECCOMP_RET_LOG:
> seccomp_log(this_syscall, 0, action, true);
> return 0;
> @@ -834,6 +971,263 @@ static long seccomp_set_mode_strict(void)
> }
>
> #ifdef CONFIG_SECCOMP_FILTER
> +static int seccomp_notify_release(struct inode *inode, struct file *file)
> +{
> + struct seccomp_filter *filter = file->private_data;
> + struct seccomp_knotif *knotif;
> +
> + mutex_lock(&filter->notify_lock);
> +
> + /*
> + * If this file is being closed because e.g. the task who owned it
> + * died, let's wake everyone up who was waiting on us.
> + */
> + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> + if (knotif->state == SECCOMP_NOTIFY_REPLIED)
> + continue;
> +
> + knotif->state = SECCOMP_NOTIFY_REPLIED;
> + knotif->error = -ENOSYS;
> + knotif->val = 0;
> +
> + complete(&knotif->ready);
> + }
> +
> + kfree(filter->notif);
> + filter->notif = NULL;
> + mutex_unlock(&filter->notify_lock);
> + __put_seccomp_filter(filter);
> + return 0;
> +}
> +
> +static long seccomp_notify_recv(struct seccomp_filter *filter,
> + void __user *buf)
> +{
> + struct seccomp_knotif *knotif = NULL, *cur;
> + struct seccomp_notif unotif;
> + ssize_t ret;
> +
> + memset(&unotif, 0, sizeof(unotif));
> +
> + ret = down_interruptible(&filter->notif->request);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&filter->notify_lock);
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->state == SECCOMP_NOTIFY_INIT) {
> + knotif = cur;
> + break;
> + }
> + }
> +
> + /*
> + * If we didn't find a notification, it could be that the task was
> + * interrupted by a fatal signal between the time we were woken and
> + * when we were able to acquire the rw lock.
> + */
> + if (!knotif) {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + unotif.id = knotif->id;
> + unotif.pid = task_pid_vnr(knotif->task);
> + unotif.data = *(knotif->data);
> +
> + knotif->state = SECCOMP_NOTIFY_SENT;
> + wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
> + ret = 0;
> +out:
> + mutex_unlock(&filter->notify_lock);
> +
> + if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
> + ret = -EFAULT;
> +
> + /*
> + * Userspace screwed up. To make sure that we keep this
> + * notification alive, let's reset it back to INIT. It
> + * may have died when we released the lock, so we need to make
> + * sure it's still around.
> + */
> + knotif = NULL;
> + mutex_lock(&filter->notify_lock);
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->id == unotif.id) {
> + knotif = cur;
> + break;
> + }
> + }
> +
> + if (knotif) {
> + knotif->state = SECCOMP_NOTIFY_INIT;
> + up(&filter->notif->request);
> + }
> + mutex_unlock(&filter->notify_lock);
> + }
> +
> + return ret;
> +}
> +
> +static long seccomp_notify_send(struct seccomp_filter *filter,
> + void __user *buf)
> +{
> + struct seccomp_notif_resp resp = {};
> + struct seccomp_knotif *knotif = NULL, *cur;
> + long ret;
> +
> + if (copy_from_user(&resp, buf, sizeof(resp)))
> + return -EFAULT;
> +
> + if (resp.flags)
> + return -EINVAL;
> +
> + ret = mutex_lock_interruptible(&filter->notify_lock);
> + if (ret < 0)
> + return ret;
> +
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->id == resp.id) {
> + knotif = cur;
> + break;
> + }
> + }
> +
> + if (!knotif) {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + /* Allow exactly one reply. */
> + if (knotif->state != SECCOMP_NOTIFY_SENT) {
> + ret = -EINPROGRESS;
> + goto out;
> + }
> +
> + ret = 0;
> + knotif->state = SECCOMP_NOTIFY_REPLIED;
> + knotif->error = resp.error;
> + knotif->val = resp.val;
> + complete(&knotif->ready);
> +out:
> + mutex_unlock(&filter->notify_lock);
> + return ret;
> +}
> +
> +static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> + void __user *buf)
> +{
> + struct seccomp_knotif *knotif = NULL;
> + u64 id;
> + long ret;
> +
> + if (copy_from_user(&id, buf, sizeof(id)))
> + return -EFAULT;
> +
> + ret = mutex_lock_interruptible(&filter->notify_lock);
> + if (ret < 0)
> + return ret;
> +
> + ret = -ENOENT;
> + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> + if (knotif->id == id) {
> + if (knotif->state == SECCOMP_NOTIFY_SENT)
> + ret = 0;
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&filter->notify_lock);
> + return ret;
> +}
> +
> +static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct seccomp_filter *filter = file->private_data;
> + void __user *buf = (void __user *)arg;
> +
> + switch (cmd) {
> + case SECCOMP_IOCTL_NOTIF_RECV:
> + return seccomp_notify_recv(filter, buf);
> + case SECCOMP_IOCTL_NOTIF_SEND:
> + return seccomp_notify_send(filter, buf);
> + case SECCOMP_IOCTL_NOTIF_ID_VALID:
> + return seccomp_notify_id_valid(filter, buf);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static __poll_t seccomp_notify_poll(struct file *file,
> + struct poll_table_struct *poll_tab)
> +{
> + struct seccomp_filter *filter = file->private_data;
> + __poll_t ret = 0;
> + struct seccomp_knotif *cur;
> +
> + poll_wait(file, &filter->notif->wqh, poll_tab);
> +
> + ret = mutex_lock_interruptible(&filter->notify_lock);
> + if (ret < 0)
> + return EPOLLERR;
> +
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->state == SECCOMP_NOTIFY_INIT)
> + ret |= EPOLLIN | EPOLLRDNORM;
> + if (cur->state == SECCOMP_NOTIFY_SENT)
> + ret |= EPOLLOUT | EPOLLWRNORM;
> + if ((ret & EPOLLIN) && (ret & EPOLLOUT))
> + break;
> + }
> +
> + mutex_unlock(&filter->notify_lock);
> +
> + return ret;
> +}
> +
> +static const struct file_operations seccomp_notify_ops = {
> + .poll = seccomp_notify_poll,
> + .release = seccomp_notify_release,
> + .unlocked_ioctl = seccomp_notify_ioctl,
> +};
> +
> +static struct file *init_listener(struct seccomp_filter *filter)
> +{
> + struct file *ret = ERR_PTR(-EBUSY);
> + struct seccomp_filter *cur;
> +
> + for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> + if (cur->notif)
> + goto out;
> + }
> +
> + ret = ERR_PTR(-ENOMEM);
> + filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL);
> + if (!filter->notif)
> + goto out;
> +
> + sema_init(&filter->notif->request, 0);
> + filter->notif->next_id = get_random_u64();
> + INIT_LIST_HEAD(&filter->notif->notifications);
> + init_waitqueue_head(&filter->notif->wqh);
> +
> + ret = anon_inode_getfile("seccomp notify", &seccomp_notify_ops,
> + filter, O_RDWR);
> + if (IS_ERR(ret))
> + goto out_notif;
> +
> + /* The file has a reference to it now */
> + __get_seccomp_filter(filter);
> +
> +out_notif:
> + if (IS_ERR(ret))
> + kfree(filter->notif);
> +out:
> + return ret;
> +}
> +
> /**
> * seccomp_set_mode_filter: internal function for setting seccomp filter
> * @flags: flags to change filter behavior
> @@ -853,6 +1247,8 @@ static long seccomp_set_mode_filter(unsigned int flags,
> const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
> struct seccomp_filter *prepared = NULL;
> long ret = -EINVAL;
> + int listener = -1;
> + struct file *listener_f = NULL;
>
> /* Validate flags. */
> if (flags & ~SECCOMP_FILTER_FLAG_MASK)
> @@ -863,13 +1259,28 @@ static long seccomp_set_mode_filter(unsigned int flags,
> if (IS_ERR(prepared))
> return PTR_ERR(prepared);
>
> + if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> + listener = get_unused_fd_flags(O_CLOEXEC);
> + if (listener < 0) {
> + ret = listener;
> + goto out_free;
> + }
> +
> + listener_f = init_listener(prepared);
> + if (IS_ERR(listener_f)) {
> + put_unused_fd(listener);
> + ret = PTR_ERR(listener_f);
> + goto out_free;
> + }
> + }
> +
> /*
> * Make sure we cannot change seccomp or nnp state via TSYNC
> * while another thread is in the middle of calling exec.
> */
> if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
> mutex_lock_killable(¤t->signal->cred_guard_mutex))
> - goto out_free;
> + goto out_put_fd;
>
> spin_lock_irq(¤t->sighand->siglock);
>
> @@ -887,6 +1298,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
> spin_unlock_irq(¤t->sighand->siglock);
> if (flags & SECCOMP_FILTER_FLAG_TSYNC)
> mutex_unlock(¤t->signal->cred_guard_mutex);
> +out_put_fd:
> + if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> + if (ret < 0) {
> + fput(listener_f);
> + put_unused_fd(listener);
> + } else {
> + fd_install(listener, listener_f);
> + ret = listener;
> + }
> + }
> out_free:
> seccomp_filter_free(prepared);
> return ret;
> @@ -911,6 +1332,7 @@ static long seccomp_get_action_avail(const char __user *uaction)
> case SECCOMP_RET_KILL_THREAD:
> case SECCOMP_RET_TRAP:
> case SECCOMP_RET_ERRNO:
> + case SECCOMP_RET_USER_NOTIF:
> case SECCOMP_RET_TRACE:
> case SECCOMP_RET_LOG:
> case SECCOMP_RET_ALLOW:
> @@ -922,6 +1344,20 @@ static long seccomp_get_action_avail(const char __user *uaction)
> return 0;
> }
>
> +static long seccomp_get_notif_sizes(void __user *usizes)
> +{
> + struct seccomp_notif_sizes sizes = {
> + .seccomp_notif = sizeof(struct seccomp_notif),
> + .seccomp_notif_resp = sizeof(struct seccomp_notif_resp),
> + .seccomp_data = sizeof(struct seccomp_data),
> + };
> +
> + if (copy_to_user(usizes, &sizes, sizeof(sizes)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> /* Common entry point for both prctl and syscall. */
> static long do_seccomp(unsigned int op, unsigned int flags,
> void __user *uargs)
> @@ -938,6 +1374,11 @@ static long do_seccomp(unsigned int op, unsigned int flags,
> return -EINVAL;
>
> return seccomp_get_action_avail(uargs);
> + case SECCOMP_GET_NOTIF_SIZES:
> + if (flags != 0)
> + return -EINVAL;
> +
> + return seccomp_get_notif_sizes(uargs);
> default:
> return -EINVAL;
> }
> @@ -1111,6 +1552,7 @@ long seccomp_get_metadata(struct task_struct *task,
> #define SECCOMP_RET_KILL_THREAD_NAME "kill_thread"
> #define SECCOMP_RET_TRAP_NAME "trap"
> #define SECCOMP_RET_ERRNO_NAME "errno"
> +#define SECCOMP_RET_USER_NOTIF_NAME "user_notif"
> #define SECCOMP_RET_TRACE_NAME "trace"
> #define SECCOMP_RET_LOG_NAME "log"
> #define SECCOMP_RET_ALLOW_NAME "allow"
> @@ -1120,6 +1562,7 @@ static const char seccomp_actions_avail[] =
> SECCOMP_RET_KILL_THREAD_NAME " "
> SECCOMP_RET_TRAP_NAME " "
> SECCOMP_RET_ERRNO_NAME " "
> + SECCOMP_RET_USER_NOTIF_NAME " "
> SECCOMP_RET_TRACE_NAME " "
> SECCOMP_RET_LOG_NAME " "
> SECCOMP_RET_ALLOW_NAME;
> @@ -1134,6 +1577,7 @@ static const struct seccomp_log_name seccomp_log_names[] = {
> { SECCOMP_LOG_KILL_THREAD, SECCOMP_RET_KILL_THREAD_NAME },
> { SECCOMP_LOG_TRAP, SECCOMP_RET_TRAP_NAME },
> { SECCOMP_LOG_ERRNO, SECCOMP_RET_ERRNO_NAME },
> + { SECCOMP_LOG_USER_NOTIF, SECCOMP_RET_USER_NOTIF_NAME },
> { SECCOMP_LOG_TRACE, SECCOMP_RET_TRACE_NAME },
> { SECCOMP_LOG_LOG, SECCOMP_RET_LOG_NAME },
> { SECCOMP_LOG_ALLOW, SECCOMP_RET_ALLOW_NAME },
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index e1473234968d..5c9768a1b8cd 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -5,6 +5,7 @@
> * Test code for seccomp bpf.
> */
>
> +#define _GNU_SOURCE
> #include <sys/types.h>
>
> /*
> @@ -40,10 +41,12 @@
> #include <sys/fcntl.h>
> #include <sys/mman.h>
> #include <sys/times.h>
> +#include <sys/socket.h>
> +#include <sys/ioctl.h>
>
> -#define _GNU_SOURCE
> #include <unistd.h>
> #include <sys/syscall.h>
> +#include <poll.h>
>
> #include "../kselftest_harness.h"
>
> @@ -133,6 +136,10 @@ struct seccomp_data {
> #define SECCOMP_GET_ACTION_AVAIL 2
> #endif
>
> +#ifndef SECCOMP_GET_NOTIF_SIZES
> +#define SECCOMP_GET_NOTIF_SIZES 3
> +#endif
> +
> #ifndef SECCOMP_FILTER_FLAG_TSYNC
> #define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> #endif
> @@ -154,6 +161,44 @@ struct seccomp_metadata {
> };
> #endif
>
> +#ifndef SECCOMP_FILTER_FLAG_NEW_LISTENER
> +#define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
> +
> +#define SECCOMP_RET_USER_NOTIF 0x7fc00000U
> +
> +#define SECCOMP_IOC_MAGIC '!'
> +#define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
> +#define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
> +#define SECCOMP_IOW(nr, type) _IOW(SECCOMP_IOC_MAGIC, nr, type)
> +#define SECCOMP_IOWR(nr, type) _IOWR(SECCOMP_IOC_MAGIC, nr, type)
> +
> +/* Flags for seccomp notification fd ioctl. */
> +#define SECCOMP_IOCTL_NOTIF_RECV SECCOMP_IOWR(0, struct seccomp_notif)
> +#define SECCOMP_IOCTL_NOTIF_SEND SECCOMP_IOWR(1, \
> + struct seccomp_notif_resp)
> +#define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64)
> +
> +struct seccomp_notif {
> + __u64 id;
> + __u32 pid;
> + __u32 flags;
> + struct seccomp_data data;
> +};
> +
> +struct seccomp_notif_resp {
> + __u64 id;
> + __s64 val;
> + __s32 error;
> + __u32 flags;
> +};
> +
> +struct seccomp_notif_sizes {
> + __u16 seccomp_notif;
> + __u16 seccomp_notif_resp;
> + __u16 seccomp_data;
> +};
> +#endif
> +
> #ifndef seccomp
> int seccomp(unsigned int op, unsigned int flags, void *args)
> {
> @@ -2077,7 +2122,8 @@ TEST(detect_seccomp_filter_flags)
> {
> unsigned int flags[] = { SECCOMP_FILTER_FLAG_TSYNC,
> SECCOMP_FILTER_FLAG_LOG,
> - SECCOMP_FILTER_FLAG_SPEC_ALLOW };
> + SECCOMP_FILTER_FLAG_SPEC_ALLOW,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER };
> unsigned int flag, all_flags;
> int i;
> long ret;
> @@ -2933,6 +2979,403 @@ TEST(get_metadata)
> ASSERT_EQ(0, kill(pid, SIGKILL));
> }
>
> +static int user_trap_syscall(int nr, unsigned int flags)
> +{
> + struct sock_filter filter[] = {
> + BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
> + offsetof(struct seccomp_data, nr)),
> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
> + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
> + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
> + };
> +
> + struct sock_fprog prog = {
> + .len = (unsigned short)ARRAY_SIZE(filter),
> + .filter = filter,
> + };
> +
> + return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
> +}
> +
> +#define USER_NOTIF_MAGIC 116983961184613L
> +TEST(user_notification_basic)
> +{
> + pid_t pid;
> + long ret;
> + int status, listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> + struct pollfd pollfd;
> +
> + struct sock_filter filter[] = {
> + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> + };
> + struct sock_fprog prog = {
> + .len = (unsigned short)ARRAY_SIZE(filter),
> + .filter = filter,
> + };
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + /* Check that we get -ENOSYS with no listener attached */
> + if (pid == 0) {
> + if (user_trap_syscall(__NR_getpid, 0) < 0)
> + exit(1);
> + ret = syscall(__NR_getpid);
> + exit(ret >= 0 || errno != ENOSYS);
> + }
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +
> + /* Add some no-op filters so for grins. */
> + EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
> + EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
> + EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
> + EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
> +
> + /* Check that the basic notification machinery works */
> + listener = user_trap_syscall(__NR_getpid,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + EXPECT_GE(listener, 0);
> +
> + /* Installing a second listener in the chain should EBUSY */
> + EXPECT_EQ(user_trap_syscall(__NR_getpid,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER),
> + -1);
> + EXPECT_EQ(errno, EBUSY);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + ret = syscall(__NR_getpid);
> + exit(ret != USER_NOTIF_MAGIC);
> + }
> +
> + pollfd.fd = listener;
> + pollfd.events = POLLIN | POLLOUT;
> +
> + EXPECT_GT(poll(&pollfd, 1, -1), 0);
> + EXPECT_EQ(pollfd.revents, POLLIN);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> +
> + pollfd.fd = listener;
> + pollfd.events = POLLIN | POLLOUT;
> +
> + EXPECT_GT(poll(&pollfd, 1, -1), 0);
> + EXPECT_EQ(pollfd.revents, POLLOUT);
> +
> + EXPECT_EQ(req.data.nr, __NR_getpid);
> +
> + resp.id = req.id;
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> +
> + /* check that we make sure flags == 0 */
> + resp.flags = 1;
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
> + EXPECT_EQ(errno, EINVAL);
> +
> + resp.flags = 0;
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST(user_notification_kill_in_middle)
> +{
> + pid_t pid;
> + long ret;
> + int listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> +
> + listener = user_trap_syscall(__NR_getpid,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + EXPECT_GE(listener, 0);
> +
> + /*
> + * Check that nothing bad happens when we kill the task in the middle
> + * of a syscall.
> + */
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + ret = syscall(__NR_getpid);
> + exit(ret != USER_NOTIF_MAGIC);
> + }
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id), 0);
> +
> + EXPECT_EQ(kill(pid, SIGKILL), 0);
> + EXPECT_EQ(waitpid(pid, NULL, 0), pid);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id), -1);
> +
> + resp.id = req.id;
> + ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp);
> + EXPECT_EQ(ret, -1);
> + EXPECT_EQ(errno, ENOENT);
> +}
> +
> +static int handled = -1;
> +
> +static void signal_handler(int signal)
> +{
> + if (write(handled, "c", 1) != 1)
> + perror("write from signal");
> +}
> +
> +TEST(user_notification_signal)
> +{
> + pid_t pid;
> + long ret;
> + int status, listener, sk_pair[2];
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> + char c;
> +
> + ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
> +
> + listener = user_trap_syscall(__NR_gettid,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + EXPECT_GE(listener, 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + close(sk_pair[0]);
> + handled = sk_pair[1];
> + if (signal(SIGUSR1, signal_handler) == SIG_ERR) {
> + perror("signal");
> + exit(1);
> + }
> + /*
> + * ERESTARTSYS behavior is a bit hard to test, because we need
> + * to rely on a signal that has not yet been handled. Let's at
> + * least check that the error code gets propagated through, and
> + * hope that it doesn't break when there is actually a signal :)
> + */
> + ret = syscall(__NR_gettid);
> + exit(!(ret == -1 && errno == 512));
> + }
> +
> + close(sk_pair[1]);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> +
> + EXPECT_EQ(kill(pid, SIGUSR1), 0);
> +
> + /*
> + * Make sure the signal really is delivered, which means we're not
> + * stuck in the user notification code any more and the notification
> + * should be dead.
> + */
> + EXPECT_EQ(read(sk_pair[0], &c, 1), 1);
> +
> + resp.id = req.id;
> + resp.error = -EPERM;
> + resp.val = 0;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
> + EXPECT_EQ(errno, ENOENT);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> +
> + resp.id = req.id;
> + resp.error = -512; /* -ERESTARTSYS */
> + resp.val = 0;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST(user_notification_closed_listener)
> +{
> + pid_t pid;
> + long ret;
> + int status, listener;
> +
> + listener = user_trap_syscall(__NR_getpid,
> + SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + EXPECT_GE(listener, 0);
> +
> + /*
> + * Check that we get an ENOSYS when the listener is closed.
> + */
> + pid = fork();
> + ASSERT_GE(pid, 0);
> + if (pid == 0) {
> + close(listener);
> + ret = syscall(__NR_getpid);
> + exit(ret != -1 && errno != ENOSYS);
> + }
> +
> + close(listener);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +/*
> + * Check that a pid in a child namespace still shows up as valid in ours.
> + */
> +TEST(user_notification_child_pid_ns)
> +{
> + pid_t pid;
> + int status, listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> +
> + ASSERT_EQ(unshare(CLONE_NEWPID), 0);
> +
> + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + ASSERT_GE(listener, 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0)
> + exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> + EXPECT_EQ(req.pid, pid);
> +
> + resp.id = req.id;
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> + close(listener);
> +}
> +
> +/*
> + * Check that a pid in a sibling (i.e. unrelated) namespace shows up as 0, i.e.
> + * invalid.
> + */
> +TEST(user_notification_sibling_pid_ns)
> +{
> + pid_t pid, pid2;
> + int status, listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> +
> + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + ASSERT_GE(listener, 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + ASSERT_EQ(unshare(CLONE_NEWPID), 0);
> +
> + pid2 = fork();
> + ASSERT_GE(pid2, 0);
> +
> + if (pid2 == 0)
> + exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
> +
> + EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> + exit(WEXITSTATUS(status));
> + }
> +
> + /* Create the sibling ns, and sibling in it. */
> + EXPECT_EQ(unshare(CLONE_NEWPID), 0);
> + EXPECT_EQ(errno, 0);
> +
> + pid2 = fork();
> + EXPECT_GE(pid2, 0);
> +
> + if (pid2 == 0) {
> + ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> + /*
> + * The pid should be 0, i.e. the task is in some namespace that
> + * we can't "see".
> + */
> + ASSERT_EQ(req.pid, 0);
> +
> + resp.id = req.id;
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> +
> + ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> + exit(0);
> + }
> +
> + close(listener);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +
> + EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST(user_notification_fault_recv)
> +{
> + pid_t pid;
> + int status, listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> +
> + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + ASSERT_GE(listener, 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0)
> + exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
> +
> + /* Do a bad recv() */
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, NULL), -1);
> + EXPECT_EQ(errno, EFAULT);
> +
> + /* We should still be able to receive this notification, though. */
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> + EXPECT_EQ(req.pid, pid);
> +
> + resp.id = req.id;
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST(seccomp_get_notif_sizes)
> +{
> + struct seccomp_notif_sizes sizes;
> +
> + EXPECT_EQ(seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes), 0);
> + EXPECT_EQ(sizes.seccomp_notif, sizeof(struct seccomp_notif));
> + EXPECT_EQ(sizes.seccomp_notif_resp, sizeof(struct seccomp_notif_resp));
> +}
> +
> /*
> * TODO:
> * - add microbenchmarks
> --
> 2.19.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v10 2/4] seccomp: switch system call argument type to void *
From: Kees Cook @ 2018-12-12 0:31 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W. Biederman,
Serge E. Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, Jann Horn, LKML, Linux Containers, Linux API
In-Reply-To: <20181209182414.30862-3-tycho@tycho.ws>
On Sun, Dec 9, 2018 at 10:24 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> The const qualifier causes problems for any code that wants to write to the
> third argument of the seccomp syscall, as we will do in a future patch in
> this series.
>
> The third argument to the seccomp syscall is documented as void *, so
> rather than just dropping the const, let's switch everything to use void *
> as well.
>
> I believe this is safe because of 1. the documentation above, 2. there's no
> real type information exported about syscalls anywhere besides the man
> pages.
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Yeah, this can be void *, as you've found. I think it was just an
artifact of the old filter code to use const char *.
Applied for -next.
-Kees
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> CC: Christian Brauner <christian@brauner.io>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> ---
> v10: change type in include/linux/syscalls.h too to avoid compilation error
> in the !CONFIG_ARCH_HAS_SYSCALL_WRAPPER case
> ---
> include/linux/seccomp.h | 2 +-
> include/linux/syscalls.h | 2 +-
> kernel/seccomp.c | 8 ++++----
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index e5320f6c8654..b5103c019cf4 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -43,7 +43,7 @@ extern void secure_computing_strict(int this_syscall);
> #endif
>
> extern long prctl_get_seccomp(void);
> -extern long prctl_set_seccomp(unsigned long, char __user *);
> +extern long prctl_set_seccomp(unsigned long, void __user *);
>
> static inline int seccomp_mode(struct seccomp *s)
> {
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..a60694fb0f58 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -879,7 +879,7 @@ asmlinkage long sys_renameat2(int olddfd, const char __user *oldname,
> int newdfd, const char __user *newname,
> unsigned int flags);
> asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
> - const char __user *uargs);
> + void __user *uargs);
> asmlinkage long sys_getrandom(char __user *buf, size_t count,
> unsigned int flags);
> asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags);
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 96afc32e041d..393e029f778a 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -924,7 +924,7 @@ static long seccomp_get_action_avail(const char __user *uaction)
>
> /* Common entry point for both prctl and syscall. */
> static long do_seccomp(unsigned int op, unsigned int flags,
> - const char __user *uargs)
> + void __user *uargs)
> {
> switch (op) {
> case SECCOMP_SET_MODE_STRICT:
> @@ -944,7 +944,7 @@ static long do_seccomp(unsigned int op, unsigned int flags,
> }
>
> SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> - const char __user *, uargs)
> + void __user *, uargs)
> {
> return do_seccomp(op, flags, uargs);
> }
> @@ -956,10 +956,10 @@ SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> *
> * Returns 0 on success or -EINVAL on failure.
> */
> -long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> +long prctl_set_seccomp(unsigned long seccomp_mode, void __user *filter)
> {
> unsigned int op;
> - char __user *uargs;
> + void __user *uargs;
>
> switch (seccomp_mode) {
> case SECCOMP_MODE_STRICT:
> --
> 2.19.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v10 1/4] seccomp: hoist struct seccomp_data recalculation higher
From: Kees Cook @ 2018-12-12 0:30 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W. Biederman,
Serge E. Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, Jann Horn, LKML, Linux Containers, Linux API
In-Reply-To: <20181209182414.30862-2-tycho@tycho.ws>
On Sun, Dec 9, 2018 at 10:24 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> In the next patch, we're going to use the sd pointer passed to
> __seccomp_filter() as the data to pass to userspace. Except that in some
> cases (__seccomp_filter(SECCOMP_RET_TRACE), emulate_vsyscall(), every time
> seccomp is inovked on power, etc.) the sd pointer will be NULL in order to
> force seccomp to recompute the register data. Previously this recomputation
> happened one level lower, in seccomp_run_filters(); this patch just moves
> it up a level higher to __seccomp_filter().
>
> Thanks Oleg for spotting this.
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
This is fine. :) Applied for -next.
-Kees
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> CC: Christian Brauner <christian@brauner.io>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> ---
> kernel/seccomp.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f2ae2324c232..96afc32e041d 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -188,7 +188,6 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
> static u32 seccomp_run_filters(const struct seccomp_data *sd,
> struct seccomp_filter **match)
> {
> - struct seccomp_data sd_local;
> u32 ret = SECCOMP_RET_ALLOW;
> /* Make sure cross-thread synced filter points somewhere sane. */
> struct seccomp_filter *f =
> @@ -198,11 +197,6 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
> if (WARN_ON(f == NULL))
> return SECCOMP_RET_KILL_PROCESS;
>
> - if (!sd) {
> - populate_seccomp_data(&sd_local);
> - sd = &sd_local;
> - }
> -
> /*
> * All filters in the list are evaluated and the lowest BPF return
> * value always takes priority (ignoring the DATA).
> @@ -658,6 +652,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> u32 filter_ret, action;
> struct seccomp_filter *match = NULL;
> int data;
> + struct seccomp_data sd_local;
>
> /*
> * Make sure that any changes to mode from another thread have
> @@ -665,6 +660,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> */
> rmb();
>
> + if (!sd) {
> + populate_seccomp_data(&sd_local);
> + sd = &sd_local;
> + }
> +
> filter_ret = seccomp_run_filters(sd, &match);
> data = filter_ret & SECCOMP_RET_DATA;
> action = filter_ret & SECCOMP_RET_ACTION_FULL;
> --
> 2.19.1
>
--
Kees Cook
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Arnd Bergmann @ 2018-12-11 23:55 UTC (permalink / raw)
To: tg
Cc: Andy Lutomirski, Linus Torvalds, the arch/x86 maintainers,
Linux Kernel Mailing List, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, vapier, H.J. Lu,
Rich Felker, x32, Will Deacon, Catalin Marinas
In-Reply-To: <Pine.BSM.4.64L.1812112327500.21176@herc.mirbsd.org>
On Wed, Dec 12, 2018 at 12:38 AM Thorsten Glaser <tg@mirbsd.de> wrote:
>
> Andy Lutomirski dixit:
>
> >What happens if someone adds a struct like:
> >
> >struct nasty_on_x32 {
> > __kernel_long_t a;
> > void * __user b;
> >};
> >
> >On x86_64, that's two 8-byte fields. On x86_32, it's two four-byte
> >fields. On x32, it's an 8-byte field and a 4-byte field. Now what?
>
> Yes, that’s indeed ugly. I understand. But don’t we already have
> this problem with architectures which support multiple ABIs at the
> same time? An amd64 kernel with i386 userspace comes to mind, or
> the multiple MIPS ABIs.
The main trouble that I see with x32 is in how it's different
from all the other compat architectures. We have a number
of drivers that support not only two ABIs (native and compat)
but also a third one specifically for x32, and more drivers
that fail on x32 because they never implemented the third
case.
Note that this is not a problem for MIPS n32 and aarch64ilp32,
which generally stick to the regular compat ABI (each compat
architecture has some minor quirks somewhere, much less so
than x32 does).
> >actually do this work. Instead we get ad hoc fixes for each syscall,
> >along the lines of preadv64v2(), which get done when somebody notices
>
> Yes, that’s absolutely ugly and ridiculous and all kinds of bad.
>
> On the other hand, from my current experience, someone (Arnd?) noticed
> all the currently existing baddies for x32 already and fixed them.
I did some, and others did more, but it's hard to ever do a complete
job, and even harder to prevent new bugs from creeping in.
> New syscalls are indeed an issue, but perhaps something generating
> copyinout stubs could help. This might allow other architectures
> that could do with a new ABI but have until now feared the overhead
> as well. (IIRC, m68k could do with a new ABI that reserves a register
> for TLS, but Geert would know. At the same time, time_t and off_t could
> be bumped to 64 bit. Something like that. If changing sizes of types
> shared between kernel and user spaces is not something feared…)
Creating a completely new ABI for an existing architecutre is generally
something you want to avoid, as this means all existing user space
has to be recompiled.
All architectures support 64-bit off_t already, and soon will support
64-bit time_t as well, at this point (as of this week's linux-next)
we mostly just need to assign a couple of syscall numbers for each
32-bit architectures.
Arnd
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Maciej W. Rozycki @ 2018-12-11 23:40 UTC (permalink / raw)
To: Thorsten Glaser
Cc: Linus Torvalds, Andrew Lutomirski, the arch/x86 maintainers,
Linux List Kernel Mailing, linux-api, Peter Anvin, Peter Zijlstra,
bp, fweimer, vapier, hjl.tools, dalias, x32, Arnd Bergmann,
Will Deacon, Catalin Marinas
In-Reply-To: <Pine.BSM.4.64L.1812112150480.21176@herc.mirbsd.org>
On Tue, 11 Dec 2018, Thorsten Glaser wrote:
> Furthermore, x32 was the first of the many *64ilp32
> architectures; I know I’ve seen amd64ilp32 and at least
> one other I don’t recall.
Maybe you mean n32 MIPS, complete kernel+userland support for which has
been available in Linux from 2001?
Maciej
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Rich Felker @ 2018-12-11 23:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: tg, Linus Torvalds, X86 ML, LKML, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, Mike Frysinger,
H. J. Lu, x32, Arnd Bergmann, Will Deacon, Catalin Marinas
In-Reply-To: <CALCETrWgpAX7FV23zHmid83SsgnwFMKD4a_-xSEgB6v0kJR5sA@mail.gmail.com>
On Tue, Dec 11, 2018 at 03:22:43PM -0800, Andy Lutomirski wrote:
> On Tue, Dec 11, 2018 at 2:14 PM Thorsten Glaser <tg@mirbsd.de> wrote:
> > I was also considering investing a nontrivial amount of
> > work into porting klibc to x32, since hpa does not wish
> > to do it himself. Thankfully I have only done a bit yet.
> >
> > Furthermore, x32 was the first of the many *64ilp32
> > architectures; I know I’ve seen amd64ilp32 and at least
> > one other I don’t recall. It will have prototyped many
> > of the problems users of these will run in, and I’d prefer
> > to keep it (completely selfish because I don’t wish to
> > have to crossgrade a whole system yet again).
>
> it kind of seems like arm64's lesson is "don't do it like x32".
>
> There's some effort going on right now to make it possible to add
> syscalls without having to muck with every single architecture. I
> don't really want x32 to derail that effort. I suppose we could say
> that x32 stays but that it simply gets no new syscalls, but that seems
> a bit lame. Unfortunately, on x86, x32 really is a third ABI that is
> not compatible in a structure-memory-layout sense with the other two.
> What happens if someone adds a struct like:
>
> struct nasty_on_x32 {
> __kernel_long_t a;
> void * __user b;
> };
>
> On x86_64, that's two 8-byte fields. On x86_32, it's two four-byte
> fields. On x32, it's an 8-byte field and a 4-byte field. Now what?
>
> I'm sure we could have some magic gcc plugin or other nifty tool that gives us:
>
> copy_from_user(struct struct_name, kernel_ptr, user_ptr);
>
> where it automatically generates code for all possible ABIs to copy
> over the struct and dispatches dynamically based on the current
> syscall ABI, but I have trouble imagining anyone volunteering to
> actually do this work. Instead we get ad hoc fixes for each syscall,
> along the lines of preadv64v2(), which get done when somebody notices
> a problem.
For what it's worth, this is exactly what I plan to do if/when I ever
get to doing my kernel with multiple/universal userspace ABI support:
pure-data markup for the data structures (and syscall argument
arrangements) userspace is speaking, with generic marshalling code.
But I don't think it's easy to retrofit to Linux, and probably
wouldn't be welcome there.
Rich
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Thorsten Glaser @ 2018-12-11 23:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, X86 ML, LKML, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, Mike Frysinger,
H. J. Lu, Rich Felker, x32, Arnd Bergmann, Will Deacon,
Catalin Marinas
In-Reply-To: <CALCETrWgpAX7FV23zHmid83SsgnwFMKD4a_-xSEgB6v0kJR5sA@mail.gmail.com>
Andy Lutomirski dixit:
>What happens if someone adds a struct like:
>
>struct nasty_on_x32 {
> __kernel_long_t a;
> void * __user b;
>};
>
>On x86_64, that's two 8-byte fields. On x86_32, it's two four-byte
>fields. On x32, it's an 8-byte field and a 4-byte field. Now what?
Yes, that’s indeed ugly. I understand. But don’t we already have
this problem with architectures which support multiple ABIs at the
same time? An amd64 kernel with i386 userspace comes to mind, or
the multiple MIPS ABIs.
>I'm sure we could have some magic gcc plugin or other nifty tool that
>gives us:
>
>copy_from_user(struct struct_name, kernel_ptr, user_ptr);
Something like that might be useful. Generate call stubs, which
then call the syscall implementation with the actual user-space
struct contents as arguments. Hm, that might be too generic to
be useful. Generate macros that can read from or write specific
structures to userspace?
I think something like this could solve other more general problems
as well, so it might be “nice to have anyway”. Of course it’s work,
and I’m not involved enough in Linux kernel programming to be able
to usefully help with it (doing too much elsewhere already).
>actually do this work. Instead we get ad hoc fixes for each syscall,
>along the lines of preadv64v2(), which get done when somebody notices
Yes, that’s absolutely ugly and ridiculous and all kinds of bad.
On the other hand, from my current experience, someone (Arnd?) noticed
all the currently existing baddies for x32 already and fixed them.
New syscalls are indeed an issue, but perhaps something generating
copyinout stubs could help. This might allow other architectures
that could do with a new ABI but have until now feared the overhead
as well. (IIRC, m68k could do with a new ABI that reserves a register
for TLS, but Geert would know. At the same time, time_t and off_t could
be bumped to 64 bit. Something like that. If changing sizes of types
shared between kernel and user spaces is not something feared…)
Thanks for considering,
//mirabilos
--
„Cool, /usr/share/doc/mksh/examples/uhr.gz ist ja ein Grund,
mksh auf jedem System zu installieren.“
-- XTaran auf der OpenRheinRuhr, ganz begeistert
(EN: “[…]uhr.gz is a reason to install mksh on every system.”)
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Rich Felker @ 2018-12-11 23:33 UTC (permalink / raw)
To: Thorsten Glaser
Cc: John Paul Adrian Glaubitz, Andy Lutomirski, X86 ML, LKML,
Linux API, H. Peter Anvin, Peter Zijlstra, Borislav Petkov,
Florian Weimer, Mike Frysinger, H. J. Lu, x32, Arnd Bergmann,
Will Deacon, Catalin Marinas, Linus Torvalds
In-Reply-To: <Pine.BSM.4.64L.1812112155170.21176@herc.mirbsd.org>
On Tue, Dec 11, 2018 at 09:59:48PM +0000, Thorsten Glaser wrote:
> John Paul Adrian Glaubitz dixit:
>
> >I can't say anything about the syscall interface. However, what I do know
> >is that the weird combination of a 32-bit userland with a 64-bit kernel
> >interface is sometimes causing issues. For example, application code usually
>
> Yes, but more and more ${foo}64ilp32 architectures are popping up.
>
> >Additionally, x32 support in many applications is either rudimentary
>
> If a signal is sent that this kind of architectures will stay, some
> people might be convinced to fix that.
>
> >It's also that the performance benefits of x32 are often eaten up by
> >the fact that none of the scripted languages that I know of provide
>
> Non-JITted languages like yours truly’s shell do benefit from it,
> though. (mksh works just fine on LP64 but its internal structures
> pack massively better on ILP32, for example.)
gcc also benefits massively, up to 2x or more performance improvement
from ILP32, just because you can double your -jN to make. The "or
more" comes from cache utilization also improving. Actually I often
use i486 gcc even on 64-bit systems for this reason, and have observed
this order of improvement despite the lack of registers and worse call
ABI. Presumably x32 would be better, but I've avoided it because of
how buggy it's been in the past.
> >If x32 is eventually to be removed, we should also take care of removing
> >x32 support from userland code. From the top of my head, this would at least
>
> I don’t think so. The patches also contain
> – stuff to support 64-bit time_t on 32-bit architectures, e.g:
> - bugfixes like printf("%lld", (long long)timet_value) instead
> of assuming time_t fits into a long (also important for other
> operating systems…)
> - generally switching from generic types like long to specific
> types like size_t, ptrdiff_t, etc.
> - there was one more but after having written two eMails I forgot it
> - oh and, of course, they lay the base for e.g. amd64ilp32 support
I would not classify any of that as part of "removing x32 support",
but rather as "reintroducing broken assumptions". I think "removing
x32 support" was intended to mean places where there's
explicitly-x32-specific code or tooling, like asm functions/fragments
in libc, x32 versions of the dispatch asm in mesa, x32 code
generation, etc. Obviously (at least I hope it's obvious) nonportable
assumptions should not be reintroduced regardless of whether x32 is
dropped.
FWIW x32 is nice as a test case for catching such nonportable
assumptions and getting them fixed.
Rich
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox