Linux userland API discussions
 help / color / mirror / Atom feed
* [PATCH v8 1/1] man-pages: seccomp.2: document syscall
From: Kees Cook @ 2014-06-24 20:56 UTC (permalink / raw)
  To: Michael Kerrisk (man-pages)
  Cc: Oleg Nesterov, Andy Lutomirski, Alexei Starovoitov, Andrew Morton,
	Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
	linux-api, linux-kernel, x86, linux-arm-kernel, linux-mips,
	linux-arch, linux-security-module, keescook
In-Reply-To: <1403642893-23107-1-git-send-email-keescook@chromium.org>

Combines documentation from prctl, in-kernel seccomp_filter.txt and
dropper.c, along with details specific to the new syscall.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v3:
 - change args to void * (luto)
 - small typo cleanups
v2:
 - add full example code, based on "dropper.c" in samples/seccomp/
---
 man2/seccomp.2 |  400 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 400 insertions(+)
 create mode 100644 man2/seccomp.2

diff --git a/man2/seccomp.2 b/man2/seccomp.2
new file mode 100644
index 0000000..f64950f
--- /dev/null
+++ b/man2/seccomp.2
@@ -0,0 +1,400 @@
+.\" Copyright (C) 2014 Kees Cook <keescook@chromium.org>
+.\" and Copyright (C) 2012 Will Drewry <wad@chromium.org>
+.\" and Copyright (C) 2008 Michael Kerrisk <mtk.manpages@gmail.com>
+.\"
+.\" %%%LICENSE_START(VERBATIM)
+.\" Permission is granted to make and distribute verbatim copies of this
+.\" manual provided the copyright notice and this permission notice are
+.\" preserved on all copies.
+.\"
+.\" Permission is granted to copy and distribute modified versions of this
+.\" manual under the conditions for verbatim copying, provided that the
+.\" entire resulting derived work is distributed under the terms of a
+.\" permission notice identical to this one.
+.\"
+.\" Since the Linux kernel and libraries are constantly changing, this
+.\" manual page may be incorrect or out-of-date.  The author(s) assume no
+.\" responsibility for errors or omissions, or for damages resulting from
+.\" the use of the information contained herein.  The author(s) may not
+.\" have taken the same level of care in the production of this manual,
+.\" which is licensed free of charge, as they might when working
+.\" professionally.
+.\"
+.\" Formatted or processed versions of this manual, if unaccompanied by
+.\" the source, must acknowledge the copyright and authors of this work.
+.\" %%%LICENSE_END
+.\"
+.TH SECCOMP 2 2014-06-23 "Linux" "Linux Programmer's Manual"
+.SH NAME
+seccomp \-
+operate on Secure Computing state of the process
+.SH SYNOPSIS
+.nf
+.B #include <linux/seccomp.h>
+.B #include <linux/filter.h>
+.B #include <linux/audit.h>
+.B #include <linux/signal.h>
+.B #include <sys/ptrace.h>
+
+.BI "int seccomp(unsigned int " operation ", unsigned int " flags ,
+.BI "            void *" args );
+.fi
+.SH DESCRIPTION
+The
+.BR seccomp ()
+system call operates on the Secure Computing (seccomp) state of the
+current process.
+
+Currently, Linux supports the following
+.IR operation
+values:
+.TP
+.BR SECCOMP_SET_MODE_STRICT
+Only system calls that the thread is permitted to make are
+.BR read (2),
+.BR write (2),
+.BR _exit (2),
+and
+.BR sigreturn (2).
+Other system calls result in the delivery of a
+.BR SIGKILL
+signal. Strict secure computing mode is useful for number-crunching
+applications that may need to execute untrusted byte code, perhaps
+obtained by reading from a pipe or socket.
+
+This operation is available only if the kernel is configured with
+.BR CONFIG_SECCOMP
+enabled.
+
+The value of
+.IR flags
+must be 0, and
+.IR args
+must be NULL.
+
+This operation is functionally identical to calling
+.IR "prctl(PR_SET_SECCOMP,\ SECCOMP_MODE_STRICT)" .
+.TP
+.BR SECCOMP_SET_MODE_FILTER
+The system calls allowed are defined by a pointer to a Berkeley Packet
+Filter (BPF) passed via
+.IR args .
+This argument is a pointer to
+.IR "struct\ sock_fprog" ;
+it can be designed to filter arbitrary system calls and system call
+arguments. If the filter is invalid, the call will fail, returning
+.BR EACCESS
+in
+.IR errno .
+
+If
+.BR fork (2),
+.BR clone (2),
+or
+.BR execve (2)
+are allowed by the filter, any child processes will be constrained to
+the same filters and system calls as the parent.
+
+Prior to using this operation, the process must call
+.IR "prctl(PR_SET_NO_NEW_PRIVS,\ 1)"
+or run with
+.BR CAP_SYS_ADMIN
+privileges in its namespace. If these are not true, the call will fail
+and return
+.BR EACCES
+in
+.IR errno .
+This requirement ensures that filter programs cannot be applied to child
+processes with greater privileges than the process that installed them.
+
+Additionally, if
+.BR prctl (2)
+or
+.BR seccomp (2)
+is allowed by the attached filter, additional filters may be layered on
+which will increase evaluation time, but allow for further reduction of
+the attack surface during execution of a process.
+
+This operation is available only if the kernel is configured with
+.BR CONFIG_SECCOMP_FILTER
+enabled.
+
+When
+.IR flags
+are 0, this operation is functionally identical to calling
+.IR "prctl(PR_SET_SECCOMP,\ SECCOMP_MODE_FILTER,\ args)" .
+
+The recognized
+.IR flags
+are:
+.RS
+.TP
+.BR SECCOMP_FILTER_FLAG_TSYNC
+When adding a new filter, synchronize all other threads of the current
+process to the same seccomp filter tree. If any thread cannot do this,
+the call will not attach the new seccomp filter, and will fail returning
+the first thread ID found that cannot synchronize.  Synchronization will
+fail if another thread is in
+.BR SECCOMP_MODE_STRICT
+or if it has attached new seccomp filters to itself, diverging from the
+calling thread's filter tree.
+.RE
+.SH FILTERS
+When adding filters via
+.BR SECCOMP_SET_MODE_FILTER ,
+.IR args
+points to a filter program:
+
+.in +4n
+.nf
+struct sock_fprog {
+    unsigned short      len;    /* Number of BPF instructions */
+    struct sock_filter *filter;
+};
+.fi
+.in
+
+Each program must contain one or more BPF instructions:
+
+.in +4n
+.nf
+struct sock_filter {    /* Filter block */
+    __u16   code;       /* Actual filter code */
+    __u8    jt;         /* Jump true */
+    __u8    jf;         /* Jump false */
+    __u32   k;          /* Generic multiuse field */
+};
+.fi
+.in
+
+When executing the instructions, the BPF program executes over the
+syscall information made available via:
+
+.in +4n
+.nf
+struct seccomp_data {
+    int nr;                     /* system call number */
+    __u32 arch;                 /* AUDIT_ARCH_* value */
+    __u64 instruction_pointer;  /* CPU instruction pointer */
+    __u64 args[6];              /* up to 6 system call arguments */
+};
+.fi
+.in
+
+A seccomp filter may return any of the following values. If multiple
+filters exist, the return value for the evaluation of a given system
+call will always use the highest precedent value. (For example,
+.BR SECCOMP_RET_KILL
+will always take precedence.)
+
+In precedence order, they are:
+.TP
+.BR SECCOMP_RET_KILL
+Results in the task exiting immediately without executing the
+system call.  The exit status of the task (status & 0x7f) will
+be
+.BR SIGSYS ,
+not
+.BR SIGKILL .
+.TP
+.BR SECCOMP_RET_TRAP
+Results in the kernel sending a
+.BR SIGSYS
+signal to the triggering task without executing the system call.
+.IR siginfo\->si_call_addr
+will show the address of the system call instruction, and
+.IR siginfo\->si_syscall
+and
+.IR siginfo\->si_arch
+will indicate which syscall was attempted.  The program counter will be
+as though the syscall happened (i.e. it will not point to the syscall
+instruction).  The return value register will contain an arch\-dependent
+value; if resuming execution, set it to something sensible.
+(The architecture dependency is because replacing it with
+.BR ENOSYS
+could overwrite some useful information.)
+
+The
+.BR SECCOMP_RET_DATA
+portion of the return value will be passed as
+.IR si_errno .
+
+.BR SIGSYS
+triggered by seccomp will have a
+.IR si_code
+of
+.BR SYS_SECCOMP .
+.TP
+.BR SECCOMP_RET_ERRNO
+Results in the lower 16-bits of the return value being passed
+to userland as the
+.IR errno
+without executing the system call.
+.TP
+.BR SECCOMP_RET_TRACE
+When returned, this value will cause the kernel to attempt to
+notify a ptrace()-based tracer prior to executing the system
+call.  If there is no tracer present,
+.BR ENOSYS
+is returned to userland and the system call is not executed.
+
+A tracer will be notified if it requests
+.BR PTRACE_O_TRACESECCOMP
+using
+.IR ptrace(PTRACE_SETOPTIONS) .
+The tracer will be notified of a
+.BR PTRACE_EVENT_SECCOMP
+and the
+.BR SECCOMP_RET_DATA
+portion of the BPF program return value will be available to the tracer
+via
+.BR PTRACE_GETEVENTMSG .
+
+The tracer can skip the system call by changing the syscall number
+to \-1.  Alternatively, the tracer can change the system call
+requested by changing the system call to a valid syscall number.  If
+the tracer asks to skip the system call, then the system call will
+appear to return the value that the tracer puts in the return value
+register.
+
+The seccomp check will not be run again after the tracer is
+notified.  (This means that seccomp-based sandboxes MUST NOT
+allow use of ptrace, even of other sandboxed processes, without
+extreme care; ptracers can use this mechanism to escape.)
+.TP
+.BR SECCOMP_RET_ALLOW
+Results in the system call being executed.
+
+If multiple filters exist, the return value for the evaluation of a
+given system call will always use the highest precedent value.
+
+Precedence is only determined using the
+.BR SECCOMP_RET_ACTION
+mask.  When multiple filters return values of the same precedence,
+only the
+.BR SECCOMP_RET_DATA
+from the most recently installed filter will be returned.
+.SH RETURN VALUE
+On success,
+.BR seccomp ()
+returns 0.
+On error, if
+.BR SECCOMP_FILTER_FLAG_TSYNC
+was used, the return value is the thread ID that caused the
+synchronization failure. On other errors, \-1 is returned, and
+.IR errno
+is set to indicate the cause of the error.
+.SH ERRORS
+.BR seccomp ()
+can fail for the following reasons:
+.TP
+.BR EACCESS
+the caller did not have the
+.BR CAP_SYS_ADMIN
+capability, or had not set
+.IR no_new_privs
+before using
+.BR SECCOMP_SET_MODE_FILTER .
+.TP
+.BR EFAULT
+.IR args
+was required to be a valid address.
+.TP
+.BR EINVAL
+.IR operation
+is unknown; or
+.IR flags
+are invalid for the given
+.IR operation
+.TP
+.BR ESRCH
+Another thread caused a failure during thread sync, but its ID could not
+be determined.
+.SH VERSIONS
+This system call first appeared in Linux 3.16.
+.\" FIXME Add glibc version
+.SH CONFORMING TO
+This system call is a nonstandard Linux extension.
+.SH NOTES
+.BR seccomp ()
+provides a superset of the functionality provided by
+.IR PR_SET_SECCOMP
+of
+.BR prctl (2) .
+(Which does not support
+.IR flags .)
+.SH EXAMPLE
+.nf
+#include <errno.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/audit.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+#include <sys/prctl.h>
+
+static int install_filter(int syscall, int arch, int error)
+{
+    struct sock_filter filter[] = {
+        /* Load architecture. */
+        BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+                 (offsetof(struct seccomp_data, arch))),
+        /* Jump forward 4 instructions on architecture mismatch. */
+        BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, arch, 0, 4),
+        /* Load syscall number. */
+        BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+                 (offsetof(struct seccomp_data, nr))),
+        /* Jump forward 1 instruction on syscall mismatch. */
+        BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, syscall, 0, 1),
+        /* Matching arch and syscall: return specific errno. */
+        BPF_STMT(BPF_RET+BPF_K,
+                 SECCOMP_RET_ERRNO|(error & SECCOMP_RET_DATA)),
+        /* Destination of syscall mismatch: Allow other syscalls. */
+        BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+        /* Destination of arch mismatch: Kill process. */
+        BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_KILL),
+    };
+    struct sock_fprog prog = {
+        .len = (unsigned short)(sizeof(filter)/sizeof(filter[0])),
+        .filter = filter,
+    };
+    if (seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog)) {
+        perror("seccomp");
+        return EXIT_FAILURE;
+    }
+    return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+    if (argc < 5) {
+        fprintf(stderr, "Usage:\\n"
+                "refuse <syscall_nr> <arch> <errno> <prog> [<args>]\\n"
+                "Hint:  AUDIT_ARCH_I386: 0x%X\\n"
+                "       AUDIT_ARCH_X86_64: 0x%X\\n"
+                "\\n", AUDIT_ARCH_I386, AUDIT_ARCH_X86_64);
+        return EXIT_FAILURE;
+    }
+    if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+        perror("prctl");
+        return EXIT_FAILURE;
+    }
+    if (install_filter(strtol(argv[1], NULL, 0),
+                       strtol(argv[2], NULL, 0),
+                       strtol(argv[3], NULL, 0)))
+        return EXIT_FAILURE;
+    execv(argv[4], &argv[4]);
+    perror("execv");
+    return EXIT_FAILURE;
+}
+.fi
+.SH SEE ALSO
+.ad l
+.nh
+.BR prctl (2),
+.BR ptrace (2),
+.BR signal (7),
+.BR socket (7)
+.ad
-- 
1.7.9.5



