* [PATCH v3 1/3] namei: implement O_BENEATH-style AT_* flags
From: Aleksa Sarai @ 2018-10-09 7:02 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Christian Brauner, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* AT_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* AT_NO_SYMLINK: Disallows symlink jumping *of any kind*. Implies
AT_NO_PROCLINK (obviously).
* AT_BENEATH: Disallow "escapes" from the starting point of the
filesystem tree during resolution (you must stay "beneath" the
starting point at all times). Currently this is done by disallowing
".." and absolute paths (either in the given path or found during
symlink resolution) entirely, as well as all "proclink" jumping.
The wholesale banning of ".." is because it is currently not safe to
allow ".." resolution (races can cause the path to be moved outside of
the root -- this is conceptually similar to historical chroot(2)
escape attacks). Future patches in this series will address this, and
will re-enable ".." resolution once it is safe. With those patches,
".." resolution will only be allowed if it remains in the root
throughout resolution (such as "a/../b" not "a/../../outside/b").
The banning of "proclink" jumping is done because it is not clear
whether semantically they should be allowed -- while some "proclinks"
are safe there are many that can cause escapes (and once a resolution
is outside of the root, AT_BENEATH will no longer detect it). Future
patches may re-enable "proclink" jumping when such jumps would remain
inside the root.
The AT_NO_*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2) (which has its own brand of O_* flags
with the same semantics). However the AT_* flags have been reserved for
future support in other *at(2) syscalls (though because of AT_EMPTY_PATH
many *at(2) operations will not need to support these flags directly).
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 174 ++++++++++++++++++++++---------
fs/open.c | 8 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 ++
include/uapi/asm-generic/fcntl.h | 17 +++
include/uapi/linux/fcntl.h | 8 ++
8 files changed, 167 insertions(+), 56 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..e343618736f7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index fb913148d4d1..76eacd3af89b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1083,14 +1089,23 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ /* If we just jumped it was because of a procfs-style link. */
+ if (unlikely(nd->flags & LOOKUP_JUMPED)) {
+ if (unlikely(nd->flags & LOOKUP_NO_PROCLINKS))
+ return ERR_PTR(-ELOOP);
+ /* Not currently safe. */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return ERR_PTR(-EXDEV);
+ }
if (IS_ERR_OR_NULL(res))
return res;
}
if (*res == '/') {
if (!nd->root.mnt)
set_root(nd);
- if (unlikely(nd_jump_root(nd)))
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
while (unlikely(*++res == '/'))
;
}
@@ -1271,12 +1286,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1333,6 +1352,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1374,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *inode = nd->inode;
while (1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1379,6 +1403,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1393,6 +1419,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1481,8 +1509,11 @@ static int path_parent_directory(struct path *path)
static int follow_dotdot(struct nameidata *nd)
{
while(1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -1491,6 +1522,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1705,6 +1738,12 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ /*
+ * AT_BENEATH resolving ".." is not currently safe -- races can cause
+ * our parent to have moved outside of the root and us to skip over it.
+ */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
if (nd->flags & LOOKUP_RCU) {
@@ -1720,6 +1759,8 @@ static int pick_link(struct nameidata *nd, struct path *link,
{
int error;
struct saved *last;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return -ELOOP;
if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
path_to_nameidata(link, nd);
return -ELOOP;
@@ -2168,13 +2209,70 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ do {
+ seq = read_seqcount_begin(&fs->seq);
+ nd->path = fs->pwd;
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
+ } while (read_seqcount_retry(&fs->seq, seq));
+ } else {
+ get_fs_pwd(current->fs, &nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ } else {
+ /* Caller must check execute permissions on the starting path component */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *dentry;
+
+ if (!f.file)
+ return -EBADF;
+
+ dentry = f.file->f_path.dentry;
+
+ if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
+ fdput(f);
+ return -ENOTDIR;
+ }
+
+ nd->path = f.file->f_path;
+ if (nd->flags & LOOKUP_RCU) {
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+ } else {
+ path_get(&nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ fdput(f);
+ }
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ nd->root = nd->path;
+ if (!(nd->flags & LOOKUP_RCU))
+ path_get(&nd->root);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
flags &= ~LOOKUP_RCU;
+ if (flags & LOOKUP_NO_SYMLINKS)
+ flags |= LOOKUP_NO_PROCLINKS;
if (flags & LOOKUP_RCU)
rcu_read_lock();
@@ -2203,53 +2301,25 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+ if (unlikely(flags & LOOKUP_XDEV)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- do {
- seq = read_seqcount_begin(&fs->seq);
- nd->path = fs->pwd;
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
- } while (read_seqcount_retry(&fs->seq, seq));
- } else {
- get_fs_pwd(current->fs, &nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_PTR(-ENOTDIR);
- }
-
- nd->path = f.file->f_path;
- if (flags & LOOKUP_RCU) {
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
- } else {
- path_get(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- fdput(f);
+ if (likely(!nd->root.mnt))
+ set_root(nd);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..80f5f566a5ff 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -988,6 +988,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOPROCLINKS)
+ lookup_flags |= LOOKUP_NO_PROCLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/fs/stat.c b/fs/stat.c
index f8e6fb2c3657..d319a468c704 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -170,8 +170,8 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
int error = -EINVAL;
unsigned int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT;
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
- AT_EMPTY_PATH | KSTAT_QUERY_FLAGS)) != 0)
+ if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
+ KSTAT_QUERY_FLAGS))
return -EINVAL;
if (flags & AT_SYMLINK_NOFOLLOW)
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..ad5bba4b5b12 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOPROCLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..5ff7f3362d1b 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_PROCLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_PROCLINKS. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..c2bf5983e46a 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,23 @@
#define O_NDELAY O_NONBLOCK
#endif
+/*
+ * These are identical to their AT_* counterparts (which affect the entireity
+ * of path resolution).
+ */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000 /* *Not* the same as capsicum's O_BENEATH! */
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000
+#endif
+#ifndef O_NOPROCLINKS
+#define O_NOPROCLINKS 00200000000
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 594b85f7cb86..551a9e2166a8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -92,5 +92,13 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+/* Flags which affect path *resolution*, not just last-component handling. */
+#define AT_BENEATH 0x10000 /* No absolute paths or ".." escaping
+ (in-path or through symlinks) */
+#define AT_XDEV 0x20000 /* No mountpoint crossing. */
+#define AT_NO_PROCLINKS 0x40000 /* No /proc/$pid/fd/... "symlinks". */
+#define AT_NO_SYMLINKS 0x80000 /* No symlinks *at all*.
+ Implies AT_NO_PROCLINKS. */
+
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.19.0
^ permalink raw reply related
* [PATCH v3 0/3] namei: implement various lookup restriction AT_* flags
From: Aleksa Sarai @ 2018-10-09 7:02 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Andy Lutomirski, David Howells, Jann Horn,
Christian Brauner, David Drysdale, containers, linux-fsdevel,
linux-api, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
Tycho Andersen, dev, linux-kernel, linux-arch
The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
As per the discussion in the AT_NO_JUMPS thread, AT_NO_JUMPS has been
split into separate flags.
* AT_XDEV blocks mountpoint crossings (both upwards and downwards).
openat("/", "tmp", AT_XDEV); // blocked
openat("/tmp", "..", AT_XDEV); // blocked
openat("/tmp", "/", AT_XDEV); // blocked
* AT_NO_PROCLINKS blocks all resolution through /proc/$pid/fd/$fd
"symlinks". Specifically, this blocks all jumps caused by a
filesystem using nd_jump_link() to shove you around in the
filesystem tree (these are referred to as "proclinks" in lieu of a
better name).
openat(AT_FDCWD, "/proc/self/root", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/fd/0", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/ns/mnt", AT_NO_PROCLINKS); // blocked
* AT_BENEATH disallows escapes from the starting dirfd using ".." or
absolute paths (either in the path or during symlink resolution).
Conceptually this flag ensures that you "stay below" the starting
point in the filesystem tree. ".." resolution is allowed if it
doesn't land you outside of the starting point (this is made safe
against races by patch 3 in this series).
openat("/root", "foo", AT_BENEATH); // *not* blocked
openat("/root", "a/../b", AT_BENEATH); // *not* blocked
openat("/root", "a/../../root/b", AT_BENEATH); // blocked
openat("/root", "/root", AT_BENEATH); // blocked
AT_BENEATH also currently disallows all "proclink" resolution
because they can trivially throw you outside of the starting point.
In a future patch we might allow such resolution (as long as it
stays within the root).
openat("/", "proc/self/exe", AT_BENEATH); // blocked
In addition, two more flags have been added to the series:
* AT_NO_SYMLINKS disallows *all* symlink resolution, and thus implies
AT_NO_PROCLINKS. Linus mentioned this is something that git would
like to have in the original discussion[5].
// assuming 'ln -s / /usr'
openat("/", "/usr/bin", AT_NO_SYMLINKS); // blocked
openat("/", "/proc/self/root", AT_NO_PROCLINKS); // blocked
* AT_THIS_ROOT is a very similar idea to AT_BENEATH, but it serves a
very different purpose. Rather than blocking resolutions if they
would go outside of the starting point, it treats the starting point
as a form of chroot(2). Container runtimes are one of the primary
justifications for this flag, as they currently have to implement
this sort of path handling racily in userspace[6].
The restrictions on "proclink" resolution are the same as with
AT_BENEATH (though in AT_THIS_ROOT's case it's not really clear how
"proclink" jumps outside of the root should be handled), and patch 3
in this series was also required to make ".." resolution safe.
Currently all of these flags are only enabled for openat(2) (and thus
have their own O_* flag names), but the corresponding AT_* flags have
been reserved so they can be added to syscalls where openat(O_PATH) is
not sufficient.
Patch changelog:
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <drysdale@google.com>
Cc: <containers@lists.linux-foundation.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-api@vger.kernel.org>
Aleksa Sarai (3):
namei: implement O_BENEATH-style AT_* flags
namei: implement AT_THIS_ROOT chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
fs/fcntl.c | 2 +-
fs/namei.c | 241 +++++++++++++++++++++++--------
fs/open.c | 10 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 8 +
include/uapi/asm-generic/fcntl.h | 20 +++
include/uapi/linux/fcntl.h | 10 ++
8 files changed, 230 insertions(+), 68 deletions(-)
--
2.19.0
^ permalink raw reply
* [PATCH v2 1/3] namei: implement O_BENEATH-style AT_* flags
From: Aleksa Sarai @ 2018-10-09 6:52 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Christian Brauner, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
In-Reply-To: <20181009065300.11053-1-cyphar@cyphar.com>
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* AT_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* AT_NO_SYMLINK: Disallows symlink jumping *of any kind*. Implies
AT_NO_PROCLINK (obviously).
* AT_BENEATH: Disallow "escapes" from the starting point of the
filesystem tree during resolution (you must stay "beneath" the
starting point at all times). Currently this is done by disallowing
".." and absolute paths (either in the given path or found during
symlink resolution) entirely, as well as all "proclink" jumping.
The wholesale banning of ".." is because it is currently not safe to
allow ".." resolution (races can cause the path to be moved outside of
the root -- this is conceptually similar to historical chroot(2)
escape attacks). Future patches in this series will address this, and
will re-enable ".." resolution once it is safe. With those patches,
".." resolution will only be allowed if it remains in the root
throughout resolution (such as "a/../b" not "a/../../outside/b").
The banning of "proclink" jumping is done because it is not clear
whether semantically they should be allowed -- while some "proclinks"
are safe there are many that can cause escapes (and once a resolution
is outside of the root, AT_BENEATH will no longer detect it). Future
patches may re-enable "proclink" jumping when such jumps would remain
inside the root.
The AT_NO_*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2) (which has its own brand of O_* flags
with the same semantics). However the AT_* flags have been reserved for
future support in other *at(2) syscalls (though because of AT_EMPTY_PATH
many *at(2) operations will not need to support these flags directly).
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 174 ++++++++++++++++++++++---------
fs/open.c | 8 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 ++
include/uapi/asm-generic/fcntl.h | 17 +++
include/uapi/linux/fcntl.h | 8 ++
8 files changed, 167 insertions(+), 56 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..e343618736f7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index fb913148d4d1..76eacd3af89b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1083,14 +1089,23 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ /* If we just jumped it was because of a procfs-style link. */
+ if (unlikely(nd->flags & LOOKUP_JUMPED)) {
+ if (unlikely(nd->flags & LOOKUP_NO_PROCLINKS))
+ return ERR_PTR(-ELOOP);
+ /* Not currently safe. */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return ERR_PTR(-EXDEV);
+ }
if (IS_ERR_OR_NULL(res))
return res;
}
if (*res == '/') {
if (!nd->root.mnt)
set_root(nd);
- if (unlikely(nd_jump_root(nd)))
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
while (unlikely(*++res == '/'))
;
}
@@ -1271,12 +1286,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1333,6 +1352,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1374,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *inode = nd->inode;
while (1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1379,6 +1403,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1393,6 +1419,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1481,8 +1509,11 @@ static int path_parent_directory(struct path *path)
static int follow_dotdot(struct nameidata *nd)
{
while(1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -1491,6 +1522,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1705,6 +1738,12 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ /*
+ * AT_BENEATH resolving ".." is not currently safe -- races can cause
+ * our parent to have moved outside of the root and us to skip over it.
+ */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
if (nd->flags & LOOKUP_RCU) {
@@ -1720,6 +1759,8 @@ static int pick_link(struct nameidata *nd, struct path *link,
{
int error;
struct saved *last;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return -ELOOP;
if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
path_to_nameidata(link, nd);
return -ELOOP;
@@ -2168,13 +2209,70 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ do {
+ seq = read_seqcount_begin(&fs->seq);
+ nd->path = fs->pwd;
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
+ } while (read_seqcount_retry(&fs->seq, seq));
+ } else {
+ get_fs_pwd(current->fs, &nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ } else {
+ /* Caller must check execute permissions on the starting path component */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *dentry;
+
+ if (!f.file)
+ return -EBADF;
+
+ dentry = f.file->f_path.dentry;
+
+ if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
+ fdput(f);
+ return -ENOTDIR;
+ }
+
+ nd->path = f.file->f_path;
+ if (nd->flags & LOOKUP_RCU) {
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+ } else {
+ path_get(&nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ fdput(f);
+ }
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ nd->root = nd->path;
+ if (!(nd->flags & LOOKUP_RCU))
+ path_get(&nd->root);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
flags &= ~LOOKUP_RCU;
+ if (flags & LOOKUP_NO_SYMLINKS)
+ flags |= LOOKUP_NO_PROCLINKS;
if (flags & LOOKUP_RCU)
rcu_read_lock();
@@ -2203,53 +2301,25 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+ if (unlikely(flags & LOOKUP_XDEV)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- do {
- seq = read_seqcount_begin(&fs->seq);
- nd->path = fs->pwd;
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
- } while (read_seqcount_retry(&fs->seq, seq));
- } else {
- get_fs_pwd(current->fs, &nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_PTR(-ENOTDIR);
- }
-
- nd->path = f.file->f_path;
- if (flags & LOOKUP_RCU) {
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
- } else {
- path_get(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- fdput(f);
+ if (likely(!nd->root.mnt))
+ set_root(nd);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..80f5f566a5ff 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -988,6 +988,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOPROCLINKS)
+ lookup_flags |= LOOKUP_NO_PROCLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/fs/stat.c b/fs/stat.c
index f8e6fb2c3657..d319a468c704 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -170,8 +170,8 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
int error = -EINVAL;
unsigned int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT;
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
- AT_EMPTY_PATH | KSTAT_QUERY_FLAGS)) != 0)
+ if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
+ KSTAT_QUERY_FLAGS))
return -EINVAL;
if (flags & AT_SYMLINK_NOFOLLOW)
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..ad5bba4b5b12 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOPROCLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..5ff7f3362d1b 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_PROCLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_PROCLINKS. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..c2bf5983e46a 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,23 @@
#define O_NDELAY O_NONBLOCK
#endif
+/*
+ * These are identical to their AT_* counterparts (which affect the entireity
+ * of path resolution).
+ */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000 /* *Not* the same as capsicum's O_BENEATH! */
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000
+#endif
+#ifndef O_NOPROCLINKS
+#define O_NOPROCLINKS 00200000000
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 594b85f7cb86..551a9e2166a8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -92,5 +92,13 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+/* Flags which affect path *resolution*, not just last-component handling. */
+#define AT_BENEATH 0x10000 /* No absolute paths or ".." escaping
+ (in-path or through symlinks) */
+#define AT_XDEV 0x20000 /* No mountpoint crossing. */
+#define AT_NO_PROCLINKS 0x40000 /* No /proc/$pid/fd/... "symlinks". */
+#define AT_NO_SYMLINKS 0x80000 /* No symlinks *at all*.
+ Implies AT_NO_PROCLINKS. */
+
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.19.0
^ permalink raw reply related
* [PATCH v2 0/3] namei: implement various lookup restriction AT_* flags
From: Aleksa Sarai @ 2018-10-09 6:52 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Andy Lutomirski, David Howells, Jann Horn,
Christian Brauner, David Drysdale, containers, linux-fsdevel,
linux-api, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
Tycho Andersen, dev, linux-kernel, linux-arch
In-Reply-To: <20181009065300.11053-1-cyphar@cyphar.com>
The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
As per the discussion in the AT_NO_JUMPS thread, AT_NO_JUMPS has been
split into separate flags.
* AT_XDEV blocks mountpoint crossings (both upwards and downwards).
openat("/", "tmp", AT_XDEV); // blocked
openat("/tmp", "..", AT_XDEV); // blocked
openat("/tmp", "/", AT_XDEV); // blocked
* AT_NO_PROCLINKS blocks all resolution through /proc/$pid/fd/$fd
"symlinks". Specifically, this blocks all jumps caused by a
filesystem using nd_jump_link() to shove you around in the
filesystem tree (these are referred to as "proclinks" in lieu of a
better name).
openat(AT_FDCWD, "/proc/self/root", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/fd/0", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/ns/mnt", AT_NO_PROCLINKS); // blocked
* AT_BENEATH disallows escapes from the starting dirfd using ".." or
absolute paths (either in the path or during symlink resolution).
Conceptually this flag ensures that you "stay below" the starting
point in the filesystem tree. ".." resolution is allowed if it
doesn't land you outside of the starting point (this is made safe
against races by patch 3 in this series).
openat("/root", "foo", AT_BENEATH); // *not* blocked
openat("/root", "a/../b", AT_BENEATH); // *not* blocked
openat("/root", "a/../../root/b", AT_BENEATH); // blocked
openat("/root", "/root", AT_BENEATH); // blocked
AT_BENEATH also currently disallows all "proclink" resolution
because they can trivially throw you outside of the starting point.
In a future patch we might allow such resolution (as long as it
stays within the root).
openat("/", "proc/self/exe", AT_BENEATH); // blocked
In addition, two more flags have been added to the series:
* AT_NO_SYMLINKS disallows *all* symlink resolution, and thus implies
AT_NO_PROCLINKS. Linus mentioned this is something that git would
like to have in the original discussion[5].
// assuming 'ln -s / /usr'
openat("/", "/usr/bin", AT_NO_SYMLINKS); // blocked
openat("/", "/proc/self/root", AT_NO_PROCLINKS); // blocked
* AT_THIS_ROOT is a very similar idea to AT_BENEATH, but it serves a
very different purpose. Rather than blocking resolutions if they
would go outside of the starting point, it treats the starting point
as a form of chroot(2). Container runtimes are one of the primary
justifications for this flag, as they currently have to implement
this sort of path handling racily in userspace[6].
The restrictions on "proclink" resolution are the same as with
AT_BENEATH (though in AT_THIS_ROOT's case it's not really clear how
"proclink" jumps outside of the root should be handled), and patch 3
in this series was also required to make ".." resolution safe.
Currently all of these flags are only enabled for openat(2) (and thus
have their own O_* flag names), but the corresponding AT_* flags have
been reserved so they can be added to syscalls where openat(O_PATH) is
not sufficient.
Patch changelog:
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <drysdale@google.com>
Cc: <containers@lists.linux-foundation.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-api@vger.kernel.org>
Aleksa Sarai (3):
namei: implement O_BENEATH-style AT_* flags
namei: implement AT_THIS_ROOT chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
fs/fcntl.c | 2 +-
fs/namei.c | 241 +++++++++++++++++++++++--------
fs/open.c | 10 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 8 +
include/uapi/asm-generic/fcntl.h | 20 +++
include/uapi/linux/fcntl.h | 10 ++
8 files changed, 230 insertions(+), 68 deletions(-)
--
2.19.0
^ permalink raw reply
* [PATCH v2 0/3] namei: implement various lookup restriction AT_* flags
From: Aleksa Sarai @ 2018-10-09 6:52 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
Andy Lutomirski, David Howells, Jann Horn, Christian Brauner,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
capsicum patchset[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
As per the discussion in the AT_NO_JUMPS thread, AT_NO_JUMPS has been
split into separate flags.
* AT_XDEV blocks mountpoint crossings (both upwards and downwards).
openat("/", "tmp", AT_XDEV); // blocked
openat("/tmp", "..", AT_XDEV); // blocked
openat("/tmp", "/", AT_XDEV); // blocked
* AT_NO_PROCLINKS blocks all resolution through /proc/$pid/fd/$fd
"symlinks". Specifically, this blocks all jumps caused by a
filesystem using nd_jump_link() to shove you around in the
filesystem tree (these are referred to as "proclinks" in lieu of a
better name).
* AT_BENEATH disallows escapes from the starting dirfd using ".." or
absolute paths (either in the path or during symlink resolution).
Conceptually this flag ensures that you "stay below" the starting
point in the filesystem tree. ".." resolution is allowed if it
doesn't land you outside of the starting point (this is made safe
against races by patch 3 in this series).
AT_BENEATH also currently disallows all "proclink" resolution
because they can trivially throw you outside of the starting point.
In a future patch we might allow such resolution (as long as it
stays within the root).
In addition, two more flags have been added to the series:
* AT_NO_SYMLINKS disallows *all* symlink resolution, and thus implies
AT_NO_PROCLINKS. Linus mentioned this is something that git would
like to have in the original discussion[5].
* AT_THIS_ROOT is a very similar idea to AT_BENEATH, but it serves a
very different purpose. Rather than blocking resolutions if they
would go outside of the starting point, it treats the starting point
as a form of chroot(2). Container runtimes are one of the primary
justifications for this flag, as they currently have to implement
this sort of path handling racily in userspace[6].
The restrictions on "proclink" resolution are the same as with
AT_BENEATH (though in AT_THIS_ROOT's case it's not really clear how
"proclink" jumps outside of the root should be handled), and patch 3
in this series was also required to make ".." resolution safe.
Patch changelog:
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe by
through __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Aleksa Sarai (3):
namei: implement O_BENEATH-style AT_* flags
namei: implement AT_THIS_ROOT chroot-like path resolution
namei: aggressively check nd->root on ".." resolution
fs/fcntl.c | 2 +-
fs/namei.c | 192 ++++++++++++++++++++++---------
fs/open.c | 10 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 8 ++
include/uapi/asm-generic/fcntl.h | 20 ++++
include/uapi/linux/fcntl.h | 10 ++
8 files changed, 193 insertions(+), 56 deletions(-)
--
2.19.0
^ permalink raw reply
* Re: [PATCH v2 3/3] tools: add selftest for BPF_F_ZERO_SEED
From: Song Liu @ 2018-10-08 23:15 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking, linux-api
In-Reply-To: <20181008103221.13468-4-lmb@cloudflare.com>
On Mon, Oct 8, 2018 at 3:34 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Check that iterating two separate hash maps produces the same
> order of keys if BPF_F_ZERO_SEED is used.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
> 1 file changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> index 9b552c0fc47d..a8d6af27803a 100644
> --- a/tools/testing/selftests/bpf/test_maps.c
> +++ b/tools/testing/selftests/bpf/test_maps.c
> @@ -257,23 +257,35 @@ static void test_hashmap_percpu(int task, void *data)
> close(fd);
> }
>
> +static int helper_fill_hashmap(int max_entries)
How about we add map_flags as the second argument of this function?
This will help
avoid the old_flags hack.
Thanks,
Song
> +{
> + int i, fd, ret;
> + long long key, value;
> +
> + fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
> + max_entries, map_flags);
> + CHECK(fd < 0,
> + "failed to create hashmap",
> + "err: %s, flags: 0x%x\n", strerror(errno), map_flags);
> +
> + for (i = 0; i < max_entries; i++) {
> + key = i; value = key;
> + ret = bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST);
> + CHECK(ret != 0,
> + "can't update hashmap",
> + "err: %s\n", strerror(ret));
> + }
> +
> + return fd;
> +}
> +
> static void test_hashmap_walk(int task, void *data)
> {
> int fd, i, max_entries = 1000;
> long long key, value, next_key;
> bool next_key_valid = true;
>
> - fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
> - max_entries, map_flags);
> - if (fd < 0) {
> - printf("Failed to create hashmap '%s'!\n", strerror(errno));
> - exit(1);
> - }
> -
> - for (i = 0; i < max_entries; i++) {
> - key = i; value = key;
> - assert(bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST) == 0);
> - }
> + fd = helper_fill_hashmap(max_entries);
>
> for (i = 0; bpf_map_get_next_key(fd, !i ? NULL : &key,
> &next_key) == 0; i++) {
> @@ -305,6 +317,39 @@ static void test_hashmap_walk(int task, void *data)
> close(fd);
> }
>
> +static void test_hashmap_zero_seed(void)
> +{
> + int i, first, second, old_flags;
> + long long key, next_first, next_second;
> +
> + old_flags = map_flags;
> + map_flags |= BPF_F_ZERO_SEED;
> +
> + first = helper_fill_hashmap(3);
> + second = helper_fill_hashmap(3);
> +
> + for (i = 0; ; i++) {
> + void *key_ptr = !i ? NULL : &key;
> +
> + if (bpf_map_get_next_key(first, key_ptr, &next_first) != 0)
> + break;
> +
> + CHECK(bpf_map_get_next_key(second, key_ptr, &next_second) != 0,
> + "next_key for second map must succeed",
> + "key_ptr: %p", key_ptr);
> + CHECK(next_first != next_second,
> + "keys must match",
> + "i: %d first: %lld second: %lld\n", i,
> + next_first, next_second);
> +
> + key = next_first;
> + }
> +
> + map_flags = old_flags;
> + close(first);
> + close(second);
> +}
> +
> static void test_arraymap(int task, void *data)
> {
> int key, next_key, fd;
> @@ -1417,6 +1462,7 @@ static void run_all_tests(void)
> test_hashmap(0, NULL);
> test_hashmap_percpu(0, NULL);
> test_hashmap_walk(0, NULL);
> + test_hashmap_zero_seed();
>
> test_arraymap(0, NULL);
> test_arraymap_percpu(0, NULL);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v2 2/3] tools: sync linux/bpf.h
From: Song Liu @ 2018-10-08 23:12 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking, linux-api
In-Reply-To: <20181008103221.13468-3-lmb@cloudflare.com>
On Mon, Oct 8, 2018 at 3:34 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Synchronize changes to linux/bpf.h from
> commit 88db241b34bf ("bpf: allow zero-initializing hash map seed").
I guess we cannot keep this hash during git-am? We probably don't
need this hash anyway, as the two patches will be applied back to back.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> tools/include/uapi/linux/bpf.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index f9187b41dff6..2c121f862082 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -253,6 +253,8 @@ enum bpf_attach_type {
> #define BPF_F_NO_COMMON_LRU (1U << 1)
> /* Specify numa node during map creation */
> #define BPF_F_NUMA_NODE (1U << 2)
> +/* Zero-initialize hash function seed. This should only be used for testing. */
> +#define BPF_F_ZERO_SEED (1U << 6)
Same as 01.
>
> /* flags for BPF_PROG_QUERY */
> #define BPF_F_QUERY_EFFECTIVE (1U << 0)
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v2 1/3] bpf: allow zero-initializing hash map seed
From: Song Liu @ 2018-10-08 23:07 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking, linux-api
In-Reply-To: <20181008103221.13468-2-lmb@cloudflare.com>
On Mon, Oct 8, 2018 at 3:34 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Add a new flag BPF_F_ZERO_SEED, which forces a hash map
> to initialize the seed to zero. This is useful when doing
> performance analysis both on individual BPF programs, as
> well as the kernel's hash table implementation.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> include/uapi/linux/bpf.h | 2 ++
> kernel/bpf/hashtab.c | 13 +++++++++++--
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f9187b41dff6..2c121f862082 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -253,6 +253,8 @@ enum bpf_attach_type {
> #define BPF_F_NO_COMMON_LRU (1U << 1)
> /* Specify numa node during map creation */
> #define BPF_F_NUMA_NODE (1U << 2)
> +/* Zero-initialize hash function seed. This should only be used for testing. */
> +#define BPF_F_ZERO_SEED (1U << 6)
Please add this line after
#define BPF_F_STACK_BUILD_ID (1U << 5)
Other than this
Acked-by: Song Liu <songliubraving@fb.com>
>
> /* flags for BPF_PROG_QUERY */
> #define BPF_F_QUERY_EFFECTIVE (1U << 0)
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 2c1790288138..4b7c76765d9d 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -23,7 +23,7 @@
>
> #define HTAB_CREATE_FLAG_MASK \
> (BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
> - BPF_F_RDONLY | BPF_F_WRONLY)
> + BPF_F_RDONLY | BPF_F_WRONLY | BPF_F_ZERO_SEED)
>
> struct bucket {
> struct hlist_nulls_head head;
> @@ -244,6 +244,7 @@ static int htab_map_alloc_check(union bpf_attr *attr)
> */
> bool percpu_lru = (attr->map_flags & BPF_F_NO_COMMON_LRU);
> bool prealloc = !(attr->map_flags & BPF_F_NO_PREALLOC);
> + bool zero_seed = (attr->map_flags & BPF_F_ZERO_SEED);
> int numa_node = bpf_map_attr_numa_node(attr);
>
> BUILD_BUG_ON(offsetof(struct htab_elem, htab) !=
> @@ -257,6 +258,10 @@ static int htab_map_alloc_check(union bpf_attr *attr)
> */
> return -EPERM;
>
> + if (zero_seed && !capable(CAP_SYS_ADMIN))
> + /* Guard against local DoS, and discourage production use. */
> + return -EPERM;
> +
> if (attr->map_flags & ~HTAB_CREATE_FLAG_MASK)
> /* reserved bits should not be used */
> return -EINVAL;
> @@ -373,7 +378,11 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
> if (!htab->buckets)
> goto free_htab;
>
> - htab->hashrnd = get_random_int();
> + if (htab->map.map_flags & BPF_F_ZERO_SEED)
> + htab->hashrnd = 0;
> + else
> + htab->hashrnd = get_random_int();
> +
> for (i = 0; i < htab->n_buckets; i++) {
> INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
> raw_spin_lock_init(&htab->buckets[i].lock);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-08 19:05 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, avagin, linux-fsdevel, Eric W. Biederman, Linux API,
dima, containers, Al Viro, James Bottomley
In-Reply-To: <CAG48ez1S7CVDCCec5F-N32BVEPckLb0Qy+PypThezwKA=8HSSg@mail.gmail.com>
Le 08/10/2018 à 13:26, Jann Horn a écrit :
> On Sat, Oct 6, 2018 at 9:36 PM Laurent Vivier <laurent@vivier.eu> wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of an another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
> [...]
...
>> @@ -838,7 +858,29 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
>> static struct dentry *bm_mount(struct file_system_type *fs_type,
>> int flags, const char *dev_name, void *data)
>> {
>> - return mount_single(fs_type, flags, data, bm_fill_super);
>> + struct user_namespace *ns = current_user_ns();
>> +
>> + /* create a new binfmt namespace
>> + * if we are not in the first user namespace
>> + * but the binfmt namespace is the first one
>> + */
>> + if (ns->binfmt_ns == NULL) {
>> + struct binfmt_namespace *new_ns;
>> +
>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>> + GFP_KERNEL);
>> + if (new_ns == NULL)
>> + return ERR_PTR(-ENOMEM);
>> + INIT_LIST_HEAD(&new_ns->entries);
>> + new_ns->enabled = 1;
>> + rwlock_init(&new_ns->entries_lock);
>> + new_ns->bm_mnt = NULL;
>> + new_ns->entry_count = 0;
>> + ns->binfmt_ns = new_ns;
>
> What happens if someone mounts two instances of the binfmt_misc
> filesystem at the same time? Would you end up creating two binfmt
> namespaces, one of which would never be freed again?
I think you're right. And there is also a problem if mount_ns() fails.
So I think I can put this sequence in bm_fill_super() to avoid these
problems.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-08 18:41 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, linux-api, containers, Akihiro Suda,
Oleg Nesterov, linux-kernel, Eric W . Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <20181008180043.GE28238@cisco.lan>
On Mon, Oct 08, 2018 at 12:00:43PM -0600, Tycho Andersen wrote:
> On Mon, Oct 08, 2018 at 05:16:30PM +0200, Christian Brauner wrote:
> > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
> >
> > So for the slow of mind aka me:
> > I'm not sure I completely understand this problem. Can you outline how
> > sendmsg() and socket() are involved in this?
> >
> > I'm also not sure that this holds (but I might misunderstand the
> > problem) afaict, you could do try to get the fd out via CLONE_FILES and
> > other means so something like:
> >
> > // let's pretend the libc wrapper for clone actually has sane semantics
> > pid = clone(CLONE_FILES);
> > if (pid == 0) {
> > fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
> >
> > // Now this fd will be valid in both parent and child.
> > // If you haven't blocked it you can inform the parent what
> > // the fd number is via pipe2(). If you have blocked it you can
> > // use dup2() and dup to a known fd number.
> > }
>
> But what if your seccomp filter wants to block both pipe2() and
> dup2()? Whatever syscall you want to use to do this could be blocked
(Fwiw, setup shared memory before the clone(CLONE_FILES) and write the
fd in the shared memory. :))
> by some seccomp policy, which means you might not be able to use this
> feature in some cases.
>
> Perhaps it's unlikely, and we can just go forward knowing this. But it
> seems like it is worth at least acknowledging that you can wedge
> yourself into a corner.
Sure, if you try really really hard to shoot yourself in the foot you'll
always be able to. From the top of my hat I'd say you can at least
probably filter the seccomp() syscall with the listener argument. Once
you've loaded the policy you're out of luck.
You also might be seccomp confided and not be able to use the ptrace()
syscall. AppArmor might prevent you from using ptrace()ing etc. pp.
So I think we really want both ways but the seccomp interface is way
cleaner. To get the fd from ptrace() you need three syscalls. With
seccomp() you need one.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-08 18:18 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module
In-Reply-To: <CAG48ez3O+wAkq+0yDfoMt73ybnCOEPpi3Mu-uF57_v=6hA8ZEw@mail.gmail.com>
On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
> > > >
> > > > So for the slow of mind aka me:
> > > > I'm not sure I completely understand this problem. Can you outline how
> > > > sendmsg() and socket() are involved in this?
> > > >
> > > > I'm also not sure that this holds (but I might misunderstand the
> > > > problem) afaict, you could do try to get the fd out via CLONE_FILES and
> > > > other means so something like:
> > > >
> > > > // let's pretend the libc wrapper for clone actually has sane semantics
> > > > pid = clone(CLONE_FILES);
> > > > if (pid == 0) {
> > > > fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
> > > >
> > > > // Now this fd will be valid in both parent and child.
> > > > // If you haven't blocked it you can inform the parent what
> > > > // the fd number is via pipe2(). If you have blocked it you can
> > > > // use dup2() and dup to a known fd number.
> > > > }
> > > >
> > > > >
> > > > > v2: fix a bug where listener mode was not unset when an unused fd was not
> > > > > available
> > > > > v3: fix refcounting bug (Oleg)
> > > > > v4: * change the listener's fd flags to be 0
> > > > > * rename GET_LISTENER to NEW_LISTENER (Matthew)
> > > > > v5: * add capable(CAP_SYS_ADMIN) requirement
> > > > > v7: * point the new listener at the right filter (Jann)
> > > > >
> > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > > > > CC: Kees Cook <keescook@chromium.org>
> > > > > CC: Andy Lutomirski <luto@amacapital.net>
> > > > > CC: Oleg Nesterov <oleg@redhat.com>
> > > > > CC: Eric W. Biederman <ebiederm@xmission.com>
> > > > > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > > > > CC: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > CC: Tyler Hicks <tyhicks@canonical.com>
> > > > > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > > > > ---
> > > > > include/linux/seccomp.h | 7 ++
> > > > > include/uapi/linux/ptrace.h | 2 +
> > > > > kernel/ptrace.c | 4 ++
> > > > > kernel/seccomp.c | 31 +++++++++
> > > > > tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
> > > > > 5 files changed, 112 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > > > > index 017444b5efed..234c61b37405 100644
> > > > > --- a/include/linux/seccomp.h
> > > > > +++ b/include/linux/seccomp.h
> > > > > @@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
> > > > > #ifdef CONFIG_SECCOMP_FILTER
> > > > > extern void put_seccomp_filter(struct task_struct *tsk);
> > > > > extern void get_seccomp_filter(struct task_struct *tsk);
> > > > > +extern long seccomp_new_listener(struct task_struct *task,
> > > > > + unsigned long filter_off);
> > > > > #else /* CONFIG_SECCOMP_FILTER */
> > > > > static inline void put_seccomp_filter(struct task_struct *tsk)
> > > > > {
> > > > > @@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> > > > > {
> > > > > return;
> > > > > }
> > > > > +static inline long seccomp_new_listener(struct task_struct *task,
> > > > > + unsigned long filter_off)
> > > > > +{
> > > > > + return -EINVAL;
> > > > > +}
> > > > > #endif /* CONFIG_SECCOMP_FILTER */
> > > > >
> > > > > #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> > > > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > > > > index d5a1b8a492b9..e80ecb1bd427 100644
> > > > > --- a/include/uapi/linux/ptrace.h
> > > > > +++ b/include/uapi/linux/ptrace.h
> > > > > @@ -73,6 +73,8 @@ struct seccomp_metadata {
> > > > > __u64 flags; /* Output: filter's flags */
> > > > > };
> > > > >
> > > > > +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> > > > > +
> > > > > /* Read signals from a shared (process wide) queue */
> > > > > #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
> > > > >
> > > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > > > index 21fec73d45d4..289960ac181b 100644
> > > > > --- a/kernel/ptrace.c
> > > > > +++ b/kernel/ptrace.c
> > > > > @@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
> > > > > ret = seccomp_get_metadata(child, addr, datavp);
> > > > > break;
> > > > >
> > > > > + case PTRACE_SECCOMP_NEW_LISTENER:
> > > > > + ret = seccomp_new_listener(child, addr);
> > > > > + break;
> > > > > +
> > > > > default:
> > > > > break;
> > > > > }
> > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > --- a/kernel/seccomp.c
> > > > > +++ b/kernel/seccomp.c
> > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > >
> > > > > return ret;
> > > > > }
> > > > > +
> > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > + unsigned long filter_off)
> > > > > +{
> > > > > + struct seccomp_filter *filter;
> > > > > + struct file *listener;
> > > > > + int fd;
> > > > > +
> > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > + return -EACCES;
> > > >
> > > > I know this might have been discussed a while back but why exactly do we
> > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > use ptrace from in there?
> > >
> > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > . Basically, the problem is that this doesn't just give you capability
> > > over the target task, but also over every other task that has the same
> > > filter installed; you need some sort of "is the caller capable over
> > > the filter and anyone who uses it" check.
> >
> > Thanks.
> > But then this new ptrace feature as it stands is imho currently broken.
> > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > if you are ns_cpabable(CAP_SYS_ADMIN) then either the new ptrace() api
> > extension should be fixed to allow for this too or the seccomp() way of
> > retrieving the pid - which I really think we want - needs to be fixed to
> > require capable(CAP_SYS_ADMIN) too.
> > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > the preferred way to solve this.
> > Everything else will just be confusing.
>
> First you say "broken", then you say "confusing". Which one do you mean?
Both. It's broken in so far as it places a seemingly unnecessary
restriction that could be fixed. You outlined one possible fix yourself
in the link you provided. And it's confusing in so far as there is a way
via seccomp() to get the fd without said requirement.
>
> Regarding requiring ns_capable() for ptrace: That means that you'll
> have to stash namespace information in the seccomp filter. You'd also
> potentially be eliding the LSM check that would normally have to occur
> between the tracer and the tracee; but I guess that's probably fine?
> CAP_SYS_ADMIN in the init namespace already has some abilities that
> LSMs can't observe; you could argue that CAP_SYS_ADMIN in another
> namespace should have similar semantics, but I'm not sure whether that
> matches what the LSM people want as semantics.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-10-08 18:00 UTC (permalink / raw)
To: Christian Brauner
Cc: Kees Cook, Jann Horn, linux-api, containers, Akihiro Suda,
Oleg Nesterov, linux-kernel, Eric W . Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <20181008151629.hkgzzsluevwtuclw@brauner.io>
On Mon, Oct 08, 2018 at 05:16:30PM +0200, Christian Brauner wrote:
> On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
>
> So for the slow of mind aka me:
> I'm not sure I completely understand this problem. Can you outline how
> sendmsg() and socket() are involved in this?
>
> I'm also not sure that this holds (but I might misunderstand the
> problem) afaict, you could do try to get the fd out via CLONE_FILES and
> other means so something like:
>
> // let's pretend the libc wrapper for clone actually has sane semantics
> pid = clone(CLONE_FILES);
> if (pid == 0) {
> fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
>
> // Now this fd will be valid in both parent and child.
> // If you haven't blocked it you can inform the parent what
> // the fd number is via pipe2(). If you have blocked it you can
> // use dup2() and dup to a known fd number.
> }
But what if your seccomp filter wants to block both pipe2() and
dup2()? Whatever syscall you want to use to do this could be blocked
by some seccomp policy, which means you might not be able to use this
feature in some cases.
Perhaps it's unlikely, and we can just go forward knowing this. But it
seems like it is worth at least acknowledging that you can wedge
yourself into a corner.
Tycho
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-08 16:42 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module
In-Reply-To: <20181008162147.ubfxxsv2425l2zsp@brauner.io>
On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
> > >
> > > So for the slow of mind aka me:
> > > I'm not sure I completely understand this problem. Can you outline how
> > > sendmsg() and socket() are involved in this?
> > >
> > > I'm also not sure that this holds (but I might misunderstand the
> > > problem) afaict, you could do try to get the fd out via CLONE_FILES and
> > > other means so something like:
> > >
> > > // let's pretend the libc wrapper for clone actually has sane semantics
> > > pid = clone(CLONE_FILES);
> > > if (pid == 0) {
> > > fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
> > >
> > > // Now this fd will be valid in both parent and child.
> > > // If you haven't blocked it you can inform the parent what
> > > // the fd number is via pipe2(). If you have blocked it you can
> > > // use dup2() and dup to a known fd number.
> > > }
> > >
> > > >
> > > > v2: fix a bug where listener mode was not unset when an unused fd was not
> > > > available
> > > > v3: fix refcounting bug (Oleg)
> > > > v4: * change the listener's fd flags to be 0
> > > > * rename GET_LISTENER to NEW_LISTENER (Matthew)
> > > > v5: * add capable(CAP_SYS_ADMIN) requirement
> > > > v7: * point the new listener at the right filter (Jann)
> > > >
> > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > > > CC: Kees Cook <keescook@chromium.org>
> > > > CC: Andy Lutomirski <luto@amacapital.net>
> > > > CC: Oleg Nesterov <oleg@redhat.com>
> > > > CC: Eric W. Biederman <ebiederm@xmission.com>
> > > > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > > > CC: Christian Brauner <christian.brauner@ubuntu.com>
> > > > CC: Tyler Hicks <tyhicks@canonical.com>
> > > > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > > > ---
> > > > include/linux/seccomp.h | 7 ++
> > > > include/uapi/linux/ptrace.h | 2 +
> > > > kernel/ptrace.c | 4 ++
> > > > kernel/seccomp.c | 31 +++++++++
> > > > tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
> > > > 5 files changed, 112 insertions(+)
> > > >
> > > > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > > > index 017444b5efed..234c61b37405 100644
> > > > --- a/include/linux/seccomp.h
> > > > +++ b/include/linux/seccomp.h
> > > > @@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
> > > > #ifdef CONFIG_SECCOMP_FILTER
> > > > extern void put_seccomp_filter(struct task_struct *tsk);
> > > > extern void get_seccomp_filter(struct task_struct *tsk);
> > > > +extern long seccomp_new_listener(struct task_struct *task,
> > > > + unsigned long filter_off);
> > > > #else /* CONFIG_SECCOMP_FILTER */
> > > > static inline void put_seccomp_filter(struct task_struct *tsk)
> > > > {
> > > > @@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> > > > {
> > > > return;
> > > > }
> > > > +static inline long seccomp_new_listener(struct task_struct *task,
> > > > + unsigned long filter_off)
> > > > +{
> > > > + return -EINVAL;
> > > > +}
> > > > #endif /* CONFIG_SECCOMP_FILTER */
> > > >
> > > > #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> > > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > > > index d5a1b8a492b9..e80ecb1bd427 100644
> > > > --- a/include/uapi/linux/ptrace.h
> > > > +++ b/include/uapi/linux/ptrace.h
> > > > @@ -73,6 +73,8 @@ struct seccomp_metadata {
> > > > __u64 flags; /* Output: filter's flags */
> > > > };
> > > >
> > > > +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> > > > +
> > > > /* Read signals from a shared (process wide) queue */
> > > > #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
> > > >
> > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > > index 21fec73d45d4..289960ac181b 100644
> > > > --- a/kernel/ptrace.c
> > > > +++ b/kernel/ptrace.c
> > > > @@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
> > > > ret = seccomp_get_metadata(child, addr, datavp);
> > > > break;
> > > >
> > > > + case PTRACE_SECCOMP_NEW_LISTENER:
> > > > + ret = seccomp_new_listener(child, addr);
> > > > + break;
> > > > +
> > > > default:
> > > > break;
> > > > }
> > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > index 44a31ac8373a..17685803a2af 100644
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > >
> > > > return ret;
> > > > }
> > > > +
> > > > +long seccomp_new_listener(struct task_struct *task,
> > > > + unsigned long filter_off)
> > > > +{
> > > > + struct seccomp_filter *filter;
> > > > + struct file *listener;
> > > > + int fd;
> > > > +
> > > > + if (!capable(CAP_SYS_ADMIN))
> > > > + return -EACCES;
> > >
> > > I know this might have been discussed a while back but why exactly do we
> > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > use ptrace from in there?
> >
> > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > . Basically, the problem is that this doesn't just give you capability
> > over the target task, but also over every other task that has the same
> > filter installed; you need some sort of "is the caller capable over
> > the filter and anyone who uses it" check.
>
> Thanks.
> But then this new ptrace feature as it stands is imho currently broken.
> If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> if you are ns_cpabable(CAP_SYS_ADMIN) then either the new ptrace() api
> extension should be fixed to allow for this too or the seccomp() way of
> retrieving the pid - which I really think we want - needs to be fixed to
> require capable(CAP_SYS_ADMIN) too.
> The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> the preferred way to solve this.
> Everything else will just be confusing.
First you say "broken", then you say "confusing". Which one do you mean?
Regarding requiring ns_capable() for ptrace: That means that you'll
have to stash namespace information in the seccomp filter. You'd also
potentially be eliding the LSM check that would normally have to occur
between the tracer and the tracee; but I guess that's probably fine?
CAP_SYS_ADMIN in the init namespace already has some abilities that
LSMs can't observe; you could argue that CAP_SYS_ADMIN in another
namespace should have similar semantics, but I'm not sure whether that
matches what the LSM people want as semantics.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-08 16:21 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <CAG48ez3zaqOXH_jHN8=+jXjOf3vhFyWUkM_6KLPi9T8HcQySeg@mail.gmail.com>
On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
> >
> > So for the slow of mind aka me:
> > I'm not sure I completely understand this problem. Can you outline how
> > sendmsg() and socket() are involved in this?
> >
> > I'm also not sure that this holds (but I might misunderstand the
> > problem) afaict, you could do try to get the fd out via CLONE_FILES and
> > other means so something like:
> >
> > // let's pretend the libc wrapper for clone actually has sane semantics
> > pid = clone(CLONE_FILES);
> > if (pid == 0) {
> > fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
> >
> > // Now this fd will be valid in both parent and child.
> > // If you haven't blocked it you can inform the parent what
> > // the fd number is via pipe2(). If you have blocked it you can
> > // use dup2() and dup to a known fd number.
> > }
> >
> > >
> > > v2: fix a bug where listener mode was not unset when an unused fd was not
> > > available
> > > v3: fix refcounting bug (Oleg)
> > > v4: * change the listener's fd flags to be 0
> > > * rename GET_LISTENER to NEW_LISTENER (Matthew)
> > > v5: * add capable(CAP_SYS_ADMIN) requirement
> > > v7: * point the new listener at the right filter (Jann)
> > >
> > > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > > CC: Kees Cook <keescook@chromium.org>
> > > CC: Andy Lutomirski <luto@amacapital.net>
> > > CC: Oleg Nesterov <oleg@redhat.com>
> > > CC: Eric W. Biederman <ebiederm@xmission.com>
> > > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > > CC: Christian Brauner <christian.brauner@ubuntu.com>
> > > CC: Tyler Hicks <tyhicks@canonical.com>
> > > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > > ---
> > > include/linux/seccomp.h | 7 ++
> > > include/uapi/linux/ptrace.h | 2 +
> > > kernel/ptrace.c | 4 ++
> > > kernel/seccomp.c | 31 +++++++++
> > > tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
> > > 5 files changed, 112 insertions(+)
> > >
> > > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > > index 017444b5efed..234c61b37405 100644
> > > --- a/include/linux/seccomp.h
> > > +++ b/include/linux/seccomp.h
> > > @@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
> > > #ifdef CONFIG_SECCOMP_FILTER
> > > extern void put_seccomp_filter(struct task_struct *tsk);
> > > extern void get_seccomp_filter(struct task_struct *tsk);
> > > +extern long seccomp_new_listener(struct task_struct *task,
> > > + unsigned long filter_off);
> > > #else /* CONFIG_SECCOMP_FILTER */
> > > static inline void put_seccomp_filter(struct task_struct *tsk)
> > > {
> > > @@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> > > {
> > > return;
> > > }
> > > +static inline long seccomp_new_listener(struct task_struct *task,
> > > + unsigned long filter_off)
> > > +{
> > > + return -EINVAL;
> > > +}
> > > #endif /* CONFIG_SECCOMP_FILTER */
> > >
> > > #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > > index d5a1b8a492b9..e80ecb1bd427 100644
> > > --- a/include/uapi/linux/ptrace.h
> > > +++ b/include/uapi/linux/ptrace.h
> > > @@ -73,6 +73,8 @@ struct seccomp_metadata {
> > > __u64 flags; /* Output: filter's flags */
> > > };
> > >
> > > +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> > > +
> > > /* Read signals from a shared (process wide) queue */
> > > #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
> > >
> > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > index 21fec73d45d4..289960ac181b 100644
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
> > > ret = seccomp_get_metadata(child, addr, datavp);
> > > break;
> > >
> > > + case PTRACE_SECCOMP_NEW_LISTENER:
> > > + ret = seccomp_new_listener(child, addr);
> > > + break;
> > > +
> > > default:
> > > break;
> > > }
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 44a31ac8373a..17685803a2af 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > >
> > > return ret;
> > > }
> > > +
> > > +long seccomp_new_listener(struct task_struct *task,
> > > + unsigned long filter_off)
> > > +{
> > > + struct seccomp_filter *filter;
> > > + struct file *listener;
> > > + int fd;
> > > +
> > > + if (!capable(CAP_SYS_ADMIN))
> > > + return -EACCES;
> >
> > I know this might have been discussed a while back but why exactly do we
> > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > use ptrace from in there?
>
> See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> . Basically, the problem is that this doesn't just give you capability
> over the target task, but also over every other task that has the same
> filter installed; you need some sort of "is the caller capable over
> the filter and anyone who uses it" check.
Thanks.
But then this new ptrace feature as it stands is imho currently broken.
If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
if you are ns_cpabable(CAP_SYS_ADMIN) then either the new ptrace() api
extension should be fixed to allow for this too or the seccomp() way of
retrieving the pid - which I really think we want - needs to be fixed to
require capable(CAP_SYS_ADMIN) too.
The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
the preferred way to solve this.
Everything else will just be confusing.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-08 15:33 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <20181008151629.hkgzzsluevwtuclw@brauner.io>
On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
>
> So for the slow of mind aka me:
> I'm not sure I completely understand this problem. Can you outline how
> sendmsg() and socket() are involved in this?
>
> I'm also not sure that this holds (but I might misunderstand the
> problem) afaict, you could do try to get the fd out via CLONE_FILES and
> other means so something like:
>
> // let's pretend the libc wrapper for clone actually has sane semantics
> pid = clone(CLONE_FILES);
> if (pid == 0) {
> fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
>
> // Now this fd will be valid in both parent and child.
> // If you haven't blocked it you can inform the parent what
> // the fd number is via pipe2(). If you have blocked it you can
> // use dup2() and dup to a known fd number.
> }
>
> >
> > v2: fix a bug where listener mode was not unset when an unused fd was not
> > available
> > v3: fix refcounting bug (Oleg)
> > v4: * change the listener's fd flags to be 0
> > * rename GET_LISTENER to NEW_LISTENER (Matthew)
> > v5: * add capable(CAP_SYS_ADMIN) requirement
> > v7: * point the new listener at the right filter (Jann)
> >
> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Andy Lutomirski <luto@amacapital.net>
> > CC: Oleg Nesterov <oleg@redhat.com>
> > CC: Eric W. Biederman <ebiederm@xmission.com>
> > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > CC: Christian Brauner <christian.brauner@ubuntu.com>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > ---
> > include/linux/seccomp.h | 7 ++
> > include/uapi/linux/ptrace.h | 2 +
> > kernel/ptrace.c | 4 ++
> > kernel/seccomp.c | 31 +++++++++
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
> > 5 files changed, 112 insertions(+)
> >
> > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > index 017444b5efed..234c61b37405 100644
> > --- a/include/linux/seccomp.h
> > +++ b/include/linux/seccomp.h
> > @@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
> > #ifdef CONFIG_SECCOMP_FILTER
> > extern void put_seccomp_filter(struct task_struct *tsk);
> > extern void get_seccomp_filter(struct task_struct *tsk);
> > +extern long seccomp_new_listener(struct task_struct *task,
> > + unsigned long filter_off);
> > #else /* CONFIG_SECCOMP_FILTER */
> > static inline void put_seccomp_filter(struct task_struct *tsk)
> > {
> > @@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> > {
> > return;
> > }
> > +static inline long seccomp_new_listener(struct task_struct *task,
> > + unsigned long filter_off)
> > +{
> > + return -EINVAL;
> > +}
> > #endif /* CONFIG_SECCOMP_FILTER */
> >
> > #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index d5a1b8a492b9..e80ecb1bd427 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -73,6 +73,8 @@ struct seccomp_metadata {
> > __u64 flags; /* Output: filter's flags */
> > };
> >
> > +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> > +
> > /* Read signals from a shared (process wide) queue */
> > #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
> >
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 21fec73d45d4..289960ac181b 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
> > ret = seccomp_get_metadata(child, addr, datavp);
> > break;
> >
> > + case PTRACE_SECCOMP_NEW_LISTENER:
> > + ret = seccomp_new_listener(child, addr);
> > + break;
> > +
> > default:
> > break;
> > }
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 44a31ac8373a..17685803a2af 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> >
> > return ret;
> > }
> > +
> > +long seccomp_new_listener(struct task_struct *task,
> > + unsigned long filter_off)
> > +{
> > + struct seccomp_filter *filter;
> > + struct file *listener;
> > + int fd;
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EACCES;
>
> I know this might have been discussed a while back but why exactly do we
> require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> use ptrace from in there?
See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
. Basically, the problem is that this doesn't just give you capability
over the target task, but also over every other task that has the same
filter installed; you need some sort of "is the caller capable over
the filter and anyone who uses it" check.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-08 15:16 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, linux-api, containers, Akihiro Suda,
Oleg Nesterov, linux-kernel, Eric W . Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <20180927151119.9989-4-tycho@tycho.ws>
On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen 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.
So for the slow of mind aka me:
I'm not sure I completely understand this problem. Can you outline how
sendmsg() and socket() are involved in this?
I'm also not sure that this holds (but I might misunderstand the
problem) afaict, you could do try to get the fd out via CLONE_FILES and
other means so something like:
// let's pretend the libc wrapper for clone actually has sane semantics
pid = clone(CLONE_FILES);
if (pid == 0) {
fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
// Now this fd will be valid in both parent and child.
// If you haven't blocked it you can inform the parent what
// the fd number is via pipe2(). If you have blocked it you can
// use dup2() and dup to a known fd number.
}
>
> v2: fix a bug where listener mode was not unset when an unused fd was not
> available
> v3: fix refcounting bug (Oleg)
> v4: * change the listener's fd flags to be 0
> * rename GET_LISTENER to NEW_LISTENER (Matthew)
> v5: * add capable(CAP_SYS_ADMIN) requirement
> v7: * point the new listener at the right filter (Jann)
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> CC: Christian Brauner <christian.brauner@ubuntu.com>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> ---
> include/linux/seccomp.h | 7 ++
> include/uapi/linux/ptrace.h | 2 +
> kernel/ptrace.c | 4 ++
> kernel/seccomp.c | 31 +++++++++
> tools/testing/selftests/seccomp/seccomp_bpf.c | 68 +++++++++++++++++++
> 5 files changed, 112 insertions(+)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index 017444b5efed..234c61b37405 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -83,6 +83,8 @@ static inline int seccomp_mode(struct seccomp *s)
> #ifdef CONFIG_SECCOMP_FILTER
> extern void put_seccomp_filter(struct task_struct *tsk);
> extern void get_seccomp_filter(struct task_struct *tsk);
> +extern long seccomp_new_listener(struct task_struct *task,
> + unsigned long filter_off);
> #else /* CONFIG_SECCOMP_FILTER */
> static inline void put_seccomp_filter(struct task_struct *tsk)
> {
> @@ -92,6 +94,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> {
> return;
> }
> +static inline long seccomp_new_listener(struct task_struct *task,
> + unsigned long filter_off)
> +{
> + return -EINVAL;
> +}
> #endif /* CONFIG_SECCOMP_FILTER */
>
> #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index d5a1b8a492b9..e80ecb1bd427 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -73,6 +73,8 @@ struct seccomp_metadata {
> __u64 flags; /* Output: filter's flags */
> };
>
> +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> +
> /* Read signals from a shared (process wide) queue */
> #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 21fec73d45d4..289960ac181b 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -1096,6 +1096,10 @@ int ptrace_request(struct task_struct *child, long request,
> ret = seccomp_get_metadata(child, addr, datavp);
> break;
>
> + case PTRACE_SECCOMP_NEW_LISTENER:
> + ret = seccomp_new_listener(child, addr);
> + break;
> +
> default:
> break;
> }
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 44a31ac8373a..17685803a2af 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
>
> return ret;
> }
> +
> +long seccomp_new_listener(struct task_struct *task,
> + unsigned long filter_off)
> +{
> + struct seccomp_filter *filter;
> + struct file *listener;
> + int fd;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
I know this might have been discussed a while back but why exactly do we
require CAP_SYS_ADMIN in init_userns and not in the target userns? What
if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
use ptrace from in there?
> +
> + filter = get_nth_filter(task, filter_off);
> + if (IS_ERR(filter))
> + return PTR_ERR(filter);
> +
> + fd = get_unused_fd_flags(0);
> + if (fd < 0) {
> + __put_seccomp_filter(filter);
> + return fd;
> + }
> +
> + listener = init_listener(task, filter);
> + __put_seccomp_filter(filter);
> + if (IS_ERR(listener)) {
> + put_unused_fd(fd);
> + return PTR_ERR(listener);
> + }
> +
> + fd_install(fd, listener);
> + return fd;
> +}
> #endif
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 5f4b836a6792..c6ba3ed5392e 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -193,6 +193,10 @@ int seccomp(unsigned int op, unsigned int flags, void *args)
> }
> #endif
>
> +#ifndef PTRACE_SECCOMP_NEW_LISTENER
> +#define PTRACE_SECCOMP_NEW_LISTENER 0x420e
> +#endif
> +
> #if __BYTE_ORDER == __LITTLE_ENDIAN
> #define syscall_arg(_n) (offsetof(struct seccomp_data, args[_n]))
> #elif __BYTE_ORDER == __BIG_ENDIAN
> @@ -3175,6 +3179,70 @@ TEST(get_user_notification_syscall)
> EXPECT_EQ(0, WEXITSTATUS(status));
> }
>
> +TEST(get_user_notification_ptrace)
> +{
> + pid_t pid;
> + int status, listener;
> + int sk_pair[2];
> + char c;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> +
> + ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + EXPECT_EQ(user_trap_syscall(__NR_getpid, 0), 0);
> +
> + /* Test that we get ENOSYS while not attached */
> + EXPECT_EQ(syscall(__NR_getpid), -1);
> + EXPECT_EQ(errno, ENOSYS);
> +
> + /* Signal we're ready and have installed the filter. */
> + EXPECT_EQ(write(sk_pair[1], "J", 1), 1);
> +
> + EXPECT_EQ(read(sk_pair[1], &c, 1), 1);
> + EXPECT_EQ(c, 'H');
> +
> + exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
> + }
> +
> + EXPECT_EQ(read(sk_pair[0], &c, 1), 1);
> + EXPECT_EQ(c, 'J');
> +
> + EXPECT_EQ(ptrace(PTRACE_ATTACH, pid), 0);
> + EXPECT_EQ(waitpid(pid, NULL, 0), pid);
> + listener = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0);
> + EXPECT_GE(listener, 0);
> +
> + /* EBUSY for second listener */
> + EXPECT_EQ(ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0), -1);
> + EXPECT_EQ(errno, EBUSY);
> +
> + EXPECT_EQ(ptrace(PTRACE_DETACH, pid, NULL, 0), 0);
> +
> + /* Now signal we are done and respond with magic */
> + EXPECT_EQ(write(sk_pair[0], "H", 1), 1);
> +
> + req.len = sizeof(req);
> + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
> +
> + resp.len = sizeof(resp);
> + resp.id = req.id;
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
> +
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +
> + close(listener);
> +}
> +
> /*
> * Check that a pid in a child namespace still shows up as valid in ours.
> */
> --
> 2.17.1
>
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Christian Brauner @ 2018-10-08 14:58 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, Linux API, Linux Containers, Akihiro Suda,
Oleg Nesterov, LKML, Eric W . Biederman,
linux-fsdevel@vger.kernel.org, Christian Brauner, Andy Lutomirski
In-Reply-To: <20180927224839.GF15491@cisco.cisco.com>
On Thu, Sep 27, 2018 at 04:48:39PM -0600, Tycho Andersen wrote:
> On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> > On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > This patch introduces a means for syscalls matched in seccomp to notify
> > > some other task that a particular filter has been triggered.
> > >
> > > The motivation for this is primarily for use with containers. For example,
> > > if a container does an init_module(), we obviously don't want to load this
> > > untrusted code, which may be compiled for the wrong version of the kernel
> > > anyway. Instead, we could parse the module image, figure out which module
> > > the container is trying to load and load it on the host.
> > >
> > > As another example, containers cannot mknod(), since this checks
> > > capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
> > > /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
> > > coding some whitelist in the kernel. Another example is mount(), which has
> > > many security restrictions for good reason, but configuration or runtime
> > > knowledge could potentially be used to relax these restrictions.
> > >
> > > This patch adds functionality that is already possible via at least two
> > > other means that I know about, both of which involve ptrace(): first, one
> > > could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> > > Unfortunately this is slow, so a faster version would be to install a
> > > filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> > > Since ptrace allows only one tracer, if the container runtime is that
> > > tracer, users inside the container (or outside) trying to debug it will not
> > > be able to use ptrace, which is annoying. It also means that older
> > > distributions based on Upstart cannot boot inside containers using ptrace,
> > > since upstart itself uses ptrace to start services.
> > >
> > > The actual implementation of this is fairly small, although getting the
> > > synchronization right was/is slightly complex.
> > >
> > > Finally, it's worth noting that the classic seccomp TOCTOU of reading
> > > memory data from the task still applies here, but can be avoided with
> > > careful design of the userspace handler: if the userspace handler reads all
> > > of the task memory that is necessary before applying its security policy,
> > > the tracee's subsequent memory edits will not be read by the tracer.
> > >
> > > v2: * make id a u64; the idea here being that it will never overflow,
> > > because 64 is huge (one syscall every nanosecond => wrap every 584
> > > years) (Andy)
> > > * prevent nesting of user notifications: if someone is already attached
> > > the tree in one place, nobody else can attach to the tree (Andy)
> > > * notify the listener of signals the tracee receives as well (Andy)
> > > * implement poll
> > > v3: * lockdep fix (Oleg)
> > > * drop unnecessary WARN()s (Christian)
> > > * rearrange error returns to be more rpetty (Christian)
> > > * fix build in !CONFIG_SECCOMP_USER_NOTIFICATION case
> > > v4: * fix implementation of poll to use poll_wait() (Jann)
> > > * change listener's fd flags to be 0 (Jann)
> > > * hoist filter initialization out of ifdefs to its own function
> > > init_user_notification()
> > > * add some more testing around poll() and closing the listener while a
> > > syscall is in action
> > > * s/GET_LISTENER/NEW_LISTENER, since you can't _get_ a listener, but it
> > > creates a new one (Matthew)
> > > * correctly handle pid namespaces, add some testcases (Matthew)
> > > * use EINPROGRESS instead of EINVAL when a notification response is
> > > written twice (Matthew)
> > > * fix comment typo from older version (SEND vs READ) (Matthew)
> > > * whitespace and logic simplification (Tobin)
> > > * add some Documentation/ bits on userspace trapping
> > > v5: * fix documentation typos (Jann)
> > > * add signalled field to struct seccomp_notif (Jann)
> > > * switch to using ioctls instead of read()/write() for struct passing
> > > (Jann)
> > > * add an ioctl to ensure an id is still valid
> > > v6: * docs typo fixes, update docs for ioctl() change (Christian)
> > > v7: * switch struct seccomp_knotif's id member to a u64 (derp :)
> > > * use notify_lock in IS_ID_VALID query to avoid racing
> > > * s/signalled/signaled (Tyler)
> > > * fix docs to reflect that ids are not globally unique (Tyler)
> > > * add a test to check -ERESTARTSYS behavior (Tyler)
> > > * drop CONFIG_SECCOMP_USER_NOTIFICATION (Tyler)
> > > * reorder USER_NOTIF in seccomp return codes list (Tyler)
> > > * return size instead of sizeof(struct user_notif) (Tyler)
> > > * ENOENT instead of EINVAL when invalid id is passed (Tyler)
> > > * drop CONFIG_SECCOMP_USER_NOTIFICATION guards (Tyler)
> > > * s/IS_ID_VALID/ID_VALID and switch ioctl to be "well behaved" (Tyler)
> > > * add a new struct notification to minimize the additions to
> > > struct seccomp_filter, also pack the necessary additions a bit more
> > > cleverly (Tyler)
> > > * switch to keeping track of the task itself instead of the pid (we'll
> > > use this for implementing PUT_FD)
> >
> > Patch-sending nit: can you put the versioning below the "---" line so
> > it isn't included in the final commit? (And I normally read these
> > backwards, so I'd expect v7 at the top, but that's not a big deal. I
> > mean... neither is the --- thing, but it makes "git am" easier for me
> > since I don't have to go edit the versioning out of the log.)
>
> Sure, will do.
>
> > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > > index 9efc0e73d50b..d4ccb32fe089 100644
> > > --- a/include/uapi/linux/seccomp.h
> > > +++ b/include/uapi/linux/seccomp.h
> > > @@ -17,9 +17,10 @@
> > > #define SECCOMP_GET_ACTION_AVAIL 2
> > >
> > > /* Valid flags for SECCOMP_SET_MODE_FILTER */
> > > -#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> > > -#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
> > > -#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
> > > +#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> > > +#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
> > > +#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
> > > +#define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
> >
> > Since these are all getting indentation updates, can you switch them
> > to BIT(0), BIT(1), etc?
>
> Will do.
>
> > > /*
> > > * All BPF programs must return a 32-bit value.
> > > @@ -35,6 +36,7 @@
> > > #define SECCOMP_RET_KILL SECCOMP_RET_KILL_THREAD
> > > #define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SIGSYS */
> > > #define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */
> > > +#define SECCOMP_RET_USER_NOTIF 0x7fc00000U /* notifies userspace */
> > > #define SECCOMP_RET_TRACE 0x7ff00000U /* pass to a tracer or disallow */
> > > #define SECCOMP_RET_LOG 0x7ffc0000U /* allow after logging */
> > > #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
> > > @@ -60,4 +62,29 @@ struct seccomp_data {
> > > __u64 args[6];
> > > };
> > >
> > > +struct seccomp_notif {
> > > + __u16 len;
> > > + __u64 id;
> > > + __u32 pid;
> > > + __u8 signaled;
> > > + struct seccomp_data data;
> > > +};
> > > +
> > > +struct seccomp_notif_resp {
> > > + __u16 len;
> > > + __u64 id;
> > > + __s32 error;
> > > + __s64 val;
> > > +};
> >
> > So, len has to come first, for versioning. However, since it's ahead
> > of a u64, this leaves a struct padding hole. pahole output:
> >
> > struct seccomp_notif {
> > __u16 len; /* 0 2 */
> >
> > /* XXX 6 bytes hole, try to pack */
> >
> > __u64 id; /* 8 8 */
> > __u32 pid; /* 16 4 */
> > __u8 signaled; /* 20 1 */
> >
> > /* XXX 3 bytes hole, try to pack */
> >
> > struct seccomp_data data; /* 24 64 */
> > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >
> > /* size: 88, cachelines: 2, members: 5 */
> > /* sum members: 79, holes: 2, sum holes: 9 */
> > /* last cacheline: 24 bytes */
> > };
> > struct seccomp_notif_resp {
> > __u16 len; /* 0 2 */
> >
> > /* XXX 6 bytes hole, try to pack */
> >
> > __u64 id; /* 8 8 */
> > __s32 error; /* 16 4 */
> >
> > /* XXX 4 bytes hole, try to pack */
> >
> > __s64 val; /* 24 8 */
> >
> > /* size: 32, cachelines: 1, members: 4 */
> > /* sum members: 22, holes: 2, sum holes: 10 */
> > /* last cacheline: 32 bytes */
> > };
> >
> > How about making len u32, and moving pid and error above "id"? This
> > leaves a hole after signaled, so changing "len" won't be sufficient
> > for versioning here. Perhaps move it after data?
>
> I'm not sure what you mean by "len won't be sufficient for versioning
> here"? Anyway, I can do some packing on these; I didn't bother before
> since I figured it's a userspace interface, so saving a few bytes
> isn't a huge deal.
>
> > > +
> > > +#define SECCOMP_IOC_MAGIC 0xF7
> >
> > Was there any specific reason for picking this value? There are lots
> > of fun ASCII code left like '!' or '*'. :)
>
> No, ! it is :)
>
> > > +
> > > +/* Flags for seccomp notification fd ioctl. */
> > > +#define SECCOMP_NOTIF_RECV _IOWR(SECCOMP_IOC_MAGIC, 0, \
> > > + struct seccomp_notif)
> > > +#define SECCOMP_NOTIF_SEND _IOWR(SECCOMP_IOC_MAGIC, 1, \
> > > + struct seccomp_notif_resp)
> > > +#define SECCOMP_NOTIF_ID_VALID _IOR(SECCOMP_IOC_MAGIC, 2, \
> > > + __u64)
> >
> > To match other UAPI ioctl, can these have a prefix of "SECCOMP_IOCTOL_..."?
> >
> > It may also be useful to match how other uapis do this, like for DRM:
> >
> > #define DRM_IOCTL_BASE 'd'
> > #define DRM_IO(nr) _IO(DRM_IOCTL_BASE,nr)
> > #define DRM_IOR(nr,type) _IOR(DRM_IOCTL_BASE,nr,type)
> > #define DRM_IOW(nr,type) _IOW(DRM_IOCTL_BASE,nr,type)
> > #define DRM_IOWR(nr,type) _IOWR(DRM_IOCTL_BASE,nr,type)
> >
> > #define DRM_IOCTL_VERSION DRM_IOWR(0x00, struct drm_version)
> > #define DRM_IOCTL_GET_UNIQUE DRM_IOWR(0x01, struct drm_unique)
> > #define DRM_IOCTL_GET_MAGIC DRM_IOR( 0x02, struct drm_auth)
> > ...
>
> Will do.
>
> >
> > > +
> > > #endif /* _UAPI_LINUX_SECCOMP_H */
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index fd023ac24e10..fa6fe9756c80 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -33,12 +33,78 @@
> > > #endif
> > >
> > > #ifdef CONFIG_SECCOMP_FILTER
> > > +#include <linux/file.h>
> > > #include <linux/filter.h>
> > > #include <linux/pid.h>
> > > #include <linux/ptrace.h>
> > > #include <linux/security.h>
> > > #include <linux/tracehook.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/anon_inodes.h>
> > > +
> > > +enum notify_state {
> > > + SECCOMP_NOTIFY_INIT,
> > > + SECCOMP_NOTIFY_SENT,
> > > + SECCOMP_NOTIFY_REPLIED,
> > > +};
> > > +
> > > +struct seccomp_knotif {
> > > + /* The struct pid of the task whose filter triggered the notification */
> > > + struct task_struct *task;
> > > +
> > > + /* The "cookie" for this request; this is unique for this filter. */
> > > + u64 id;
> > > +
> > > + /* Whether or not this task has been given an interruptible signal. */
> > > + bool signaled;
> > > +
> > > + /*
> > > + * The seccomp data. This pointer is valid the entire time this
> > > + * notification is active, since it comes from __seccomp_filter which
> > > + * eclipses the entire lifecycle here.
> > > + */
> > > + const struct seccomp_data *data;
> > > +
> > > + /*
> > > + * Notification states. When SECCOMP_RET_USER_NOTIF is returned, a
> > > + * struct seccomp_knotif is created and starts out in INIT. Once the
> > > + * handler reads the notification off of an FD, it transitions to SENT.
> > > + * If a signal is received the state transitions back to INIT and
> > > + * another message is sent. When the userspace handler replies, state
> > > + * transitions to REPLIED.
> > > + */
> > > + enum notify_state state;
> > > +
> > > + /* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
> > > + int error;
> > > + long val;
> > > +
> > > + /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
> > > + struct completion ready;
> > > +
> > > + struct list_head list;
> > > +};
> > > +
> > > +/**
> > > + * struct notification - container for seccomp userspace notifications. Since
> > > + * most seccomp filters will not have notification listeners attached and this
> > > + * structure is fairly large, we store the notification-specific stuff in a
> > > + * separate structure.
> > > + *
> > > + * @request: A semaphore that users of this notification can wait on for
> > > + * changes. Actual reads and writes are still controlled with
> > > + * filter->notify_lock.
> > > + * @notify_lock: A lock for all notification-related accesses.
> > > + * @next_id: The id of the next request.
> > > + * @notifications: A list of struct seccomp_knotif elements.
> > > + * @wqh: A wait queue for poll.
> > > + */
> > > +struct notification {
> > > + struct semaphore request;
> > > + u64 next_id;
> > > + struct list_head notifications;
> > > + wait_queue_head_t wqh;
> > > +};
> > >
> > > /**
> > > * struct seccomp_filter - container for seccomp BPF programs
> > > @@ -66,6 +132,8 @@ struct seccomp_filter {
> > > bool log;
> > > struct seccomp_filter *prev;
> > > struct bpf_prog *prog;
> > > + struct notification *notif;
> > > + struct mutex notify_lock;
> > > };
> > >
> > > /* Limit any path through the tree to 256KB worth of instructions. */
> > > @@ -392,6 +460,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
> > > if (!sfilter)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > + mutex_init(&sfilter->notify_lock);
> > > ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
> > > seccomp_check_filter, save_orig);
> > > if (ret < 0) {
> > > @@ -556,11 +625,13 @@ static void seccomp_send_sigsys(int syscall, int reason)
> > > #define SECCOMP_LOG_TRACE (1 << 4)
> > > #define SECCOMP_LOG_LOG (1 << 5)
> > > #define SECCOMP_LOG_ALLOW (1 << 6)
> > > +#define SECCOMP_LOG_USER_NOTIF (1 << 7)
> > >
> > > static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_PROCESS |
> > > SECCOMP_LOG_KILL_THREAD |
> > > SECCOMP_LOG_TRAP |
> > > SECCOMP_LOG_ERRNO |
> > > + SECCOMP_LOG_USER_NOTIF |
> > > SECCOMP_LOG_TRACE |
> > > SECCOMP_LOG_LOG;
> > >
> > > @@ -581,6 +652,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> > > case SECCOMP_RET_TRACE:
> > > log = requested && seccomp_actions_logged & SECCOMP_LOG_TRACE;
> > > break;
> > > + case SECCOMP_RET_USER_NOTIF:
> > > + log = requested && seccomp_actions_logged & SECCOMP_LOG_USER_NOTIF;
> > > + break;
> > > case SECCOMP_RET_LOG:
> > > log = seccomp_actions_logged & SECCOMP_LOG_LOG;
> > > break;
> > > @@ -652,6 +726,73 @@ void secure_computing_strict(int this_syscall)
> > > #else
> > >
> > > #ifdef CONFIG_SECCOMP_FILTER
> > > +static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
> > > +{
> > > + /* Note: overflow is ok here, the id just needs to be unique */
> >
> > Maybe just clarify in the comment: unique to the filter.
> >
> > > + return filter->notif->next_id++;
> >
> > Also, it might be useful to add for both documentation and lockdep:
> >
> > lockdep_assert_held(filter->notif->notify_lock);
> >
> > into this function?
>
> Will do.
>
> >
> > > +}
> > > +
> > > +static void seccomp_do_user_notification(int this_syscall,
> > > + struct seccomp_filter *match,
> > > + const struct seccomp_data *sd)
> > > +{
> > > + int err;
> > > + long ret = 0;
> > > + struct seccomp_knotif n = {};
> > > +
> > > + mutex_lock(&match->notify_lock);
> > > + err = -ENOSYS;
> > > + if (!match->notif)
> > > + goto out;
> > > +
> > > + n.task = current;
> > > + n.state = SECCOMP_NOTIFY_INIT;
> > > + n.data = sd;
> > > + n.id = seccomp_next_notify_id(match);
> > > + init_completion(&n.ready);
> > > +
> > > + list_add(&n.list, &match->notif->notifications);
> > > + wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
> > > +
> > > + mutex_unlock(&match->notify_lock);
> > > + up(&match->notif->request);
> > > +
> >
> > Maybe add a big comment here saying this is where we're waiting for a reply?
>
> Will do.
>
> > > + err = wait_for_completion_interruptible(&n.ready);
> > > + mutex_lock(&match->notify_lock);
> > > +
> > > + /*
> > > + * Here it's possible we got a signal and then had to wait on the mutex
> > > + * while the reply was sent, so let's be sure there wasn't a response
> > > + * in the meantime.
> > > + */
> > > + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> > > + /*
> > > + * We got a signal. Let's tell userspace about it (potentially
> > > + * again, if we had already notified them about the first one).
> > > + */
> > > + n.signaled = true;
> > > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > > + n.state = SECCOMP_NOTIFY_INIT;
> > > + up(&match->notif->request);
> > > + }
> > > + mutex_unlock(&match->notify_lock);
> > > + err = wait_for_completion_killable(&n.ready);
> > > + mutex_lock(&match->notify_lock);
> > > + if (err < 0)
> > > + goto remove_list;
> > > + }
> > > +
> > > + ret = n.val;
> > > + err = n.error;
> > > +
> > > +remove_list:
> > > + list_del(&n.list);
> > > +out:
> > > + mutex_unlock(&match->notify_lock);
> > > + syscall_set_return_value(current, task_pt_regs(current),
> > > + err, ret);
> > > +}
> > > +
> > > static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> > > const bool recheck_after_trace)
> > > {
> > > @@ -728,6 +869,9 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> > >
> > > return 0;
> > >
> > > + case SECCOMP_RET_USER_NOTIF:
> > > + seccomp_do_user_notification(this_syscall, match, sd);
> > > + goto skip;
> >
> > Nit: please add a blank line here (to match the other cases).
> >
> > > case SECCOMP_RET_LOG:
> > > seccomp_log(this_syscall, 0, action, true);
> > > return 0;
> > > @@ -834,6 +978,9 @@ static long seccomp_set_mode_strict(void)
> > > }
> > >
> > > #ifdef CONFIG_SECCOMP_FILTER
> > > +static struct file *init_listener(struct task_struct *,
> > > + struct seccomp_filter *);
> >
> > Why is the forward declaration needed instead of just moving the
> > function here? I didn't see anything in it that looked like it
> > couldn't move.
>
> I think there was a cycle in some earlier version, but I agree there
> isn't now. I'll fix it.
>
> > > +
> > > /**
> > > * seccomp_set_mode_filter: internal function for setting seccomp filter
> > > * @flags: flags to change filter behavior
> > > @@ -853,6 +1000,8 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > > const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
> > > struct seccomp_filter *prepared = NULL;
> > > long ret = -EINVAL;
> > > + int listener = 0;
> >
> > Nit: "invalid fd" should be -1, not 0.
> >
> > > + struct file *listener_f = NULL;
> > >
> > > /* Validate flags. */
> > > if (flags & ~SECCOMP_FILTER_FLAG_MASK)
> > > @@ -863,13 +1012,28 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > > if (IS_ERR(prepared))
> > > return PTR_ERR(prepared);
> > >
> > > + if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> > > + listener = get_unused_fd_flags(0);
> >
> > As with the other place pointed out by Jann, this should maybe be O_CLOEXEC too?
>
> Yep, will do.
>
> > > + if (listener < 0) {
> > > + ret = listener;
> > > + goto out_free;
> > > + }
> > > +
> > > + listener_f = init_listener(current, prepared);
> > > + if (IS_ERR(listener_f)) {
> > > + put_unused_fd(listener);
> > > + ret = PTR_ERR(listener_f);
> > > + goto out_free;
> > > + }
> > > + }
> > > +
> > > /*
> > > * Make sure we cannot change seccomp or nnp state via TSYNC
> > > * while another thread is in the middle of calling exec.
> > > */
> > > if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
> > > mutex_lock_killable(¤t->signal->cred_guard_mutex))
> > > - goto out_free;
> > > + goto out_put_fd;
> > >
> > > spin_lock_irq(¤t->sighand->siglock);
> > >
> > > @@ -887,6 +1051,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > > spin_unlock_irq(¤t->sighand->siglock);
> > > if (flags & SECCOMP_FILTER_FLAG_TSYNC)
> > > mutex_unlock(¤t->signal->cred_guard_mutex);
> > > +out_put_fd:
> > > + if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> > > + if (ret < 0) {
> > > + fput(listener_f);
> > > + put_unused_fd(listener);
> > > + } else {
> > > + fd_install(listener, listener_f);
> > > + ret = listener;
> > > + }
> > > + }
> >
> > Can you update the kern-docs for seccomp_set_mode_filter(), since we
> > can now return positive values?
> >
> > * Returns 0 on success or -EINVAL on failure.
> >
> > (this shoudln't say only -EINVAL, I realize too)
>
> Sure, I can fix both of these.
>
> > I have to say, I'm vaguely nervous about changing the semantics here
> > for passing back the fd as the return code from the seccomp() syscall.
> > Alternatives seem less appealing, though: changing the meaning of the
> > uargs parameter when SECCOMP_FILTER_FLAG_NEW_LISTENER is set, for
> > example. Hmm.
>
> From my perspective we can drop this whole thing. The only thing I'll
> ever use is the ptrace version. Someone at some point (I don't
> remember who, maybe stgraber) suggested this version would be useful
> as well.
So I think we want to have the ability to get an fd via seccomp().
Especially, if we all we worry about are weird semantics. When we
discussed this we knew the whole patchset was going to be weird. :)
This is a seccomp feature so seccomp should - if feasible - equip you
with everything to use it in a meaningful way without having to go
through a different kernel api. I know ptrace and seccomp are
already connected but I still find this cleaner. :)
Another thing is that the container itself might be traced for some
reason while you still might want to get an fd out.
Also, I wonder what happens if you want to filter the ptrace() syscall
itself? Then you'd deadlock?
Also, it seems that getting an fd via ptrace requires CAP_SYS_ADMIN in
the inital user namespace (which I just realized now) whereas getting
the fd via seccomp() doesn't seem to.
>
> Anyway, let me know if your nervousness outweighs this, I'm happy to
> drop it.
>
> > > @@ -1342,3 +1520,259 @@ static int __init seccomp_sysctl_init(void)
> > > device_initcall(seccomp_sysctl_init)
> > >
> > > #endif /* CONFIG_SYSCTL */
> > > +
> > > +#ifdef CONFIG_SECCOMP_FILTER
> > > +static int seccomp_notify_release(struct inode *inode, struct file *file)
> > > +{
> > > + struct seccomp_filter *filter = file->private_data;
> > > + struct seccomp_knotif *knotif;
> > > +
> > > + mutex_lock(&filter->notify_lock);
> > > +
> > > + /*
> > > + * If this file is being closed because e.g. the task who owned it
> > > + * died, let's wake everyone up who was waiting on us.
> > > + */
> > > + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> > > + if (knotif->state == SECCOMP_NOTIFY_REPLIED)
> > > + continue;
> > > +
> > > + knotif->state = SECCOMP_NOTIFY_REPLIED;
> > > + knotif->error = -ENOSYS;
> > > + knotif->val = 0;
> > > +
> > > + complete(&knotif->ready);
> > > + }
> > > +
> > > + wake_up_all(&filter->notif->wqh);
> > > + kfree(filter->notif);
> > > + filter->notif = NULL;
> > > + mutex_unlock(&filter->notify_lock);
> >
> > It looks like that means nothing waiting on knotif->ready can access
> > filter->notif without rechecking it, yes?
> >
> > e.g. in seccomp_do_user_notification() I see:
> >
> > up(&match->notif->request);
> >
> > I *think* this isn't reachable due to the test for n.state !=
> > SECCOMP_NOTIFY_REPLIED, though. Perhaps, just for sanity and because
> > it's not fast-path, we could add a WARN_ON() while checking for
> > unreplied signal death?
> >
> > n.signaled = true;
> > if (n.state == SECCOMP_NOTIFY_SENT) {
> > n.state = SECCOMP_NOTIFY_INIT;
> > if (!WARN_ON(match->notif))
> > up(&match->notif->request);
> > }
> > mutex_unlock(&match->notify_lock);
>
> So this code path should actually be safe, since notify_lock is held
> throughout, as it is in the release handler. However, there is one just above
> it that is not, because we do:
>
> mutex_unlock(&match->notify_lock);
> up(&match->notif->request);
>
> When this was all a member of struct seccomp_filter the order didn't matter,
> but now it very much does, and I think you're right that these statements need
> to be reordered. There maybe others, I'll check everything else as well.
>
> >
> > > + __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;
> >
> > I'd prefer this casting happen in seccomp_notify_ioctl(). This keeps
> > anything from accidentally using "arg" directly here.
>
> Will do.
>
> > > +
> > > + if (copy_from_user(&size, buf, sizeof(size)))
> > > + return -EFAULT;
> > > +
> > > + ret = down_interruptible(&filter->notif->request);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + mutex_lock(&filter->notify_lock);
> > > + list_for_each_entry(cur, &filter->notif->notifications, list) {
> > > + if (cur->state == SECCOMP_NOTIFY_INIT) {
> > > + knotif = cur;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /*
> > > + * If we didn't find a notification, it could be that the task was
> > > + * interrupted between the time we were woken and when we were able to
> > > + * acquire the rw lock.
> > > + */
> > > + if (!knotif) {
> > > + ret = -ENOENT;
> > > + goto out;
> > > + }
> > > +
> > > + size = min_t(size_t, size, sizeof(unotif));
> > > +
> >
> > It is possible (though unlikely given the type widths involved here)
> > for unotif = {} to not initialize padding, so I would recommend an
> > explicit memset(&unotif, 0, sizeof(unotif)) here.
>
> Orly? I didn't know that, thanks.
>
> > > + unotif.len = size;
> > > + unotif.id = knotif->id;
> > > + unotif.pid = task_pid_vnr(knotif->task);
> > > + unotif.signaled = knotif->signaled;
> > > + unotif.data = *(knotif->data);
> > > +
> > > + if (copy_to_user(buf, &unotif, size)) {
> > > + ret = -EFAULT;
> > > + goto out;
> > > + }
> > > +
> > > + ret = size;
> > > + knotif->state = SECCOMP_NOTIFY_SENT;
> > > + wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
> > > +
> > > +
> > > +out:
> > > + mutex_unlock(&filter->notify_lock);
> >
> > Is there some way to rearrange the locking here to avoid holding the
> > mutex while doing copy_to_user() (which userspace could block with
> > userfaultfd, and then stall all the other notifications for this
> > filter)?
>
> Yes, I don't think it'll cause any problems to release the lock earlier.
>
> > > + 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;
> >
> > Same cast note as above.
> >
> > > +
> > > + 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;
> >
> > For sanity checking on a double-read from userspace, please add:
> >
> > if (resp.len != size)
> > return -EINVAL;
>
> Won't that fail if sizeof(resp) < resp.len, because of the min_t()?
>
> > > +static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> > > + unsigned long arg)
> > > +{
> > > + struct seccomp_knotif *knotif = NULL;
> > > + void __user *buf = (void __user *)arg;
> > > + u64 id;
> > > + long ret;
> > > +
> > > + if (copy_from_user(&id, buf, sizeof(id)))
> > > + return -EFAULT;
> > > +
> > > + ret = mutex_lock_interruptible(&filter->notify_lock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = -1;
> >
> > Isn't this EPERM? Shouldn't it be -ENOENT?
>
> Yes, I wasn't thinking of errno here, I'll switch it.
>
> > > + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> > > + if (knotif->id == id) {
> > > + ret = 0;
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > +out:
> > > + mutex_unlock(&filter->notify_lock);
> > > + return ret;
> > > +}
> > > +
> > > +static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long arg)
> > > +{
> > > + struct seccomp_filter *filter = file->private_data;
> > > +
> > > + switch (cmd) {
> > > + case SECCOMP_NOTIF_RECV:
> > > + return seccomp_notify_recv(filter, arg);
> > > + case SECCOMP_NOTIF_SEND:
> > > + return seccomp_notify_send(filter, arg);
> > > + case SECCOMP_NOTIF_ID_VALID:
> > > + return seccomp_notify_id_valid(filter, arg);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static __poll_t seccomp_notify_poll(struct file *file,
> > > + struct poll_table_struct *poll_tab)
> > > +{
> > > + struct seccomp_filter *filter = file->private_data;
> > > + __poll_t ret = 0;
> > > + struct seccomp_knotif *cur;
> > > +
> > > + poll_wait(file, &filter->notif->wqh, poll_tab);
> > > +
> > > + ret = mutex_lock_interruptible(&filter->notify_lock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + list_for_each_entry(cur, &filter->notif->notifications, list) {
> > > + if (cur->state == SECCOMP_NOTIFY_INIT)
> > > + ret |= EPOLLIN | EPOLLRDNORM;
> > > + if (cur->state == SECCOMP_NOTIFY_SENT)
> > > + ret |= EPOLLOUT | EPOLLWRNORM;
> > > + if (ret & EPOLLIN && ret & EPOLLOUT)
> >
> > My eyes! :) Can you wrap the bit operations in parens here?
> >
> > > + break;
> > > + }
> >
> > Should POLLERR be handled here too? I don't quite see the conditions
> > that might be exposed? All the processes die for the filter, which
> > does what here?
>
> I think it shouldn't do anything, because I was thinking of the semantics of
> poll() as "when a tracee does a syscall that matches, fire". So a task could
> start, never make a targeted syscall, and exit, and poll() shouldn't return a
> value. Maybe it's useful to write that down somewhere, though.
>
> > > + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_RECV, &req), sizeof(req));
> > > + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_ID_VALID, &req.id), 0);
> > > +
> > > + EXPECT_EQ(kill(pid, SIGKILL), 0);
> > > + EXPECT_EQ(waitpid(pid, NULL, 0), pid);
> > > +
> > > + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_ID_VALID, &req.id), -1);
> >
> > Please document SECCOMP_NOTIF_ID_VALID in seccomp_filter.rst. I had
> > been wondering what it's for, and now I see it's kind of an advisory
> > "is the other end still alive?" test.
>
> Yes, in fact it's necessary for avoiding races. There's some comments in the
> sample code, but I'll update seccomp_filter.rst too.
>
> > > +
> > > + resp.id = req.id;
> > > + ret = ioctl(listener, SECCOMP_NOTIF_SEND, &resp);
> > > + EXPECT_EQ(ret, -1);
> > > + EXPECT_EQ(errno, ENOENT);
> > > +
> > > + /*
> > > + * Check that we get another notification about a signal in the middle
> > > + * of a syscall.
> > > + */
> > > + pid = fork();
> > > + ASSERT_GE(pid, 0);
> > > +
> > > + if (pid == 0) {
> > > + if (signal(SIGUSR1, signal_handler) == SIG_ERR) {
> > > + perror("signal");
> > > + exit(1);
> > > + }
> > > + ret = syscall(__NR_getpid);
> > > + exit(ret != USER_NOTIF_MAGIC);
> > > + }
> > > +
> > > + ret = read_notif(listener, &req);
> > > + EXPECT_EQ(ret, sizeof(req));
> > > + EXPECT_EQ(errno, 0);
> > > +
> > > + EXPECT_EQ(kill(pid, SIGUSR1), 0);
> > > +
> > > + ret = read_notif(listener, &req);
> > > + EXPECT_EQ(req.signaled, 1);
> > > + EXPECT_EQ(ret, sizeof(req));
> > > + EXPECT_EQ(errno, 0);
> > > +
> > > + resp.len = sizeof(resp);
> > > + resp.id = req.id;
> > > + resp.error = -512; /* -ERESTARTSYS */
> > > + resp.val = 0;
> > > +
> > > + EXPECT_EQ(ioctl(listener, SECCOMP_NOTIF_SEND, &resp), sizeof(resp));
> > > +
> > > + ret = read_notif(listener, &req);
> > > + resp.len = sizeof(resp);
> > > + resp.id = req.id;
> > > + resp.error = 0;
> > > + resp.val = USER_NOTIF_MAGIC;
> > > + ret = ioctl(listener, SECCOMP_NOTIF_SEND, &resp);
> >
> > I was slightly confused here: why have there been 3 reads? I was
> > expecting one notification for hitting getpid and one from catching a
> > signal. But in rereading, I see that NOTIF_RECV will return the most
> > recently unresponded notification, yes?
>
> The three reads are:
>
> 1. original syscall
> # send SIGUSR1
> 2. another notif with signaled is set
> # respond with -ERESTARTSYS to make sure that works
> 3. this is the result of -ERESTARTSYS
>
> > But... catching a signal replaces the existing seccomp_knotif? I
> > remain confused about how signal handling is meant to work here. What
> > happens if two signals get sent? It looks like you just block without
> > allowing more signals? (Thank you for writing the tests!)
>
> Yes, that's the idea. This is an implementation of Andy's pseudocode:
> https://lkml.org/lkml/2018/3/15/1122
>
> > (And can you document the expected behavior in the seccomp_filter.rst too?)
>
> Will do.
>
> >
> > Looking good!
>
> Thanks for your review!
>
> Tycho
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH v7 2/6] seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE
From: Christian Brauner @ 2018-10-08 13:55 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, linux-api, containers, Akihiro Suda,
Oleg Nesterov, linux-kernel, Eric W . Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski
In-Reply-To: <20180927151119.9989-3-tycho@tycho.ws>
On Thu, Sep 27, 2018 at 09:11:15AM -0600, Tycho Andersen wrote:
> In the next commit we'll use this same mnemonic to get a listener for the
> nth filter, so we need it available outside of CHECKPOINT_RESTORE in the
> USER_NOTIFICATION case as well.
>
> v2: new in v2
> v3: no changes
> v4: no changes
> v5: switch to CHECKPOINT_RESTORE || USER_NOTIFICATION to avoid warning when
> only CONFIG_SECCOMP_FILTER is enabled.
> v7: drop USER_NOTIFICATION bits
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> CC: Christian Brauner <christian.brauner@ubuntu.com>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> kernel/seccomp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index fa6fe9756c80..44a31ac8373a 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1158,7 +1158,7 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> return do_seccomp(op, 0, uargs);
> }
>
> -#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> +#if defined(CONFIG_SECCOMP_FILTER)
> static struct seccomp_filter *get_nth_filter(struct task_struct *task,
> unsigned long filter_off)
> {
> @@ -1205,6 +1205,7 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
> return filter;
> }
>
> +#if defined(CONFIG_CHECKPOINT_RESTORE)
> long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> void __user *data)
> {
> @@ -1277,7 +1278,8 @@ long seccomp_get_metadata(struct task_struct *task,
> __put_seccomp_filter(filter);
> return ret;
> }
> -#endif
> +#endif /* CONFIG_CHECKPOINT_RESTORE */
> +#endif /* CONFIG_SECCOMP_FILTER */
>
> #ifdef CONFIG_SYSCTL
>
> --
> 2.17.1
>
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-08 11:26 UTC (permalink / raw)
To: Laurent Vivier
Cc: kernel list, avagin, linux-fsdevel, Eric W. Biederman, Linux API,
dima, containers, Al Viro, James Bottomley
In-Reply-To: <20181006193546.29340-2-laurent@vivier.eu>
On Sat, Oct 6, 2018 at 9:36 PM Laurent Vivier <laurent@vivier.eu> wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
[...]
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + while (ns) {
> + if (ns->binfmt_ns)
> + return ns->binfmt_ns;
> + ns = ns->parent;
> + }
> + return NULL;
> +}
If the value being read can change under you, please use READ_ONCE().
Also: That "return NULL" can never happen, right? You should probably
at least put a WARN(...) in there.
[...]
> @@ -838,7 +858,29 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (ns->binfmt_ns == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return ERR_PTR(-ENOMEM);
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + ns->binfmt_ns = new_ns;
What happens if someone mounts two instances of the binfmt_misc
filesystem at the same time? Would you end up creating two binfmt
namespaces, one of which would never be freed again?
> + }
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
[...]
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..da4950282ea1 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
> if (!setup_userns_sysctls(ns))
> goto fail_keyring;
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + ns->binfmt_ns = NULL;
> +#endif
Isn't this unnecessary? The namespace is allocated with all fields zeroed:
ns = kmem_cache_zalloc(user_ns_cachep, GFP_KERNEL);
^ permalink raw reply
* Re: [RFC v3 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-08 10:58 UTC (permalink / raw)
To: avagin
Cc: Laurent Vivier, kernel list, Andrei Vagin, Linux API, containers,
dima, James Bottomley, Al Viro, linux-fsdevel, Eric W. Biederman
In-Reply-To: <20181006060427.GA15164@gmail.com>
On Sat, Oct 6, 2018 at 8:04 AM Andrei Vagin <avagin@gmail.com> wrote:
> On Thu, Oct 04, 2018 at 12:50:22AM +0200, Laurent Vivier wrote:
> > This patch allows to have a different binfmt_misc configuration
> > for each new user namespace. By default, the binfmt_misc configuration
> > is the one of the host, but if the binfmt_misc filesystem is mounted
> > in the new namespace a new empty binfmt instance is created and used
> > in this namespace.
> >
> > For instance, using "unshare" we can start a chroot of an another
> > architecture and configure the binfmt_misc interpreter without being root
> > to run the binaries in this chroot.
> >
> > Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> > fs/binfmt_misc.c | 85 +++++++++++++++++++++++-----------
> > include/linux/user_namespace.h | 15 ++++++
> > kernel/user.c | 14 ++++++
> > kernel/user_namespace.c | 9 ++++
> > 4 files changed, 95 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> > index aa4a7a23ff99..78780bc87506 100644
> > --- a/fs/binfmt_misc.c
> > +++ b/fs/binfmt_misc.c
> > @@ -38,9 +38,6 @@ enum {
> > VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> > };
> >
> > -static LIST_HEAD(entries);
> > -static int enabled = 1;
> > -
> > enum {Enabled, Magic};
> > #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> > #define MISC_FMT_OPEN_BINARY (1 << 30)
> > @@ -60,10 +57,7 @@ typedef struct {
> > struct file *interp_file;
> > } Node;
> >
> > -static DEFINE_RWLOCK(entries_lock);
> > static struct file_system_type bm_fs_type;
> > -static struct vfsmount *bm_mnt;
> > -static int entry_count;
> >
> > /*
> > * Max length of the register string. Determined by:
> > @@ -85,13 +79,13 @@ static int entry_count;
> > * if we do, return the node, else NULL
> > * locking is done in load_misc_binary
> > */
> > -static Node *check_file(struct linux_binprm *bprm)
> > +static Node *check_file(struct user_namespace *ns, struct linux_binprm *bprm)
> > {
> > char *p = strrchr(bprm->interp, '.');
> > struct list_head *l;
> >
> > /* Walk all the registered handlers. */
> > - list_for_each(l, &entries) {
> > + list_for_each(l, &ns->binfmt_ns->entries) {
> > Node *e = list_entry(l, Node, list);
> > char *s;
> > int j;
> > @@ -133,17 +127,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> > struct file *interp_file = NULL;
> > int retval;
> > int fd_binary = -1;
> > + struct user_namespace *ns = current_user_ns();
> >
> > retval = -ENOEXEC;
> > - if (!enabled)
> > + if (!ns->binfmt_ns->enabled)
> > return retval;
> >
> > /* to keep locking time low, we copy the interpreter string */
> > - read_lock(&entries_lock);
> > - fmt = check_file(bprm);
> > + read_lock(&ns->binfmt_ns->entries_lock);
>
> It looks like ns->binfmt_ns isn't protected by any lock and
> ns->binfmt_ns can be changed between read_lock() and read_unlock().
>
> This can be fixed if ns->binfmt_ns will be dereferenced only once in
> this function:
>
> struct binfmt_namespace *binfmt_ns = ns->binfmt_ns;
Technically, wouldn't you want READ_ONCE(ns->binfmt_ns)?
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Jann Horn @ 2018-10-08 10:50 UTC (permalink / raw)
To: cyphar
Cc: Eric W. Biederman, Al Viro, jlayton, Bruce Fields, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <20181006021002.6vzsdwd3klddbmji@ryuk>
On Sat, Oct 6, 2018 at 4:10 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-10-05, Jann Horn <jannh@google.com> wrote:
> > > What if we took rename_lock (call it nd->r_seq) at the start of the
> > > resolution, and then only tried the __d_path-style check
> > >
> > > if (read_seqretry(&rename_lock, nd->r_seq) ||
> > > read_seqretry(&mount_lock, nd->m_seq))
> > > /* do the __d_path lookup. */
> > >
> > > That way you would only hit the slow path if there were concurrent
> > > renames or mounts *and* you are doing a path resolution with
> > > AT_THIS_ROOT or AT_BENEATH. I've attached a modified patch that does
> > > this (and after some testing it also appears to work).
> >
> > Yeah, I think that might do the job.
>
> *phew* I was all out of other ideas. :P
>
> > > ---
> > > fs/namei.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 46 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 6f995e6de6b1..12c9be175cb4 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -493,7 +493,7 @@ struct nameidata {
> > > struct path root;
> > > struct inode *inode; /* path.dentry.d_inode */
> > > unsigned int flags;
> > > - unsigned seq, m_seq;
> > > + unsigned seq, m_seq, r_seq;
> > > int last_type;
> > > unsigned depth;
> > > int total_link_count;
> > > @@ -1375,6 +1375,27 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > > return -EXDEV;
> > > break;
> > > }
> > > + if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
> > > + (read_seqretry(&rename_lock, nd->r_seq) ||
> > > + read_seqretry(&mount_lock, nd->m_seq)))) {
> > > + char *pathbuf, *pathptr;
> > > +
> > > + nd->r_seq = read_seqbegin(&rename_lock);
> > > + /* Cannot take m_seq here. */
> > > +
> > > + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> > > + if (!pathbuf)
> > > + return -ECHILD;
> > > + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> > > + kfree(pathbuf);
> >
> > You're doing this check before actually looking up the parent, right?
> > So as long as I don't trigger the "path_equal(&nd->path, &nd->root)"
> > check that you do for O_BENEATH, escaping up by one level is possible,
> > right? You should probably move this check so that it happens after
> > following "..".
>
> Yup, you're right. I'll do that.
>
> > (Also: I assume that you're going to get rid of that memory allocation
> > in a future version.)
>
> Sure. Would you prefer adding some scratch space in nameidata, or that I
> change __d_path so it accepts NULL as the buffer (and thus it doesn't
> actually do any string operations)?
Well, I think accepting a NULL buffer would be much cleaner; but keep
in mind that I'm just someone making suggestions, Al Viro is the one
who has to like your code. :P
> > > if (nd->path.dentry != nd->path.mnt->mnt_root) {
> > > int ret = path_parent_directory(&nd->path);
> > > if (ret)
> > > @@ -2269,6 +2311,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> > > nd->last_type = LAST_ROOT; /* if there are only slashes... */
> > > nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
> > > nd->depth = 0;
> > > + nd->m_seq = read_seqbegin(&mount_lock);
> > > + nd->r_seq = read_seqbegin(&rename_lock);
> >
> > This means that now, attempting to perform a lookup while something is
> > holding the rename_lock will spin on the lock. I don't know whether
> > that's a problem in practice though. Does anyone on this thread know
> > whether this is problematic?
>
> I could make it so that we only take &rename_lock
> if (nd->flags & (FOLLOW_BENEATH | FOLLOW_CHROOT)),
> since it's not used outside of that path.
I think that might be a sensible change; but as I said, I don't
actually know whether it's necessary, and it would be very helpful if
someone who actually knows commented on this.
^ permalink raw reply
* [PATCH v2 3/3] tools: add selftest for BPF_F_ZERO_SEED
From: Lorenz Bauer @ 2018-10-08 10:32 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181008103221.13468-1-lmb@cloudflare.com>
Check that iterating two separate hash maps produces the same
order of keys if BPF_F_ZERO_SEED is used.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
1 file changed, 57 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 9b552c0fc47d..a8d6af27803a 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -257,23 +257,35 @@ static void test_hashmap_percpu(int task, void *data)
close(fd);
}
+static int helper_fill_hashmap(int max_entries)
+{
+ int i, fd, ret;
+ long long key, value;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+ max_entries, map_flags);
+ CHECK(fd < 0,
+ "failed to create hashmap",
+ "err: %s, flags: 0x%x\n", strerror(errno), map_flags);
+
+ for (i = 0; i < max_entries; i++) {
+ key = i; value = key;
+ ret = bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST);
+ CHECK(ret != 0,
+ "can't update hashmap",
+ "err: %s\n", strerror(ret));
+ }
+
+ return fd;
+}
+
static void test_hashmap_walk(int task, void *data)
{
int fd, i, max_entries = 1000;
long long key, value, next_key;
bool next_key_valid = true;
- fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
- max_entries, map_flags);
- if (fd < 0) {
- printf("Failed to create hashmap '%s'!\n", strerror(errno));
- exit(1);
- }
-
- for (i = 0; i < max_entries; i++) {
- key = i; value = key;
- assert(bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST) == 0);
- }
+ fd = helper_fill_hashmap(max_entries);
for (i = 0; bpf_map_get_next_key(fd, !i ? NULL : &key,
&next_key) == 0; i++) {
@@ -305,6 +317,39 @@ static void test_hashmap_walk(int task, void *data)
close(fd);
}
+static void test_hashmap_zero_seed(void)
+{
+ int i, first, second, old_flags;
+ long long key, next_first, next_second;
+
+ old_flags = map_flags;
+ map_flags |= BPF_F_ZERO_SEED;
+
+ first = helper_fill_hashmap(3);
+ second = helper_fill_hashmap(3);
+
+ for (i = 0; ; i++) {
+ void *key_ptr = !i ? NULL : &key;
+
+ if (bpf_map_get_next_key(first, key_ptr, &next_first) != 0)
+ break;
+
+ CHECK(bpf_map_get_next_key(second, key_ptr, &next_second) != 0,
+ "next_key for second map must succeed",
+ "key_ptr: %p", key_ptr);
+ CHECK(next_first != next_second,
+ "keys must match",
+ "i: %d first: %lld second: %lld\n", i,
+ next_first, next_second);
+
+ key = next_first;
+ }
+
+ map_flags = old_flags;
+ close(first);
+ close(second);
+}
+
static void test_arraymap(int task, void *data)
{
int key, next_key, fd;
@@ -1417,6 +1462,7 @@ static void run_all_tests(void)
test_hashmap(0, NULL);
test_hashmap_percpu(0, NULL);
test_hashmap_walk(0, NULL);
+ test_hashmap_zero_seed();
test_arraymap(0, NULL);
test_arraymap_percpu(0, NULL);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] tools: sync linux/bpf.h
From: Lorenz Bauer @ 2018-10-08 10:32 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181008103221.13468-1-lmb@cloudflare.com>
Synchronize changes to linux/bpf.h from
commit 88db241b34bf ("bpf: allow zero-initializing hash map seed").
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f9187b41dff6..2c121f862082 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -253,6 +253,8 @@ enum bpf_attach_type {
#define BPF_F_NO_COMMON_LRU (1U << 1)
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
+/* Zero-initialize hash function seed. This should only be used for testing. */
+#define BPF_F_ZERO_SEED (1U << 6)
/* flags for BPF_PROG_QUERY */
#define BPF_F_QUERY_EFFECTIVE (1U << 0)
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-08 10:32 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181008103221.13468-1-lmb@cloudflare.com>
Add a new flag BPF_F_ZERO_SEED, which forces a hash map
to initialize the seed to zero. This is useful when doing
performance analysis both on individual BPF programs, as
well as the kernel's hash table implementation.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/hashtab.c | 13 +++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f9187b41dff6..2c121f862082 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -253,6 +253,8 @@ enum bpf_attach_type {
#define BPF_F_NO_COMMON_LRU (1U << 1)
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
+/* Zero-initialize hash function seed. This should only be used for testing. */
+#define BPF_F_ZERO_SEED (1U << 6)
/* flags for BPF_PROG_QUERY */
#define BPF_F_QUERY_EFFECTIVE (1U << 0)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 2c1790288138..4b7c76765d9d 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -23,7 +23,7 @@
#define HTAB_CREATE_FLAG_MASK \
(BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
- BPF_F_RDONLY | BPF_F_WRONLY)
+ BPF_F_RDONLY | BPF_F_WRONLY | BPF_F_ZERO_SEED)
struct bucket {
struct hlist_nulls_head head;
@@ -244,6 +244,7 @@ static int htab_map_alloc_check(union bpf_attr *attr)
*/
bool percpu_lru = (attr->map_flags & BPF_F_NO_COMMON_LRU);
bool prealloc = !(attr->map_flags & BPF_F_NO_PREALLOC);
+ bool zero_seed = (attr->map_flags & BPF_F_ZERO_SEED);
int numa_node = bpf_map_attr_numa_node(attr);
BUILD_BUG_ON(offsetof(struct htab_elem, htab) !=
@@ -257,6 +258,10 @@ static int htab_map_alloc_check(union bpf_attr *attr)
*/
return -EPERM;
+ if (zero_seed && !capable(CAP_SYS_ADMIN))
+ /* Guard against local DoS, and discourage production use. */
+ return -EPERM;
+
if (attr->map_flags & ~HTAB_CREATE_FLAG_MASK)
/* reserved bits should not be used */
return -EINVAL;
@@ -373,7 +378,11 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
if (!htab->buckets)
goto free_htab;
- htab->hashrnd = get_random_int();
+ if (htab->map.map_flags & BPF_F_ZERO_SEED)
+ htab->hashrnd = 0;
+ else
+ htab->hashrnd = get_random_int();
+
for (i = 0; i < htab->n_buckets; i++) {
INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
raw_spin_lock_init(&htab->buckets[i].lock);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-10-08 10:32 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
Allow forcing the seed of a hash table to zero, for deterministic
execution during benchmarking and testing.
Comments adressed from v1:
* Add comment to discourage production use to linux/bpf.h
* Require CAP_SYS_ADMIN
Lorenz Bauer (3):
bpf: allow zero-initializing hash map seed
tools: sync linux/bpf.h
tools: add selftest for BPF_F_ZERO_SEED
include/uapi/linux/bpf.h | 2 +
kernel/bpf/hashtab.c | 13 ++++-
tools/include/uapi/linux/bpf.h | 2 +
tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
4 files changed, 72 insertions(+), 13 deletions(-)
--
2.17.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox