* Re: [PATCH 03/11] UAPI: virtio_net: Fix use of C++ keywords as structural members [ver #2]
From: Michael S. Tsirkin @ 2018-09-06 15:02 UTC (permalink / raw)
To: David Howells
Cc: linux-api, linux-kbuild, Jason Wang, virtualization, linux-kernel
In-Reply-To: <153622552270.14298.14568295161017777604.stgit@warthog.procyon.org.uk>
On Thu, Sep 06, 2018 at 10:18:42AM +0100, David Howells wrote:
> The virtio_net_ctrl_hdr struct uses a C++ keyword as structural members. Fix
> this by inserting an anonymous union that provides an alternative name and
> then hide the reserved name in C++.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: "Michael S. Tsirkin" <mst@redhat.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: virtualization@lists.linux-foundation.org
> ---
>
> include/uapi/linux/virtio_net.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index a3715a3224c1..967142bc0e05 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -150,7 +150,12 @@ struct virtio_net_hdr_mrg_rxbuf {
> * command goes in between.
> */
> struct virtio_net_ctrl_hdr {
> - __u8 class;
> + union {
> +#ifndef __cplusplus
> + __u8 class;
> +#endif
> + __u8 _class;
> + };
> __u8 cmd;
> } __attribute__((packed));
>
So if we are going to do this, I think I'd prefer something like:
struct virtio_net_ctrl_hdr_v2 {
__u8 cmd_class;
__u8 cmd;
};
And then hide the whole old structure. This also gets rid of the packed
keyword which we don't really need here.
Only issue is virtio_net_ctrl_hdr_v2 is ugly. But oh well.
And then rework at least QEMU to use the v2 of the header.
Quite a bit of churn, so I don't think it makes sense to apply just this
one in isolation - only if rest of the changes go in.
--
MST
^ permalink raw reply
* [PATCH v6 0/5] seccomp trap to userspace
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
Hi all,
Here's a v6 of the seccomp trap to userspace series. v5 [1] was fairly
quiet, with Christian reminding me that I had forgotten to update the
docs for the ioctl change. Other than that, there are no changes.
[1]: https://lkml.org/lkml/2018/8/28/590
Thoughts welcome,
Tycho
Tycho Andersen (5):
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
seccomp: add support for passing fds via USER_NOTIF
samples: add an example of seccomp user trap
Documentation/ioctl/ioctl-number.txt | 1 +
.../userspace-api/seccomp_filter.rst | 84 +++
arch/Kconfig | 9 +
include/linux/seccomp.h | 18 +-
include/uapi/linux/ptrace.h | 2 +
include/uapi/linux/seccomp.h | 36 +-
kernel/ptrace.c | 4 +
kernel/seccomp.c | 538 +++++++++++++++-
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 312 ++++++++++
tools/testing/selftests/seccomp/seccomp_bpf.c | 587 +++++++++++++++++-
12 files changed, 1586 insertions(+), 13 deletions(-)
create mode 100644 samples/seccomp/user-trap.c
--
2.17.1
^ permalink raw reply
* [PATCH v6 1/5] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
In-Reply-To: <20180906152859.7810-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)
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 +++
arch/Kconfig | 9 +
include/linux/seccomp.h | 7 +-
include/uapi/linux/seccomp.h | 33 +-
kernel/seccomp.c | 453 +++++++++++++++++-
tools/testing/selftests/seccomp/seccomp_bpf.c | 403 +++++++++++++++-
7 files changed, 969 insertions(+), 10 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..d1498885c1c7 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 globally unique ``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/arch/Kconfig b/arch/Kconfig
index 6801123932a5..42f3585d925d 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -419,6 +419,15 @@ config SECCOMP_FILTER
See Documentation/userspace-api/seccomp_filter.rst for details.
+config SECCOMP_USER_NOTIFICATION
+ bool "Enable the SECCOMP_RET_USER_NOTIF seccomp action"
+ depends on SECCOMP_FILTER
+ help
+ Enable SECCOMP_RET_USER_NOTIF, a return code which can be used by seccomp
+ programs to notify a userspace listener that a particular event happened.
+
+ See Documentation/userspace-api/seccomp_filter.rst for details.
+
config HAVE_STACKPROTECTOR
bool
help
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..aa5878972128 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 signalled;
+ 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_IS_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..a09eb5c05f68 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -33,6 +33,7 @@
#endif
#ifdef CONFIG_SECCOMP_FILTER
+#include <linux/file.h>
#include <linux/filter.h>
#include <linux/pid.h>
#include <linux/ptrace.h>
@@ -40,6 +41,53 @@
#include <linux/tracehook.h>
#include <linux/uaccess.h>
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+#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 pid *pid;
+
+ /* The "cookie" for this request; this is unique for this filter. */
+ u32 id;
+
+ /* Whether or not this task has been given an interruptible signal. */
+ bool signalled;
+
+ /*
+ * 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;
+};
+#endif
+
/**
* struct seccomp_filter - container for seccomp BPF programs
*
@@ -66,6 +114,30 @@ struct seccomp_filter {
bool log;
struct seccomp_filter *prev;
struct bpf_prog *prog;
+
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+ /*
+ * A semaphore that users of this notification can wait on for
+ * changes. Actual reads and writes are still controlled with
+ * filter->notify_lock.
+ */
+ struct semaphore request;
+
+ /* A lock for all notification-related accesses. */
+ struct mutex notify_lock;
+
+ /* Is there currently an attached listener? */
+ bool has_listener;
+
+ /* The id of the next request. */
+ u64 next_id;
+
+ /* A list of struct seccomp_knotif elements. */
+ struct list_head notifications;
+
+ /* A wait queue for poll. */
+ wait_queue_head_t wqh;
+#endif
};
/* Limit any path through the tree to 256KB worth of instructions. */
@@ -359,6 +431,19 @@ static inline void seccomp_sync_threads(unsigned long flags)
}
}
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+static void init_user_notification(struct seccomp_filter *sfilter)
+{
+ mutex_init(&sfilter->notify_lock);
+ sema_init(&sfilter->request, 0);
+ INIT_LIST_HEAD(&sfilter->notifications);
+ sfilter->next_id = get_random_u64();
+ init_waitqueue_head(&sfilter->wqh);
+}
+#else
+static inline void init_user_notification(struct seccomp_filter *sfilter) { }
+#endif
+
/**
* seccomp_prepare_filter: Prepares a seccomp filter for use.
* @fprog: BPF program to install
@@ -392,6 +477,8 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
if (!sfilter)
return ERR_PTR(-ENOMEM);
+ init_user_notification(sfilter);
+
ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
seccomp_check_filter, save_orig);
if (ret < 0) {
@@ -556,13 +643,15 @@ 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_TRACE |
- SECCOMP_LOG_LOG;
+ SECCOMP_LOG_LOG |
+ SECCOMP_LOG_USER_NOTIF;
static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
bool requested)
@@ -581,6 +670,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;
@@ -651,6 +743,83 @@ void secure_computing_strict(int this_syscall)
}
#else
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
+{
+ /* Note: overflow is ok here, the id just needs to be unique */
+ return filter->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->has_listener)
+ goto out;
+
+ n.pid = task_pid(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->notifications);
+ wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
+
+ mutex_unlock(&match->notify_lock);
+ up(&match->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.signalled = true;
+ if (n.state == SECCOMP_NOTIFY_SENT) {
+ n.state = SECCOMP_NOTIFY_INIT;
+ up(&match->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);
+}
+#else
+static void seccomp_do_user_notification(int this_syscall,
+ struct seccomp_filter *match,
+ const struct seccomp_data *sd)
+{
+ seccomp_log(this_syscall, SIGSYS, SECCOMP_RET_USER_NOTIF, true);
+ do_exit(SIGSYS);
+}
+#endif
+
#ifdef CONFIG_SECCOMP_FILTER
static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
const bool recheck_after_trace)
@@ -728,6 +897,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 +1006,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 +1028,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 +1040,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 +1079,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;
@@ -915,6 +1117,9 @@ static long seccomp_get_action_avail(const char __user *uaction)
case SECCOMP_RET_LOG:
case SECCOMP_RET_ALLOW:
break;
+ case SECCOMP_RET_USER_NOTIF:
+ if (IS_ENABLED(CONFIG_SECCOMP_USER_NOTIFICATION))
+ break;
default:
return -EOPNOTSUPP;
}
@@ -1111,6 +1316,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 +1326,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;
@@ -1137,6 +1344,7 @@ static const struct seccomp_log_name seccomp_log_names[] = {
{ SECCOMP_LOG_TRACE, SECCOMP_RET_TRACE_NAME },
{ SECCOMP_LOG_LOG, SECCOMP_RET_LOG_NAME },
{ SECCOMP_LOG_ALLOW, SECCOMP_RET_ALLOW_NAME },
+ { SECCOMP_LOG_USER_NOTIF, SECCOMP_RET_USER_NOTIF_NAME },
{ }
};
@@ -1342,3 +1550,244 @@ static int __init seccomp_sysctl_init(void)
device_initcall(seccomp_sysctl_init)
#endif /* CONFIG_SYSCTL */
+
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+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->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->wqh);
+ filter->has_listener = false;
+ 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->request);
+ if (ret < 0)
+ return ret;
+
+ mutex_lock(&filter->notify_lock);
+ list_for_each_entry(cur, &filter->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 = pid_vnr(knotif->pid);
+ unotif.signalled = knotif->signalled;
+ unotif.data = *(knotif->data);
+
+ if (copy_to_user(buf, &unotif, size)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = sizeof(unotif);
+ knotif->state = SECCOMP_NOTIFY_SENT;
+ wake_up_poll(&filter->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->notifications, list) {
+ if (knotif->id == resp.id)
+ break;
+ }
+
+ if (!knotif || knotif->id != resp.id) {
+ ret = -EINVAL;
+ 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_is_id_valid(struct seccomp_filter *filter,
+ unsigned long arg)
+{
+ struct seccomp_knotif *knotif = NULL;
+ void __user *buf = (void __user *)arg;
+ u64 id;
+
+ if (copy_from_user(&id, buf, sizeof(id)))
+ return -EFAULT;
+
+ list_for_each_entry(knotif, &filter->notifications, list) {
+ if (knotif->id == id)
+ return 1;
+ }
+
+ return 0;
+}
+
+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_IS_ID_VALID:
+ return seccomp_notify_is_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->wqh, poll_tab);
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ return ret;
+
+ list_for_each_entry(cur, &filter->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->has_listener)
+ goto out;
+ }
+
+ 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);
+ filter->has_listener = true;
+
+out:
+ for (cur = task->seccomp.filter; cur; cur = cur->prev) {
+ mutex_unlock(&cur->notify_lock);
+ if (cur == last_locked)
+ break;
+ }
+
+ return ret;
+}
+#else
+static struct file *init_listener(struct task_struct *task,
+ struct seccomp_filter *filter)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index e1473234968d..89f2c788a06b 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_IS_ID_VALID _IOR(SECCOMP_IOC_MAGIC, 2, \
+ __u64)
+struct seccomp_notif {
+ __u16 len;
+ __u64 id;
+ __u32 pid;
+ __u8 signalled;
+ 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,373 @@ 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_IS_ID_VALID, &req.id), 1);
+
+ EXPECT_EQ(kill(pid, SIGKILL), 0);
+ EXPECT_EQ(waitpid(pid, NULL, 0), pid);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_IS_ID_VALID, &req.id), 0);
+
+ resp.id = req.id;
+ ret = ioctl(listener, SECCOMP_NOTIF_SEND, &resp);
+ EXPECT_EQ(ret, -1);
+ EXPECT_EQ(errno, EINVAL);
+
+ /*
+ * 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.signalled, 1);
+ EXPECT_EQ(ret, sizeof(req));
+ EXPECT_EQ(errno, 0);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ 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 v6 2/5] seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
In-Reply-To: <20180906152859.7810-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.
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 | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index a09eb5c05f68..ed786655186d 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1188,7 +1188,8 @@ 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_CHECKPOINT_RESTORE) || \
+ defined(CONFIG_SECCOMP_USER_NOTIFICATION)
static struct seccomp_filter *get_nth_filter(struct task_struct *task,
unsigned long filter_off)
{
@@ -1235,6 +1236,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)
{
@@ -1307,7 +1309,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 v6 3/5] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
In-Reply-To: <20180906152859.7810-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
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 | 11 +++
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, 116 insertions(+)
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 017444b5efed..c17c7d051af0 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -112,4 +112,15 @@ static inline long seccomp_get_metadata(struct task_struct *task,
return -EINVAL;
}
#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
+
+#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
+extern long seccomp_new_listener(struct task_struct *task,
+ unsigned long filter_off);
+#else
+static inline long seccomp_new_listener(struct task_struct *task,
+ unsigned long filter_off)
+{
+ return -EINVAL;
+}
+#endif/* CONFIG_SECCOMP_USER_NOTIFICATION */
#endif /* _LINUX_SECCOMP_H */
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 ed786655186d..580888785324 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1787,6 +1787,37 @@ 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, task->seccomp.filter);
+ __put_seccomp_filter(filter);
+ if (IS_ERR(listener)) {
+ put_unused_fd(fd);
+ return PTR_ERR(listener);
+ }
+
+ fd_install(fd, listener);
+ return fd;
+}
#else
static struct file *init_listener(struct task_struct *task,
struct seccomp_filter *filter)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 89f2c788a06b..61b8e3c5c06b 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
@@ -3165,6 +3169,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 v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
In-Reply-To: <20180906152859.7810-1-tycho@tycho.ws>
The idea here is that the userspace handler should be able to pass an fd
back to the trapped task, for example so it can be returned from socket().
I've proposed one API here, but I'm open to other options. In particular,
this only lets you return an fd from a syscall, which may not be enough in
all cases. For example, if an fd is written to an output parameter instead
of returned, the current API can't handle this. Another case is that
netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
ever decides to install an fd and output it, we wouldn't be able to handle
this either.
Still, the vast majority of interesting cases are covered by this API, so
perhaps it is Enough.
I've left it as a separate commit for two reasons:
* It illustrates the way in which we would grow struct seccomp_notif and
struct seccomp_notif_resp without using netlink
* It shows just how little code is needed to accomplish this :)
v2: new in v2
v3: no changes
v4: * pass fd flags back from userspace as well (Jann)
* update same cgroup data on fd pass as SCM_RIGHTS (Alban)
* only set the REPLIED state /after/ successful fdget (Alban)
* reflect GET_LISTENER -> NEW_LISTENER changes
* add to the new Documentation/ on user notifications about fd replies
v5: * fix documentation typo (O_EXCL -> O_CLOEXEC)
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>
---
.../userspace-api/seccomp_filter.rst | 11 ++
include/uapi/linux/seccomp.h | 3 +
kernel/seccomp.c | 51 +++++++-
tools/testing/selftests/seccomp/seccomp_bpf.c | 116 ++++++++++++++++++
4 files changed, 179 insertions(+), 2 deletions(-)
diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index d1498885c1c7..1c0aab306426 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
__u64 id;
__s32 error;
__s64 val;
+ __u8 return_fd;
+ __u32 fd;
+ __u32 fd_flags;
};
Users can read via ``ioctl(SECCOMP_NOTIF_RECV)`` (or ``poll()``) on a seccomp
@@ -256,6 +259,14 @@ 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.
+Userspace can also return file descriptors. For example, one may decide to
+intercept ``socket()`` syscalls, and return some file descriptor from those
+based on some policy. To return a file descriptor, the ``return_fd`` member
+should be non-zero, the ``fd`` argument should be the fd in the listener's
+table to send to the tracee (similar to how ``SCM_RIGHTS`` works), and
+``fd_flags`` should be the flags that the fd in the tracee's table is opened
+with (e.g. ``O_CLOEXEC`` or similar).
+
Sysctls
=======
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index aa5878972128..93f1bd5c7cf0 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -75,6 +75,9 @@ struct seccomp_notif_resp {
__u64 id;
__s32 error;
__s64 val;
+ __u8 return_fd;
+ __u32 fd;
+ __u32 fd_flags;
};
#define SECCOMP_IOC_MAGIC 0xF7
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 580888785324..4a6db4076ec5 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -43,6 +43,7 @@
#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
#include <linux/anon_inodes.h>
+#include <net/cls_cgroup.h>
enum notify_state {
SECCOMP_NOTIFY_INIT,
@@ -80,6 +81,8 @@ struct seccomp_knotif {
/* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
int error;
long val;
+ struct file *file;
+ unsigned int flags;
/* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
struct completion ready;
@@ -800,10 +803,44 @@ static void seccomp_do_user_notification(int this_syscall,
goto remove_list;
}
- ret = n.val;
- err = n.error;
+ if (n.file) {
+ int fd;
+ struct socket *sock;
+
+ fd = get_unused_fd_flags(n.flags);
+ if (fd < 0) {
+ err = fd;
+ ret = -1;
+ goto remove_list;
+ }
+
+ /*
+ * Similar to what SCM_RIGHTS does, let's re-set the cgroup
+ * data to point ot the tracee's cgroups instead of the
+ * listener's.
+ */
+ sock = sock_from_file(n.file, &err);
+ if (sock) {
+ sock_update_netprioidx(&sock->sk->sk_cgrp_data);
+ sock_update_classid(&sock->sk->sk_cgrp_data);
+ }
+
+ ret = fd;
+ err = 0;
+
+ fd_install(fd, n.file);
+ /* Don't fput, since fd has a reference now */
+ n.file = NULL;
+ } else {
+ ret = n.val;
+ err = n.error;
+ }
+
remove_list:
+ if (n.file)
+ fput(n.file);
+
list_del(&n.list);
out:
mutex_unlock(&match->notify_lock);
@@ -1675,10 +1712,20 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
goto out;
}
+ if (resp.return_fd) {
+ knotif->flags = resp.fd_flags;
+ knotif->file = fget(resp.fd);
+ if (!knotif->file) {
+ ret = -EBADF;
+ 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);
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 61b8e3c5c06b..c756722faa88 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -182,6 +182,9 @@ struct seccomp_notif_resp {
__u64 id;
__s32 error;
__s64 val;
+ __u8 return_fd;
+ __u32 fd;
+ __u32 fd_flags;
};
#endif
@@ -3233,6 +3236,119 @@ TEST(get_user_notification_ptrace)
close(listener);
}
+TEST(user_notification_pass_fd)
+{
+ pid_t pid;
+ int status, listener;
+ int sk_pair[2];
+ char c;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+ long ret;
+
+ ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ int fd;
+ char buf[16];
+
+ 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');
+ close(sk_pair[1]);
+
+ /* An fd from getpid(). Let the games begin. */
+ fd = syscall(__NR_getpid);
+ EXPECT_GT(fd, 0);
+ EXPECT_EQ(read(fd, buf, sizeof(buf)), 12);
+ close(fd);
+
+ exit(strcmp("hello world", buf));
+ }
+
+ 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 installing so it can do a getpid */
+ EXPECT_EQ(write(sk_pair[0], "H", 1), 1);
+ close(sk_pair[0]);
+
+ /* Make a new socket pair so we can send half across */
+ EXPECT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
+
+ ret = read_notif(listener, &req);
+ EXPECT_EQ(ret, sizeof(req));
+ EXPECT_EQ(errno, 0);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.return_fd = 1;
+ resp.fd = sk_pair[1];
+ resp.fd_flags = 0;
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
+ close(sk_pair[1]);
+
+ EXPECT_EQ(write(sk_pair[0], "hello world\0", 12), 12);
+ close(sk_pair[0]);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ close(listener);
+}
+
+TEST(user_notification_struct_size_mismatch)
+{
+ pid_t pid;
+ long ret;
+ int status, listener, len;
+ struct seccomp_notif req;
+ struct seccomp_notif_resp resp;
+
+ listener = user_trap_syscall(__NR_getpid,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ EXPECT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ ret = syscall(__NR_getpid);
+ exit(ret != USER_NOTIF_MAGIC);
+ }
+
+ req.len = sizeof(req);
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
+
+ /*
+ * Only write a partial structure: this is what was available before we
+ * had fd support.
+ */
+ len = offsetof(struct seccomp_notif_resp, val) + sizeof(resp.val);
+ resp.len = len;
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+ EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), len);
+
+ 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.
*/
--
2.17.1
^ permalink raw reply related
* [PATCH v6 5/5] samples: add an example of seccomp user trap
From: Tycho Andersen @ 2018-09-06 15:28 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,
Tycho Andersen
In-Reply-To: <20180906152859.7810-1-tycho@tycho.ws>
The idea here is just to give a demonstration of how one could safely use
the SECCOMP_RET_USER_NOTIF feature to do mount policies. This particular
policy is (as noted in the comment) not very interesting, but it serves to
illustrate how one might apply a policy dodging the various TOCTOU issues.
v5: new in v5
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>
---
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 312 ++++++++++++++++++++++++++++++++++++
3 files changed, 319 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index 78fb78184291..d1e2e817d556 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -1,3 +1,4 @@
bpf-direct
bpf-fancy
dropper
+user-trap
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index cf34ff6b4065..4920903c8009 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
+hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct user-trap
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -16,6 +16,10 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
+user-trap-objs := user-trap.o
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -33,6 +37,7 @@ HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
+HOSTLDLIBS_user-trap += $(MFLAG)
endif
always := $(hostprogs-m)
endif
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
new file mode 100644
index 000000000000..571eb32fd80b
--- /dev/null
+++ b/samples/seccomp/user-trap.c
@@ -0,0 +1,312 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stddef.h>
+#include <sys/sysmacros.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/user.h>
+#include <sys/ioctl.h>
+#include <sys/ptrace.h>
+#include <sys/mount.h>
+#include <linux/limits.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+
+/*
+ * Because of some grossness, we can't include linux/ptrace.h here, so we
+ * re-define PTRACE_SECCOMP_NEW_LISTENER.
+ */
+#ifndef PTRACE_SECCOMP_NEW_LISTENER
+#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
+#endif
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+static int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+static int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+static int handle_req(struct seccomp_notif *req,
+ struct seccomp_notif_resp *resp, int listener)
+{
+ char path[PATH_MAX], source[PATH_MAX], target[PATH_MAX];
+ int ret = -1, mem;
+
+ resp->len = sizeof(*resp);
+ resp->id = req->id;
+ resp->error = -EPERM;
+ resp->val = 0;
+
+ if (req->data.nr != __NR_mount) {
+ fprintf(stderr, "huh? trapped something besides mknod? %d\n", req->data.nr);
+ return -1;
+ }
+
+ /* Only allow bind mounts. */
+ if (!(req->data.args[3] & MS_BIND))
+ return 0;
+
+ /*
+ * Ok, let's read the task's memory to see where they wanted their
+ * mount to go.
+ */
+ snprintf(path, sizeof(path), "/proc/%d/mem", req->pid);
+ mem = open(path, O_RDONLY);
+ if (mem < 0) {
+ perror("open mem");
+ return -1;
+ }
+
+ /*
+ * Now we avoid a TOCTOU: we referred to a pid by its pid, but since
+ * the pid that made the syscall may have died, we need to confirm that
+ * the pid is still valid after we open its /proc/pid/mem file. We can
+ * ask the listener fd this as follows.
+ *
+ * Note that this check should occur *after* any task-specific
+ * resources are opened, to make sure that the task has not died and
+ * we're not wrongly reading someone else's state in order to make
+ * decisions.
+ */
+ if (ioctl(listener, SECCOMP_NOTIF_IS_ID_VALID, &req->id) != 1) {
+ fprintf(stderr, "task died before we could map its memory\n");
+ goto out;
+ }
+
+ /*
+ * Phew, we've got the right /proc/pid/mem. Now we can read it. Note
+ * that to avoid another TOCTOU, we should read all of the pointer args
+ * before we decide to allow the syscall.
+ */
+ if (lseek(mem, req->data.args[0], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, source, sizeof(source));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ if (lseek(mem, req->data.args[1], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, target, sizeof(target));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ /*
+ * Our policy is to only allow bind mounts inside /tmp. This isn't very
+ * interesting, because we could do unprivlieged bind mounts with user
+ * namespaces already, but you get the idea.
+ */
+ if (!strncmp(source, "/tmp", 4) && !strncmp(target, "/tmp", 4)) {
+ if (mount(source, target, NULL, req->data.args[3], NULL) < 0) {
+ ret = -1;
+ perror("actual mount");
+ goto out;
+ }
+ resp->error = 0;
+ }
+
+ /* Even if we didn't allow it because of policy, generating the
+ * response was be a success, because we want to tell the worker EPERM.
+ */
+ ret = 0;
+
+out:
+ close(mem);
+ return ret;
+}
+
+int main(void)
+{
+ int sk_pair[2], ret = 1, status, listener;
+ pid_t worker = 0 , tracer = 0;
+ char c;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ return 1;
+ }
+
+ worker = fork();
+ if (worker < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+
+ if (worker == 0) {
+ if (user_trap_syscall(__NR_mount, 0) < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+
+ if (setuid(1000) < 0) {
+ perror("setuid");
+ exit(1);
+ }
+
+ if (write(sk_pair[1], "a", 1) != 1) {
+ perror("write");
+ exit(1);
+ }
+
+ if (read(sk_pair[1], &c, 1) != 1) {
+ perror("write");
+ exit(1);
+ }
+
+ if (mkdir("/tmp/foo", 0755) < 0) {
+ perror("mkdir");
+ exit(1);
+ }
+
+ if (mount("/dev/sda", "/tmp/foo", NULL, 0, NULL) != -1) {
+ fprintf(stderr, "huh? mounted /dev/sda?\n");
+ exit(1);
+ }
+
+ if (errno != EPERM) {
+ perror("bad error from mount");
+ exit(1);
+ }
+
+ if (mount("/tmp/foo", "/tmp/foo", NULL, MS_BIND, NULL) < 0) {
+ perror("mount");
+ exit(1);
+ }
+
+ exit(0);
+ }
+
+ if (read(sk_pair[0], &c, 1) != 1) {
+ perror("read ready signal");
+ goto out_kill;
+ }
+
+ if (ptrace(PTRACE_ATTACH, worker) < 0) {
+ perror("ptrace");
+ goto out_kill;
+ }
+
+ if (waitpid(worker, NULL, 0) != worker) {
+ perror("waitpid");
+ goto out_kill;
+ }
+
+ listener = ptrace(PTRACE_SECCOMP_NEW_LISTENER, worker, 0);
+ if (listener < 0) {
+ perror("ptrace get listener");
+ goto out_kill;
+ }
+
+ if (ptrace(PTRACE_DETACH, worker, NULL, 0) < 0) {
+ perror("ptrace detach");
+ goto out_kill;
+ }
+
+ if (write(sk_pair[0], "a", 1) != 1) {
+ perror("write");
+ exit(1);
+ }
+
+ tracer = fork();
+ if (tracer < 0) {
+ perror("fork");
+ goto out_kill;
+ }
+
+ if (tracer == 0) {
+ while (1) {
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ req.len = sizeof(req);
+ if (ioctl(listener, SECCOMP_NOTIF_RECV, &req) != sizeof(req)) {
+ perror("ioctl recv");
+ goto out_close;
+ }
+
+ if (handle_req(&req, &resp, listener) < 0)
+ goto out_close;
+
+ if (ioctl(listener, SECCOMP_NOTIF_SEND, &resp) != sizeof(resp)) {
+ perror("ioctl send");
+ goto out_close;
+ }
+ }
+out_close:
+ close(listener);
+ exit(1);
+ }
+
+ close(listener);
+
+ if (waitpid(worker, &status, 0) != worker) {
+ perror("waitpid");
+ goto out_kill;
+ }
+
+ if (umount2("/tmp/foo", MNT_DETACH) < 0 && errno != EINVAL) {
+ perror("umount2");
+ goto out_kill;
+ }
+
+ if (remove("/tmp/foo") < 0 && errno != ENOENT) {
+ perror("remove");
+ exit(1);
+ }
+
+ if (!WIFEXITED(status) || WEXITSTATUS(status)) {
+ fprintf(stderr, "worker exited nonzero\n");
+ goto out_kill;
+ }
+
+ ret = 0;
+
+out_kill:
+ if (tracer > 0)
+ kill(tracer, SIGKILL);
+ if (worker > 0)
+ kill(worker, SIGKILL);
+
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ return ret;
+}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-09-06 15:45 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <20180906152859.7810-4-tycho@tycho.ws>
On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> 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.
[...]
> +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, task->seccomp.filter);
Did you mean to write something like `init_listener(task, filter)` here?
^ permalink raw reply
* Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-09-06 15:50 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <CAG48ez3gRpGZT4Vk7ZZXk5dkYeVfnGsaqeFZeOio0GpqFd=Vow@mail.gmail.com>
On Thu, Sep 06, 2018 at 05:45:25PM +0200, Jann Horn wrote:
> On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > 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.
> [...]
> > +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, task->seccomp.filter);
>
> Did you mean to write something like `init_listener(task, filter)` here?
Yes, yes I did. Thanks, Jann.
Tycho
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Jann Horn @ 2018-09-06 16:15 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <20180906152859.7810-5-tycho@tycho.ws>
On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
[...]
> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> index d1498885c1c7..1c0aab306426 100644
> --- a/Documentation/userspace-api/seccomp_filter.rst
> +++ b/Documentation/userspace-api/seccomp_filter.rst
> @@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
> __u64 id;
> __s32 error;
> __s64 val;
> + __u8 return_fd;
> + __u32 fd;
> + __u32 fd_flags;
Normally, syscalls that take an optional file descriptor accept a
signed 32-bit number, with -1 standing for "no file descriptor". Is
there a reason why this uses a separate variable to signal whether an
fd was provided?
Apart from that, this patch looks good to me.
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-09-06 16:22 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <CAG48ez2oQnAb=OMGOUz0GfSWMsjqydPe8dmp6L=QswSQpSpVXA@mail.gmail.com>
On Thu, Sep 06, 2018 at 06:15:18PM +0200, Jann Horn wrote:
> On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > The idea here is that the userspace handler should be able to pass an fd
> > back to the trapped task, for example so it can be returned from socket().
> [...]
> > diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> > index d1498885c1c7..1c0aab306426 100644
> > --- a/Documentation/userspace-api/seccomp_filter.rst
> > +++ b/Documentation/userspace-api/seccomp_filter.rst
> > @@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
> > __u64 id;
> > __s32 error;
> > __s64 val;
> > + __u8 return_fd;
> > + __u32 fd;
> > + __u32 fd_flags;
>
> Normally, syscalls that take an optional file descriptor accept a
> signed 32-bit number, with -1 standing for "no file descriptor". Is
> there a reason why this uses a separate variable to signal whether an
> fd was provided?
No real reason other than I looked at the bpf code and they were using
__u32 for bpf (but I think in their case the fd args are not
optional). I'll switch it to __s32/-1 for the next version.
> Apart from that, this patch looks good to me.
Thanks,
Tycho
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-09-06 18:30 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <20180906162246.GB3326@cisco.cisco.com>
On Thu, Sep 06, 2018 at 10:22:46AM -0600, Tycho Andersen wrote:
> On Thu, Sep 06, 2018 at 06:15:18PM +0200, Jann Horn wrote:
> > On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > The idea here is that the userspace handler should be able to pass an fd
> > > back to the trapped task, for example so it can be returned from socket().
> > [...]
> > > diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> > > index d1498885c1c7..1c0aab306426 100644
> > > --- a/Documentation/userspace-api/seccomp_filter.rst
> > > +++ b/Documentation/userspace-api/seccomp_filter.rst
> > > @@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
> > > __u64 id;
> > > __s32 error;
> > > __s64 val;
> > > + __u8 return_fd;
> > > + __u32 fd;
> > > + __u32 fd_flags;
> >
> > Normally, syscalls that take an optional file descriptor accept a
> > signed 32-bit number, with -1 standing for "no file descriptor". Is
> > there a reason why this uses a separate variable to signal whether an
> > fd was provided?
>
> No real reason other than I looked at the bpf code and they were using
> __u32 for bpf (but I think in their case the fd args are not
> optional). I'll switch it to __s32/-1 for the next version.
Oh, I think there is a reason actually: since this is an API addition,
the "0" value needs to be the previously default behavior if userspace
doesn't specify it. Since the previously default behavior was not to
return an fd, and we want to allow fd == 0, we need the extra flag to
make this work.
This is really only a problem because we're introducing this stuff in
a second patch (mostly to illustrate how extending the response
structure would work). I could fold this into the first patch if we
want, or we could keep the return_fd bits if the illustration is
useful.
Tycho
^ permalink raw reply
* Re: [PATCH v6 1/5] seccomp: add a return code to trap to userspace
From: Tyler Hicks @ 2018-09-06 22:15 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Akihiro Suda, Jann Horn
In-Reply-To: <20180906152859.7810-2-tycho@tycho.ws>
[-- Attachment #1: Type: text/plain, Size: 34795 bytes --]
Hey Tycho - I'm finally getting around to reviewing this patch set. I
don't have access to previous review comments while I'm doing this
review so I hope I'm not revisiting too many previous discussions.
On 2018-09-06 09:28:55, Tycho Andersen wrote:
> This patch introduces a means for syscalls matched in seccomp to notify
> some other task that a particular filter has been triggered.
>
> The motivation for this is primarily for use with containers. For example,
> if a container does an init_module(), we obviously don't want to load this
> untrusted code, which may be compiled for the wrong version of the kernel
> anyway. Instead, we could parse the module image, figure out which module
> the container is trying to load and load it on the host.
>
> As another example, containers cannot 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)
>
> 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 +++
> arch/Kconfig | 9 +
> include/linux/seccomp.h | 7 +-
> include/uapi/linux/seccomp.h | 33 +-
> kernel/seccomp.c | 453 +++++++++++++++++-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 403 +++++++++++++++-
> 7 files changed, 969 insertions(+), 10 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..d1498885c1c7 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 globally unique ``id``, the
This documentation says that id is "globally unique" but an in-code
comment below says "this is unique for this filter". IIUC, the id is
only guaranteed to be unique for the filter so this documentation should
be updated slightly to make it clear that the id is only global in those
terms.
> +``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/arch/Kconfig b/arch/Kconfig
> index 6801123932a5..42f3585d925d 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -419,6 +419,15 @@ config SECCOMP_FILTER
>
> See Documentation/userspace-api/seccomp_filter.rst for details.
>
> +config SECCOMP_USER_NOTIFICATION
Did someone request a Kconfig option for this new feature? If not, I
think that nuking the Kconfig option would reduce the test matrix. No
other filter flags have their own build time option but maybe it makes
sense in this case if this filter flag exposes the kernel to significant
new attack surface since there's more to this than just a new filter
flag.
If someone has a requirement to disable this feature, maybe it'd be
better to leave the decision up to the distro *and* the admin via a
sysctl instead of taking the admin out of the decision with a build
time option.
> + bool "Enable the SECCOMP_RET_USER_NOTIF seccomp action"
> + depends on SECCOMP_FILTER
> + help
> + Enable SECCOMP_RET_USER_NOTIF, a return code which can be used by seccomp
> + programs to notify a userspace listener that a particular event happened.
> +
> + See Documentation/userspace-api/seccomp_filter.rst for details.
> +
> config HAVE_STACKPROTECTOR
> bool
> help
> 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..aa5878972128 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 signalled;
I think signaled is the best spelling to go with. There are a lot of
other instances of signalled in the kernel sources but, ultimately, it
makes sense to follow the lead of the WIFSIGNALED macro from wait(2).
> + 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)
This is pedantic but it would make sense to me to have the ioctl names
match the struct names. That would leave us with:
#define SECCOMP_NOTIF _IOWR(SECCOMP_IOC_MAGIC, 0, \
struct seccomp_notif)
#define SECCOMP_NOTIF_RESP _IOWR(SECCOMP_IOC_MAGIC, 1, \
struct seccomp_notif_resp)
Change it if you agree. Ignore this comment if you don't.
> +#define SECCOMP_NOTIF_IS_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..a09eb5c05f68 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -33,6 +33,7 @@
> #endif
>
> #ifdef CONFIG_SECCOMP_FILTER
> +#include <linux/file.h>
> #include <linux/filter.h>
> #include <linux/pid.h>
> #include <linux/ptrace.h>
> @@ -40,6 +41,53 @@
> #include <linux/tracehook.h>
> #include <linux/uaccess.h>
>
> +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> +#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 pid *pid;
> +
> + /* The "cookie" for this request; this is unique for this filter. */
> + u32 id;
> +
> + /* Whether or not this task has been given an interruptible signal. */
> + bool signalled;
> +
> + /*
> + * 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;
> +};
> +#endif
> +
> /**
> * struct seccomp_filter - container for seccomp BPF programs
> *
> @@ -66,6 +114,30 @@ struct seccomp_filter {
> bool log;
> struct seccomp_filter *prev;
> struct bpf_prog *prog;
> +
> +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> + /*
> + * A semaphore that users of this notification can wait on for
> + * changes. Actual reads and writes are still controlled with
> + * filter->notify_lock.
> + */
> + struct semaphore request;
> +
> + /* A lock for all notification-related accesses. */
> + struct mutex notify_lock;
> +
> + /* Is there currently an attached listener? */
> + bool has_listener;
> +
> + /* The id of the next request. */
> + u64 next_id;
> +
> + /* A list of struct seccomp_knotif elements. */
> + struct list_head notifications;
> +
> + /* A wait queue for poll. */
> + wait_queue_head_t wqh;
> +#endif
I suspect that these additions would benefit from better struct packing
since there could be a lot of seccomp_filter structs floating around in
memory on a system with a large number of running containers or
otherwise sandboxed processes.
IIRC, there's a 3 byte hole following the log member that could be used
by has_listener, at least, and I'm not sure how the rest of the new
members affect things.
> };
>
> /* Limit any path through the tree to 256KB worth of instructions. */
> @@ -359,6 +431,19 @@ static inline void seccomp_sync_threads(unsigned long flags)
> }
> }
>
> +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> +static void init_user_notification(struct seccomp_filter *sfilter)
> +{
> + mutex_init(&sfilter->notify_lock);
> + sema_init(&sfilter->request, 0);
> + INIT_LIST_HEAD(&sfilter->notifications);
> + sfilter->next_id = get_random_u64();
> + init_waitqueue_head(&sfilter->wqh);
> +}
> +#else
> +static inline void init_user_notification(struct seccomp_filter *sfilter) { }
> +#endif
> +
> /**
> * seccomp_prepare_filter: Prepares a seccomp filter for use.
> * @fprog: BPF program to install
> @@ -392,6 +477,8 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
> if (!sfilter)
> return ERR_PTR(-ENOMEM);
>
> + init_user_notification(sfilter);
> +
> ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
> seccomp_check_filter, save_orig);
> if (ret < 0) {
> @@ -556,13 +643,15 @@ 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_TRACE |
> - SECCOMP_LOG_LOG;
> + SECCOMP_LOG_LOG |
> + SECCOMP_LOG_USER_NOTIF;
>
> static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> bool requested)
> @@ -581,6 +670,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;
> @@ -651,6 +743,83 @@ void secure_computing_strict(int this_syscall)
> }
> #else
>
> +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> +static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
> +{
> + /* Note: overflow is ok here, the id just needs to be unique */
> + return filter->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->has_listener)
> + goto out;
> +
> + n.pid = task_pid(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->notifications);
> + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> +
> + mutex_unlock(&match->notify_lock);
> + up(&match->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.signalled = true;
> + if (n.state == SECCOMP_NOTIFY_SENT) {
> + n.state = SECCOMP_NOTIFY_INIT;
> + up(&match->request);
> + }
> + mutex_unlock(&match->notify_lock);
Is it intentional that you call mutex_unlocked() followed by up() when
initially waiting but then switch up the order before re-waiting here? I
don't yet fully understand the locking but this looked odd to me.
> + 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);
> +}
> +#else
> +static void seccomp_do_user_notification(int this_syscall,
> + struct seccomp_filter *match,
> + const struct seccomp_data *sd)
> +{
> + seccomp_log(this_syscall, SIGSYS, SECCOMP_RET_USER_NOTIF, true);
> + do_exit(SIGSYS);
> +}
> +#endif
> +
> #ifdef CONFIG_SECCOMP_FILTER
> static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> const bool recheck_after_trace)
> @@ -728,6 +897,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 +1006,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 +1028,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 +1040,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 +1079,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;
> @@ -915,6 +1117,9 @@ static long seccomp_get_action_avail(const char __user *uaction)
> case SECCOMP_RET_LOG:
> case SECCOMP_RET_ALLOW:
> break;
> + case SECCOMP_RET_USER_NOTIF:
> + if (IS_ENABLED(CONFIG_SECCOMP_USER_NOTIFICATION))
> + break;
Lets add a "/* fall through */" comment here to support the ongoing
effort of marking these sorts of cases in prep for
-Wimplicit-fallthrough.
> default:
> return -EOPNOTSUPP;
> }
> @@ -1111,6 +1316,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 +1326,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;
> @@ -1137,6 +1344,7 @@ static const struct seccomp_log_name seccomp_log_names[] = {
> { SECCOMP_LOG_TRACE, SECCOMP_RET_TRACE_NAME },
> { SECCOMP_LOG_LOG, SECCOMP_RET_LOG_NAME },
> { SECCOMP_LOG_ALLOW, SECCOMP_RET_ALLOW_NAME },
> + { SECCOMP_LOG_USER_NOTIF, SECCOMP_RET_USER_NOTIF_NAME },
Probably best to keep this list in order. Can you stick it in front of
the element for TRACE?
> { }
> };
>
> @@ -1342,3 +1550,244 @@ static int __init seccomp_sysctl_init(void)
> device_initcall(seccomp_sysctl_init)
>
> #endif /* CONFIG_SYSCTL */
> +
> +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> +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->notifications, list) {
> + if (knotif->state == SECCOMP_NOTIFY_REPLIED)
> + continue;
> +
> + knotif->state = SECCOMP_NOTIFY_REPLIED;
> + knotif->error = -ENOSYS;
ENOSYS seems odd to me since the functionality is implemented. Is EIO
more appropriate? It feels like it better matches an EIO from read(2),
for example.
> + knotif->val = 0;
> +
> + complete(&knotif->ready);
> + }
> +
> + wake_up_all(&filter->wqh);
> + filter->has_listener = false;
> + 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->request);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&filter->notify_lock);
> + list_for_each_entry(cur, &filter->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 = pid_vnr(knotif->pid);
> + unotif.signalled = knotif->signalled;
> + unotif.data = *(knotif->data);
> +
> + if (copy_to_user(buf, &unotif, size)) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = sizeof(unotif);
I would have thought that ret = size since only size bytes are copied.
> + knotif->state = SECCOMP_NOTIFY_SENT;
> + wake_up_poll(&filter->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->notifications, list) {
> + if (knotif->id == resp.id)
> + break;
> + }
> +
> + if (!knotif || knotif->id != resp.id) {
> + ret = -EINVAL;
ENOENT here instead? It clearly conveys that there is no notification
matching the requested ID. We'll probably have a more ambiguous error
path that we can use to abuse EINVAL. :)
> + 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_is_id_valid(struct seccomp_filter *filter,
> + unsigned long arg)
> +{
> + struct seccomp_knotif *knotif = NULL;
> + void __user *buf = (void __user *)arg;
> + u64 id;
> +
> + if (copy_from_user(&id, buf, sizeof(id)))
> + return -EFAULT;
> +
> + list_for_each_entry(knotif, &filter->notifications, list) {
> + if (knotif->id == id)
> + return 1;
> + }
> +
> + return 0;
I understand the desire to return 1 from
ioctl(fd, SECCOMP_NOTIF_IS_ID_VALID, id) when id is valid but it goes
against the common case where a syscall returns 0 on success. Also, the
ioctl_list(2) man page states:
Decent ioctls return 0 on success and -1 on error, ...
The only suggestion that I have here is to change the ioctl name to
SECCOMP_NOTIF_VALID_ID (or similiar) and return 0 if the id is valid and
-EINVAL if the id is invalid. I don't feel strongly about it so take it
or leave it.
> +}
> +
> +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_IS_ID_VALID:
> + return seccomp_notify_is_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->wqh, poll_tab);
> +
> + ret = mutex_lock_interruptible(&filter->notify_lock);
> + if (ret < 0)
> + return ret;
> +
> + list_for_each_entry(cur, &filter->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->has_listener)
> + goto out;
> + }
> +
> + 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);
> + filter->has_listener = true;
> +
> +out:
> + for (cur = task->seccomp.filter; cur; cur = cur->prev) {
> + mutex_unlock(&cur->notify_lock);
> + if (cur == last_locked)
> + break;
> + }
> +
> + return ret;
> +}
> +#else
> +static struct file *init_listener(struct task_struct *task,
> + struct seccomp_filter *filter)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index e1473234968d..89f2c788a06b 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.
> */
[...]
I only gave the tests a quick review so far but nothing stood out.
I'm anxious to give this patch set some testing. I'll get to the other
patches soon.
Tyler
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/2] prctl: add PR_[GS]ET_KILLABLE
From: Andrew Morton @ 2018-09-06 22:42 UTC (permalink / raw)
To: Jürg Billeter
Cc: Oleg Nesterov, Thomas Gleixner, Eric Biederman, linux-api,
linux-kernel
In-Reply-To: <20180803144021.56920-2-j@bitron.ch>
On Fri, 3 Aug 2018 16:40:21 +0200 Jürg Billeter <j@bitron.ch> wrote:
> PR_SET_KILLABLE clears the SIGNAL_UNKILLABLE flag. This allows
> CLONE_NEWPID tasks to restore normal signal behavior, opting out of the
> special signal protection for init processes. This prctl does not allow
> setting the SIGNAL_UNKILLABLE flag, only clearing.
>
> The SIGNAL_UNKILLABLE flag, which is implicitly set for tasks cloned
> with CLONE_NEWPID, has the effect of ignoring all signals (from
> userspace) if the corresponding handler is set to SIG_DFL. The only
> exceptions are SIGKILL and SIGSTOP and they are only accepted if raised
> from an ancestor namespace.
>
> SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
> While a task with the SIGNAL_UNKILLABLE flag could install handlers for
> these signals, this is not sufficient to implement a shell that uses
> CLONE_NEWPID for child processes:
>
> * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
> itself, it's not possible to implement the stop action in a custom
> SIGTSTP handler.
> * Many applications do not install handlers for these signals and
> thus, job control won't work properly with unmodified applications.
>
> There are other scenarios besides job control in a shell where
> applications rely on the default actions as described in signal(7) and
> PID isolation may be useful. This new prctl makes the signal protection
> for "init" processes optional, without breaking backward compatibility.
This one is above my pay grade. Eric & Oleg: could you please provide
input?
^ permalink raw reply
* Re: [PATCH v6 1/5] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-09-07 15:45 UTC (permalink / raw)
To: Tyler Hicks
Cc: Kees Cook, linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Akihiro Suda, Jann Horn
In-Reply-To: <20180906221412.GA26209@sec>
Hey Tyler,
On Thu, Sep 06, 2018 at 10:15:12PM +0000, Tyler Hicks wrote:
> > +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 globally unique ``id``, the
>
> This documentation says that id is "globally unique" but an in-code
> comment below says "this is unique for this filter". IIUC, the id is
> only guaranteed to be unique for the filter so this documentation should
> be updated slightly to make it clear that the id is only global in those
> terms.
Yup, thanks.
> > +``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/arch/Kconfig b/arch/Kconfig
> > index 6801123932a5..42f3585d925d 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -419,6 +419,15 @@ config SECCOMP_FILTER
> >
> > See Documentation/userspace-api/seccomp_filter.rst for details.
> >
> > +config SECCOMP_USER_NOTIFICATION
>
> Did someone request a Kconfig option for this new feature? If not, I
> think that nuking the Kconfig option would reduce the test matrix. No
> other filter flags have their own build time option but maybe it makes
> sense in this case if this filter flag exposes the kernel to significant
> new attack surface since there's more to this than just a new filter
> flag.
>
> If someone has a requirement to disable this feature, maybe it'd be
> better to leave the decision up to the distro *and* the admin via a
> sysctl instead of taking the admin out of the decision with a build
> time option.
No, there was no explicit request by anyone, I just did it so I
wouldn't offend anyone with this code. I'll drop it for the next
version.
> > /**
> > * struct seccomp_filter - container for seccomp BPF programs
> > *
> > @@ -66,6 +114,30 @@ struct seccomp_filter {
> > bool log;
> > struct seccomp_filter *prev;
> > struct bpf_prog *prog;
> > +
> > +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> > + /*
> > + * A semaphore that users of this notification can wait on for
> > + * changes. Actual reads and writes are still controlled with
> > + * filter->notify_lock.
> > + */
> > + struct semaphore request;
> > +
> > + /* A lock for all notification-related accesses. */
> > + struct mutex notify_lock;
> > +
> > + /* Is there currently an attached listener? */
> > + bool has_listener;
> > +
> > + /* The id of the next request. */
> > + u64 next_id;
> > +
> > + /* A list of struct seccomp_knotif elements. */
> > + struct list_head notifications;
> > +
> > + /* A wait queue for poll. */
> > + wait_queue_head_t wqh;
> > +#endif
>
> I suspect that these additions would benefit from better struct packing
> since there could be a lot of seccomp_filter structs floating around in
> memory on a system with a large number of running containers or
> otherwise sandboxed processes.
>
> IIRC, there's a 3 byte hole following the log member that could be used
> by has_listener, at least, and I'm not sure how the rest of the new
> members affect things.
Ok, I'll take a look.
> > +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->has_listener)
> > + goto out;
> > +
> > + n.pid = task_pid(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->notifications);
> > + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> > +
> > + mutex_unlock(&match->notify_lock);
> > + up(&match->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.signalled = true;
> > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > + n.state = SECCOMP_NOTIFY_INIT;
> > + up(&match->request);
> > + }
> > + mutex_unlock(&match->notify_lock);
>
> Is it intentional that you call mutex_unlocked() followed by up() when
> initially waiting but then switch up the order before re-waiting here? I
> don't yet fully understand the locking but this looked odd to me.
No, not intentional. Assuming everything is correct, the order doesn't
matter here. It might be slightly better to do the up() after, since
then the woken task won't immediately sleep waiting on the mutex, but
who knows :)
> > +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;
> > @@ -915,6 +1117,9 @@ static long seccomp_get_action_avail(const char __user *uaction)
> > case SECCOMP_RET_LOG:
> > case SECCOMP_RET_ALLOW:
> > break;
> > + case SECCOMP_RET_USER_NOTIF:
> > + if (IS_ENABLED(CONFIG_SECCOMP_USER_NOTIFICATION))
> > + break;
>
> Lets add a "/* fall through */" comment here to support the ongoing
> effort of marking these sorts of cases in prep for
> -Wimplicit-fallthrough.
Will do.
> > default:
> > return -EOPNOTSUPP;
> > }
> > @@ -1111,6 +1316,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 +1326,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;
> > @@ -1137,6 +1344,7 @@ static const struct seccomp_log_name seccomp_log_names[] = {
> > { SECCOMP_LOG_TRACE, SECCOMP_RET_TRACE_NAME },
> > { SECCOMP_LOG_LOG, SECCOMP_RET_LOG_NAME },
> > { SECCOMP_LOG_ALLOW, SECCOMP_RET_ALLOW_NAME },
> > + { SECCOMP_LOG_USER_NOTIF, SECCOMP_RET_USER_NOTIF_NAME },
>
> Probably best to keep this list in order. Can you stick it in front of
> the element for TRACE?
Yep!
> > + /*
> > + * 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->notifications, list) {
> > + if (knotif->state == SECCOMP_NOTIFY_REPLIED)
> > + continue;
> > +
> > + knotif->state = SECCOMP_NOTIFY_REPLIED;
> > + knotif->error = -ENOSYS;
>
> ENOSYS seems odd to me since the functionality is implemented. Is EIO
> more appropriate? It feels like it better matches an EIO from read(2),
> for example.
I copied the ENOSYS usage from SECCOMP_RET_TRACE: when there is no
tracer attached, it responds to any SECCOMP_RET_TRACE with ENOSYS.
Seems like keeping the same behavior here is useful.
> > + unotif.len = size;
> > + unotif.id = knotif->id;
> > + unotif.pid = pid_vnr(knotif->pid);
> > + unotif.signalled = knotif->signalled;
> > + unotif.data = *(knotif->data);
> > +
> > + if (copy_to_user(buf, &unotif, size)) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + ret = sizeof(unotif);
>
> I would have thought that ret = size since only size bytes are copied.
Yes, right you are.
> > + knotif->state = SECCOMP_NOTIFY_SENT;
> > + wake_up_poll(&filter->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->notifications, list) {
> > + if (knotif->id == resp.id)
> > + break;
> > + }
> > +
> > + if (!knotif || knotif->id != resp.id) {
> > + ret = -EINVAL;
>
> ENOENT here instead? It clearly conveys that there is no notification
> matching the requested ID. We'll probably have a more ambiguous error
> path that we can use to abuse EINVAL. :)
Yes, will do :)
> > + 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_is_id_valid(struct seccomp_filter *filter,
> > + unsigned long arg)
> > +{
> > + struct seccomp_knotif *knotif = NULL;
> > + void __user *buf = (void __user *)arg;
> > + u64 id;
> > +
> > + if (copy_from_user(&id, buf, sizeof(id)))
> > + return -EFAULT;
> > +
> > + list_for_each_entry(knotif, &filter->notifications, list) {
> > + if (knotif->id == id)
> > + return 1;
> > + }
> > +
> > + return 0;
>
> I understand the desire to return 1 from
> ioctl(fd, SECCOMP_NOTIF_IS_ID_VALID, id) when id is valid but it goes
> against the common case where a syscall returns 0 on success. Also, the
> ioctl_list(2) man page states:
>
> Decent ioctls return 0 on success and -1 on error, ...
>
> The only suggestion that I have here is to change the ioctl name to
> SECCOMP_NOTIF_VALID_ID (or similiar) and return 0 if the id is valid and
> -EINVAL if the id is invalid. I don't feel strongly about it so take it
> or leave it.
Sure, will do.
> > +}
> > +
> > +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_IS_ID_VALID:
> > + return seccomp_notify_is_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->wqh, poll_tab);
> > +
> > + ret = mutex_lock_interruptible(&filter->notify_lock);
> > + if (ret < 0)
> > + return ret;
> > +
> > + list_for_each_entry(cur, &filter->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->has_listener)
> > + goto out;
> > + }
> > +
> > + 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);
> > + filter->has_listener = true;
> > +
> > +out:
> > + for (cur = task->seccomp.filter; cur; cur = cur->prev) {
> > + mutex_unlock(&cur->notify_lock);
> > + if (cur == last_locked)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +#else
> > +static struct file *init_listener(struct task_struct *task,
> > + struct seccomp_filter *filter)
> > +{
> > + return ERR_PTR(-EINVAL);
> > +}
> > +#endif
> > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > index e1473234968d..89f2c788a06b 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.
> > */
>
> [...]
>
> I only gave the tests a quick review so far but nothing stood out.
>
> I'm anxious to give this patch set some testing. I'll get to the other
> patches soon.
Thanks!
Tycho
^ permalink raw reply
* Re: [PATCH 00/10] Removing SEND_SIG_FORCED
From: Thomas Gleixner @ 2018-09-08 11:03 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: linux-kernel, Oleg Nesterov, Linus Torvalds, linux-api
In-Reply-To: <87musyl3fa.fsf@xmission.com>
On Mon, 3 Sep 2018, Eric W. Biederman wrote:
> SEND_SIG_FORCED has two functions. It forces a pid namespace init to
> receive a signal it would ordinarily ignore, and it causes a siginfo to
> not be allocated for a signal.
>
> This patchset makes __send_signal a little bit smarter so that it can
> detect when to apply these optimizations and the senders of signals
> don't have to worry about them.
>
> The coupling of forcing a signal to the pid namespace init and not
> allocating siginfo resulted in serveral minor bugs where a signal
> sent by the kernel was marked SI_USER suggesting another userspace
> process sent that signal. I have cc'd linux-api in case anyone cares
> about these minor userspace visible differences.
>
> Unless someone notices a bug I intend to merge these changes through my
> tree during the next merge window. While there are numerous fixes here
> none of them appear to be the kind that fixes real world problems so I
> don't see any urgency here.
>
> Please look read and send my your review. I will be out for about a
> week so I will address any comments when I get back.
Went through it carefuly and could not spot anything. Great work!
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply
* Re: [PATCH v6 1/5] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-09-08 20:35 UTC (permalink / raw)
To: Tyler Hicks
Cc: Kees Cook, linux-kernel, containers, linux-api, Andy Lutomirski,
Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Akihiro Suda, Jann Horn
In-Reply-To: <20180906221412.GA26209@sec>
On Thu, Sep 06, 2018 at 10:15:12PM +0000, Tyler Hicks wrote:
> On 2018-09-06 09:28:55, Tycho Andersen wrote:
> > /**
> > * struct seccomp_filter - container for seccomp BPF programs
> > *
> > @@ -66,6 +114,30 @@ struct seccomp_filter {
> > bool log;
> > struct seccomp_filter *prev;
> > struct bpf_prog *prog;
> > +
> > +#ifdef CONFIG_SECCOMP_USER_NOTIFICATION
> > + /*
> > + * A semaphore that users of this notification can wait on for
> > + * changes. Actual reads and writes are still controlled with
> > + * filter->notify_lock.
> > + */
> > + struct semaphore request;
> > +
> > + /* A lock for all notification-related accesses. */
> > + struct mutex notify_lock;
> > +
> > + /* Is there currently an attached listener? */
> > + bool has_listener;
> > +
> > + /* The id of the next request. */
> > + u64 next_id;
> > +
> > + /* A list of struct seccomp_knotif elements. */
> > + struct list_head notifications;
> > +
> > + /* A wait queue for poll. */
> > + wait_queue_head_t wqh;
> > +#endif
>
> I suspect that these additions would benefit from better struct packing
> since there could be a lot of seccomp_filter structs floating around in
> memory on a system with a large number of running containers or
> otherwise sandboxed processes.
>
> IIRC, there's a 3 byte hole following the log member that could be used
> by has_listener, at least, and I'm not sure how the rest of the new
> members affect things.
So it turns out the additions are fairly major. The previous
sizeof(struct seccomp_filter) == 24 bytes on x86_64, with the three
byte hole you mentioned.
The new members alone actual sizes are:
sizeof(struct sempahore) request == 80
sizeof(struct mutex) notify_lock == 128
sizeof(struct list_head) notifications == 16
sizeof(struct wait_queue_head_t) wqh == 72
+ the base types of next_id, has_listener gives a grand total of 305
additional bytes, assuming it's packed perfectly. That seems like
quite a huge hit for everyone to endure, especially since it won't be
perfectly packed.
Instead, what if we add a struct notification, and a struct
notification* to struct seccomp_filter? Then we can drop the bool
has_listener because we can use a null test, and the 304 bytes are
only paid by people who actually use this feature (as well as the cost
of an additional indirection, but who cares, they're trapping to
userspace anyway). Unless I hear any objections, I'll do this for v7
:)
Tycho
^ permalink raw reply
* Re: [PATCH v7] Add udmabuf misc device
From: Laurent Pinchart @ 2018-09-10 12:12 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: open list:KERNEL SELFTEST FRAMEWORK, Tomeu Vizoso,
Jonathan Corbet, David Airlie, linux-api, open list:DOCUMENTATION,
open list, dri-devel, moderated list:DMA BUFFER SHARING FRAMEWORK,
Shuah Khan, open list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <20180827093444.23623-1-kraxel@redhat.com>
Hi Gerd,
Thank you for the patch.
CC'ing the linux-api mailing list as this creates a new userspace API.
On Monday, 27 August 2018 12:34:44 EEST Gerd Hoffmann wrote:
> A driver to let userspace turn memfd regions into dma-bufs.
>
> Use case: Allows qemu create dmabufs for the vga framebuffer or
> virtio-gpu ressources. Then they can be passed around to display
> those guest things on the host. To spice client for classic full
> framebuffer display, and hopefully some day to wayland server for
> seamless guest window display.
>
> qemu test branch:
> https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf
>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> Documentation/ioctl/ioctl-number.txt | 1 +
> include/uapi/linux/udmabuf.h | 33 +++
> drivers/dma-buf/udmabuf.c | 287 +++++++++++++++++++
> tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++
> MAINTAINERS | 16 ++
> drivers/dma-buf/Kconfig | 8 +
> drivers/dma-buf/Makefile | 1 +
> tools/testing/selftests/drivers/dma-buf/Makefile | 5 +
> 8 files changed, 447 insertions(+)
> create mode 100644 include/uapi/linux/udmabuf.h
> create mode 100644 drivers/dma-buf/udmabuf.c
> create mode 100644 tools/testing/selftests/drivers/dma-buf/udmabuf.c
> create mode 100644 tools/testing/selftests/drivers/dma-buf/Makefile
[snip]
> diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h
> new file mode 100644
> index 0000000000..46b6532ed8
> --- /dev/null
> +++ b/include/uapi/linux/udmabuf.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_UDMABUF_H
> +#define _UAPI_LINUX_UDMABUF_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define UDMABUF_FLAGS_CLOEXEC 0x01
> +
> +struct udmabuf_create {
> + __u32 memfd;
> + __u32 flags;
> + __u64 offset;
> + __u64 size;
> +};
> +
> +struct udmabuf_create_item {
> + __u32 memfd;
> + __u32 __pad;
> + __u64 offset;
> + __u64 size;
> +};
> +
> +struct udmabuf_create_list {
> + __u32 flags;
> + __u32 count;
> + struct udmabuf_create_item list[];
> +};
> +
> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create)
Why do you start at 0x42 if you reserve the 0x40-0x4f range ?
> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list)
Where's the documentation ? :-)
> +#endif /* _UAPI_LINUX_UDMABUF_H */
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> new file mode 100644
> index 0000000000..8e24204526
> --- /dev/null
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/miscdevice.h>
> +#include <linux/dma-buf.h>
> +#include <linux/highmem.h>
> +#include <linux/cred.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/memfd.h>
Could you please keep the #include alphabetically sorted ? It helps locating
duplicates.
> +#include <uapi/linux/udmabuf.h>
I think you can just #include <linux/udmabuf.h>, no need to use the uapi/
prefix.
> +struct udmabuf {
> + u32 pagecount;
> + struct page **pages;
> +};
> +
> +static int udmabuf_vm_fault(struct vm_fault *vmf)
> +{
> + struct vm_area_struct *vma = vmf->vma;
> + struct udmabuf *ubuf = vma->vm_private_data;
> +
> + if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
> + return VM_FAULT_SIGBUS;
Just curious, when do you expect this to happen ?
> + vmf->page = ubuf->pages[vmf->pgoff];
> + get_page(vmf->page);
> + return 0;
> +}
> +
> +static const struct vm_operations_struct udmabuf_vm_ops = {
> + .fault = udmabuf_vm_fault,
> +};
> +
> +static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
> +{
> + struct udmabuf *ubuf = buf->priv;
> +
> + if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
> + return -EINVAL;
> +
> + vma->vm_ops = &udmabuf_vm_ops;
> + vma->vm_private_data = ubuf;
> + return 0;
> +}
> +
> +static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
> + enum dma_data_direction direction)
> +{
> + struct udmabuf *ubuf = at->dmabuf->priv;
> + struct sg_table *sg;
> +
> + sg = kzalloc(sizeof(*sg), GFP_KERNEL);
> + if (!sg)
> + goto err1;
You can return ERR_PTR(-ENOMEM) directly.
> + if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
> + 0, ubuf->pagecount << PAGE_SHIFT,
> + GFP_KERNEL) < 0)
Shouldn't you propagate the return value from sg_alloc_table_from_pages() ?
> + goto err2;
> + if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction))
> + goto err3;
> +
> + return sg;
> +
> +err3:
> + sg_free_table(sg);
> +err2:
> + kfree(sg);
> +err1:
> + return ERR_PTR(-ENOMEM);
You can merge all these labels with
error:
if (sg) {
sg_free_table(sg);
kfree(sg);
}
return ERR_PTR(-ENOMEM);
> +}
> +
> +static void unmap_udmabuf(struct dma_buf_attachment *at,
> + struct sg_table *sg,
> + enum dma_data_direction direction)
> +{
> + sg_free_table(sg);
> + kfree(sg);
> +}
> +
> +static void release_udmabuf(struct dma_buf *buf)
> +{
> + struct udmabuf *ubuf = buf->priv;
> + pgoff_t pg;
> +
> + for (pg = 0; pg < ubuf->pagecount; pg++)
Shouldn't both pg and pagecount have the same type ? Why does the loop counter
qualify for a pgoff_t and not the pagecount field ? Granted, the pgoff_t is
documented as "The type of an index in the page cache", so pagecount doesn't
really quality, but the fact that one is an unsigned long and the other a u32
makes me think that something is wrong.
> + put_page(ubuf->pages[pg]);
> + kfree(ubuf->pages);
> + kfree(ubuf);
> +}
> +
> +static void *kmap_udmabuf(struct dma_buf *buf, unsigned long page_num)
> +{
> + struct udmabuf *ubuf = buf->priv;
> + struct page *page = ubuf->pages[page_num];
> +
> + return kmap(page);
> +}
> +
> +static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num,
> + void *vaddr)
> +{
> + kunmap(vaddr);
> +}
> +
> +static struct dma_buf_ops udmabuf_ops = {
static const struct
> + .map_dma_buf = map_udmabuf,
> + .unmap_dma_buf = unmap_udmabuf,
> + .release = release_udmabuf,
> + .map = kmap_udmabuf,
> + .unmap = kunmap_udmabuf,
> + .mmap = mmap_udmabuf,
> +};
> +
> +#define SEALS_WANTED (F_SEAL_SHRINK)
> +#define SEALS_DENIED (F_SEAL_WRITE)
> +
> +static long udmabuf_create(struct udmabuf_create_list *head,
> + struct udmabuf_create_item *list)
Those two structures are not modified by the function, you can make them
const.
> +{
> + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> + struct file *memfd = NULL;
> + struct udmabuf *ubuf;
> + struct dma_buf *buf;
> + pgoff_t pgoff, pgcnt, pgidx, pgbuf;
> + struct page *page;
> + int seals, ret = -EINVAL;
> + u32 i, flags;
> +
> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
sizeof(*ubuf)
> + if (!ubuf)
> + return -ENOMEM;
> +
> + for (i = 0; i < head->count; i++) {
> + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
> + goto err_free_ubuf;
> + if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
> + goto err_free_ubuf;
> + ubuf->pagecount += list[i].size >> PAGE_SHIFT;
Is there a risk of overflowing pagecount ?
> + }
> + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *),
> + GFP_KERNEL);
sizeof(*ubuf->pages)
> + if (!ubuf->pages) {
> + ret = -ENOMEM;
> + goto err_free_ubuf;
> + }
> +
> + pgbuf = 0;
> + for (i = 0; i < head->count; i++) {
> + memfd = fget(list[i].memfd);
> + if (!memfd)
> + goto err_put_pages;
> + if (!shmem_mapping(file_inode(memfd)->i_mapping))
> + goto err_put_pages;
> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
> + if (seals == -EINVAL ||
> + (seals & SEALS_WANTED) != SEALS_WANTED ||
> + (seals & SEALS_DENIED) != 0)
> + goto err_put_pages;
All these conditions will return -EINVAL. I'm not familiar with the memfd API,
should some error conditions return a different error code to make them
distinguishable by userspace ?
> + pgoff = list[i].offset >> PAGE_SHIFT;
> + pgcnt = list[i].size >> PAGE_SHIFT;
> + for (pgidx = 0; pgidx < pgcnt; pgidx++) {
> + page = shmem_read_mapping_page(
> + file_inode(memfd)->i_mapping, pgoff + pgidx);
Can't pgoff + pgcnt overflow the total number of avialble pages ?
> + if (IS_ERR(page)) {
> + ret = PTR_ERR(page);
> + goto err_put_pages;
> + }
> + ubuf->pages[pgbuf++] = page;
> + }
> + fput(memfd);
> + }
> + memfd = NULL;
I'd move this line just after fput(memfd) inside the loop to avoid
introduction bugs in the future if we add code that can break from the loop
before the fget() call.
> + exp_info.ops = &udmabuf_ops;
> + exp_info.size = ubuf->pagecount << PAGE_SHIFT;
> + exp_info.priv = ubuf;
> +
> + buf = dma_buf_export(&exp_info);
> + if (IS_ERR(buf)) {
> + ret = PTR_ERR(buf);
> + goto err_put_pages;
> + }
> +
> + flags = 0;
> + if (head->flags & UDMABUF_FLAGS_CLOEXEC)
> + flags |= O_CLOEXEC;
> + return dma_buf_fd(buf, flags);
> +
> +err_put_pages:
> + while (pgbuf > 0)
> + put_page(ubuf->pages[--pgbuf]);
If you initialize pgbuf to 0 you can merge the two error labels.
> +err_free_ubuf:
> + fput(memfd);
> + kfree(ubuf->pages);
> + kfree(ubuf);
> + return ret;
> +}
> +
> +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg)
> +{
> + struct udmabuf_create create;
> + struct udmabuf_create_list head;
> + struct udmabuf_create_item list;
> +
> + if (copy_from_user(&create, (void __user *)arg,
> + sizeof(struct udmabuf_create)))
sizeof(create)
> + return -EFAULT;
> +
> + head.flags = create.flags;
> + head.count = 1;
> + list.memfd = create.memfd;
> + list.offset = create.offset;
> + list.size = create.size;
> +
> + return udmabuf_create(&head, &list);
> +}
> +
> +static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
> +{
> + struct udmabuf_create_list head;
> + struct udmabuf_create_item *list;
> + int ret = -EINVAL;
> + u32 lsize;
> +
> + if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
> + return -EFAULT;
> + if (head.count > 1024)
> + return -EINVAL;
> + lsize = sizeof(struct udmabuf_create_item) * head.count;
> + list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
> + if (IS_ERR(list))
> + return PTR_ERR(list);
> +
> + ret = udmabuf_create(&head, list);
> + kfree(list);
> + return ret;
> +}
> +
> +static long udmabuf_ioctl(struct file *filp, unsigned int ioctl,
> + unsigned long arg)
> +{
> + long ret;
> +
> + switch (ioctl) {
> + case UDMABUF_CREATE:
> + ret = udmabuf_ioctl_create(filp, arg);
> + break;
> + case UDMABUF_CREATE_LIST:
> + ret = udmabuf_ioctl_create_list(filp, arg);
> + break;
> + default:
> + ret = -EINVAL;
The proper error code for invalid ioctls is -ENOTTY.
> + break;
> + }
> + return ret;
> +}
> +
> +static const struct file_operations udmabuf_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = udmabuf_ioctl,
> +};
> +
> +static struct miscdevice udmabuf_misc = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "udmabuf",
> + .fops = &udmabuf_fops,
> +};
> +
> +static int __init udmabuf_dev_init(void)
> +{
> + return misc_register(&udmabuf_misc);
> +}
> +
> +static void __exit udmabuf_dev_exit(void)
> +{
> + misc_deregister(&udmabuf_misc);
> +}
> +
> +module_init(udmabuf_dev_init)
> +module_exit(udmabuf_dev_exit)
> +
> +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/tools/testing/selftests/drivers/dma-buf/udmabuf.c
> b/tools/testing/selftests/drivers/dma-buf/udmabuf.c new file mode 100644
> index 0000000000..376b1d6730
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <malloc.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/syscall.h>
> +#include <linux/memfd.h>
> +#include <linux/udmabuf.h>
> +
> +#define TEST_PREFIX "drivers/dma-buf/udmabuf"
> +#define NUM_PAGES 4
> +
> +static int memfd_create(const char *name, unsigned int flags)
> +{
> + return syscall(__NR_memfd_create, name, flags);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + struct udmabuf_create create;
> + int devfd, memfd, buf, ret;
> + off_t size;
> + void *mem;
> +
> + devfd = open("/dev/udmabuf", O_RDWR);
> + if (devfd < 0) {
> + printf("%s: [skip,no-udmabuf]\n", TEST_PREFIX);
> + exit(77);
> + }
> +
> + memfd = memfd_create("udmabuf-test", MFD_CLOEXEC);
> + if (memfd < 0) {
> + printf("%s: [skip,no-memfd]\n", TEST_PREFIX);
> + exit(77);
> + }
> +
> + size = getpagesize() * NUM_PAGES;
> + ret = ftruncate(memfd, size);
> + if (ret == -1) {
> + printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> + exit(1);
> + }
> +
> + memset(&create, 0, sizeof(create));
> +
> + /* should fail (offset not page aligned) */
> + create.memfd = memfd;
> + create.offset = getpagesize()/2;
> + create.size = getpagesize();
> + buf = ioctl(devfd, UDMABUF_CREATE, &create);
> + if (buf >= 0) {
> + printf("%s: [FAIL,test-1]\n", TEST_PREFIX);
> + exit(1);
> + }
> +
> + /* should fail (size not multiple of page) */
> + create.memfd = memfd;
> + create.offset = 0;
> + create.size = getpagesize()/2;
> + buf = ioctl(devfd, UDMABUF_CREATE, &create);
> + if (buf >= 0) {
> + printf("%s: [FAIL,test-2]\n", TEST_PREFIX);
> + exit(1);
> + }
> +
> + /* should fail (not memfd) */
> + create.memfd = 0; /* stdin */
> + create.offset = 0;
> + create.size = size;
> + buf = ioctl(devfd, UDMABUF_CREATE, &create);
> + if (buf >= 0) {
> + printf("%s: [FAIL,test-3]\n", TEST_PREFIX);
> + exit(1);
> + }
> +
> + /* should work */
> + create.memfd = memfd;
> + create.offset = 0;
> + create.size = size;
> + buf = ioctl(devfd, UDMABUF_CREATE, &create);
> + if (buf < 0) {
> + printf("%s: [FAIL,test-4]\n", TEST_PREFIX);
> + exit(1);
> + }
> +
> + fprintf(stderr, "%s: ok\n", TEST_PREFIX);
> + close(buf);
> + close(memfd);
> + close(devfd);
> + return 0;
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a5b256b259..11a9b04277 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14934,6 +14934,14 @@ S: Maintained
> F: Documentation/filesystems/udf.txt
> F: fs/udf/
>
> +UDMABUF DRIVER
> +M: Gerd Hoffmann <kraxel@redhat.com>
> +L: dri-devel@lists.freedesktop.org
> +S: Maintained
> +F: drivers/dma-buf/udmabuf.c
> +F: include/uapi/linux/udmabuf.h
> +F: tools/testing/selftests/drivers/dma-buf/udmabuf.c
> +
> UDRAW TABLET
> M: Bastien Nocera <hadess@hadess.net>
> L: linux-input@vger.kernel.org
> @@ -15343,6 +15351,14 @@ F: arch/x86/um/
> F: fs/hostfs/
> F: fs/hppfs/
>
> +USERSPACE DMA BUFFER DRIVER
> +M: Gerd Hoffmann <kraxel@redhat.com>
> +S: Maintained
> +L: dri-devel@lists.freedesktop.org
> +F: drivers/dma-buf/udmabuf.c
> +F: include/uapi/linux/udmabuf.h
> +T: git git://anongit.freedesktop.org/drm/drm-misc
One entry should be enough.
> USERSPACE I/O (UIO)
> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> S: Maintained
[snip]
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 11/11] UAPI: Check headers build for C++ [ver #2]
From: kbuild test robot @ 2018-09-10 16:26 UTC (permalink / raw)
Cc: kbuild-all, linux-api, linux-kbuild, Masahiro Yamada,
Michal Marek, linux-kernel, dhowells
In-Reply-To: <153622557736.14298.10350496216090814292.stgit@warthog.procyon.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]
Hi David,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Howells/UAPI-drm-Fix-use-of-C-keywords-as-structural-members-ver-2/20180907-092121
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> find: '../usr/include': No such file or directory
In file included from test.cpp:1:0:
>> test.h:8:10: fatal error: linux/types.h: No such file or directory
#include <linux/types.h>
^~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [headers_install] Error 1
make: *** [sub-make] Error 2
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26038 bytes --]
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Jann Horn @ 2018-09-10 17:00 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <20180906183018.GC3326@cisco.cisco.com>
On Thu, Sep 6, 2018 at 8:30 PM Tycho Andersen <tycho@tycho.ws> wrote:
> On Thu, Sep 06, 2018 at 10:22:46AM -0600, Tycho Andersen wrote:
> > On Thu, Sep 06, 2018 at 06:15:18PM +0200, Jann Horn wrote:
> > > On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > The idea here is that the userspace handler should be able to pass an fd
> > > > back to the trapped task, for example so it can be returned from socket().
> > > [...]
> > > > diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> > > > index d1498885c1c7..1c0aab306426 100644
> > > > --- a/Documentation/userspace-api/seccomp_filter.rst
> > > > +++ b/Documentation/userspace-api/seccomp_filter.rst
> > > > @@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
> > > > __u64 id;
> > > > __s32 error;
> > > > __s64 val;
> > > > + __u8 return_fd;
> > > > + __u32 fd;
> > > > + __u32 fd_flags;
> > >
> > > Normally, syscalls that take an optional file descriptor accept a
> > > signed 32-bit number, with -1 standing for "no file descriptor". Is
> > > there a reason why this uses a separate variable to signal whether an
> > > fd was provided?
> >
> > No real reason other than I looked at the bpf code and they were using
> > __u32 for bpf (but I think in their case the fd args are not
> > optional). I'll switch it to __s32/-1 for the next version.
>
> Oh, I think there is a reason actually: since this is an API addition,
> the "0" value needs to be the previously default behavior if userspace
> doesn't specify it. Since the previously default behavior was not to
> return an fd, and we want to allow fd == 0, we need the extra flag to
> make this work.
>
> This is really only a problem because we're introducing this stuff in
> a second patch (mostly to illustrate how extending the response
> structure would work). I could fold this into the first patch if we
> want, or we could keep the return_fd bits if the illustration is
> useful.
I feel like adding extra struct fields just so that it is possible to
write programs against the intermediate new API between two kernel
commits is taking things a bit far.
^ permalink raw reply
* Re: [PATCH 11/11] UAPI: Check headers build for C++ [ver #2]
From: kbuild test robot @ 2018-09-10 17:02 UTC (permalink / raw)
Cc: kbuild-all, linux-api, linux-kbuild, Masahiro Yamada,
Michal Marek, linux-kernel, dhowells
In-Reply-To: <153622557736.14298.10350496216090814292.stgit@warthog.procyon.org.uk>
[-- Attachment #1: Type: text/plain, Size: 5884 bytes --]
Hi David,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Howells/UAPI-drm-Fix-use-of-C-keywords-as-structural-members-ver-2/20180907-092121
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from test.h:225:0,
from test.cpp:1:
>> ../usr/include/linux/patchkey.h:15:2: error: #error "patchkey.h included directly"
#error "patchkey.h included directly"
^~~~~
In file included from ../usr/include/asm/shmbuf.h:6:0,
from test.h:83,
from test.cpp:1:
>> ../usr/include/asm-generic/shmbuf.h:26:20: error: field 'shm_perm' has incomplete type 'ipc64_perm'
struct ipc64_perm shm_perm; /* operation perms */
^~~~~~~~
../usr/include/asm-generic/shmbuf.h:26:9: note: forward declaration of 'struct ipc64_perm'
struct ipc64_perm shm_perm; /* operation perms */
^~~~~~~~~~
In file included from ../usr/include/asm/msgbuf.h:6:0,
from test.h:112,
from test.cpp:1:
>> ../usr/include/asm-generic/msgbuf.h:25:20: error: field 'msg_perm' has incomplete type 'ipc64_perm'
struct ipc64_perm msg_perm;
^~~~~~~~
In file included from ../usr/include/asm/shmbuf.h:6:0,
from test.h:83,
from test.cpp:1:
../usr/include/asm-generic/shmbuf.h:26:9: note: forward declaration of 'struct ipc64_perm'
struct ipc64_perm shm_perm; /* operation perms */
^~~~~~~~~~
In file included from test.h:868:0,
from test.cpp:1:
../usr/include/sound/skl-tplg-interface.h:146:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 set_params:2;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:147:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 rsvd:30;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:148:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 param_id;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:149:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 max;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:166:2: error: 'u16' does not name a type; did you mean '__u16'?
u16 module_id;
^~~
__u16
../usr/include/sound/skl-tplg-interface.h:167:2: error: 'u16' does not name a type; did you mean '__u16'?
u16 instance_id;
^~~
__u16
../usr/include/sound/skl-tplg-interface.h:171:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 channels;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:172:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 freq;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:173:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 bit_depth;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:174:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 valid_bit_depth;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:175:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 ch_cfg;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:176:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 interleaving_style;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:177:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 sample_type;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:178:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 ch_map;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:182:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 set_params:2;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:183:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 rsvd:30;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:184:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 param_id;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:185:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 caps_size;
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:186:2: error: 'u32' does not name a type; did you mean '__u32'?
u32 caps[HDA_SST_CFG_MAX];
^~~
__u32
../usr/include/sound/skl-tplg-interface.h:190:2: error: 'u8' does not name a type; did you mean '__u8'?
u8 pipe_id;
^~
__u8
../usr/include/sound/skl-tplg-interface.h:191:2: error: 'u8' does not name a type; did you mean '__u8'?
u8 pipe_priority;
^~
__u8
../usr/include/sound/skl-tplg-interface.h:192:2: error: 'u16' does not name a type; did you mean '__u16'?
u16 conn_type:4;
^~~
__u16
../usr/include/sound/skl-tplg-interface.h:193:2: error: 'u16' does not name a type; did you mean '__u16'?
u16 rsvd:4;
--
>> find: '../usr/include': No such file or directory
In file included from test.cpp:1:0:
>> test.h:8:10: fatal error: linux/types.h: No such file or directory
#include <linux/types.h>
^~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [headers_install] Error 1
make: *** [sub-make] Error 2
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26038 bytes --]
^ permalink raw reply
* Re: [PATCH 07/11] UAPI: netfilter: Fix symbol collision issues [ver #2]
From: kbuild test robot @ 2018-09-10 17:32 UTC (permalink / raw)
Cc: kbuild-all, linux-api, linux-kbuild, netfilter-devel, coreteam,
linux-kernel, dhowells
In-Reply-To: <153622555128.14298.11612304975343930534.stgit@warthog.procyon.org.uk>
[-- Attachment #1: Type: text/plain, Size: 8915 bytes --]
Hi David,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Howells/UAPI-drm-Fix-use-of-C-keywords-as-structural-members-ver-2/20180907-092121
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net/ipv4/netfilter/ipt_ECN.c: In function 'set_ect_tcp':
>> net/ipv4/netfilter/ipt_ECN.c:58:28: error: 'IPT_ECN_OP_SET_ECE' undeclared (first use in this function); did you mean 'IPT_ECN_OP_MATCH_ECE'?
if ((!(einfo->operation & IPT_ECN_OP_SET_ECE) ||
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_MATCH_ECE
net/ipv4/netfilter/ipt_ECN.c:58:28: note: each undeclared identifier is reported only once for each function it appears in
>> net/ipv4/netfilter/ipt_ECN.c:60:28: error: 'IPT_ECN_OP_SET_CWR' undeclared (first use in this function); did you mean 'IPT_ECN_OP_SET_ECE'?
(!(einfo->operation & IPT_ECN_OP_SET_CWR) ||
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_SET_ECE
net/ipv4/netfilter/ipt_ECN.c: In function 'ecn_tg':
>> net/ipv4/netfilter/ipt_ECN.c:84:25: error: 'IPT_ECN_OP_SET_IP' undeclared (first use in this function); did you mean 'IPT_ECN_OP_MATCH_IP'?
if (einfo->operation & IPT_ECN_OP_SET_IP)
^~~~~~~~~~~~~~~~~
IPT_ECN_OP_MATCH_IP
>> net/ipv4/netfilter/ipt_ECN.c:88:26: error: 'IPT_ECN_OP_SET_ECE' undeclared (first use in this function); did you mean 'IPT_ECN_OP_SET_IP'?
if (einfo->operation & (IPT_ECN_OP_SET_ECE | IPT_ECN_OP_SET_CWR) &&
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_SET_IP
net/ipv4/netfilter/ipt_ECN.c:88:47: error: 'IPT_ECN_OP_SET_CWR' undeclared (first use in this function); did you mean 'IPT_ECN_OP_SET_ECE'?
if (einfo->operation & (IPT_ECN_OP_SET_ECE | IPT_ECN_OP_SET_CWR) &&
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_SET_ECE
net/ipv4/netfilter/ipt_ECN.c: In function 'ecn_tg_check':
>> net/ipv4/netfilter/ipt_ECN.c:101:25: error: 'IPT_ECN_OP_MASK' undeclared (first use in this function); did you mean 'IPT_ECN_IP_MASK'?
if (einfo->operation & IPT_ECN_OP_MASK)
^~~~~~~~~~~~~~~
IPT_ECN_IP_MASK
net/ipv4/netfilter/ipt_ECN.c:107:27: error: 'IPT_ECN_OP_SET_ECE' undeclared (first use in this function); did you mean 'IPT_ECN_OP_MATCH_ECE'?
if ((einfo->operation & (IPT_ECN_OP_SET_ECE|IPT_ECN_OP_SET_CWR)) &&
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_MATCH_ECE
net/ipv4/netfilter/ipt_ECN.c:107:46: error: 'IPT_ECN_OP_SET_CWR' undeclared (first use in this function); did you mean 'IPT_ECN_OP_SET_ECE'?
if ((einfo->operation & (IPT_ECN_OP_SET_ECE|IPT_ECN_OP_SET_CWR)) &&
^~~~~~~~~~~~~~~~~~
IPT_ECN_OP_SET_ECE
vim +58 net/ipv4/netfilter/ipt_ECN.c
^1da177e4 Linus Torvalds 2005-04-16 45
e1931b784 Jan Engelhardt 2007-07-07 46 /* Return false if there was an error. */
e1931b784 Jan Engelhardt 2007-07-07 47 static inline bool
3db05fea5 Herbert Xu 2007-10-15 48 set_ect_tcp(struct sk_buff *skb, const struct ipt_ECN_info *einfo)
^1da177e4 Linus Torvalds 2005-04-16 49 {
^1da177e4 Linus Torvalds 2005-04-16 50 struct tcphdr _tcph, *tcph;
6a19d6147 Al Viro 2006-09-28 51 __be16 oldval;
^1da177e4 Linus Torvalds 2005-04-16 52
af901ca18 André Goddard Rosa 2009-11-14 53 /* Not enough header? */
3db05fea5 Herbert Xu 2007-10-15 54 tcph = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(_tcph), &_tcph);
^1da177e4 Linus Torvalds 2005-04-16 55 if (!tcph)
e1931b784 Jan Engelhardt 2007-07-07 56 return false;
^1da177e4 Linus Torvalds 2005-04-16 57
fd841326d Patrick McHardy 2005-08-20 @58 if ((!(einfo->operation & IPT_ECN_OP_SET_ECE) ||
fd841326d Patrick McHardy 2005-08-20 59 tcph->ece == einfo->proto.tcp.ece) &&
7c4e36bc1 Jan Engelhardt 2007-07-07 @60 (!(einfo->operation & IPT_ECN_OP_SET_CWR) ||
7c4e36bc1 Jan Engelhardt 2007-07-07 61 tcph->cwr == einfo->proto.tcp.cwr))
e1931b784 Jan Engelhardt 2007-07-07 62 return true;
^1da177e4 Linus Torvalds 2005-04-16 63
3db05fea5 Herbert Xu 2007-10-15 64 if (!skb_make_writable(skb, ip_hdrlen(skb) + sizeof(*tcph)))
e1931b784 Jan Engelhardt 2007-07-07 65 return false;
3db05fea5 Herbert Xu 2007-10-15 66 tcph = (void *)ip_hdr(skb) + ip_hdrlen(skb);
^1da177e4 Linus Torvalds 2005-04-16 67
6a19d6147 Al Viro 2006-09-28 68 oldval = ((__be16 *)tcph)[6];
^1da177e4 Linus Torvalds 2005-04-16 69 if (einfo->operation & IPT_ECN_OP_SET_ECE)
^1da177e4 Linus Torvalds 2005-04-16 70 tcph->ece = einfo->proto.tcp.ece;
^1da177e4 Linus Torvalds 2005-04-16 71 if (einfo->operation & IPT_ECN_OP_SET_CWR)
^1da177e4 Linus Torvalds 2005-04-16 72 tcph->cwr = einfo->proto.tcp.cwr;
^1da177e4 Linus Torvalds 2005-04-16 73
be0ea7d5d Patrick McHardy 2007-11-30 74 inet_proto_csum_replace2(&tcph->check, skb,
4b048d6d9 Tom Herbert 2015-08-17 75 oldval, ((__be16 *)tcph)[6], false);
e1931b784 Jan Engelhardt 2007-07-07 76 return true;
^1da177e4 Linus Torvalds 2005-04-16 77 }
^1da177e4 Linus Torvalds 2005-04-16 78
^1da177e4 Linus Torvalds 2005-04-16 79 static unsigned int
4b560b447 Jan Engelhardt 2009-07-05 80 ecn_tg(struct sk_buff *skb, const struct xt_action_param *par)
^1da177e4 Linus Torvalds 2005-04-16 81 {
7eb355865 Jan Engelhardt 2008-10-08 82 const struct ipt_ECN_info *einfo = par->targinfo;
^1da177e4 Linus Torvalds 2005-04-16 83
^1da177e4 Linus Torvalds 2005-04-16 @84 if (einfo->operation & IPT_ECN_OP_SET_IP)
3db05fea5 Herbert Xu 2007-10-15 85 if (!set_ect_ip(skb, einfo))
^1da177e4 Linus Torvalds 2005-04-16 86 return NF_DROP;
^1da177e4 Linus Torvalds 2005-04-16 87
3666ed1c4 Joe Perches 2009-11-23 @88 if (einfo->operation & (IPT_ECN_OP_SET_ECE | IPT_ECN_OP_SET_CWR) &&
3666ed1c4 Joe Perches 2009-11-23 89 ip_hdr(skb)->protocol == IPPROTO_TCP)
3db05fea5 Herbert Xu 2007-10-15 90 if (!set_ect_tcp(skb, einfo))
^1da177e4 Linus Torvalds 2005-04-16 91 return NF_DROP;
^1da177e4 Linus Torvalds 2005-04-16 92
6709dbbb1 Jan Engelhardt 2007-02-07 93 return XT_CONTINUE;
^1da177e4 Linus Torvalds 2005-04-16 94 }
^1da177e4 Linus Torvalds 2005-04-16 95
135367b8f Jan Engelhardt 2010-03-19 96 static int ecn_tg_check(const struct xt_tgchk_param *par)
^1da177e4 Linus Torvalds 2005-04-16 97 {
af5d6dc20 Jan Engelhardt 2008-10-08 98 const struct ipt_ECN_info *einfo = par->targinfo;
af5d6dc20 Jan Engelhardt 2008-10-08 99 const struct ipt_entry *e = par->entryinfo;
^1da177e4 Linus Torvalds 2005-04-16 100
0cc9501f9 Florian Westphal 2018-02-09 @101 if (einfo->operation & IPT_ECN_OP_MASK)
d6b00a534 Jan Engelhardt 2010-03-25 102 return -EINVAL;
0cc9501f9 Florian Westphal 2018-02-09 103
0cc9501f9 Florian Westphal 2018-02-09 104 if (einfo->ip_ect & ~IPT_ECN_IP_MASK)
d6b00a534 Jan Engelhardt 2010-03-25 105 return -EINVAL;
0cc9501f9 Florian Westphal 2018-02-09 106
3666ed1c4 Joe Perches 2009-11-23 107 if ((einfo->operation & (IPT_ECN_OP_SET_ECE|IPT_ECN_OP_SET_CWR)) &&
3666ed1c4 Joe Perches 2009-11-23 108 (e->ip.proto != IPPROTO_TCP || (e->ip.invflags & XT_INV_PROTO))) {
b26066447 Florian Westphal 2018-02-09 109 pr_info_ratelimited("cannot use operation on non-tcp rule\n");
d6b00a534 Jan Engelhardt 2010-03-25 110 return -EINVAL;
^1da177e4 Linus Torvalds 2005-04-16 111 }
d6b00a534 Jan Engelhardt 2010-03-25 112 return 0;
^1da177e4 Linus Torvalds 2005-04-16 113 }
^1da177e4 Linus Torvalds 2005-04-16 114
:::::: The code at line 58 was first introduced by commit
:::::: fd841326d73096ad79be9c3fa348f9ad04541cc2 [NETFILTER]: Fix ECN target TCP marking
:::::: TO: Patrick McHardy <kaber@trash.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41196 bytes --]
^ permalink raw reply
* [PATCH v6 0/2] vfs: fix dedupe permission check
From: Mark Fasheh @ 2018-09-10 23:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, linux-fsdevel, linux-api, Michael Kerrisk, linux-btrfs,
linux-xfs, Darrick J . Wong, Adam Borowski, David Sterba,
Mark Fasheh
Hi Andrew/Al,
Could I please have these patches put in a tree for more public testing?
They've hit fsdevel a few times now, I have links to the discussions in the
change log below.
The following patches fix a couple of issues with the permission check
we do in vfs_dedupe_file_range().
The first patch expands our check to allow dedupe of a file if the
user owns it or otherwise would be allowed to write to it.
Current behavior is that we'll allow dedupe only if:
- the user is an admin (root)
- the user has the file open for write
This makes it impossible for a user to dedupe their own file set
unless they do it as root, or ensure that all files have write
permission. There's a couple of duperemove bugs open for this:
https://github.com/markfasheh/duperemove/issues/129
https://github.com/markfasheh/duperemove/issues/86
The other problem we have is also related to forcing the user to open
target files for write - A process trying to exec a file currently
being deduped gets ETXTBUSY. The answer (as above) is to allow them to
open the targets ro - root can already do this. There was a patch from
Adam Borowski to fix this back in 2016:
https://lkml.org/lkml/2016/7/17/130
which I have incorporated into my changes.
The 2nd patch fixes our return code for permission denied to be
EPERM. For some reason we're returning EINVAL - I think that's
probably my fault. At any rate, we need to be returning something
descriptive of the actual problem, otherwise callers see EINVAL and
can't really make a valid determination of what's gone wrong.
This has also popped up in duperemove, mostly in the form of cryptic
error messages. Because this is a code returned to userspace, I did
check the other users of extent-same that I could find. Both 'bees'
and 'rust-btrfs' do the same as duperemove and simply report the error
(as they should).
One way I tested these patches was to make non-root owned files with
read-only permissions and see if I could dedupe them as the owning user. For
example, the following script fails on an unpatched kernel and succeeds with
the patches applied.
TESTDIR=/btrfs
USER=mfasheh
rm -f $TESTDIR/file*
dd if=/dev/zero of=$TESTDIR/file1 count=1024 bs=1024
dd if=/dev/zero of=$TESTDIR/file2 count=1024 bs=1024
chown $USER $TESTDIR/file*
chmod 444 $TESTDIR/file*
# open file* for read and dedupe
sudo -u $USER duperemove -Ad $TESTDIR/file*
Lastly, I have an update to the fi_deduperange manpage to reflect these
changes. That patch is attached below.
git pull https://github.com/markfasheh/linux dedupe-perms
Thanks,
--Mark
Changes from V5 to V6:
- Rebase and retest on 4.19-rc3
- Add a note on testing
Changes from V4 to V5:
- Rebase and retest on 4.18-rc8
- Place updated manpage patch below, CC linux-api
- V4 discussion: https://patchwork.kernel.org/patch/10530365/
Changes from V3 to V4:
- Add a patch (below) to ioctl_fideduperange.2 explaining our
changes. I will send this patch once the kernel update is
accepted. Thanks to Darrick Wong for this suggestion.
- V3 discussion: https://www.spinics.net/lists/linux-btrfs/msg79135.html
Changes from V2 to V3:
- Return bool from allow_file_dedupe
- V2 discussion: https://www.spinics.net/lists/linux-btrfs/msg78421.html
Changes from V1 to V2:
- Add inode_permission check as suggested by Adam Borowski
- V1 discussion: https://marc.info/?l=linux-xfs&m=152606684017965&w=2
From: Mark Fasheh <mfasheh@suse.de>
[PATCH] ioctl_fideduperange.2: clarify permission requirements
dedupe permission checks were recently relaxed - update our man page to
reflect those changes.
Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
man2/ioctl_fideduperange.2 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/man2/ioctl_fideduperange.2 b/man2/ioctl_fideduperange.2
index 84d20a276..4040ee064 100644
--- a/man2/ioctl_fideduperange.2
+++ b/man2/ioctl_fideduperange.2
@@ -105,9 +105,12 @@ The field
must be zero.
During the call,
.IR src_fd
-must be open for reading and
+must be open for reading.
.IR dest_fd
-must be open for writing.
+can be open for writing, or reading.
+If
+.IR dest_fd
+is open for reading, the user must have write access to the file.
The combined size of the struct
.IR file_dedupe_range
and the struct
@@ -185,8 +188,8 @@ This can appear if the filesystem does not support deduplicating either file
descriptor, or if either file descriptor refers to special inodes.
.TP
.B EPERM
-.IR dest_fd
-is immutable.
+This will be returned if the user lacks permission to dedupe the file referenced by
+.IR dest_fd .
.TP
.B ETXTBSY
One of the files is a swap file.
--
2.15.1
^ permalink raw reply related
* [PATCH 1/2] vfs: allow dedupe of user owned read-only files
From: Mark Fasheh @ 2018-09-10 23:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, linux-fsdevel, linux-api, Michael Kerrisk, linux-btrfs,
linux-xfs, Darrick J . Wong, Adam Borowski, David Sterba,
Mark Fasheh
In-Reply-To: <20180910232118.14424-1-mfasheh@suse.de>
The permission check in vfs_dedupe_file_range_one() is too coarse - We only
allow dedupe of the destination file if the user is root, or they have the
file open for write.
This effectively limits a non-root user from deduping their own read-only
files. In addition, the write file descriptor that the user is forced to
hold open can prevent execution of files. As file data during a dedupe
does not change, the behavior is unexpected and this has caused a number of
issue reports. For an example, see:
https://github.com/markfasheh/duperemove/issues/129
So change the check so we allow dedupe on the target if:
- the root or admin is asking for it
- the process has write access
- the owner of the file is asking for the dedupe
- the process could get write access
That way users can open read-only and still get dedupe.
Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
fs/read_write.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 39b4a21dd933..be0e8723a049 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1964,6 +1964,20 @@ int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
}
EXPORT_SYMBOL(vfs_dedupe_file_range_compare);
+/* Check whether we are allowed to dedupe the destination file */
+static bool allow_file_dedupe(struct file *file)
+{
+ if (capable(CAP_SYS_ADMIN))
+ return true;
+ if (file->f_mode & FMODE_WRITE)
+ return true;
+ if (uid_eq(current_fsuid(), file_inode(file)->i_uid))
+ return true;
+ if (!inode_permission(file_inode(file), MAY_WRITE))
+ return true;
+ return false;
+}
+
int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
struct file *dst_file, loff_t dst_pos, u64 len)
{
@@ -1978,7 +1992,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
goto out_drop_write;
ret = -EINVAL;
- if (!(capable(CAP_SYS_ADMIN) || (dst_file->f_mode & FMODE_WRITE)))
+ if (!allow_file_dedupe(dst_file))
goto out_drop_write;
ret = -EXDEV;
--
2.15.1
^ permalink raw reply related
* [PATCH 2/2] vfs: dedupe should return EPERM if permission is not granted
From: Mark Fasheh @ 2018-09-10 23:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, linux-fsdevel, linux-api, Michael Kerrisk, linux-btrfs,
linux-xfs, Darrick J . Wong, Adam Borowski, David Sterba,
Mark Fasheh
In-Reply-To: <20180910232118.14424-1-mfasheh@suse.de>
Right now we return EINVAL if a process does not have permission to dedupe a
file. This was an oversight on my part. EPERM gives a true description of
the nature of our error, and EINVAL is already used for the case that the
filesystem does not support dedupe.
Signed-off-by: Mark Fasheh <mfasheh@suse.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Acked-by: David Sterba <dsterba@suse.com>
---
fs/read_write.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index be0e8723a049..c734bc2880a5 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1991,7 +1991,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
if (ret < 0)
goto out_drop_write;
- ret = -EINVAL;
+ ret = -EPERM;
if (!allow_file_dedupe(dst_file))
goto out_drop_write;
--
2.15.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