-- 
Kees Cook                                            @outflux.net

^ permalink raw reply related

* [patch 1/4] timerfd: Implement show_fdinfo method
From: Cyrill Gorcunov @ 2014-06-24 22:02 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA
  Cc: Michael Kerrisk, Thomas Gleixner, Andrew Morton, Andrey Vagin,
	Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140623190345.354980826-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>

For checkpoint/restore of timerfd files we need to know how exactly
the timer were armed, to be able to recreate it on restore stage.
Thus implement show_fdinfo method which provides enough information
for that.

One of significant changes I think is the addition of @settime_flags
member. Currently there are two flags TFD_TIMER_ABSTIME and
TFD_TIMER_CANCEL_ON_SET, and the second can be found from
@might_cancel variable but in case if the flags will be extended
in future we most probably will have to somehow remember them
explicitly anyway so I guss doing that right now won't hurt.

To not bloat the timerfd_ctx structure I've converted @expired
to short integer and defined @settime_flags as short too.

v2 (by avagin@, vdavydov@ and tglx@):

 - Add it_value/it_interval fields
 - Save flags being used in timerfd_setup in context

v3 (by tglx@):
 - don't forget to use CONFIG_PROC_FS

v4 (by akpm@):
 -Use define timerfd_show NULL for non c/r config

