* [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-09-27 15:11 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel, Tycho Andersen
In-Reply-To: <20180927151119.9989-1-tycho@tycho.ws>
As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
version which can acquire filters is useful. There are at least two reasons
this is preferable, even though it uses ptrace:
1. You can control tasks that aren't cooperating with you
2. You can control tasks whose filters block sendmsg() and socket(); if the
task installs a filter which blocks these calls, there's no way with
SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.
v2: fix a bug where listener mode was not unset when an unused fd was not
available
v3: fix refcounting bug (Oleg)
v4: * change the listener's fd flags to be 0
* rename GET_LISTENER to NEW_LISTENER (Matthew)
v5: * add capable(CAP_SYS_ADMIN) requirement
v7: * point the new listener at the right filter (Jann)
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@ubuntu.com>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
include/linux/seccomp.h | 7 ++
include/uapi/linux/ptrace.h | 2 +
kernel/ptrace.c | 4 ++
kernel/seccomp.c | 31 +++++++++
tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
5 files changed, 112 insertions(+)
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 017444b5efed..234c61b37405 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
#ifdef CONFIG_SECCOMP_FILTER
extern void put_seccomp_filter(struct task_struct *tsk);
extern void get_seccomp_filter(struct task_struct *tsk);
+extern long seccomp_new_listener(struct task_struct *task,
+ unsigned long filter_off);
#else /* CONFIG_SECCOMP_FILTER */
static inline void put_seccomp_filter(struct task_struct *tsk)
{
@@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
{
return;
}
+static inline long seccomp_new_listener(struct task_struct *task,
+ unsigned long filter_off)
+{
+ return -EINVAL;
+}
#endif /* CONFIG_SECCOMP_FILTER */
#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..e80ecb1bd427 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,8 @@ struct seccomp_metadata {
__u64 flags; /* Output: filter's flags */
};
+#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
+
/* Read signals from a shared (process wide) queue */
#define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 21fec73d45d4..289960ac181b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
ret = seccomp_get_metadata(child, addr, datavp);
break;
+ case PTRACE_SECCOMP_NEW_LISTENER:
+ ret = seccomp_new_listener(child, addr);
+ break;
+
default:
break;
}
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 44a31ac8373a..17685803a2af 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
return ret;
}
+
+long seccomp_new_listener(struct task_struct *task,
+ unsigned long filter_off)
+{
+ struct seccomp_filter *filter;
+ struct file *listener;
+ int fd;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ filter = get_nth_filter(task, filter_off);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);
+
+ fd = get_unused_fd_flags(0);
+ if (fd < 0) {
+ __put_seccomp_filter(filter);
+ return fd;
+ }
+
+ listener = init_listener(task, filter);
+ __put_seccomp_filter(filter);
+ if (IS_ERR(listener)) {
+ put_unused_fd(fd);
+ return PTR_ERR(listener);
+ }
+
+ fd_install(fd, listener);
+ return fd;
+}
#endif
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 5f4b836a6792..c6ba3ed5392e 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -193,6 +193,10 @@ int seccomp(unsigned int op, unsigned int flags, void *args)
}
#endif
+#ifndef PTRACE_SECCOMP_NEW_LISTENER
+#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
+#endif
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
#define syscall_arg(_n) (offsetof(struct seccomp_data, args[_n]))
#elif __BYTE_ORDER == __BIG_ENDIAN
@@ -3175,6 +3179,70 @@ TEST(get_user_notification_syscall)
EXPECT_EQ(0, WEXITSTATUS(status));
}
+TEST(get_user_notification_ptrace)
+{
+ pid_t pid;
+ int status, listener;
+ int sk_pair[2];
+ char c;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ EXPECT_EQ(user_trap_syscall(__NR_getpid, 0), 0);
+
+ /* Test that we get ENOSYS while not attached */
+ EXPECT_EQ(syscall(__NR_getpid), -1);
+ EXPECT_EQ(errno, ENOSYS);
+
+ /* Signal we're ready and have installed the filter. */
+ EXPECT_EQ(write(sk_pair[1], "J", 1), 1);
+
+ EXPECT_EQ(read(sk_pair[1], &c, 1), 1);
+ EXPECT_EQ(c, 'H');
+
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+ }
+
+ EXPECT_EQ(read(sk_pair[0], &c, 1), 1);
+ EXPECT_EQ(c, 'J');
+
+ EXPECT_EQ(ptrace(PTRACE_ATTACH, pid), 0);
+ EXPECT_EQ(waitpid(pid, NULL, 0), pid);
+ listener = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0);
+ EXPECT_GE(listener, 0);
+
+ /* EBUSY for second listener */
+ EXPECT_EQ(ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0), -1);
+ EXPECT_EQ(errno, EBUSY);
+
+ EXPECT_EQ(ptrace(PTRACE_DETACH, pid, NULL, 0), 0);
+
+ /* Now signal we are done and respond with magic */
+ EXPECT_EQ(write(sk_pair[0], "H", 1), 1);
+
+ req.len = sizeof(req);
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+
+ 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 child namespace still shows up as valid in ours.
*/
--
2.17.1
^ permalink raw reply related
* [PATCH v7 2/6] seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE
From: Tycho Andersen @ 2018-09-27 15:11 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel, Tycho Andersen
In-Reply-To: <20180927151119.9989-1-tycho@tycho.ws>
In the next commit we'll use this same mnemonic to get a listener for the
nth filter, so we need it available outside of CHECKPOINT_RESTORE in the
USER_NOTIFICATION case as well.
v2: new in v2
v3: no changes
v4: no changes
v5: switch to CHECKPOINT_RESTORE || USER_NOTIFICATION to avoid warning when
only CONFIG_SECCOMP_FILTER is enabled.
v7: drop USER_NOTIFICATION bits
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@ubuntu.com>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
kernel/seccomp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index fa6fe9756c80..44a31ac8373a 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1158,7 +1158,7 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
return do_seccomp(op, 0, uargs);
}
-#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
+#if defined(CONFIG_SECCOMP_FILTER)
static struct seccomp_filter *get_nth_filter(struct task_struct *task,
unsigned long filter_off)
{
@@ -1205,6 +1205,7 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
return filter;
}
+#if defined(CONFIG_CHECKPOINT_RESTORE)
long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
void __user *data)
{
@@ -1277,7 +1278,8 @@ long seccomp_get_metadata(struct task_struct *task,
__put_seccomp_filter(filter);
return ret;
}
-#endif
+#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_SECCOMP_FILTER */
#ifdef CONFIG_SYSCTL
--
2.17.1
^ permalink raw reply related
* [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-09-27 15:11 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel, Tycho Andersen
In-Reply-To: <20180927151119.9989-1-tycho@tycho.ws>
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 mknod(), since this checks
capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
/dev/zero should be ok for containers to mknod, but we'd like to avoid hard
coding some whitelist in the kernel. Another example is mount(), which has
many security restrictions for good reason, but configuration or runtime
knowledge could potentially be used to relax these restrictions.
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 start services.
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.
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)
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@ubuntu.com>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
Documentation/ioctl/ioctl-number.txt | 1 +
.../userspace-api/seccomp_filter.rst | 73 +++
include/linux/seccomp.h | 7 +-
include/uapi/linux/seccomp.h | 33 +-
kernel/seccomp.c | 436 +++++++++++++++++-
tools/testing/selftests/seccomp/seccomp_bpf.c | 413 ++++++++++++++++-
6 files changed, 954 insertions(+), 9 deletions(-)
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 13a7c999c04a..31e9707f7e06 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -345,4 +345,5 @@ Code Seq#(hex) Include File Comments
<mailto:raph@8d.com>
0xF6 all LTTng Linux Trace Toolkit Next Generation
<mailto:mathieu.desnoyers@efficios.com>
+0xF7 00-1F uapi/linux/seccomp.h
0xFD all linux/dm-ioctl.h
diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index 82a468bc7560..d2e61f1c0a0b 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,74 @@ 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.
+
+There are currently two APIs to acquire a userspace notification fd for a
+particular filter. The first is when the filter is installed, the task
+installing the filter can ask the ``seccomp()`` syscall:
+
+.. code-block::
+
+ 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. Alternatively, a filter fd can be
+acquired via:
+
+.. code-block::
+
+ fd = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0);
+
+which grabs the 0th filter for some task which the tracer has privilege over.
+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::
+
+ struct seccomp_notif {
+ __u16 len;
+ __u64 id;
+ pid_t pid;
+ __u8 signalled;
+ struct seccomp_data data;
+ };
+
+ struct seccomp_notif_resp {
+ __u16 len;
+ __u64 id;
+ __s32 error;
+ __s64 val;
+ };
+
+Users can read via ``ioctl(SECCOMP_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 flag 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_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/map_files/``. 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 e5320f6c8654..017444b5efed 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..d4ccb32fe089 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -17,9 +17,10 @@
#define SECCOMP_GET_ACTION_AVAIL 2
/* 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 +36,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 +62,29 @@ struct seccomp_data {
__u64 args[6];
};
+struct seccomp_notif {
+ __u16 len;
+ __u64 id;
+ __u32 pid;
+ __u8 signaled;
+ struct seccomp_data data;
+};
+
+struct seccomp_notif_resp {
+ __u16 len;
+ __u64 id;
+ __s32 error;
+ __s64 val;
+};
+
+#define SECCOMP_IOC_MAGIC 0xF7
+
+/* Flags for seccomp notification fd ioctl. */
+#define SECCOMP_NOTIF_RECV _IOWR(SECCOMP_IOC_MAGIC, 0, \
+ struct seccomp_notif)
+#define SECCOMP_NOTIF_SEND _IOWR(SECCOMP_IOC_MAGIC, 1, \
+ struct seccomp_notif_resp)
+#define SECCOMP_NOTIF_ID_VALID _IOR(SECCOMP_IOC_MAGIC, 2, \
+ __u64)
+
#endif /* _UAPI_LINUX_SECCOMP_H */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index fd023ac24e10..fa6fe9756c80 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -33,12 +33,78 @@
#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;
+
+ /* Whether or not this task has been given an interruptible signal. */
+ bool signaled;
+
+ /*
+ * 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.
+ * @notify_lock: A lock for all notification-related accesses.
+ * @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
@@ -66,6 +132,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. */
@@ -392,6 +460,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) {
@@ -556,11 +625,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;
@@ -581,6 +652,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;
@@ -652,6 +726,73 @@ 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 */
+ 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);
+ wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
+
+ mutex_unlock(&match->notify_lock);
+ up(&match->notif->request);
+
+ err = wait_for_completion_interruptible(&n.ready);
+ mutex_lock(&match->notify_lock);
+
+ /*
+ * Here it's possible we got a signal and then had to wait on the mutex
+ * while the reply was sent, so let's be sure there wasn't a response
+ * in the meantime.
+ */
+ if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
+ /*
+ * We got a signal. Let's tell userspace about it (potentially
+ * again, if we had already notified them about the first one).
+ */
+ n.signaled = true;
+ if (n.state == SECCOMP_NOTIFY_SENT) {
+ n.state = SECCOMP_NOTIFY_INIT;
+ up(&match->notif->request);
+ }
+ mutex_unlock(&match->notify_lock);
+ err = wait_for_completion_killable(&n.ready);
+ mutex_lock(&match->notify_lock);
+ if (err < 0)
+ goto remove_list;
+ }
+
+ ret = n.val;
+ err = n.error;
+
+remove_list:
+ 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 +869,9 @@ 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 +978,9 @@ static long seccomp_set_mode_strict(void)
}
#ifdef CONFIG_SECCOMP_FILTER
+static struct file *init_listener(struct task_struct *,
+ struct seccomp_filter *);
+
/**
* seccomp_set_mode_filter: internal function for setting seccomp filter
* @flags: flags to change filter behavior
@@ -853,6 +1000,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 = 0;
+ struct file *listener_f = NULL;
/* Validate flags. */
if (flags & ~SECCOMP_FILTER_FLAG_MASK)
@@ -863,13 +1012,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(0);
+ if (listener < 0) {
+ ret = listener;
+ goto out_free;
+ }
+
+ listener_f = init_listener(current, 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 +1051,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 +1085,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:
@@ -1111,6 +1286,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 +1296,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 +1311,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 },
@@ -1342,3 +1520,259 @@ static int __init seccomp_sysctl_init(void)
device_initcall(seccomp_sysctl_init)
#endif /* CONFIG_SYSCTL */
+
+#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);
+ }
+
+ wake_up_all(&filter->notif->wqh);
+ 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,
+ unsigned long arg)
+{
+ struct seccomp_knotif *knotif = NULL, *cur;
+ struct seccomp_notif unotif = {};
+ ssize_t ret;
+ u16 size;
+ void __user *buf = (void __user *)arg;
+
+ if (copy_from_user(&size, buf, sizeof(size)))
+ return -EFAULT;
+
+ 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 between the time we were woken and when we were able to
+ * acquire the rw lock.
+ */
+ if (!knotif) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ size = min_t(size_t, size, sizeof(unotif));
+
+ unotif.len = size;
+ unotif.id = knotif->id;
+ unotif.pid = task_pid_vnr(knotif->task);
+ unotif.signaled = knotif->signaled;
+ unotif.data = *(knotif->data);
+
+ if (copy_to_user(buf, &unotif, size)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = size;
+ knotif->state = SECCOMP_NOTIFY_SENT;
+ wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
+
+
+out:
+ mutex_unlock(&filter->notify_lock);
+ return ret;
+}
+
+static long seccomp_notify_send(struct seccomp_filter *filter,
+ unsigned long arg)
+{
+ struct seccomp_notif_resp resp = {};
+ struct seccomp_knotif *knotif = NULL;
+ long ret;
+ u16 size;
+ void __user *buf = (void __user *)arg;
+
+ if (copy_from_user(&size, buf, sizeof(size)))
+ return -EFAULT;
+ size = min_t(size_t, size, sizeof(resp));
+ if (copy_from_user(&resp, buf, size))
+ return -EFAULT;
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ return ret;
+
+ list_for_each_entry(knotif, &filter->notif->notifications, list) {
+ if (knotif->id == resp.id)
+ break;
+ }
+
+ if (!knotif || knotif->id != resp.id) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ /* Allow exactly one reply. */
+ if (knotif->state != SECCOMP_NOTIFY_SENT) {
+ ret = -EINPROGRESS;
+ goto out;
+ }
+
+ ret = size;
+ 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,
+ unsigned long arg)
+{
+ struct seccomp_knotif *knotif = NULL;
+ void __user *buf = (void __user *)arg;
+ 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 = -1;
+ list_for_each_entry(knotif, &filter->notif->notifications, list) {
+ if (knotif->id == id) {
+ 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;
+
+ switch (cmd) {
+ case SECCOMP_NOTIF_RECV:
+ return seccomp_notify_recv(filter, arg);
+ case SECCOMP_NOTIF_SEND:
+ return seccomp_notify_send(filter, arg);
+ case SECCOMP_NOTIF_ID_VALID:
+ return seccomp_notify_id_valid(filter, arg);
+ 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 ret;
+
+ 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 task_struct *task,
+ struct seccomp_filter *filter)
+{
+ struct file *ret = ERR_PTR(-EBUSY);
+ struct seccomp_filter *cur, *last_locked = NULL;
+ int filter_nesting = 0;
+
+ for (cur = task->seccomp.filter; cur; cur = cur->prev) {
+ mutex_lock_nested(&cur->notify_lock, filter_nesting);
+ filter_nesting++;
+ last_locked = cur;
+ 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);
+ INIT_LIST_HEAD(&filter->notif->notifications);
+ filter->notif->next_id = get_random_u64();
+ init_waitqueue_head(&filter->notif->wqh);
+
+ ret = anon_inode_getfile("seccomp notify", &seccomp_notify_ops,
+ filter, O_RDWR);
+ if (IS_ERR(ret))
+ goto out;
+
+
+ /* The file has a reference to it now */
+ __get_seccomp_filter(filter);
+
+out:
+ for (cur = task->seccomp.filter; cur; cur = cur->prev) {
+ mutex_unlock(&cur->notify_lock);
+ if (cur == last_locked)
+ break;
+ }
+
+ return ret;
+}
+#endif
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index e1473234968d..5f4b836a6792 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"
@@ -154,6 +157,34 @@ 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 0xF7
+#define SECCOMP_NOTIF_RECV _IOWR(SECCOMP_IOC_MAGIC, 0, \
+ struct seccomp_notif)
+#define SECCOMP_NOTIF_SEND _IOWR(SECCOMP_IOC_MAGIC, 1, \
+ struct seccomp_notif_resp)
+#define SECCOMP_NOTIF_ID_VALID _IOR(SECCOMP_IOC_MAGIC, 2, \
+ __u64)
+struct seccomp_notif {
+ __u16 len;
+ __u64 id;
+ __u32 pid;
+ __u8 signaled;
+ struct seccomp_data data;
+};
+
+struct seccomp_notif_resp {
+ __u16 len;
+ __u64 id;
+ __s32 error;
+ __s64 val;
+};
+#endif
+
#ifndef seccomp
int seccomp(unsigned int op, unsigned int flags, void *args)
{
@@ -2077,7 +2108,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 +2965,383 @@ 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);
+}
+
+static int read_notif(int listener, struct seccomp_notif *req)
+{
+ int ret;
+
+ do {
+ errno = 0;
+ req->len = sizeof(*req);
+ ret = ioctl(listener, SECCOMP_NOTIF_RECV, req);
+ } while (ret == -1 && errno == ENOENT);
+ return ret;
+}
+
+static void signal_handler(int signal)
+{
+}
+
+#define USER_NOTIF_MAGIC 116983961184613L
+TEST(get_user_notification_syscall)
+{
+ 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 that we (don't) trigger lockdep. */
+ 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);
+
+ req.len = sizeof(req);
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
+
+ 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.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ /*
+ * 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_NOTIF_RECV, &req), sizeof(req));
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_ID_VALID, &req.id), 0);
+
+ EXPECT_EQ(kill(pid, SIGKILL), 0);
+ EXPECT_EQ(waitpid(pid, NULL, 0), pid);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_ID_VALID, &req.id), -1);
+
+ resp.id = req.id;
+ ret = ioctl(listener, SECCOMP_NOTIF_SEND, &resp);
+ EXPECT_EQ(ret, -1);
+ EXPECT_EQ(errno, ENOENT);
+
+ /*
+ * Check that we get another notification about a signal in the middle
+ * of a syscall.
+ */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ if (signal(SIGUSR1, signal_handler) == SIG_ERR) {
+ perror("signal");
+ exit(1);
+ }
+ ret = syscall(__NR_getpid);
+ exit(ret != USER_NOTIF_MAGIC);
+ }
+
+ ret = read_notif(listener, &req);
+ EXPECT_EQ(ret, sizeof(req));
+ EXPECT_EQ(errno, 0);
+
+ EXPECT_EQ(kill(pid, SIGUSR1), 0);
+
+ ret = read_notif(listener, &req);
+ EXPECT_EQ(req.signaled, 1);
+ EXPECT_EQ(ret, sizeof(req));
+ EXPECT_EQ(errno, 0);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = -512; /* -ERESTARTSYS */
+ resp.val = 0;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+
+ ret = read_notif(listener, &req);
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+ ret = ioctl(listener, SECCOMP_NOTIF_SEND, &resp);
+ EXPECT_EQ(ret, sizeof(resp));
+ EXPECT_EQ(errno, 0);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ /*
+ * 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;
+ int sk_pair[2];
+ char c;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ EXPECT_EQ(user_trap_syscall(__NR_getpid, 0), 0);
+
+ /* Signal we're ready and have installed the filter. */
+ EXPECT_EQ(write(sk_pair[1], "J", 1), 1);
+
+ EXPECT_EQ(read(sk_pair[1], &c, 1), 1);
+ EXPECT_EQ(c, 'H');
+
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+ }
+
+ EXPECT_EQ(read(sk_pair[0], &c, 1), 1);
+ EXPECT_EQ(c, 'J');
+
+ EXPECT_EQ(ptrace(PTRACE_ATTACH, pid), 0);
+ EXPECT_EQ(waitpid(pid, NULL, 0), pid);
+ listener = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0);
+ EXPECT_GE(listener, 0);
+ EXPECT_EQ(ptrace(PTRACE_DETACH, pid, NULL, 0), 0);
+
+ /* Now signal we are done and respond with magic */
+ EXPECT_EQ(write(sk_pair[0], "H", 1), 1);
+
+ req.len = sizeof(req);
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
+ EXPECT_EQ(req.pid, pid);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+
+ 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;
+ int sk_pair[2];
+ char c;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ int child_pair[2];
+
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, child_pair), 0);
+
+ pid2 = fork();
+ ASSERT_GE(pid2, 0);
+
+ if (pid2 == 0) {
+ close(child_pair[0]);
+ EXPECT_EQ(user_trap_syscall(__NR_getpid, 0), 0);
+
+ /* Signal we're ready and have installed the filter. */
+ EXPECT_EQ(write(child_pair[1], "J", 1), 1);
+
+ EXPECT_EQ(read(child_pair[1], &c, 1), 1);
+ EXPECT_EQ(c, 'H');
+
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+ }
+
+ /* check that child has installed the filter */
+ EXPECT_EQ(read(child_pair[0], &c, 1), 1);
+ EXPECT_EQ(c, 'J');
+
+ /* tell parent who child is */
+ EXPECT_EQ(write(sk_pair[1], &pid2, sizeof(pid2)), sizeof(pid2));
+
+ /* parent has installed listener, tell child to call syscall */
+ EXPECT_EQ(read(sk_pair[1], &c, 1), 1);
+ EXPECT_EQ(c, 'H');
+ EXPECT_EQ(write(child_pair[0], "H", 1), 1);
+
+ EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ exit(WEXITSTATUS(status));
+ }
+
+ EXPECT_EQ(read(sk_pair[0], &pid2, sizeof(pid2)), sizeof(pid2));
+
+ EXPECT_EQ(ptrace(PTRACE_ATTACH, pid2), 0);
+ EXPECT_EQ(waitpid(pid2, NULL, 0), pid2);
+ listener = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid2, 0);
+ EXPECT_GE(listener, 0);
+ EXPECT_EQ(errno, 0);
+ EXPECT_EQ(ptrace(PTRACE_DETACH, pid2, NULL, 0), 0);
+
+ /* 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) {
+ req.len = sizeof(req);
+ ASSERT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
+ /*
+ * The pid should be 0, i.e. the task is in some namespace that
+ * we can't "see".
+ */
+ ASSERT_EQ(req.pid, 0);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ ASSERT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+ exit(0);
+ }
+
+ close(listener);
+
+ /* Now signal we are done setting up sibling listener. */
+ EXPECT_EQ(write(sk_pair[0], "H", 1), 1);
+
+ 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));
+}
+
+
/*
* TODO:
* - add microbenchmarks
--
2.17.1
^ permalink raw reply related
* [PATCH v7 0/6] seccomp trap to userspace
From: Tycho Andersen @ 2018-09-27 15:11 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel, Tycho Andersen
Hi all,
Here's v7 of the seccomp trap to userspace set. There are various minor
changes and bug fixes, but two major changes:
* We now pass fds to the tracee via an ioctl, and do it immediately when
the ioctl is called. For this we needed some help from the vfs, so
I've put the one patch in this series and cc'd fsdevel. This does have
the advantage that the feature is now totally decoupled from the rest
of the set, which is itself useful (thanks Andy!)
* Instead of putting all of the notification related stuff into the
struct seccomp_filter, it now lives in its own struct notification,
which is pointed to by struct seccomp_filter. This will save a lot of
memory (thanks Tyler!)
v6 discussion: https://lkml.org/lkml/2018/9/6/769
Thoughts welcome,
Tycho
Tycho Andersen (6):
seccomp: add a return code to trap to userspace
seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE
seccomp: add a way to get a listener fd from ptrace
files: add a replace_fd_files() function
seccomp: add a way to pass FDs via a notification fd
samples: add an example of seccomp user trap
Documentation/ioctl/ioctl-number.txt | 1 +
.../userspace-api/seccomp_filter.rst | 89 +++
fs/file.c | 22 +-
include/linux/file.h | 8 +
include/linux/seccomp.h | 14 +-
include/uapi/linux/ptrace.h | 2 +
include/uapi/linux/seccomp.h | 42 +-
kernel/ptrace.c | 4 +
kernel/seccomp.c | 527 ++++++++++++++-
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 312 +++++++++
tools/testing/selftests/seccomp/seccomp_bpf.c | 607 +++++++++++++++++-
13 files changed, 1617 insertions(+), 19 deletions(-)
create mode 100644 samples/seccomp/user-trap.c
--
2.17.1
^ permalink raw reply
* Re: [PATCH] rseq/selftests: fix parametrized test with -fpie
From: Shuah Khan @ 2018-09-27 13:58 UTC (permalink / raw)
To: Steven Rostedt, Mathieu Desnoyers
Cc: Thomas Gleixner, Shuah Khan, linux-kselftest, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andi Kleen,
Chris Lameter, Ben Maurer, Josh Triplett, Linus Torvalds
In-Reply-To: <20180927092235.77282e35@vmware.local.home>
On 09/27/2018 07:22 AM, Steven Rostedt wrote:
> On Tue, 25 Sep 2018 13:39:36 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> ----- On Sep 18, 2018, at 9:53 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>>
>>> On x86-64, the parametrized selftest code for rseq crashes with a
>>> segmentation fault when compiled with -fpie. This happens when the
>>> param_test binary is loaded at an address beyond 32-bit on x86-64.
>>>
>>> The issue is caused by use of a 32-bit register to hold the address
>>> of the loop counter variable.
>>>
>>> Fix this by using a 64-bit register to calculate the address of the
>>> loop counter variables as an offset from rip.
>>
>> Should this fix go through tip or the selftests tree ?
>>
>
> I usually have changes like this pulled through Shuah's tree with an
> ack from the maintainer of the code that it tests.
>
> -- Steve
>
Right that is what I prefer. I usually wait for an Ack. I can pull this
in.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com
^ permalink raw reply
* Re: [PATCH] rseq/selftests: fix parametrized test with -fpie
From: Steven Rostedt @ 2018-09-27 13:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Shuah Khan, linux-kselftest, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andi Kleen,
Chris Lameter, Ben Maurer, Josh Triplett, Linus Torvalds
In-Reply-To: <1367962117.10271.1537897176578.JavaMail.zimbra@efficios.com>
On Tue, 25 Sep 2018 13:39:36 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Sep 18, 2018, at 9:53 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>
> > On x86-64, the parametrized selftest code for rseq crashes with a
> > segmentation fault when compiled with -fpie. This happens when the
> > param_test binary is loaded at an address beyond 32-bit on x86-64.
> >
> > The issue is caused by use of a 32-bit register to hold the address
> > of the loop counter variable.
> >
> > Fix this by using a 64-bit register to calculate the address of the
> > loop counter variables as an offset from rip.
>
> Should this fix go through tip or the selftests tree ?
>
I usually have changes like this pulled through Shuah's tree with an
ack from the maintainer of the code that it tests.
-- Steve
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Kees Cook @ 2018-09-27 2:03 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, Alexey Dobriyan, Ken Chen, kernel list,
linux-fsdevel@vger.kernel.org, Will Deacon, Laura Abbott,
Andy Lutomirski, Security Officers, Catalin Marinas,
Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
Linux API
In-Reply-To: <CAG48ez0M90ajea8Cee32VjRV4A5CjM8FA1kpdmL6yThBrGf9Lg@mail.gmail.com>
On Wed, Sep 26, 2018 at 6:19 PM, Jann Horn <jannh@google.com> wrote:
> On Thu, Sep 13, 2018 at 4:39 PM Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
>> > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>> >>
>> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> >> > +linux-api, I guess
>> >> >
>> >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>> >> >>
>> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> >> >> in order to prevent a local attacker from exploiting racy stack unwinding
>> >> >> to leak kernel task stack contents.
>> >> >> See the added comment for a longer rationale.
>> >> >>
>> >> >> There don't seem to be any users of this userspace API that can't
>> >> >> gracefully bail out if reading from the file fails. Therefore, I believe
>> >> >> that this change is unlikely to break things.
>> >> >> In the case that this patch does end up needing a revert, the next-best
>> >> >> solution might be to fake a single-entry stack based on wchan.
>> >> >>
>> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> >> >> Cc: stable@vger.kernel.org
>> >> >> Signed-off-by: Jann Horn <jannh@google.com>
>> >> >> ---
>> >> >> fs/proc/base.c | 14 ++++++++++++++
>> >> >> 1 file changed, 14 insertions(+)
>> >> >>
>> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> >> index ccf86f16d9f0..7e9f07bf260d 100644
>> >> >> --- a/fs/proc/base.c
>> >> >> +++ b/fs/proc/base.c
>> >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>> >> >> unsigned long *entries;
>> >> >> int err;
>> >> >>
>> >> >> + /*
>> >> >> + * The ability to racily run the kernel stack unwinder on a running task
>> >> >> + * and then observe the unwinder output is scary; while it is useful for
>> >> >> + * debugging kernel issues, it can also allow an attacker to leak kernel
>> >> >> + * stack contents.
>> >> >> + * Doing this in a manner that is at least safe from races would require
>> >> >> + * some work to ensure that the remote task can not be scheduled; and
>> >> >> + * even then, this would still expose the unwinder as local attack
>> >> >> + * surface.
>> >> >> + * Therefore, this interface is restricted to root.
>> >> >> + */
>> >> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> >> >> + return -EACCES;
>> >>
>> >> In the past, we've avoided hard errors like this in favor of just
>> >> censoring the output. Do we want to be more cautious here? (i.e.
>> >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>> >
>> > In my mind, this is different because it's a place where we don't have
>> > to selectively censor output while preserving parts of it, and it's a
>> > place where, as Laura said, it's useful to make lack of privileges
>> > clearly visible because that informs users that they may have to retry
>> > with more privileges.
>> >
>> > Of course, if you have an example of software that actually breaks due
>> > to this, I'll change it. But I looked at the three things in Debian
>> > codesearch that seem to use it, and from what I can tell, they all
>> > bail out cleanly when the read fails.
>>
>> I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> What do I need to do to get this merged? Oh, I think I misread
> MAINTAINERS - Alexey Dobriyan is not a maintainer, just a reviewer. So
> I guess this has to go via Andrew Morton? Should I resend the patch
> with Andrew in the recipient list?
Yeah, traditionally Andrew has taken /proc patches.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Jann Horn @ 2018-09-27 1:19 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
Will Deacon, Laura Abbott, Andy Lutomirski, security,
Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Linux API
In-Reply-To: <CAGXu5jLLC6dp7T5AmKur0dioc767vvJFms8wj9syLBvQzcYQpg@mail.gmail.com>
On Thu, Sep 13, 2018 at 4:39 PM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
> > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
> >>
> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> >> > +linux-api, I guess
> >> >
> >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
> >> >>
> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> >> >> in order to prevent a local attacker from exploiting racy stack unwinding
> >> >> to leak kernel task stack contents.
> >> >> See the added comment for a longer rationale.
> >> >>
> >> >> There don't seem to be any users of this userspace API that can't
> >> >> gracefully bail out if reading from the file fails. Therefore, I believe
> >> >> that this change is unlikely to break things.
> >> >> In the case that this patch does end up needing a revert, the next-best
> >> >> solution might be to fake a single-entry stack based on wchan.
> >> >>
> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> >> >> Cc: stable@vger.kernel.org
> >> >> Signed-off-by: Jann Horn <jannh@google.com>
> >> >> ---
> >> >> fs/proc/base.c | 14 ++++++++++++++
> >> >> 1 file changed, 14 insertions(+)
> >> >>
> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> >> index ccf86f16d9f0..7e9f07bf260d 100644
> >> >> --- a/fs/proc/base.c
> >> >> +++ b/fs/proc/base.c
> >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> >> >> unsigned long *entries;
> >> >> int err;
> >> >>
> >> >> + /*
> >> >> + * The ability to racily run the kernel stack unwinder on a running task
> >> >> + * and then observe the unwinder output is scary; while it is useful for
> >> >> + * debugging kernel issues, it can also allow an attacker to leak kernel
> >> >> + * stack contents.
> >> >> + * Doing this in a manner that is at least safe from races would require
> >> >> + * some work to ensure that the remote task can not be scheduled; and
> >> >> + * even then, this would still expose the unwinder as local attack
> >> >> + * surface.
> >> >> + * Therefore, this interface is restricted to root.
> >> >> + */
> >> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> >> >> + return -EACCES;
> >>
> >> In the past, we've avoided hard errors like this in favor of just
> >> censoring the output. Do we want to be more cautious here? (i.e.
> >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
> >
> > In my mind, this is different because it's a place where we don't have
> > to selectively censor output while preserving parts of it, and it's a
> > place where, as Laura said, it's useful to make lack of privileges
> > clearly visible because that informs users that they may have to retry
> > with more privileges.
> >
> > Of course, if you have an example of software that actually breaks due
> > to this, I'll change it. But I looked at the three things in Debian
> > codesearch that seem to use it, and from what I can tell, they all
> > bail out cleanly when the read fails.
>
> I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
What do I need to do to get this merged? Oh, I think I misread
MAINTAINERS - Alexey Dobriyan is not a maintainer, just a reviewer. So
I guess this has to go via Andrew Morton? Should I resend the patch
with Andrew in the recipient list?
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Dmitry Safonov @ 2018-09-26 17:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andrey Vagin, Dmitry Safonov, linux-kernel@vger.kernel.org,
Adrian Reber, Andy Lutomirski, Christian Brauner, Cyrill Gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov,
Pavel Emelianov, Shuah Khan, Thomas Gleixner,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org,
Alexey Dobriyan <adob>
In-Reply-To: <87zhw4rwiq.fsf@xmission.com>
2018-09-26 18:36 GMT+01:00 Eric W. Biederman <ebiederm@xmission.com>:
> The advantage of timekeeping_update per time namespace is that it allows
> different lengths of seconds per time namespace. Which allows testing
> ntp and the kernel in interesting ways while still having a working
> production configuration on the same system.
Just a quick note: the different length of second per namespace sounds
very interesting in my POV, I remember I've seen this article:
http://publish.illinois.edu/science-of-security-lablet/files/2014/05/DSSnet-A-Smart-Grid-Modeling-Platform-Combining-Electrical-Power-Distributtion-System-Simulation-and-Software-Defined-Networking-Emulation.pdf
And their realisation with a simulation of time going with different speed
per-pid (with vdso disabled):
https://github.com/littlepretty/VirtualTimeKernel
Thanks,
Dmitry
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Eric W. Biederman @ 2018-09-26 17:36 UTC (permalink / raw)
To: Andrey Vagin
Cc: Dmitry Safonov, linux-kernel@vger.kernel.org, Dmitry Safonov,
Adrian Reber, Andy Lutomirski, Christian Brauner, Cyrill Gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov,
Pavel Emelianov, Shuah Khan, Thomas Gleixner,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <20180925014150.GA6302@outlook.office365.com>
Andrey Vagin <avagin@virtuozzo.com> writes:
> On Tue, Sep 25, 2018 at 12:02:32AM +0200, Eric W. Biederman wrote:
>> Andrey Vagin <avagin@virtuozzo.com> writes:
>>
>> > On Fri, Sep 21, 2018 at 02:27:29PM +0200, Eric W. Biederman wrote:
>> >> Dmitry Safonov <dima@arista.com> writes:
>> >>
>> >> > Discussions around time virtualization are there for a long time.
>> >> > The first attempt to implement time namespace was in 2006 by Jeff Dike.
>> >> > From that time, the topic appears on and off in various discussions.
>> >> >
>> >> > There are two main use cases for time namespaces:
>> >> > 1. change date and time inside a container;
>> >> > 2. adjust clocks for a container restored from a checkpoint.
>> >> >
>> >> > “It seems like this might be one of the last major obstacles keeping
>> >> > migration from being used in production systems, given that not all
>> >> > containers and connections can be migrated as long as a time dependency
>> >> > is capable of messing it up.” (by github.com/dav-ell)
>> >> >
>> >> > The kernel provides access to several clocks: CLOCK_REALTIME,
>> >> > CLOCK_MONOTONIC, CLOCK_BOOTTIME. Last two clocks are monotonous, but the
>> >> > start points for them are not defined and are different for each running
>> >> > system. When a container is migrated from one node to another, all
>> >> > clocks have to be restored into consistent states; in other words, they
>> >> > have to continue running from the same points where they have been
>> >> > dumped.
>> >> >
>> >> > The main idea behind this patch set is adding per-namespace offsets for
>> >> > system clocks. When a process in a non-root time namespace requests
>> >> > time of a clock, a namespace offset is added to the current value of
>> >> > this clock on a host and the sum is returned.
>> >> >
>> >> > All offsets are placed on a separate page, this allows up to map it as
>> >> > part of vvar into user processes and use offsets from vdso calls.
>> >> >
>> >> > Now offsets are implemented for CLOCK_MONOTONIC and CLOCK_BOOTTIME
>> >> > clocks.
>> >> >
>> >> > Questions to discuss:
>> >> >
>> >> > * Clone flags exhaustion. Currently there is only one unused clone flag
>> >> > bit left, and it may be worth to use it to extend arguments of the clone
>> >> > system call.
>> >> >
>> >> > * Realtime clock implementation details:
>> >> > Is having a simple offset enough?
>> >> > What to do when date and time is changed on the host?
>> >> > Is there a need to adjust vfs modification and creation times?
>> >> > Implementation for adjtime() syscall.
>> >>
>> >> Overall I support this effort. In my quick skim this code looked good.
>> >
>> > Hi Eric,
>> >
>> > Thank you for the feedback.
>> >
>> >>
>> >> My feeling is that we need to be able to support running ntpd and
>> >> support one namespace doing googles smoothing of leap seconds while
>> >> another namespace takes the leap second.
>> >>
>> >> What I was imagining when I was last thinking about this was one
>> >> instance of struct timekeeper aka tk_core per time namespace. That
>> >> structure already keeps offsets for all of the various clocks from
>> >> the kerne internal time sources. What would be needed would be to
>> >> pass in an appropriate time namespace pointer.
>> >>
>> >> I could be completely wrong as I have not take the time to completely
>> >> trace through the code. Have you looked at pushing the time namespace
>> >> down as far as tk_core?
>> >>
>> >> What I think would be the big advantage (besides ntp working) is that
>> >> the bulk of the code could be reused. Allowing testing of the kernel's
>> >> time code by setting up a new time namespace. So a person in production
>> >> could setup a time namespace with the time set ahead a little bit and
>> >> be able to verify that the kernel handles the upcoming leap second
>> >> properly.
>> >>
>> >
>> > It is an interesting idea, but I have a few questions:
>> >
>> > 1. Does it mean that timekeeping_update() will be called for each
>> > namespace? This functions is called periodically, it updates times on the
>> > timekeeper structure, updates vsyscall_gtod_data, etc. What will be an
>> > overhead of this?
>>
>> I don't know if periodically is a proper characterization. There may be
>> a code path that does that. But from what I can see timekeeping_update
>> is the guts of settimeofday (and a few related functions).
>>
>> So it appears to make sense for timekeeping_update to be per namespace.
>>
>> Hmm. Looking at what is updated in the vsyscall_gtod_data it does
>> look like you would have to periodically update things, but I don't know
>> big that period would be. As long as the period is reasonably large,
>> or the time namespaces were sufficiently deschronized it should not
>> be a problem. But that is the class of problem that could make
>> my ideal impractical if there is measuarable overhead.
>>
>> Where were you seeing timekeeping_update being called periodically?
>
> timekeeping_update() is called HZ times per-second:
>
> [ 67.912858] timekeeping_update.cold.26+0x5/0xa
> [ 67.913332] timekeeping_advance+0x361/0x5c0
> [ 67.913857] ? tick_sched_do_timer+0x55/0x70
> [ 67.914409] ? tick_sched_do_timer+0x70/0x70
> [ 67.914947] tick_sched_do_timer+0x55/0x70
> [ 67.915505] tick_sched_timer+0x27/0x70
> [ 67.916042] __hrtimer_run_queues+0x10f/0x440
> [ 67.916639] hrtimer_interrupt+0x100/0x220
> [ 67.917305] smp_apic_timer_interrupt+0x79/0x220
> [ 67.918030] apic_timer_interrupt+0xf/0x20
Interesting.
Reading the code the calling sequence there is:
tick_sched_do_timer
tick_do_update_jiffies64
update_wall_time
timekeeping_advance
timekeepging_update
If I read that properly under the right nohz circumstances that update
can be delayed indefinitely.
So I think we could prototype a time namespace that was per
timekeeping_update and just had update_wall_time iterate through
all of the time namespaces.
I don't think the naive version would scale to very many time
namespaces.
At the same time using the techniques from the nohz work and a little
smarts I expect we could get the code to scale.
I think this direction is definitely worth exploring. My experience
with namespaces is that if we don't get the advanced features working
there is little to no interest from the core developers of the code,
and the namespaces don't solve additional problems. Which makes the
namespace a hard sell. Especially when it does not solve problems the
developers of the subsystem have.
The advantage of timekeeping_update per time namespace is that it allows
different lengths of seconds per time namespace. Which allows testing
ntp and the kernel in interesting ways while still having a working
production configuration on the same system.
Eric
^ permalink raw reply
* Re: [patch v3] mm, thp: always specify disabled vmas as nh in smaps
From: Vlastimil Babka @ 2018-09-26 8:40 UTC (permalink / raw)
To: David Rientjes, Andrew Morton
Cc: Michal Hocko, Alexey Dobriyan, Kirill A. Shutemov, linux-kernel,
linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1809251449060.96762@chino.kir.corp.google.com>
On 9/25/18 11:50 PM, David Rientjes wrote:
> Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> introduced a regression in that userspace cannot always determine the set
> of vmas where thp is disabled.
>
> Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> to determine if a vma has been disabled from being backed by hugepages.
>
> Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
> be disabled and emit "nh" as a flag for the corresponding vmas as part of
> /proc/pid/smaps. After the commit, thp is disabled by means of an mm
> flag and "nh" is not emitted.
>
> This causes smaps parsing libraries to assume a vma is enabled for thp
> and ends up puzzling the user on why its memory is not backed by thp.
>
> This also clears the "hg" flag to make the behavior of MADV_HUGEPAGE and
> PR_SET_THP_DISABLE definitive.
>
> Fixes: 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> Signed-off-by: David Rientjes <rientjes@google.com>
Well, as Andrew said, we had the opportunity to provide a more complete
info to userspace e.g. with Michal's suggested /proc/pid/status
enhancement. If this is good enough for you (and nobody else cares) then
I won't block it either. It would be unfortunate though if we could not
revert this in case the MMF_DISABLE_THP querying is implemented later.
Hopefully the only consumers are internal tools such as yours, which can
be easily adapted...
Vlastimil
^ permalink raw reply
* Re: [patch v3] mm, thp: always specify disabled vmas as nh in smaps
From: Michal Hocko @ 2018-09-26 7:17 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20180926061247.GB18685@dhcp22.suse.cz>
On Wed 26-09-18 08:12:47, Michal Hocko wrote:
> On Tue 25-09-18 14:50:52, David Rientjes wrote:
> [...]
> Let's put my general disagreement with the approach asside for a while.
> If this is really the best way forward the is the implementation really
> correct?
>
> > + /*
> > + * Disabling thp is possible through both MADV_NOHUGEPAGE and
> > + * PR_SET_THP_DISABLE. Both historically used VM_NOHUGEPAGE. Since
> > + * the introduction of MMF_DISABLE_THP, however, userspace needs the
> > + * ability to detect vmas where thp is not eligible in the same manner.
> > + */
> > + if (vma->vm_mm && test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) {
> > + flags &= ~VM_HUGEPAGE;
> > + flags |= VM_NOHUGEPAGE;
> > + }
>
> Do we want to report all vmas nh? Shouldn't we limit that to THP-able
> mappings? It seems quite strange that an application started without
> PR_SET_THP_DISABLE wouldn't report nh for most mappings while it would
> otherwise. Also when can we have vma->vm_mm == NULL?
Hmm, after re-reading your documentation update to "A process mapping
may be advised to not be backed by transparent hugepages by either
madvise(MADV_NOHUGEPAGE) or prctl(PR_SET_THP_DISABLE)." the
implementation matches so scratch my comment.
As I've said, I am not happy about this approach but if there is a
general agreement this is really the best we can do I will not stand in
the way.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [patch v3] mm, thp: always specify disabled vmas as nh in smaps
From: Michal Hocko @ 2018-09-26 6:12 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1809251449060.96762@chino.kir.corp.google.com>
On Tue 25-09-18 14:50:52, David Rientjes wrote:
[...]
Let's put my general disagreement with the approach asside for a while.
If this is really the best way forward the is the implementation really
correct?
> + /*
> + * Disabling thp is possible through both MADV_NOHUGEPAGE and
> + * PR_SET_THP_DISABLE. Both historically used VM_NOHUGEPAGE. Since
> + * the introduction of MMF_DISABLE_THP, however, userspace needs the
> + * ability to detect vmas where thp is not eligible in the same manner.
> + */
> + if (vma->vm_mm && test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) {
> + flags &= ~VM_HUGEPAGE;
> + flags |= VM_NOHUGEPAGE;
> + }
Do we want to report all vmas nh? Shouldn't we limit that to THP-able
mappings? It seems quite strange that an application started without
PR_SET_THP_DISABLE wouldn't report nh for most mappings while it would
otherwise. Also when can we have vma->vm_mm == NULL?
> +
> seq_puts(m, "VmFlags: ");
> for (i = 0; i < BITS_PER_LONG; i++) {
> if (!mnemonics[i][0])
> continue;
> - if (vma->vm_flags & (1UL << i)) {
> + if (flags & (1UL << i)) {
> seq_putc(m, mnemonics[i][0]);
> seq_putc(m, mnemonics[i][1]);
> seq_putc(m, ' ');
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: Michal Hocko @ 2018-09-26 6:06 UTC (permalink / raw)
To: Andrew Morton
Cc: David Rientjes, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20180925150406.872aab9f4f945193e5915d69@linux-foundation.org>
On Tue 25-09-18 15:04:06, Andrew Morton wrote:
> On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
>
> > > > It is also used in
> > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > this, and those tests now break.
> > >
> > > This sounds like a bit of an abuse to me. It shows how an internal
> > > implementation detail leaks out to the userspace which is something we
> > > should try to avoid.
> > >
> >
> > Well, it's already how this has worked for years before commit
> > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > as long as you don't break userspace who relies on what is exported to it
> > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > being backed by hugepages.
>
> 1860033237d4 was over a year ago so perhaps we don't need to be
> too worried about restoring the old interface. In which case
> we have an opportunity to make improvements such as that suggested
> by Michal?
Yeah, can we add a way to export PR_SET_THP_DISABLE to userspace
somehow? E.g. /proc/<pid>/status. It is a process wide thing so
reporting it per VMA sounds strange at best.
This would also keep a sane (and currently documented) semantic for
the smaps flag to be really
hg - huge page advise flag
nh - no-huge page advise flag
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: David Rientjes @ 2018-09-26 0:55 UTC (permalink / raw)
To: Andrew Morton
Cc: Michal Hocko, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20180925150406.872aab9f4f945193e5915d69@linux-foundation.org>
On Tue, 25 Sep 2018, Andrew Morton wrote:
> > > > It is also used in
> > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > this, and those tests now break.
> > >
> > > This sounds like a bit of an abuse to me. It shows how an internal
> > > implementation detail leaks out to the userspace which is something we
> > > should try to avoid.
> > >
> >
> > Well, it's already how this has worked for years before commit
> > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > as long as you don't break userspace who relies on what is exported to it
> > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > being backed by hugepages.
>
> 1860033237d4 was over a year ago so perhaps we don't need to be
> too worried about restoring the old interface. In which case
> we have an opportunity to make improvements such as that suggested
> by Michal?
>
The only way to determine if a vma was thp disabled prior to this commit
was parsing VmFlags from /proc/pid/smaps. That was possible either
through MADV_NOHUGEPAGE or PR_SET_THP_DISABLE. It is perfectly legitimate
for a test case to check if either are being set correctly through
userspace libraries or through the kernel itself in the manner in which
the kernel exports this information. It is also perfectly legitimate for
userspace to cull through information in the only way it is exported by
the kernel to identify reasons for why applications are not having their
heap backed by transparent hugepages: the mapping is disabled, the
application is hitting the limit for its mem cgroup, we are low on memory,
or there are fragmentation issues. Differentiating between those is
something our userspace does, and was broken by 1860033237d4.
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: Andrew Morton @ 2018-09-25 22:04 UTC (permalink / raw)
To: David Rientjes
Cc: Michal Hocko, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1809251440001.94921@chino.kir.corp.google.com>
On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> > > It is also used in
> > > automated testing to ensure that vmas get disabled for thp appropriately
> > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > this, and those tests now break.
> >
> > This sounds like a bit of an abuse to me. It shows how an internal
> > implementation detail leaks out to the userspace which is something we
> > should try to avoid.
> >
>
> Well, it's already how this has worked for years before commit
> 1860033237d4 broke it. Changing the implementation in the kernel is fine
> as long as you don't break userspace who relies on what is exported to it
> and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> being backed by hugepages.
1860033237d4 was over a year ago so perhaps we don't need to be
too worried about restoring the old interface. In which case
we have an opportunity to make improvements such as that suggested
by Michal?
^ permalink raw reply
* [patch v3] mm, thp: always specify disabled vmas as nh in smaps
From: David Rientjes @ 2018-09-25 21:50 UTC (permalink / raw)
To: Andrew Morton, Vlastimil Babka
Cc: Michal Hocko, Alexey Dobriyan, Kirill A. Shutemov, linux-kernel,
linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1809251248450.50347@chino.kir.corp.google.com>
Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
introduced a regression in that userspace cannot always determine the set
of vmas where thp is disabled.
Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
to determine if a vma has been disabled from being backed by hugepages.
Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
be disabled and emit "nh" as a flag for the corresponding vmas as part of
/proc/pid/smaps. After the commit, thp is disabled by means of an mm
flag and "nh" is not emitted.
This causes smaps parsing libraries to assume a vma is enabled for thp
and ends up puzzling the user on why its memory is not backed by thp.
This also clears the "hg" flag to make the behavior of MADV_HUGEPAGE and
PR_SET_THP_DISABLE definitive.
Fixes: 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
Signed-off-by: David Rientjes <rientjes@google.com>
---
v3:
- reword Documentation/filesystems/proc.txt for eligibility
v2:
- clear VM_HUGEPAGE per Vlastimil
- update Documentation/filesystems/proc.txt to be explicit
Documentation/filesystems/proc.txt | 7 ++++++-
fs/proc/task_mmu.c | 14 +++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -491,9 +491,14 @@ manner. The codes are the following:
sd - soft-dirty flag
mm - mixed map area
hg - huge page advise flag
- nh - no-huge page advise flag
+ nh - no-huge page advise flag [*]
mg - mergable advise flag
+ [*] A process mapping may be advised to not be backed by transparent hugepages
+ by either madvise(MADV_NOHUGEPAGE) or prctl(PR_SET_THP_DISABLE). See
+ Documentation/admin-guide/mm/transhuge.rst for system-wide and process
+ mapping policies.
+
Note that there is no guarantee that every flag and associated mnemonic will
be present in all further kernel releases. Things get changed, the flags may
be vanished or the reverse -- new added.
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -653,13 +653,25 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
#endif
#endif /* CONFIG_ARCH_HAS_PKEYS */
};
+ unsigned long flags = vma->vm_flags;
size_t i;
+ /*
+ * Disabling thp is possible through both MADV_NOHUGEPAGE and
+ * PR_SET_THP_DISABLE. Both historically used VM_NOHUGEPAGE. Since
+ * the introduction of MMF_DISABLE_THP, however, userspace needs the
+ * ability to detect vmas where thp is not eligible in the same manner.
+ */
+ if (vma->vm_mm && test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) {
+ flags &= ~VM_HUGEPAGE;
+ flags |= VM_NOHUGEPAGE;
+ }
+
seq_puts(m, "VmFlags: ");
for (i = 0; i < BITS_PER_LONG; i++) {
if (!mnemonics[i][0])
continue;
- if (vma->vm_flags & (1UL << i)) {
+ if (flags & (1UL << i)) {
seq_putc(m, mnemonics[i][0]);
seq_putc(m, mnemonics[i][1]);
seq_putc(m, ' ');
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: David Rientjes @ 2018-09-25 21:45 UTC (permalink / raw)
To: Michal Hocko
Cc: Vlastimil Babka, Andrew Morton, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20180925202959.GY18685@dhcp22.suse.cz>
On Tue, 25 Sep 2018, Michal Hocko wrote:
> > This is used to identify heap mappings that should be able to fault thp
> > but do not, and they normally point to a low-on-memory or fragmentation
> > issue. After commit 1860033237d4, our users of PR_SET_THP_DISABLE no
> > longer show "nh" for their heap mappings so they get reported as having a
> > low thp ratio when in reality it is disabled.
>
> I am still not sure I understand the issue completely. How are PR_SET_THP_DISABLE
> users any different from the global THP disabled case? Is this only
> about the scope? E.g the one who checks for the state cannot check the
> PR_SET_THP_DISABLE state? Besides that what are consequences of the
> low ratio? Is this an example of somebody using the prctl and still
> complaining or an external observer trying to do something useful which
> ends up doing contrary?
>
Yes, that is how I found out about this. The system-wide policy can be
determined from /sys/kernel/mm/transparent_hugepage/enabled. If it is
"always" and heap mappings are not being backed by hugepages and lack the
"nh" flag, it was considered as a likely fragmentation issue before commit
1860033237d4. After commit 1860033237d4, the heap mapping for
PR_SET_THP_DISABLE users was not showing it actually is prevented from
faulting thp because of policy, not because of fragmentation.
> > It is also used in
> > automated testing to ensure that vmas get disabled for thp appropriately
> > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > this, and those tests now break.
>
> This sounds like a bit of an abuse to me. It shows how an internal
> implementation detail leaks out to the userspace which is something we
> should try to avoid.
>
Well, it's already how this has worked for years before commit
1860033237d4 broke it. Changing the implementation in the kernel is fine
as long as you don't break userspace who relies on what is exported to it
and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
being backed by hugepages.
> > I'll reword this to explicitly state that "hg" and "nh" mappings either
> > allow or disallow thp backing.
>
> How are you going to distinguish a regular THP-able mapping then? I am
> still not sure how this is supposed to work. Could you be more specific.
You look for "[heap]" in smaps to determine where the heap mapping is.
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: Michal Hocko @ 2018-09-25 20:29 UTC (permalink / raw)
To: David Rientjes
Cc: Vlastimil Babka, Andrew Morton, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1809251248450.50347@chino.kir.corp.google.com>
On Tue 25-09-18 12:52:09, David Rientjes wrote:
> On Mon, 24 Sep 2018, Vlastimil Babka wrote:
>
> > On 9/24/18 10:02 PM, Michal Hocko wrote:
> > > On Mon 24-09-18 21:56:03, Michal Hocko wrote:
> > >> On Mon 24-09-18 12:30:07, David Rientjes wrote:
> > >>> Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> > >>> introduced a regression in that userspace cannot always determine the set
> > >>> of vmas where thp is ineligible.
> > >>>
> > >>> Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> > >>> to determine if a vma is eligible to be backed by hugepages.
> > >>
> > >> I was under impression that nh resp hg flags only tell about the madvise
> > >> status. How do you exactly use these flags in an application?
> > >>
>
> This is used to identify heap mappings that should be able to fault thp
> but do not, and they normally point to a low-on-memory or fragmentation
> issue. After commit 1860033237d4, our users of PR_SET_THP_DISABLE no
> longer show "nh" for their heap mappings so they get reported as having a
> low thp ratio when in reality it is disabled.
I am still not sure I understand the issue completely. How are PR_SET_THP_DISABLE
users any different from the global THP disabled case? Is this only
about the scope? E.g the one who checks for the state cannot check the
PR_SET_THP_DISABLE state? Besides that what are consequences of the
low ratio? Is this an example of somebody using the prctl and still
complaining or an external observer trying to do something useful which
ends up doing contrary?
> It is also used in
> automated testing to ensure that vmas get disabled for thp appropriately
> and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> this, and those tests now break.
This sounds like a bit of an abuse to me. It shows how an internal
implementation detail leaks out to the userspace which is something we
should try to avoid.
> > >> Your eligible rules as defined here:
> > >>
> > >>> + [*] A process mapping is eligible to be backed by transparent hugepages (thp)
> > >>> + depending on system-wide settings and the mapping itself. See
> > >>> + Documentation/admin-guide/mm/transhuge.rst for default behavior. If a
> > >>> + mapping has a flag of "nh", it is not eligible to be backed by hugepages
> > >>> + in any condition, either because of prctl(PR_SET_THP_DISABLE) or
> > >>> + madvise(MADV_NOHUGEPAGE). PR_SET_THP_DISABLE takes precedence over any
> > >>> + MADV_HUGEPAGE.
> > >>
> > >> doesn't seem to match the reality. I do not see all the file backed
> > >> mappings to be nh marked. So is this really about eligibility rather
> > >> than the madvise status? Maybe it is just the above documentation that
> > >> needs to be updated.
> >
> > Yeah the change from madvise to eligibility in the doc seems to go too far.
> >
>
> I'll reword this to explicitly state that "hg" and "nh" mappings either
> allow or disallow thp backing.
How are you going to distinguish a regular THP-able mapping then? I am
still not sure how this is supposed to work. Could you be more specific.
Let's say I have a THP-able mapping (shmem resp. anon for the current
implementation). What is the the matrix for hg/nh wrt. madvice/nomadvise
PR_SET_THP_DISABLE and global THP enabled/disable.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH 09/11] UAPI: ndctl: Fix g++-unsupported initialisation in headers [ver #2]
From: Dan Williams @ 2018-09-25 20:22 UTC (permalink / raw)
To: David Howells
Cc: Linux API, linux-nvdimm, Linux Kernel Mailing List,
linux-kbuild-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <153622556444.14298.5971956747198405225.stgit-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
On Thu, Sep 6, 2018 at 2:19 AM David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> The following code in the linux/ndctl header file:
>
> static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
> {
> static const char * const names[] = {
> [ND_CMD_ARS_CAP] = "ars_cap",
> [ND_CMD_ARS_START] = "ars_start",
> [ND_CMD_ARS_STATUS] = "ars_status",
> [ND_CMD_CLEAR_ERROR] = "clear_error",
> [ND_CMD_CALL] = "cmd_call",
> };
>
> if (cmd < ARRAY_SIZE(names) && names[cmd])
> return names[cmd];
> return "unknown";
> }
>
> is broken in a number of ways:
>
> (1) ARRAY_SIZE() is not generally defined.
>
> (2) g++ does not support "non-trivial" array initialisers fully yet.
>
> (3) Every file that calls this function will acquire a copy of names[].
>
> The same goes for nvdimm_cmd_name().
>
> Fix all three by converting to a switch statement where each case returns a
> string. That way if cmd is a constant, the compiler can trivially reduce it
> and, if not, the compiler can use a shared lookup table if it thinks that is
> more efficient.
>
> A better way would be to remove these functions and their arrays from the
> header entirely.
>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
...again let me know if you'll take this with g++ series or want me to
carry it directly.
^ permalink raw reply
* Re: [PATCH 10/11] UAPI: ndctl: Remove use of PAGE_SIZE [ver #2]
From: Dan Williams @ 2018-09-25 20:17 UTC (permalink / raw)
To: David Howells
Cc: Linux API, linux-nvdimm, Linux Kernel Mailing List,
linux-kbuild-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <153622557095.14298.10281337300003012767.stgit-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
On Thu, Sep 6, 2018 at 2:19 AM David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> The macro PAGE_SIZE isn't valid outside of the kernel, so it should not
> appear in UAPI headers.
>
> Furthermore, the actual machine page size could theoretically change from
> an application's point of view if it's running in a container that gets
> migrated to another machine (say 4K/ppc64 to 64K/ppc64).
>
> Fixes: f2ba5a5baecf ("libnvdimm, namespace: make min namespace size 4K")
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Let me know if you want me to pick this up for 4.20.
^ permalink raw reply
* Re: [patch v2] mm, thp: always specify ineligible vmas as nh in smaps
From: David Rientjes @ 2018-09-25 19:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Andrew Morton, Alexey Dobriyan, Kirill A. Shutemov,
linux-kernel, linux-mm, linux-api
In-Reply-To: <0aa3eb55-82c0-eba3-b12c-2ba22e052a8e@suse.cz>
On Mon, 24 Sep 2018, Vlastimil Babka wrote:
> On 9/24/18 10:02 PM, Michal Hocko wrote:
> > On Mon 24-09-18 21:56:03, Michal Hocko wrote:
> >> On Mon 24-09-18 12:30:07, David Rientjes wrote:
> >>> Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> >>> introduced a regression in that userspace cannot always determine the set
> >>> of vmas where thp is ineligible.
> >>>
> >>> Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> >>> to determine if a vma is eligible to be backed by hugepages.
> >>
> >> I was under impression that nh resp hg flags only tell about the madvise
> >> status. How do you exactly use these flags in an application?
> >>
This is used to identify heap mappings that should be able to fault thp
but do not, and they normally point to a low-on-memory or fragmentation
issue. After commit 1860033237d4, our users of PR_SET_THP_DISABLE no
longer show "nh" for their heap mappings so they get reported as having a
low thp ratio when in reality it is disabled. It is also used in
automated testing to ensure that vmas get disabled for thp appropriately
and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
this, and those tests now break.
> >> Your eligible rules as defined here:
> >>
> >>> + [*] A process mapping is eligible to be backed by transparent hugepages (thp)
> >>> + depending on system-wide settings and the mapping itself. See
> >>> + Documentation/admin-guide/mm/transhuge.rst for default behavior. If a
> >>> + mapping has a flag of "nh", it is not eligible to be backed by hugepages
> >>> + in any condition, either because of prctl(PR_SET_THP_DISABLE) or
> >>> + madvise(MADV_NOHUGEPAGE). PR_SET_THP_DISABLE takes precedence over any
> >>> + MADV_HUGEPAGE.
> >>
> >> doesn't seem to match the reality. I do not see all the file backed
> >> mappings to be nh marked. So is this really about eligibility rather
> >> than the madvise status? Maybe it is just the above documentation that
> >> needs to be updated.
>
> Yeah the change from madvise to eligibility in the doc seems to go too far.
>
I'll reword this to explicitly state that "hg" and "nh" mappings either
allow or disallow thp backing.
^ permalink raw reply
* Re: [PATCH] rseq/selftests: fix parametrized test with -fpie
From: Mathieu Desnoyers @ 2018-09-25 17:39 UTC (permalink / raw)
To: Thomas Gleixner, Shuah Khan, linux-kselftest
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <20180918135328.32034-1-mathieu.desnoyers@efficios.com>
----- On Sep 18, 2018, at 9:53 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> On x86-64, the parametrized selftest code for rseq crashes with a
> segmentation fault when compiled with -fpie. This happens when the
> param_test binary is loaded at an address beyond 32-bit on x86-64.
>
> The issue is caused by use of a 32-bit register to hold the address
> of the loop counter variable.
>
> Fix this by using a 64-bit register to calculate the address of the
> loop counter variables as an offset from rip.
Should this fix go through tip or the selftests tree ?
Thanks,
Mathieu
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: <stable@vger.kernel.org> # v4.18
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Joel Fernandes <joelaf@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dave Watson <davejwatson@fb.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: linux-kselftest@vger.kernel.org
> Cc: "H . Peter Anvin" <hpa@zytor.com>
> Cc: Chris Lameter <cl@linux.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Paul Turner <pjt@google.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ben Maurer <bmaurer@fb.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> tools/testing/selftests/rseq/param_test.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/rseq/param_test.c
> b/tools/testing/selftests/rseq/param_test.c
> index 615252331813..4bc071525bf7 100644
> --- a/tools/testing/selftests/rseq/param_test.c
> +++ b/tools/testing/selftests/rseq/param_test.c
> @@ -56,15 +56,13 @@ unsigned int yield_mod_cnt, nr_abort;
> printf(fmt, ## __VA_ARGS__); \
> } while (0)
>
> -#if defined(__x86_64__) || defined(__i386__)
> +#ifdef __i386__
>
> #define INJECT_ASM_REG "eax"
>
> #define RSEQ_INJECT_CLOBBER \
> , INJECT_ASM_REG
>
> -#ifdef __i386__
> -
> #define RSEQ_INJECT_ASM(n) \
> "mov asm_loop_cnt_" #n ", %%" INJECT_ASM_REG "\n\t" \
> "test %%" INJECT_ASM_REG ",%%" INJECT_ASM_REG "\n\t" \
> @@ -76,9 +74,16 @@ unsigned int yield_mod_cnt, nr_abort;
>
> #elif defined(__x86_64__)
>
> +#define INJECT_ASM_REG_P "rax"
> +#define INJECT_ASM_REG "eax"
> +
> +#define RSEQ_INJECT_CLOBBER \
> + , INJECT_ASM_REG_P \
> + , INJECT_ASM_REG
> +
> #define RSEQ_INJECT_ASM(n) \
> - "lea asm_loop_cnt_" #n "(%%rip), %%" INJECT_ASM_REG "\n\t" \
> - "mov (%%" INJECT_ASM_REG "), %%" INJECT_ASM_REG "\n\t" \
> + "lea asm_loop_cnt_" #n "(%%rip), %%" INJECT_ASM_REG_P "\n\t" \
> + "mov (%%" INJECT_ASM_REG_P "), %%" INJECT_ASM_REG "\n\t" \
> "test %%" INJECT_ASM_REG ",%%" INJECT_ASM_REG "\n\t" \
> "jz 333f\n\t" \
> "222:\n\t" \
> @@ -86,10 +91,6 @@ unsigned int yield_mod_cnt, nr_abort;
> "jnz 222b\n\t" \
> "333:\n\t"
>
> -#else
> -#error "Unsupported architecture"
> -#endif
> -
> #elif defined(__ARMEL__)
>
> #define RSEQ_INJECT_INPUT \
> --
> 2.11.0
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v4 03/27] x86/fpu/xstate: Enable XSAVES system states
From: Yu-cheng Yu @ 2018-09-25 17:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek, Randy
In-Reply-To: <20180925170355.GD30146@hirez.programming.kicks-ass.net>
On Tue, 2018-09-25 at 19:03 +0200, Peter Zijlstra wrote:
> On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
> > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> > index 4bd56079048f..9f51b0e1da25 100644
> > --- a/arch/x86/kernel/fpu/core.c
> > +++ b/arch/x86/kernel/fpu/core.c
> > @@ -365,8 +365,13 @@ void fpu__drop(struct fpu *fpu)
> > */
> > static inline void copy_init_user_fpstate_to_fpregs(void)
> > {
> > + /*
> > + * Only XSAVES user states are copied.
> > + * System states are preserved.
> > + */
> > if (use_xsave())
> > - copy_kernel_to_xregs(&init_fpstate.xsave, -1);
> > + copy_kernel_to_xregs(&init_fpstate.xsave,
> > + xfeatures_mask_user);
>
> By my counting, that doesn't qualify for a line-break, it hits 80.
>
> If you were to do this line-break, coding style would have you liberally
> sprinkle {} around.
Ok, will fix it.
^ permalink raw reply
* [REVIEW][PATCH 6/6] signal: Use a smaller struct siginfo in the kernel
From: Eric W. Biederman @ 2018-09-25 17:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-api, linux-arch, Oleg Nesterov, Linus Torvalds,
Eric W. Biederman
In-Reply-To: <87h8idv6nw.fsf@xmission.com>
We reserve 128 bytes for struct siginfo but only use about 48 bytes on
64bit and 32 bytes on 32bit. Someday we might use more but it is unlikely
to be anytime soon.
Userspace seems content with just enough bytes of siginfo to implement
sigqueue. Or in the case of checkpoint/restart reinjecting signals
the kernel has sent.
Reducing the stack footprint and the work to copy siginfo around from
2 cachelines to 1 cachelines seems worth doing even if I don't have
benchmarks to show a performance difference.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
include/linux/signal.h | 2 +
include/linux/signal_types.h | 5 +--
kernel/signal.c | 82 ++++++++++++++++++++++++++++--------
3 files changed, 67 insertions(+), 22 deletions(-)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 70031b10b918..706a499d1eb1 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -22,6 +22,8 @@ static inline void clear_siginfo(kernel_siginfo_t *info)
memset(info, 0, sizeof(*info));
}
+#define SI_EXPANSION_SIZE (sizeof(struct siginfo) - sizeof(struct kernel_siginfo))
+
int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h
index 2a40a9c5e4ad..f8a90ae9c6ec 100644
--- a/include/linux/signal_types.h
+++ b/include/linux/signal_types.h
@@ -10,10 +10,7 @@
#include <uapi/linux/signal.h>
typedef struct kernel_siginfo {
- union {
- __SIGINFO;
- int _si_pad[SI_MAX_SIZE/sizeof(int)];
- };
+ __SIGINFO;
} kernel_siginfo_t;
/*
diff --git a/kernel/signal.c b/kernel/signal.c
index 161cad4e448c..1c2dd117fee0 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2844,27 +2844,48 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
}
#endif
+static const struct {
+ unsigned char limit, layout;
+} sig_sicodes[] = {
+ [SIGILL] = { NSIGILL, SIL_FAULT },
+ [SIGFPE] = { NSIGFPE, SIL_FAULT },
+ [SIGSEGV] = { NSIGSEGV, SIL_FAULT },
+ [SIGBUS] = { NSIGBUS, SIL_FAULT },
+ [SIGTRAP] = { NSIGTRAP, SIL_FAULT },
+#if defined(SIGEMT)
+ [SIGEMT] = { NSIGEMT, SIL_FAULT },
+#endif
+ [SIGCHLD] = { NSIGCHLD, SIL_CHLD },
+ [SIGPOLL] = { NSIGPOLL, SIL_POLL },
+ [SIGSYS] = { NSIGSYS, SIL_SYS },
+};
+
+static bool known_siginfo_layout(int sig, int si_code)
+{
+ if (si_code == SI_KERNEL)
+ return true;
+ else if ((si_code > SI_USER)) {
+ if (sig_specific_sicodes(sig)) {
+ if (si_code <= sig_sicodes[sig].limit)
+ return true;
+ }
+ else if (si_code <= NSIGPOLL)
+ return true;
+ }
+ else if (si_code >= SI_DETHREAD)
+ return true;
+ else if (si_code == SI_ASYNCNL)
+ return true;
+ return false;
+}
+
enum siginfo_layout siginfo_layout(int sig, int si_code)
{
enum siginfo_layout layout = SIL_KILL;
if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
- static const struct {
- unsigned char limit, layout;
- } filter[] = {
- [SIGILL] = { NSIGILL, SIL_FAULT },
- [SIGFPE] = { NSIGFPE, SIL_FAULT },
- [SIGSEGV] = { NSIGSEGV, SIL_FAULT },
- [SIGBUS] = { NSIGBUS, SIL_FAULT },
- [SIGTRAP] = { NSIGTRAP, SIL_FAULT },
-#if defined(SIGEMT)
- [SIGEMT] = { NSIGEMT, SIL_FAULT },
-#endif
- [SIGCHLD] = { NSIGCHLD, SIL_CHLD },
- [SIGPOLL] = { NSIGPOLL, SIL_POLL },
- [SIGSYS] = { NSIGSYS, SIL_SYS },
- };
- if ((sig < ARRAY_SIZE(filter)) && (si_code <= filter[sig].limit)) {
- layout = filter[sig].layout;
+ if ((sig < ARRAY_SIZE(sig_sicodes)) &&
+ (si_code <= sig_sicodes[sig].limit)) {
+ layout = sig_sicodes[sig].layout;
/* Handle the exceptions */
if ((sig == SIGBUS) &&
(si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
@@ -2889,17 +2910,42 @@ enum siginfo_layout siginfo_layout(int sig, int si_code)
return layout;
}
+static inline char __user *si_expansion(const siginfo_t __user *info)
+{
+ return ((char __user *)info) + sizeof(struct kernel_siginfo);
+}
+
int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
{
+ char __user *expansion = si_expansion(to);
if (copy_to_user(to, from , sizeof(struct kernel_siginfo)))
return -EFAULT;
+ if (clear_user(expansion, SI_EXPANSION_SIZE))
+ return -EFAULT;
return 0;
}
int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
{
- if (copy_from_user(to, from, sizeof(struct siginfo)))
+ if (copy_from_user(to, from, sizeof(struct kernel_siginfo)))
return -EFAULT;
+ if (unlikely(!known_siginfo_layout(to->si_signo, to->si_code))) {
+ char __user *expansion = si_expansion(from);
+ char buf[SI_EXPANSION_SIZE];
+ int i;
+ /*
+ * An unknown si_code might need more than
+ * sizeof(struct kernel_siginfo) bytes. Verify all of the
+ * extra bytes are 0. This guarantees copy_siginfo_to_user
+ * will return this data to userspace exactly.
+ */
+ if (copy_from_user(&buf, expansion, SI_EXPANSION_SIZE))
+ return -EFAULT;
+ for (i = 0; i < SI_EXPANSION_SIZE; i++) {
+ if (buf[i] != 0)
+ return -E2BIG;
+ }
+ }
return 0;
}
--
2.17.1
^ permalink raw reply related
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