CC: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---

A nit fixed in timerfd_show for non c/r config

 fs/timerfd.c |   34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -35,8 +35,9 @@ struct timerfd_ctx {
 	ktime_t moffs;
 	wait_queue_head_t wqh;
 	u64 ticks;
-	int expired;
 	int clockid;
+	short unsigned expired;
+	short unsigned settime_flags;	/* to show in fdinfo */
 	struct rcu_head rcu;
 	struct list_head clist;
 	bool might_cancel;
@@ -196,6 +197,8 @@ static int timerfd_setup(struct timerfd_
 		if (timerfd_canceled(ctx))
 			return -ECANCELED;
 	}
+
+	ctx->settime_flags = flags & TFD_SETTIME_FLAGS;
 	return 0;
 }
 
@@ -284,11 +287,40 @@ static ssize_t timerfd_read(struct file
 	return res;
 }
 
+#ifdef CONFIG_PROC_FS
+static int timerfd_show(struct seq_file *m, struct file *file)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	struct itimerspec t;
+
+	spin_lock_irq(&ctx->wqh.lock);
+	t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
+	t.it_interval = ktime_to_timespec(ctx->tintv);
+	spin_unlock_irq(&ctx->wqh.lock);
+
+	return seq_printf(m,
+			  "clockid: %d\n"
+			  "ticks: %llu\n"
+			  "settime flags: 0%o\n"
+			  "it_value: (%llu, %llu)\n"
+			  "it_interval: (%llu, %llu)\n",
+			  ctx->clockid, (unsigned long long)ctx->ticks,
+			  ctx->settime_flags,
+			  (unsigned long long)t.it_value.tv_sec,
+			  (unsigned long long)t.it_value.tv_nsec,
+			  (unsigned long long)t.it_interval.tv_sec,
+			  (unsigned long long)t.it_interval.tv_nsec);
+}
+#else
+#define timerfd_show NULL
+#endif
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
 	.llseek		= noop_llseek,
+	.show_fdinfo	= timerfd_show,
 };
 
 static int timerfd_fget(int fd, struct fd *p)

^ permalink raw reply

* [patch 3/4] timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks
From: Cyrill Gorcunov @ 2014-06-24 22:03 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA
  Cc: Michael Kerrisk, Thomas Gleixner, Andrew Morton, Andrey Vagin,
	Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140623190345.499956567-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>

The read() of timerfd files allows to fetch the number of timer ticks
while there is no way to set it back from userspace.

To restore the timer's state as it was at checkpoint moment we need
a path to bring @ticks back. Initially I thought about writing ticks
back via write() interface but it seems such API is somehow obscure.

Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
command which allows to adjust @ticks into non-zero value waking
up the waiters.

I wrapped code with CONFIG_CHECKPOINT_RESTORE which can be
dropped off if there users except c/r camp appear.

v2 (by akpm@):
 -Use define timerfd_ioctl NULL for non c/r config

CC: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---

A nit fixed for for non c/r config in timerfd_ioctl declaration

 fs/timerfd.c            |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/timerfd.h |    5 +++++
 2 files changed, 42 insertions(+)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -315,12 +315,49 @@ static int timerfd_show(struct seq_file
 #define timerfd_show NULL
 #endif
 
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	int ret = 0;
+
+	switch (cmd) {
+	case TFD_IOC_SET_TICKS: {
+		u64 ticks;
+
+		if (get_user(ticks, (u64 __user *)arg))
+			return -EFAULT;
+		if (!ticks)
+			return -EINVAL;
+
+		spin_lock_irq(&ctx->wqh.lock);
+		if (!timerfd_canceled(ctx)) {
+			ctx->ticks = ticks;
+			if (ticks)
+				wake_up_locked(&ctx->wqh);
+		} else
+			ret = -ECANCELED;
+		spin_unlock_irq(&ctx->wqh.lock);
+		break;
+	}
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+
+	return ret;
+}
+#else
+#define timerfd_ioctl NULL
+#endif
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
 	.llseek		= noop_llseek,
 	.show_fdinfo	= timerfd_show,
+	.unlocked_ioctl	= timerfd_ioctl,
 };
 
 static int timerfd_fget(int fd, struct fd *p)
Index: linux-2.6.git/include/linux/timerfd.h
===================================================================
--- linux-2.6.git.orig/include/linux/timerfd.h
+++ linux-2.6.git/include/linux/timerfd.h
@@ -11,6 +11,9 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
+/* For _IO helpers */
+#include <linux/ioctl.h>
+
 /*
  * CAREFUL: Check include/asm-generic/fcntl.h when defining
  * new flags, since they might collide with O_* ones. We want
@@ -29,4 +32,6 @@
 /* Flags for timerfd_settime.  */
 #define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
 
+#define TFD_IOC_SET_TICKS	_IOW('T', 0, u64)
+
 #endif /* _LINUX_TIMERFD_H */

^ permalink raw reply

* Re: [PATCH] mm: update the description for madvise_remove
From: David Rientjes @ 2014-06-24 22:44 UTC (permalink / raw)
  To: Wang Sheng-Hui, Michael Kerrisk
  Cc: Andrew Morton, Naoya Horiguchi, Johannes Weiner, Andi Kleen,
	Vladimir Cernov, linux-mm, linux-kernel, linux-api
In-Reply-To: <53A9116B.9030004@gmail.com>

On Tue, 24 Jun 2014, Wang Sheng-Hui wrote:

> 
> Currently, we have more filesystems supporting fallocate, e.g
> ext4/btrfs. Remove the outdated comment for madvise_remove.
> 
> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> ---
>  mm/madvise.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index a402f8f..0938b30 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -292,9 +292,6 @@ static long madvise_dontneed(struct vm_area_struct *vma,
>  /*
>   * Application wants to free up the pages and associated backing store.
>   * This is effectively punching a hole into the middle of a file.
> - *
> - * NOTE: Currently, only shmfs/tmpfs is supported for this operation.
> - * Other filesystems return -ENOSYS.
>   */
>  static long madvise_remove(struct vm_area_struct *vma,
>                                 struct vm_area_struct **prev,

[For those without context: this patch has been merged into the -mm tree.]

This reference also exists in the man-page for madvise(2), are you 
planning on removing it as well?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply

* Re: [PATCH] mm: update the description for madvise_remove
From: Wang Sheng-Hui @ 2014-06-25  1:58 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, Naoya Horiguchi, Johannes Weiner, Andi Kleen,
	Vladimir Cernov, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-man-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.02.1406241542040.29176-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>

Patch to man-page.

[PATCH] madvise.2: update the description for MADV_REMOVE

Currently we have more filesystems supporting fallcate, e.g ext4/btrfs,
which can response to MADV_REMOVE gracefully.

And if filesystems don't support fallocate, the return error would be
EOPNOTSUPP, instead of ENOSYS.

Signed-off-by: Wang Sheng-Hui <shhuiw-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 man2/madvise.2 | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/man2/madvise.2 b/man2/madvise.2
index 032ead7..4ce869c 100644
--- a/man2/madvise.2
+++ b/man2/madvise.2
@@ -99,13 +99,9 @@ or zero-fill-on-demand pages for mappings
 without an underlying file.
 .TP
 .BR MADV_REMOVE " (since Linux 2.6.16)"
-Free up a given range of pages
-and its associated backing store.
-Currently,
-.\" 2.6.18-rc5
-only shmfs/tmpfs supports this; other filesystems return with the
-error
-.BR ENOSYS .
+Free up a given range of pages and its associated backing store.
+Filesystems that don't support fallocate will return error
+.BR EOPNOTSUPP.
 .\" Databases want to use this feature to drop a section of their
 .\" bufferpool (shared memory segments) - without writing back to
 .\" disk/swap space.  This feature is also useful for supporting
-- 
1.8.3.2



On 2014年06月25日 06:44, David Rientjes wrote:
> On Tue, 24 Jun 2014, Wang Sheng-Hui wrote:
> 
>>
>> Currently, we have more filesystems supporting fallocate, e.g
>> ext4/btrfs. Remove the outdated comment for madvise_remove.
>>
>> Signed-off-by: Wang Sheng-Hui <shhuiw-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  mm/madvise.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index a402f8f..0938b30 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -292,9 +292,6 @@ static long madvise_dontneed(struct vm_area_struct *vma,
>>  /*
>>   * Application wants to free up the pages and associated backing store.
>>   * This is effectively punching a hole into the middle of a file.
>> - *
>> - * NOTE: Currently, only shmfs/tmpfs is supported for this operation.
>> - * Other filesystems return -ENOSYS.
>>   */
>>  static long madvise_remove(struct vm_area_struct *vma,
>>                                 struct vm_area_struct **prev,
> 
> [For those without context: this patch has been merged into the -mm tree.]
> 
> This reference also exists in the man-page for madvise(2), are you 
> planning on removing it as well?
> 

^ permalink raw reply related

* RE: [PATCH v2] ns: introduce getnspid syscall
From: chenhanxiao-BthXqXjhjHXQFUHtdCDX3A @ 2014-06-25 10:00 UTC (permalink / raw)
  To: Serge E. Hallyn, Eric W. Biederman
  Cc: Richard Weinberger,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Pavel Emelyanov,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Serge Hallyn,
	Oleg Nesterov, David Howells, Al Viro, Gotou, Yasunori
In-Reply-To: <20140623133230.GA31817-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>

Hi,

> -----Original Message-----
> From: Serge E. Hallyn [mailto:serge@hallyn.com]
> Sent: Monday, June 23, 2014 9:33 PM
> To: Chen, Hanxiao/陈 晗霄
> Cc: Richard Weinberger; containers@lists.linux-foundation.org;
> linux-kernel@vger.kernel.org; Pavel Emelyanov; linux-api@vger.kernel.org;
> Serge Hallyn; Oleg Nesterov; David Howells; Eric W. Biederman; Al Viro
> Subject: Re: [PATCH v2] ns: introduce getnspid syscall
> 
> > > >
> > >
> > > I don't think that adding a new system call for this is a good solution.
> > > We need a more generic way. I bet people are interested in more than just
> PID
> > > numbers.
> >
> > Could you please give some hints on how to expand this interface?
> >
> > >
> > > I agree with Eric that a procfs solution is more appropriate.
> > >
> >
> > Procfs is a good solution, but syscall is not bad though.
> 
> I might be inclined to agree, except that in this case you are still
> needing mounted procfs anyway to get the proc/$pid/ns/pid fds.
> 
> I'm sorry, I've not been watching this thread, so this probably has been
> considered and decided against, but I'm going to ask anyway.  Keeping
> in mind both checkpoint-restart and and introspection for use in a
> setns'd commend, why not make it
> 
> pid_t getnspid(pid_t query_pid, pid_t observer_pid)
> 
> which returns the process id of query_pid as seen from observer_pid's
> pidns?
> 

But this could be confused in nested ns.

Ex:
(thanks for Pavel's figure)
    init_pid_ns    ns1         ns2
t1  2
t2   `- 3          1 
t3       `- 4      `- 5        1
t4  5

a) getnspid(1, 1):
We expected it could return t2's pid(2nd 1 as pid
such as systemd in init_pid_ns),
but t3'pid is also an appropriate result.
We may get more than one returns.

b) getnspid(5, 1):
(1st 5 was expected as  pid in ns1)
t3'pid and t4's pid could both be the answer.
We could not determine which one is what we want.

So something unique like fds of ns should be
a better reference. 

Thanks,
- Chen

> 
> > Procfs works for me, but that seems could not fit
> > Pavel's requirement.
> > His opinion is that a syscall is a more generic interface
> > than proc files, and  also very helpful.
> > And syscall could tell whether a pid lives in a specific pid namespace,
> > much convenient than procfs.
> >
> > Thanks,
> > - Chen
> 
> > _______________________________________________
> > Containers mailing list
> > Containers@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/containers


^ permalink raw reply

* hello
From: ellenawa william @ 2014-06-25 12:12 UTC (permalink / raw)





Good day

Please did you receive the mail i sent to you last time.

^ permalink raw reply

* Re: [PATCH v8 1/1] man-pages: seccomp.2: document syscall
From: One Thousand Gnomes @ 2014-06-25 13:04 UTC (permalink / raw)
  To: Kees Cook
  Cc: Michael Kerrisk (man-pages), Oleg Nesterov, Andy Lutomirski,
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, linux-api, linux-kernel, x86,
	linux-arm-kernel, linux-mips, linux-arch, linux-security-module
In-Reply-To: <20140624205615.GW5412@outflux.net>

On Tue, 24 Jun 2014 13:56:15 -0700
Kees Cook <keescook@chromium.org> wrote:

> Combines documentation from prctl, in-kernel seccomp_filter.txt and
> dropper.c, along with details specific to the new syscall.

What is the license on the example ? Probably you want to propogate the
minimal form of the text in seccomp/dropper into the document example to
avoid confusion ?

Alan

^ permalink raw reply

* Re: [PATCH v8 4/9] sched: move no_new_privs into new atomic flags
From: Oleg Nesterov @ 2014-06-25 13:43 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
	linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403642893-23107-5-git-send-email-keescook@chromium.org>

On 06/24, Kees Cook wrote:
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1307,8 +1307,7 @@ struct task_struct {
>  				 * execve */
>  	unsigned in_iowait:1;
>
> -	/* task may not gain privileges */
> -	unsigned no_new_privs:1;
> +	unsigned long atomic_flags; /* Flags needing atomic access. */
>
>  	/* Revert to default priority/policy when forking */
>  	unsigned sched_reset_on_fork:1;

Agreed, personally I like it more than seccomp->flags.

But probably it would be better to place the new member before/after
other bitfields to save the space?

Oleg.

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Oleg Nesterov @ 2014-06-25 13:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
	linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403642893-23107-6-git-send-email-keescook@chromium.org>

On 06/24, Kees Cook wrote:
>
> +static inline void seccomp_assign_mode(struct task_struct *task,
> +				       unsigned long seccomp_mode)
> +{
> +	BUG_ON(!spin_is_locked(&task->sighand->siglock));
> +
> +	task->seccomp.mode = seccomp_mode;
> +	set_tsk_thread_flag(task, TIF_SECCOMP);
> +}

OK, but unless task == current this can race with secure_computing().
I think this needs smp_mb__before_atomic() and secure_computing() needs
rmb() after test_bit(TIF_SECCOMP).

Otherwise, can't __secure_computing() hit BUG() if it sees the old
mode == SECCOMP_MODE_DISABLED ?

Or seccomp_run_filters() can see ->filters == NULL and WARN(),
smp_load_acquire() only serializes that LOAD with the subsequent memory
operations.

Oleg.

^ permalink raw reply

* Re: [PATCH v8 3/9] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-06-25 14:03 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
	linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403642893-23107-4-git-send-email-keescook@chromium.org>

On 06/24, Kees Cook wrote:
>
> @@ -524,6 +529,8 @@ static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter)
>  	}
>  #endif
>
> +	spin_lock_irqsave(&current->sighand->siglock, irqflags);
> +

Well, I won't argue if you prefer to use _irqsave "just in case".

But irqs must be enabled in syscall paths, you could use spin_lock_irq().
The same for seccomp_set_mode_filter() added later.

Oleg.

^ permalink raw reply

* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-06-25 14:21 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
	linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403642893-23107-10-git-send-email-keescook@chromium.org>

On 06/24, Kees Cook wrote:
>
> +static void seccomp_sync_threads(void)
> +{
> +	struct task_struct *thread, *caller;
> +
> +	BUG_ON(!spin_is_locked(&current->sighand->siglock));
> +
> +	/* Synchronize all threads. */
> +	caller = current;
> +	for_each_thread(caller, thread) {
> +		/* Get a task reference for the new leaf node. */
> +		get_seccomp_filter(caller);
> +		/*
> +		 * Drop the task reference to the shared ancestor since
> +		 * current's path will hold a reference.  (This also
> +		 * allows a put before the assignment.)
> +		 */
> +		put_seccomp_filter(thread);
> +		thread->seccomp.filter = caller->seccomp.filter;
> +		/* Opt the other thread into seccomp if needed.
> +		 * As threads are considered to be trust-realm
> +		 * equivalent (see ptrace_may_access), it is safe to
> +		 * allow one thread to transition the other.
> +		 */
> +		if (thread->seccomp.mode == SECCOMP_MODE_DISABLED) {
> +			/*
> +			 * Don't let an unprivileged task work around
> +			 * the no_new_privs restriction by creating
> +			 * a thread that sets it up, enters seccomp,
> +			 * then dies.
> +			 */
> +			if (task_no_new_privs(caller))
> +				task_set_no_new_privs(thread);
> +
> +			seccomp_assign_mode(thread, SECCOMP_MODE_FILTER);
> +		}
> +	}
> +}

OK, personally I think this all make sense. I even think that perhaps
SECCOMP_FILTER_FLAG_TSYNC should allow filter == NULL, a thread might
want to "sync" without adding the new filter, but this is minor/offtopic.

But. Doesn't this change add the new security hole?

Obviously, we should not allow to install a filter and then (say) exec
a suid binary, that is why we have no_new_privs/LSM_UNSAFE_NO_NEW_PRIVS.

But what if "thread->seccomp.filter = caller->seccomp.filter" races with
any user of task_no_new_privs() ? Say, suppose this thread has already
passed check_unsafe_exec/etc and it is going to exec the suid binary?

Oleg.

^ permalink raw reply

* Re: [PATCH v2] ns: introduce getnspid syscall
From: Serge Hallyn @ 2014-06-25 14:38 UTC (permalink / raw)
  To: chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org
  Cc: Richard Weinberger,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	Oleg Nesterov, Al Viro,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	David Howells, Eric W. Biederman,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <5871495633F38949900D2BF2DC04883E551F09-ZEd+hNNJ6a5ZYpXjqAkB5jz3u5zwRJJDAzI0kPv9QBlmR6Xm/wNWPw@public.gmane.org>

Quoting chenhanxiao@cn.fujitsu.com (chenhanxiao@cn.fujitsu.com):
> Hi,
> 
> > -----Original Message-----
> > From: Serge E. Hallyn [mailto:serge@hallyn.com]
> > Sent: Monday, June 23, 2014 9:33 PM
> > To: Chen, Hanxiao/陈 晗霄
> > Cc: Richard Weinberger; containers@lists.linux-foundation.org;
> > linux-kernel@vger.kernel.org; Pavel Emelyanov; linux-api@vger.kernel.org;
> > Serge Hallyn; Oleg Nesterov; David Howells; Eric W. Biederman; Al Viro
> > Subject: Re: [PATCH v2] ns: introduce getnspid syscall
> > 
> > > > >
> > > >
> > > > I don't think that adding a new system call for this is a good solution.
> > > > We need a more generic way. I bet people are interested in more than just
> > PID
> > > > numbers.
> > >
> > > Could you please give some hints on how to expand this interface?
> > >
> > > >
> > > > I agree with Eric that a procfs solution is more appropriate.
> > > >
> > >
> > > Procfs is a good solution, but syscall is not bad though.
> > 
> > I might be inclined to agree, except that in this case you are still
> > needing mounted procfs anyway to get the proc/$pid/ns/pid fds.
> > 
> > I'm sorry, I've not been watching this thread, so this probably has been
> > considered and decided against, but I'm going to ask anyway.  Keeping
> > in mind both checkpoint-restart and and introspection for use in a
> > setns'd commend, why not make it
> > 
> > pid_t getnspid(pid_t query_pid, pid_t observer_pid)
> > 
> > which returns the process id of query_pid as seen from observer_pid's
> > pidns?
> > 
> 
> But this could be confused in nested ns.
> 
> Ex:
> (thanks for Pavel's figure)
>     init_pid_ns    ns1         ns2
> t1  2
> t2   `- 3          1 
> t3       `- 4      `- 5        1
> t4  5
> 
> a) getnspid(1, 1):
> We expected it could return t2's pid(2nd 1 as pid

Clearly the passed-in pids should be interpreted as relative
to current's pidns.  There can be no ambiguity at that point,
unless I'm overlooking something.

> such as systemd in init_pid_ns),
> but t3'pid is also an appropriate result.
> We may get more than one returns.
> 
> b) getnspid(5, 1):
> (1st 5 was expected as  pid in ns1)
> t3'pid and t4's pid could both be the answer.
> We could not determine which one is what we want.
> 
> So something unique like fds of ns should be
> a better reference. 
> 
> Thanks,
> - Chen
> 
> > 
> > > Procfs works for me, but that seems could not fit
> > > Pavel's requirement.
> > > His opinion is that a syscall is a more generic interface
> > > than proc files, and  also very helpful.
> > > And syscall could tell whether a pid lives in a specific pid namespace,
> > > much convenient than procfs.
> > >
> > > Thanks,
> > > - Chen
> > 
> > > _______________________________________________
> > > Containers mailing list
> > > Containers@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/containers
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCH v8 4/9] sched: move no_new_privs into new atomic flags
From: Kees Cook @ 2014-06-25 14:44 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <20140625134354.GA7892@redhat.com>

On Wed, Jun 25, 2014 at 6:43 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/24, Kees Cook wrote:
>>
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1307,8 +1307,7 @@ struct task_struct {
>>                                * execve */
>>       unsigned in_iowait:1;
>>
>> -     /* task may not gain privileges */
>> -     unsigned no_new_privs:1;
>> +     unsigned long atomic_flags; /* Flags needing atomic access. */
>>
>>       /* Revert to default priority/policy when forking */
>>       unsigned sched_reset_on_fork:1;
>
> Agreed, personally I like it more than seccomp->flags.
>
> But probably it would be better to place the new member before/after
> other bitfields to save the space?

Sure, I'll move it down. (Though I thought the compiler was smarter about that.)

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Kees Cook @ 2014-06-25 14:51 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <20140625135121.GB7892@redhat.com>

On Wed, Jun 25, 2014 at 6:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/24, Kees Cook wrote:
>>
>> +static inline void seccomp_assign_mode(struct task_struct *task,
>> +                                    unsigned long seccomp_mode)
>> +{
>> +     BUG_ON(!spin_is_locked(&task->sighand->siglock));
>> +
>> +     task->seccomp.mode = seccomp_mode;
>> +     set_tsk_thread_flag(task, TIF_SECCOMP);
>> +}
>
> OK, but unless task == current this can race with secure_computing().
> I think this needs smp_mb__before_atomic() and secure_computing() needs
> rmb() after test_bit(TIF_SECCOMP).
>
> Otherwise, can't __secure_computing() hit BUG() if it sees the old
> mode == SECCOMP_MODE_DISABLED ?
>
> Or seccomp_run_filters() can see ->filters == NULL and WARN(),
> smp_load_acquire() only serializes that LOAD with the subsequent memory
> operations.

Hm, actually, now I'm worried about smp_load_acquire() being too slow
in run_filters().

The ordering must be:
- task->seccomp.filter must be valid before
- task->seccomp.mode is set, which must be valid before
- TIF_SECCOMP is set

But I don't want to impact secure_computing(). What's the best way to
make sure this ordering is respected?

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-06-25 15:08 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <20140625142121.GD7892@redhat.com>

On Wed, Jun 25, 2014 at 7:21 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/24, Kees Cook wrote:
>>
>> +static void seccomp_sync_threads(void)
>> +{
>> +     struct task_struct *thread, *caller;
>> +
>> +     BUG_ON(!spin_is_locked(&current->sighand->siglock));
>> +
>> +     /* Synchronize all threads. */
>> +     caller = current;
>> +     for_each_thread(caller, thread) {
>> +             /* Get a task reference for the new leaf node. */
>> +             get_seccomp_filter(caller);
>> +             /*
>> +              * Drop the task reference to the shared ancestor since
>> +              * current's path will hold a reference.  (This also
>> +              * allows a put before the assignment.)
>> +              */
>> +             put_seccomp_filter(thread);
>> +             thread->seccomp.filter = caller->seccomp.filter;
>> +             /* Opt the other thread into seccomp if needed.
>> +              * As threads are considered to be trust-realm
>> +              * equivalent (see ptrace_may_access), it is safe to
>> +              * allow one thread to transition the other.
>> +              */
>> +             if (thread->seccomp.mode == SECCOMP_MODE_DISABLED) {
>> +                     /*
>> +                      * Don't let an unprivileged task work around
>> +                      * the no_new_privs restriction by creating
>> +                      * a thread that sets it up, enters seccomp,
>> +                      * then dies.
>> +                      */
>> +                     if (task_no_new_privs(caller))
>> +                             task_set_no_new_privs(thread);
>> +
>> +                     seccomp_assign_mode(thread, SECCOMP_MODE_FILTER);
>> +             }
>> +     }
>> +}
>
> OK, personally I think this all make sense. I even think that perhaps
> SECCOMP_FILTER_FLAG_TSYNC should allow filter == NULL, a thread might
> want to "sync" without adding the new filter, but this is minor/offtopic.
>
> But. Doesn't this change add a new security hole?
>
> Obviously, we should not allow to install a filter and then (say) exec
> a suid binary, that is why we have no_new_privs/LSM_UNSAFE_NO_NEW_PRIVS.
>
> But what if "thread->seccomp.filter = caller->seccomp.filter" races with
> any user of task_no_new_privs() ? Say, suppose this thread has already
> passed check_unsafe_exec/etc and it is going to exec the suid binary?

Oh, ew. Yeah. It looks like there's a cred lock to be held to combat this?

I wonder if changes to nnp need to "flushed" during syscall entry
instead of getting updated externally/asynchronously? That way it
won't be out of sync with the seccomp mode/filters.

Perhaps secure computing needs to check some (maybe seccomp-only)
atomic flags and flip on the "real" nnp if found?

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 1/1] man-pages: seccomp.2: document syscall
From: Kees Cook @ 2014-06-25 15:10 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Michael Kerrisk (man-pages), Oleg Nesterov, Andy Lutomirski,
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, LKML, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <20140625140440.6870eac1@alan.etchedpixels.co.uk>

On Wed, Jun 25, 2014 at 6:04 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Tue, 24 Jun 2014 13:56:15 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> Combines documentation from prctl, in-kernel seccomp_filter.txt and
>> dropper.c, along with details specific to the new syscall.
>
> What is the license on the example ? Probably you want to propogate the
> minimal form of the text in seccomp/dropper into the document example to
> avoid confusion ?

What is the license of the other code examples in man-pages?

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Andy Lutomirski @ 2014-06-25 16:10 UTC (permalink / raw)
  To: Kees Cook
  Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CAGXu5jJkFxh4K=40xuh6tu3kUf4oJM8Dry+4upBdRieW3FNLgw@mail.gmail.com>

On Wed, Jun 25, 2014 at 7:51 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 25, 2014 at 6:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 06/24, Kees Cook wrote:
>>>
>>> +static inline void seccomp_assign_mode(struct task_struct *task,
>>> +                                    unsigned long seccomp_mode)
>>> +{
>>> +     BUG_ON(!spin_is_locked(&task->sighand->siglock));
>>> +
>>> +     task->seccomp.mode = seccomp_mode;
>>> +     set_tsk_thread_flag(task, TIF_SECCOMP);
>>> +}
>>
>> OK, but unless task == current this can race with secure_computing().
>> I think this needs smp_mb__before_atomic() and secure_computing() needs
>> rmb() after test_bit(TIF_SECCOMP).
>>
>> Otherwise, can't __secure_computing() hit BUG() if it sees the old
>> mode == SECCOMP_MODE_DISABLED ?
>>
>> Or seccomp_run_filters() can see ->filters == NULL and WARN(),
>> smp_load_acquire() only serializes that LOAD with the subsequent memory
>> operations.
>
> Hm, actually, now I'm worried about smp_load_acquire() being too slow
> in run_filters().
>
> The ordering must be:
> - task->seccomp.filter must be valid before
> - task->seccomp.mode is set, which must be valid before
> - TIF_SECCOMP is set
>
> But I don't want to impact secure_computing(). What's the best way to
> make sure this ordering is respected?

Remove the ordering requirement, perhaps?

What if you moved mode into seccomp.filter?  Then there would be
little reason to check TIF_SECCOMP from secure_computing; instead, you
could smp_load_acquire (or read_barrier_depends, maybe) seccomp.filter
from secure_computing and pass the result as a parameter to
__secure_computing.  Or you could even remove the distinction between
secure_computing and __secure_computing -- it's essentially useless
anyway to split entry hook approaches like my x86 fastpath prototype.

--Andy

^ permalink raw reply

* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-06-25 16:52 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CAGXu5jJtLrjbobZC1FD4WV-Jm2p7cRGa1aSPK-d_isnfCZAHdA@mail.gmail.com>

On 06/25, Kees Cook wrote:
>
> On Wed, Jun 25, 2014 at 7:21 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > But. Doesn't this change add a new security hole?
> >
> > Obviously, we should not allow to install a filter and then (say) exec
> > a suid binary, that is why we have no_new_privs/LSM_UNSAFE_NO_NEW_PRIVS.
> >
> > But what if "thread->seccomp.filter = caller->seccomp.filter" races with
> > any user of task_no_new_privs() ? Say, suppose this thread has already
> > passed check_unsafe_exec/etc and it is going to exec the suid binary?
>
> Oh, ew. Yeah. It looks like there's a cred lock to be held to combat this?

Yes, cred_guard_mutex looks like an obvious choice... Hmm, but somehow
initially I thought that the fix won't be simple. Not sure why.

Yes, at least this should close the race with suid-exec. And there are no
other users. Except apparmor, and I hope you will check it because I simply
do not know what it does ;)

> I wonder if changes to nnp need to "flushed" during syscall entry
> instead of getting updated externally/asynchronously? That way it
> won't be out of sync with the seccomp mode/filters.
>
> Perhaps secure computing needs to check some (maybe seccomp-only)
> atomic flags and flip on the "real" nnp if found?

Not sure I understand you, could you clarify?

But I was also worried that task_no_new_privs(current) is no longer stable
inside the syscall paths, perhaps this is what you meant? However I do not
see something bad here... And this has nothing to do with the race above.



Also. Even ignoring no_new_privs, SECCOMP_FILTER_FLAG_TSYNC is not atomic
and we can do nothing with this fact (unless it try to freeze the thread
group somehow), perhaps it makes sense to document this somehow.

I mean, suppose you want to ensure write-to-file is not possible, so you
do seccomp(SECCOMP_FILTER_FLAG_TSYNC, nack_write_to_file_filter). You can't
assume that this has effect right after seccomp() returns, this can obviously
race with a sub-thread which has already entered sys_write().

Once again, I am not arguing, just I think it makes sense to at least mention
the limitations during the discussion.

Oleg.

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Kees Cook @ 2014-06-25 16:54 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CALCETrUBNmLnpa+LM91om2RSpR6SjupP-EdefzhU1Me4nv3Dfw@mail.gmail.com>

On Wed, Jun 25, 2014 at 9:10 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Jun 25, 2014 at 7:51 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Jun 25, 2014 at 6:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 06/24, Kees Cook wrote:
>>>>
>>>> +static inline void seccomp_assign_mode(struct task_struct *task,
>>>> +                                    unsigned long seccomp_mode)
>>>> +{
>>>> +     BUG_ON(!spin_is_locked(&task->sighand->siglock));
>>>> +
>>>> +     task->seccomp.mode = seccomp_mode;
>>>> +     set_tsk_thread_flag(task, TIF_SECCOMP);
>>>> +}
>>>
>>> OK, but unless task == current this can race with secure_computing().
>>> I think this needs smp_mb__before_atomic() and secure_computing() needs
>>> rmb() after test_bit(TIF_SECCOMP).
>>>
>>> Otherwise, can't __secure_computing() hit BUG() if it sees the old
>>> mode == SECCOMP_MODE_DISABLED ?
>>>
>>> Or seccomp_run_filters() can see ->filters == NULL and WARN(),
>>> smp_load_acquire() only serializes that LOAD with the subsequent memory
>>> operations.
>>
>> Hm, actually, now I'm worried about smp_load_acquire() being too slow
>> in run_filters().
>>
>> The ordering must be:
>> - task->seccomp.filter must be valid before
>> - task->seccomp.mode is set, which must be valid before
>> - TIF_SECCOMP is set
>>
>> But I don't want to impact secure_computing(). What's the best way to
>> make sure this ordering is respected?
>
> Remove the ordering requirement, perhaps?
>
> What if you moved mode into seccomp.filter?  Then there would be
> little reason to check TIF_SECCOMP from secure_computing; instead, you
> could smp_load_acquire (or read_barrier_depends, maybe) seccomp.filter
> from secure_computing and pass the result as a parameter to
> __secure_computing.  Or you could even remove the distinction between
> secure_computing and __secure_computing -- it's essentially useless
> anyway to split entry hook approaches like my x86 fastpath prototype.

The TIF_SECCOMP is needed for the syscall entry path. The check in
secure_computing() is just because the "I am being traced" trigger
includes a call to secure_computing, which filters out tracing
reasons.

Your fast path work would clean a lot of that up, as you say. But it
still doesn't change the ordering check here. TIF_SECCOMP indicates
seccomp.mode must be checked, so that ordering will remain, and if
mode == FILTER, seccomp.filter must be valid.

Isn't there a way we can force the assignment ordering in seccomp_assign_mode()?

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Oleg Nesterov @ 2014-06-25 17:00 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CAGXu5jJkFxh4K=40xuh6tu3kUf4oJM8Dry+4upBdRieW3FNLgw@mail.gmail.com>

On 06/25, Kees Cook wrote:
>
> On Wed, Jun 25, 2014 at 6:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 06/24, Kees Cook wrote:
> >>
> >> +static inline void seccomp_assign_mode(struct task_struct *task,
> >> +                                    unsigned long seccomp_mode)
> >> +{
> >> +     BUG_ON(!spin_is_locked(&task->sighand->siglock));
> >> +
> >> +     task->seccomp.mode = seccomp_mode;
> >> +     set_tsk_thread_flag(task, TIF_SECCOMP);
> >> +}
> >
> > OK, but unless task == current this can race with secure_computing().
> > I think this needs smp_mb__before_atomic() and secure_computing() needs
> > rmb() after test_bit(TIF_SECCOMP).
> >
> > Otherwise, can't __secure_computing() hit BUG() if it sees the old
> > mode == SECCOMP_MODE_DISABLED ?
> >
> > Or seccomp_run_filters() can see ->filters == NULL and WARN(),
> > smp_load_acquire() only serializes that LOAD with the subsequent memory
> > operations.
>
> Hm, actually, now I'm worried about smp_load_acquire() being too slow
> in run_filters().
>
> The ordering must be:
> - task->seccomp.filter must be valid before
> - task->seccomp.mode is set, which must be valid before
> - TIF_SECCOMP is set
>
> But I don't want to impact secure_computing(). What's the best way to
> make sure this ordering is respected?

Cough, confused... can't understand even after I read the email from Andy.

We do not care if __secure_computing() misses the recently added filter,
this can happen anyway, whatever we do.

seccomp.mode is frozen after we set it != SECCOMP_MODE_DISABLED.

So we should only worry if set_tsk_thread_flag(TIF_SECCOMP) actually
changes this bit and makes __secure_computing() possible. If we add
smp_mb__before_atomic() into seccomp_assign_mode() and rmb() at the
start of __secure_computing() everything should be fine?

Oleg.

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Andy Lutomirski @ 2014-06-25 17:03 UTC (permalink / raw)
  To: Kees Cook
  Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API,
	x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
	linux-security-module
In-Reply-To: <CAGXu5j+J11zJnuFR8bYKAXizAHhCx4R+uJE_QH6zC3q2udkpaQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Wed, Jun 25, 2014 at 9:54 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, Jun 25, 2014 at 9:10 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Wed, Jun 25, 2014 at 7:51 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>> On Wed, Jun 25, 2014 at 6:51 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On 06/24, Kees Cook wrote:
>>>>>
>>>>> +static inline void seccomp_assign_mode(struct task_struct *task,
>>>>> +                                    unsigned long seccomp_mode)
>>>>> +{
>>>>> +     BUG_ON(!spin_is_locked(&task->sighand->siglock));
>>>>> +
>>>>> +     task->seccomp.mode = seccomp_mode;
>>>>> +     set_tsk_thread_flag(task, TIF_SECCOMP);
>>>>> +}
>>>>
>>>> OK, but unless task == current this can race with secure_computing().
>>>> I think this needs smp_mb__before_atomic() and secure_computing() needs
>>>> rmb() after test_bit(TIF_SECCOMP).
>>>>
>>>> Otherwise, can't __secure_computing() hit BUG() if it sees the old
>>>> mode == SECCOMP_MODE_DISABLED ?
>>>>
>>>> Or seccomp_run_filters() can see ->filters == NULL and WARN(),
>>>> smp_load_acquire() only serializes that LOAD with the subsequent memory
>>>> operations.
>>>
>>> Hm, actually, now I'm worried about smp_load_acquire() being too slow
>>> in run_filters().
>>>
>>> The ordering must be:
>>> - task->seccomp.filter must be valid before
>>> - task->seccomp.mode is set, which must be valid before
>>> - TIF_SECCOMP is set
>>>
>>> But I don't want to impact secure_computing(). What's the best way to
>>> make sure this ordering is respected?
>>
>> Remove the ordering requirement, perhaps?
>>
>> What if you moved mode into seccomp.filter?  Then there would be
>> little reason to check TIF_SECCOMP from secure_computing; instead, you
>> could smp_load_acquire (or read_barrier_depends, maybe) seccomp.filter
>> from secure_computing and pass the result as a parameter to
>> __secure_computing.  Or you could even remove the distinction between
>> secure_computing and __secure_computing -- it's essentially useless
>> anyway to split entry hook approaches like my x86 fastpath prototype.
>
> The TIF_SECCOMP is needed for the syscall entry path. The check in
> secure_computing() is just because the "I am being traced" trigger
> includes a call to secure_computing, which filters out tracing
> reasons.

Right.  I'm suggesting just checking a single indication that seccomp
is on from the process in the seccomp code so that the order doesn't
matter.

IOW, TIF_SECCOMP causes __secure_computing to be invoked, but the race
only seems to matter because of the warning and the BUG.  I think that
both can be fixed if you merge mode into filter so that
__secure_computing atomically checks one condition.

>
> Your fast path work would clean a lot of that up, as you say. But it
> still doesn't change the ordering check here. TIF_SECCOMP indicates
> seccomp.mode must be checked, so that ordering will remain, and if
> mode == FILTER, seccomp.filter must be valid.
>
> Isn't there a way we can force the assignment ordering in seccomp_assign_mode()?

Write the filter, then smp_mb (or maybe a weaker barrier is okay),
then set the bit.

--Andy

>
> -Kees
>
> --
> Kees Cook
> Chrome OS Security



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply

* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-06-25 17:09 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API,
	x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
	linux-security-module
In-Reply-To: <20140625165209.GA14720-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 06/25, Kees Cook wrote:
>>
>> On Wed, Jun 25, 2014 at 7:21 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> > But. Doesn't this change add a new security hole?
>> >
>> > Obviously, we should not allow to install a filter and then (say) exec
>> > a suid binary, that is why we have no_new_privs/LSM_UNSAFE_NO_NEW_PRIVS.
>> >
>> > But what if "thread->seccomp.filter = caller->seccomp.filter" races with
>> > any user of task_no_new_privs() ? Say, suppose this thread has already
>> > passed check_unsafe_exec/etc and it is going to exec the suid binary?
>>
>> Oh, ew. Yeah. It looks like there's a cred lock to be held to combat this?
>
> Yes, cred_guard_mutex looks like an obvious choice... Hmm, but somehow
> initially I thought that the fix won't be simple. Not sure why.
>
> Yes, at least this should close the race with suid-exec. And there are no
> other users. Except apparmor, and I hope you will check it because I simply
> do not know what it does ;)
>
>> I wonder if changes to nnp need to "flushed" during syscall entry
>> instead of getting updated externally/asynchronously? That way it
>> won't be out of sync with the seccomp mode/filters.
>>
>> Perhaps secure computing needs to check some (maybe seccomp-only)
>> atomic flags and flip on the "real" nnp if found?
>
> Not sure I understand you, could you clarify?

Instead of having TSYNC change the nnp bit, it can set a new flag, say:

    task->seccomp.flags |= SECCOMP_NEEDS_NNP;

This would be set along with seccomp.mode, seccomp.filter, and
TIF_SECCOMP. Then, during the next secure_computing() call that thread
makes, it would check the flag:

    if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
        task->nnp = 1;

This means that nnp couldn't change in the middle of a running syscall.

Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
above would actually make things worse, since now we'd have a thread
with seccomp set up, and no nnp. If it was in the middle of exec,
we're still causing a problem.

I think we'd also need a way to either delay the seccomp changes, or
to notice this condition during exec. Bleh.

What actually happens with a multi-threaded process calls exec? I
assume all the other threads are destroyed?

> But I was also worried that task_no_new_privs(current) is no longer stable
> inside the syscall paths, perhaps this is what you meant? However I do not
> see something bad here... And this has nothing to do with the race above.
>
> Also. Even ignoring no_new_privs, SECCOMP_FILTER_FLAG_TSYNC is not atomic
> and we can do nothing with this fact (unless it try to freeze the thread
> group somehow), perhaps it makes sense to document this somehow.
>
> I mean, suppose you want to ensure write-to-file is not possible, so you
> do seccomp(SECCOMP_FILTER_FLAG_TSYNC, nack_write_to_file_filter). You can't
> assume that this has effect right after seccomp() returns, this can obviously
> race with a sub-thread which has already entered sys_write().
>
> Once again, I am not arguing, just I think it makes sense to at least mention
> the limitations during the discussion.

Right -- this is an accepted limitation. I will call it out
specifically in the man-page; that's a good idea.

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply

* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-06-25 17:24 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
	Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
	Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CAGXu5jLHDew1fifGY_mWgwcH7evm0T8rqSnBrw4XpoAXGK+t-Q@mail.gmail.com>

On 06/25, Kees Cook wrote:
>
> On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Yes, at least this should close the race with suid-exec. And there are no
> > other users. Except apparmor, and I hope you will check it because I simply
> > do not know what it does ;)
> >
> >> I wonder if changes to nnp need to "flushed" during syscall entry
> >> instead of getting updated externally/asynchronously? That way it
> >> won't be out of sync with the seccomp mode/filters.
> >>
> >> Perhaps secure computing needs to check some (maybe seccomp-only)
> >> atomic flags and flip on the "real" nnp if found?
> >
> > Not sure I understand you, could you clarify?
>
> Instead of having TSYNC change the nnp bit, it can set a new flag, say:
>
>     task->seccomp.flags |= SECCOMP_NEEDS_NNP;
>
> This would be set along with seccomp.mode, seccomp.filter, and
> TIF_SECCOMP. Then, during the next secure_computing() call that thread
> makes, it would check the flag:
>
>     if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
>         task->nnp = 1;
>
> This means that nnp couldn't change in the middle of a running syscall.

Aha, so you were worried about the same thing. Not sure we need this,
but at least I understand you and...

> Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
> above would actually make things worse, since now we'd have a thread
> with seccomp set up, and no nnp. If it was in the middle of exec,
> we're still causing a problem.

Yes ;)

> I think we'd also need a way to either delay the seccomp changes, or
> to notice this condition during exec. Bleh.

Hmm. confused again,

> What actually happens with a multi-threaded process calls exec? I
> assume all the other threads are destroyed?

Yes. But this is the point-of-no-return, de_thread() is called after the execing
thared has already passed (say) check_unsafe_exec().

However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
and drops it in install_exec_creds(), so it should solve the problem?

Oleg.

^ permalink raw reply

* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Oleg Nesterov @ 2014-06-25 17:32 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Kees Cook, LKML, Michael Kerrisk (man-pages), Alexei Starovoitov,
	Andrew Morton, Daniel Borkmann, Will Drewry, Julien Tinnes,
	David Drysdale, Linux API, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
	linux-security-module
In-Reply-To: <CALCETrVrs8sb19+UUqyFEpAFzTih5dkAwn-WpQjfgPcPJMpP5g@mail.gmail.com>

On 06/25, Andy Lutomirski wrote:
>
> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
> then set the bit.

Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().

But I still can't understand the rest of your discussion about the
ordering we need ;)

Oleg.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox