* [PATCH v12 09/12] namei: LOOKUP_IN_ROOT: chroot-like path resolution
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
The primary motivation for the need for this flag is container runtimes
which have to interact with malicious root filesystems in the host
namespaces. One of the first requirements for a container runtime to be
secure against a malicious rootfs is that they correctly scope symlinks
(that is, they should be scoped as though they are chroot(2)ed into the
container's rootfs) and ".."-style paths[*]. The already-existing
LOOKUP_NO_XDEV and LOOKUP_NO_MAGICLINKS help defend against other
potential attacks in a malicious rootfs scenario.
Currently most container runtimes try to do this resolution in
userspace[1], causing many potential race conditions. In addition, the
"obvious" alternative (actually performing a {ch,pivot_}root(2))
requires a fork+exec (for some runtimes) which is *very* costly if
necessary for every filesystem operation involving a container.
[*] At the moment, ".." and magic-link jumping are disallowed for the
same reason it is disabled for LOOKUP_BENEATH -- currently it is not
safe to allow it. Future patches may enable it unconditionally once
we have resolved the possible races (for "..") and semantics (for
magic-link jumping).
The most significant *at(2) semantic change with LOOKUP_IN_ROOT is that
absolute pathnames no longer cause the dirfd to be ignored completely.
The rationale is that LOOKUP_IN_ROOT must necessarily chroot-scope
symlinks with absolute paths to dirfd, and so doing it for the base path
seems to be the most consistent behaviour (and also avoids foot-gunning
users who want to scope paths that are absolute).
[1]: https://github.com/cyphar/filepath-securejoin
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 41 +++++++++++++++++++++++++++++++----------
include/linux/namei.h | 1 +
2 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 2e18ce5a313e..0352d275bd13 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -676,7 +676,7 @@ static int unlazy_walk(struct nameidata *nd)
goto out1;
if (!nd->root.mnt) {
/* Restart from path_init() if nd->root was cleared. */
- if (nd->flags & LOOKUP_BENEATH)
+ if (nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))
goto out;
} else if (!(nd->flags & LOOKUP_ROOT)) {
if (unlikely(!legitimize_path(nd, &nd->root, nd->root_seq)))
@@ -809,10 +809,18 @@ static int complete_walk(struct nameidata *nd)
return status;
}
-static void set_root(struct nameidata *nd)
+static int set_root(struct nameidata *nd)
{
struct fs_struct *fs = current->fs;
+ /*
+ * Jumping to the real root as part of LOOKUP_IN_ROOT is a BUG in namei,
+ * but we still have to ensure it doesn't happen because it will cause a
+ * breakout from the dirfd.
+ */
+ if (WARN_ON(nd->flags & LOOKUP_IN_ROOT))
+ return -ENOTRECOVERABLE;
+
if (nd->flags & LOOKUP_RCU) {
unsigned seq;
@@ -824,6 +832,7 @@ static void set_root(struct nameidata *nd)
} else {
get_fs_root(fs, &nd->root);
}
+ return 0;
}
static void path_put_conditional(struct path *path, struct nameidata *nd)
@@ -854,6 +863,11 @@ static int nd_jump_root(struct nameidata *nd)
if (nd->path.mnt != NULL && nd->path.mnt != nd->root.mnt)
return -EXDEV;
}
+ if (!nd->root.mnt) {
+ int error = set_root(nd);
+ if (error)
+ return error;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1100,15 +1114,13 @@ const char *get_link(struct nameidata *nd)
if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
return ERR_PTR(-ELOOP);
/* Not currently safe. */
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return ERR_PTR(-EXDEV);
}
if (IS_ERR_OR_NULL(res))
return res;
}
if (*res == '/') {
- if (!nd->root.mnt)
- set_root(nd);
error = nd_jump_root(nd);
if (unlikely(error))
return ERR_PTR(error);
@@ -1744,15 +1756,20 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ int error = 0;
+
/*
* LOOKUP_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))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return -EXDEV;
- if (!nd->root.mnt)
- set_root(nd);
+ if (!nd->root.mnt) {
+ error = set_root(nd);
+ if (error)
+ return error;
+ }
if (nd->flags & LOOKUP_RCU) {
return follow_dotdot_rcu(nd);
} else
@@ -2251,9 +2268,13 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
+ /* LOOKUP_IN_ROOT treats absolute paths as being relative-to-dirfd. */
+ if (flags & LOOKUP_IN_ROOT)
+ while (*s == '/')
+ s++;
+
/* Figure out the starting path and root (if needed). */
if (*s == '/') {
- set_root(nd);
error = nd_jump_root(nd);
if (unlikely(error))
return ERR_PTR(error);
@@ -2298,7 +2319,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
fdput(f);
}
/* For scoped-lookups we need to set the root to the dirfd as well. */
- if (flags & LOOKUP_BENEATH) {
+ if (flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)) {
nd->root = nd->path;
if (flags & LOOKUP_RCU)
nd->root_seq = nd->seq;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index be407415c28a..ec2c6c588ea7 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -57,6 +57,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_NO_MAGICLINKS 0x080000 /* No /proc/$pid/fd/ "symlink" crossing. */
#define LOOKUP_NO_SYMLINKS 0x100000 /* No symlink crossing *at all*.
Implies LOOKUP_NO_MAGICLINKS. */
+#define LOOKUP_IN_ROOT 0x200000 /* Treat dirfd as %current->fs->root. */
extern int path_pts(struct path *path);
--
2.23.0
^ permalink raw reply related
* [PATCH v12 08/12] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, David Drysdale, Andy Lutomirski, Linus Torvalds,
Eric Biederman, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Rasmus Villemoes, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Aleksa Sarai, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-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 LOOKUP_FOLLOW).
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.
These flags are exposed to userspace through openat2(2) in a later
patchset.
* LOOKUP_NO_XDEV: Disallow mount-point crossing (both *down* into one,
or *up* from one). Both bind-mounts and cross-filesystem mounts are
blocked by this flag. The naming is based on "find -xdev" as well as
-EXDEV (though find(1) doesn't walk upwards, the semantics seem
obvious).
* LOOKUP_NO_MAGICLINKS: Disallows ->get_link "symlink" (or rather,
magic-link) 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).
* LOOKUP_NO_SYMLINKS: Disallows resolution through symlinks of any kind
(including magic-links).
* LOOKUP_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 magic-link 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 magic-link jumping is done because it is not clear
whether semantically they should be allowed -- while some magic-links
are safe there are many that can cause escapes (and once a
resolution is outside of the root, O_BENEATH will no longer detect
it). Future patches may re-enable magic-link jumping when such jumps
would remain inside the root.
The LOOKUP_NO_*LINK flags return -ELOOP if path resolution would
violates their requirement, while the others all return -EXDEV.
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: 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/namei.c | 85 ++++++++++++++++++++++++++++++++++++-------
include/linux/namei.h | 7 ++++
2 files changed, 78 insertions(+), 14 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index e39b573fcc4d..2e18ce5a313e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -674,7 +674,11 @@ static int unlazy_walk(struct nameidata *nd)
goto out2;
if (unlikely(!legitimize_path(nd, &nd->path, nd->seq)))
goto out1;
- if (nd->root.mnt && !(nd->flags & LOOKUP_ROOT)) {
+ if (!nd->root.mnt) {
+ /* Restart from path_init() if nd->root was cleared. */
+ if (nd->flags & LOOKUP_BENEATH)
+ goto out;
+ } else if (!(nd->flags & LOOKUP_ROOT)) {
if (unlikely(!legitimize_path(nd, &nd->root, nd->root_seq)))
goto out;
}
@@ -843,6 +847,13 @@ 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_NO_XDEV)) {
+ /* Absolute path arguments to path_init() are allowed. */
+ if (nd->path.mnt != NULL && nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1051,6 +1062,9 @@ const char *get_link(struct nameidata *nd)
int error;
const char *res;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return ERR_PTR(-ELOOP);
+
if (!(nd->flags & LOOKUP_RCU)) {
touch_atime(&last->link);
cond_resched();
@@ -1082,14 +1096,22 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ if (nd->flags & LOOKUP_MAGICLINK_JUMPED) {
+ if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
+ 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 == '/'))
;
}
@@ -1270,12 +1292,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_NO_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;
@@ -1332,6 +1358,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_NO_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1352,8 +1380,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;
@@ -1378,6 +1409,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1392,6 +1425,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1480,8 +1515,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)
@@ -1490,6 +1528,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1704,6 +1744,13 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ /*
+ * LOOKUP_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) {
@@ -2170,6 +2217,7 @@ static int link_path_walk(const char *name, struct nameidata *nd)
/* 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)
@@ -2202,11 +2250,13 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+
+ /* Figure out the starting path and root (if needed). */
if (*s == '/') {
set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
} else if (nd->dfd == AT_FDCWD) {
if (flags & LOOKUP_RCU) {
struct fs_struct *fs = current->fs;
@@ -2222,7 +2272,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
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);
@@ -2247,8 +2296,16 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->inode = nd->path.dentry->d_inode;
}
fdput(f);
- return s;
}
+ /* For scoped-lookups we need to set the root to the dirfd as well. */
+ if (flags & LOOKUP_BENEATH) {
+ nd->root = nd->path;
+ if (flags & LOOKUP_RCU)
+ nd->root_seq = nd->seq;
+ else
+ path_get(&nd->root);
+ }
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index bd6d3eb7764d..be407415c28a 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -51,6 +51,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_DOWN 0x8000
#define LOOKUP_MAGICLINK_JUMPED 0x10000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x020000 /* No escaping from starting point. */
+#define LOOKUP_NO_XDEV 0x040000 /* No mountpoint crossing. */
+#define LOOKUP_NO_MAGICLINKS 0x080000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x100000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_MAGICLINKS. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
--
2.23.0
^ permalink raw reply related
* [PATCH v12 07/12] open: O_EMPTYPATH: procfs-less file descriptor re-opening
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
Userspace has made use of /proc/self/fd very liberally to allow for
descriptors to be re-opened. There are a wide variety of uses for this
feature, but it has always required constructing a pathname and could
not be done without procfs mounted. The obvious solution for this is to
extend openat(2) to have an AT_EMPTY_PATH-equivalent -- O_EMPTYPATH.
Now that descriptor re-opening has been made safe through the new
magic-link resolution restrictions, we can replicate these restrictions
for O_EMPTYPATH. In particular, we only allow "upgrading" the file
descriptor if the corresponding FMODE_PATH_* bit is set (or the
FMODE_{READ,WRITE} cases for non-O_PATH file descriptors).
When doing openat(O_EMPTYPATH|O_PATH), O_PATH takes precedence and
O_EMPTYPATH is ignored. Very few users ever have a need to O_PATH
re-open an existing file descriptor, and so accommodating them at the
expense of further complicating O_PATH makes little sense. Ultimately,
if users ask for this we can always add RESOLVE_EMPTY_PATH to
resolveat(2) in the future.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
arch/alpha/include/uapi/asm/fcntl.h | 1 +
arch/parisc/include/uapi/asm/fcntl.h | 39 ++++++++++++++--------------
arch/sparc/include/uapi/asm/fcntl.h | 1 +
fs/fcntl.c | 2 +-
fs/namei.c | 20 ++++++++++++++
fs/open.c | 7 ++++-
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 +++
8 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/arch/alpha/include/uapi/asm/fcntl.h b/arch/alpha/include/uapi/asm/fcntl.h
index 50bdc8e8a271..1f879bade68b 100644
--- a/arch/alpha/include/uapi/asm/fcntl.h
+++ b/arch/alpha/include/uapi/asm/fcntl.h
@@ -34,6 +34,7 @@
#define O_PATH 040000000
#define __O_TMPFILE 0100000000
+#define O_EMPTYPATH 0200000000
#define F_GETLK 7
#define F_SETLK 8
diff --git a/arch/parisc/include/uapi/asm/fcntl.h b/arch/parisc/include/uapi/asm/fcntl.h
index 03ce20e5ad7d..5d709058a76f 100644
--- a/arch/parisc/include/uapi/asm/fcntl.h
+++ b/arch/parisc/include/uapi/asm/fcntl.h
@@ -2,26 +2,27 @@
#ifndef _PARISC_FCNTL_H
#define _PARISC_FCNTL_H
-#define O_APPEND 000000010
-#define O_BLKSEEK 000000100 /* HPUX only */
-#define O_CREAT 000000400 /* not fcntl */
-#define O_EXCL 000002000 /* not fcntl */
-#define O_LARGEFILE 000004000
-#define __O_SYNC 000100000
+#define O_APPEND 0000000010
+#define O_BLKSEEK 0000000100 /* HPUX only */
+#define O_CREAT 0000000400 /* not fcntl */
+#define O_EXCL 0000002000 /* not fcntl */
+#define O_LARGEFILE 0000004000
+#define __O_SYNC 0000100000
#define O_SYNC (__O_SYNC|O_DSYNC)
-#define O_NONBLOCK 000200004 /* HPUX has separate NDELAY & NONBLOCK */
-#define O_NOCTTY 000400000 /* not fcntl */
-#define O_DSYNC 001000000 /* HPUX only */
-#define O_RSYNC 002000000 /* HPUX only */
-#define O_NOATIME 004000000
-#define O_CLOEXEC 010000000 /* set close_on_exec */
-
-#define O_DIRECTORY 000010000 /* must be a directory */
-#define O_NOFOLLOW 000000200 /* don't follow links */
-#define O_INVISIBLE 004000000 /* invisible I/O, for DMAPI/XDSM */
-
-#define O_PATH 020000000
-#define __O_TMPFILE 040000000
+#define O_NONBLOCK 0000200004 /* HPUX has separate NDELAY & NONBLOCK */
+#define O_NOCTTY 0000400000 /* not fcntl */
+#define O_DSYNC 0001000000 /* HPUX only */
+#define O_RSYNC 0002000000 /* HPUX only */
+#define O_NOATIME 0004000000
+#define O_CLOEXEC 0010000000 /* set close_on_exec */
+
+#define O_DIRECTORY 0000010000 /* must be a directory */
+#define O_NOFOLLOW 0000000200 /* don't follow links */
+#define O_INVISIBLE 0004000000 /* invisible I/O, for DMAPI/XDSM */
+
+#define O_PATH 0020000000
+#define __O_TMPFILE 0040000000
+#define O_EMPTYPATH 0100000000
#define F_GETLK64 8
#define F_SETLK64 9
diff --git a/arch/sparc/include/uapi/asm/fcntl.h b/arch/sparc/include/uapi/asm/fcntl.h
index 67dae75e5274..dc86c9eaf950 100644
--- a/arch/sparc/include/uapi/asm/fcntl.h
+++ b/arch/sparc/include/uapi/asm/fcntl.h
@@ -37,6 +37,7 @@
#define O_PATH 0x1000000
#define __O_TMPFILE 0x2000000
+#define O_EMPTYPATH 0x4000000
#define F_GETOWN 5 /* for sockets. */
#define F_SETOWN 6 /* for sockets. */
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 3d40771e8e7c..4cf05a2fd162 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index 54d57dad0f91..e39b573fcc4d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3571,6 +3571,24 @@ static int trailing_magiclink(struct nameidata *nd, int acc_mode,
return may_open_magiclink(upgrade_mask, acc_mode);
}
+static int do_emptypath(struct nameidata *nd, const struct open_flags *op,
+ struct file *file)
+{
+ int error;
+ /* We don't support AT_FDCWD (since O_PATH is disallowed here). */
+ struct fd f = fdget_raw(nd->dfd);
+
+ if (!f.file)
+ return -EBADF;
+
+ /* Apply trailing_magiclink()-like restrictions. */
+ error = may_open_magiclink(f.file->f_mode, op->acc_mode);
+ if (!error)
+ error = vfs_open(&f.file->f_path, file);
+ fdput(f);
+ return error;
+}
+
static struct file *path_openat(struct nameidata *nd,
const struct open_flags *op, unsigned flags)
{
@@ -3583,6 +3601,8 @@ static struct file *path_openat(struct nameidata *nd,
if (unlikely(file->f_flags & __O_TMPFILE)) {
error = do_tmpfile(nd, flags, op, file);
+ } else if (unlikely(file->f_flags & O_EMPTYPATH)) {
+ error = do_emptypath(nd, op, file);
} else if (unlikely(file->f_flags & O_PATH)) {
/* Inlined path_lookupat() with a trailing_magiclink() check. */
fmode_t opath_mask = op->opath_mask;
diff --git a/fs/open.c b/fs/open.c
index 806a75d685e1..310b896eecf0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1015,6 +1015,8 @@ 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_EMPTYPATH)
+ lookup_flags |= LOOKUP_EMPTY;
op->lookup_flags = lookup_flags;
return 0;
}
@@ -1076,14 +1078,17 @@ long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode)
{
struct open_flags op;
int fd = build_open_flags(flags, mode, &op);
+ int empty = 0;
struct filename *tmp;
if (fd)
return fd;
- tmp = getname(filename);
+ tmp = getname_flags(filename, op.lookup_flags, &empty);
if (IS_ERR(tmp))
return PTR_ERR(tmp);
+ if (!empty)
+ op.open_flag &= ~O_EMPTYPATH;
fd = get_unused_fd_flags(flags);
if (fd >= 0) {
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index d019df946cb2..2868ae6c8fc1 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,7 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_EMPTYPATH)
#ifndef force_o_largefile
#define force_o_largefile() (!IS_ENABLED(CONFIG_ARCH_32BIT_OFF_T))
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..ae6862f69cc2 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
#define __O_TMPFILE 020000000
#endif
+#ifndef O_EMPTYPATH
+#define O_EMPTYPATH 040000000
+#endif
+
/* a horrid kludge trying to make sure that this will fail on old kernels */
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
--
2.23.0
^ permalink raw reply related
* [PATCH v12 06/12] procfs: switch magic-link modes to be more sane
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
Now that magic-link modes are obeyed for file re-opening purposes, some
of the pre-existing magic-link modes need to be adjusted to be more
semantically correct.
The most blatant example of this is /proc/self/exe, which had a mode of
a+rwx even though tautologically the file could never be opened for
writing (because it is the current->mm of a live process).
With the new O_PATH restrictions, changing the default mode of these
magic-links allows us to avoid delayed-access attacks such as we saw in
CVE-2019-5736.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/proc/base.c | 20 ++++++++++----------
fs/proc/namespaces.c | 2 +-
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..297242174402 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -133,9 +133,9 @@ struct pid_entry {
#define DIR(NAME, MODE, iops, fops) \
NOD(NAME, (S_IFDIR|(MODE)), &iops, &fops, {} )
-#define LNK(NAME, get_link) \
- NOD(NAME, (S_IFLNK|S_IRWXUGO), \
- &proc_pid_link_inode_operations, NULL, \
+#define LNK(NAME, MODE, get_link) \
+ NOD(NAME, (S_IFLNK|(MODE)), \
+ &proc_pid_link_inode_operations, NULL, \
{ .proc_get_link = get_link } )
#define REG(NAME, MODE, fops) \
NOD(NAME, (S_IFREG|(MODE)), NULL, &fops, {})
@@ -3028,9 +3028,9 @@ static const struct pid_entry tgid_base_stuff[] = {
REG("numa_maps", S_IRUGO, proc_pid_numa_maps_operations),
#endif
REG("mem", S_IRUSR|S_IWUSR, proc_mem_operations),
- LNK("cwd", proc_cwd_link),
- LNK("root", proc_root_link),
- LNK("exe", proc_exe_link),
+ LNK("cwd", S_IRWXUGO, proc_cwd_link),
+ LNK("root", S_IRWXUGO, proc_root_link),
+ LNK("exe", S_IRUGO|S_IXUGO, proc_exe_link),
REG("mounts", S_IRUGO, proc_mounts_operations),
REG("mountinfo", S_IRUGO, proc_mountinfo_operations),
REG("mountstats", S_IRUSR, proc_mountstats_operations),
@@ -3429,11 +3429,11 @@ static const struct pid_entry tid_base_stuff[] = {
REG("numa_maps", S_IRUGO, proc_pid_numa_maps_operations),
#endif
REG("mem", S_IRUSR|S_IWUSR, proc_mem_operations),
- LNK("cwd", proc_cwd_link),
- LNK("root", proc_root_link),
- LNK("exe", proc_exe_link),
+ LNK("cwd", S_IRWXUGO, proc_cwd_link),
+ LNK("root", S_IRWXUGO, proc_root_link),
+ LNK("exe", S_IRUGO|S_IXUGO, proc_exe_link),
REG("mounts", S_IRUGO, proc_mounts_operations),
- REG("mountinfo", S_IRUGO, proc_mountinfo_operations),
+ REG("mountinfo", S_IRUGO, proc_mountinfo_operations),
#ifdef CONFIG_PROC_PAGE_MONITOR
REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
REG("smaps", S_IRUGO, proc_pid_smaps_operations),
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index dd2b35f78b09..cd1e130913f7 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -94,7 +94,7 @@ static struct dentry *proc_ns_instantiate(struct dentry *dentry,
struct inode *inode;
struct proc_inode *ei;
- inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRWXUGO);
+ inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRUGO);
if (!inode)
return ERR_PTR(-ENOENT);
--
2.23.0
^ permalink raw reply related
* [PATCH v12 05/12] namei: obey trailing magic-link DAC permissions
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Andy Lutomirski, Eric Biederman, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
The ability for userspace to "re-open" file descriptors through
/proc/self/fd has been a very useful tool for all sorts of usecases
(container runtimes are one common example). However, the current
interface for doing this has resulted in some pretty subtle security
holes. Userspace can re-open a file descriptor with more permissions
than the original, which can result in cases such as /proc/$pid/exe
being re-opened O_RDWR at a later date even though (by definition)
/proc/$pid/exe cannot be opened for writing. When combined with O_PATH
the results can get even more confusing.
We cannot block this outright. Aside from userspace already depending on
it, it's a useful feature which can actually increase the security of
userspace. For instance, LXC keeps an O_PATH of the container's
/dev/pts/ptmx that gets re-opened to create new ptys and then uses
TIOCGPTPEER to get the slave end. This allows for pty allocation without
resolving paths inside an (untrusted) container's rootfs. There isn't a
trivial way of doing this that is as straight-forward and safe as O_PATH
re-opening.
Instead we have to restrict it in such a way that it doesn't break
(good) users but does block potential attackers. The solution applied in
this patch is to restrict *re-opening* (not resolution through)
magic-links by requiring that mode of the link be obeyed. Normal
symlinks have modes of a+rwx but magic-links have other modes. These
magic-link modes were historically ignored during path resolution, but
they've now been re-purposed for more useful ends.
It is also necessary to define semantics for the mode of an O_PATH
descriptor, since re-opening a magic-link through an O_PATH needs to be
just as restricted as the corresponding magic-link -- otherwise the
above protection can be bypassed. There are two distinct cases:
1. The target is a regular file (not a magic-link). Userspace depends
on being able to re-open the O_PATH of a regular file, so we must
define the mode to be a+rwx.
2. The target is a magic-link. In this case, we simply copy the mode of
the magic-link. This results in an O_PATH of a magic-link
effectively acting as a no-op in terms of how much re-opening
privileges a process has.
CAP_DAC_OVERRIDE can be used to override all of these restrictions, but
we only permit &init_userns's capabilities to affect these semantics.
The reason for this is that there isn't a clear way to track what
user_ns is the original owner of a given O_PATH chain -- thus an
unprivileged user could create a new userns and O_PATH the file
descriptor, owning it. All signs would indicate that the user really
does have CAP_DAC_OVERRIDE over the new descriptor and the protection
would be bypassed. We thus opt for the more conservative approach.
I have run this patch on several machines for several days. So far, the
only processes which have hit this case ("loadkeys" and "kbd_mode" from
the kbd package[1]) gracefully handle the permission error and do not
cause any user-visible problems. In order to give users a heads-up, a
warning is output to dmesg whenever may_open_magiclink() refuses access.
[1]: http://git.altlinux.org/people/legion/packages/kbd.git
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Christian Brauner <christian@brauner.io>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Documentation/filesystems/path-lookup.rst | 12 +--
fs/internal.h | 1 +
fs/namei.c | 105 +++++++++++++++++++---
fs/open.c | 3 +-
fs/proc/fd.c | 23 ++++-
include/linux/fs.h | 4 +
include/linux/namei.h | 1 +
7 files changed, 130 insertions(+), 19 deletions(-)
diff --git a/Documentation/filesystems/path-lookup.rst b/Documentation/filesystems/path-lookup.rst
index 434a07b0002b..a57d78ec8bee 100644
--- a/Documentation/filesystems/path-lookup.rst
+++ b/Documentation/filesystems/path-lookup.rst
@@ -1310,12 +1310,14 @@ longer needed.
``LOOKUP_JUMPED`` means that the current dentry was chosen not because
it had the right name but for some other reason. This happens when
following "``..``", following a symlink to ``/``, crossing a mount point
-or accessing a "``/proc/$PID/fd/$FD``" symlink. In this case the
-filesystem has not been asked to revalidate the name (with
-``d_revalidate()``). In such cases the inode may still need to be
-revalidated, so ``d_op->d_weak_revalidate()`` is called if
+or accessing a "``/proc/$PID/fd/$FD``" symlink (also known as a "magic
+link"). In this case the filesystem has not been asked to revalidate the
+name (with ``d_revalidate()``). In such cases the inode may still need
+to be revalidated, so ``d_op->d_weak_revalidate()`` is called if
``LOOKUP_JUMPED`` is set when the look completes - which may be at the
-final component or, when creating, unlinking, or renaming, at the penultimate component.
+final component or, when creating, unlinking, or renaming, at the
+penultimate component. ``LOOKUP_MAGICLINK_JUMPED`` is set alongside
+``LOOKUP_JUMPED`` if a magic-link was traversed.
Final-component flags
~~~~~~~~~~~~~~~~~~~~~
diff --git a/fs/internal.h b/fs/internal.h
index 315fcd8d237c..f48449a43626 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -119,6 +119,7 @@ struct open_flags {
int acc_mode;
int intent;
int lookup_flags;
+ fmode_t opath_mask;
};
extern struct file *do_filp_open(int dfd, struct filename *pathname,
const struct open_flags *op);
diff --git a/fs/namei.c b/fs/namei.c
index 209c51a5226c..54d57dad0f91 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -872,7 +872,7 @@ void nd_jump_link(struct path *path)
nd->path = *path;
nd->inode = nd->path.dentry->d_inode;
- nd->flags |= LOOKUP_JUMPED;
+ nd->flags |= LOOKUP_JUMPED | LOOKUP_MAGICLINK_JUMPED;
}
static inline void put_link(struct nameidata *nd)
@@ -1066,6 +1066,7 @@ const char *get_link(struct nameidata *nd)
return ERR_PTR(error);
nd->last_type = LAST_BIND;
+ nd->flags &= ~LOOKUP_MAGICLINK_JUMPED;
res = READ_ONCE(inode->i_link);
if (!res) {
const char * (*get)(struct dentry *, struct inode *,
@@ -3501,16 +3502,73 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
return error;
}
-static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
+/**
+ * may_reopen_magiclink - Check permissions for opening a trailing magic-link
+ * @upgrade_mask: the upgrade-mask of the magic-link
+ * @acc_mode: ACC_MODE which the user is attempting
+ *
+ * We block magic-link re-opening if the @upgrade_mask is more strict than the
+ * @acc_mode being requested, unless the user is capable(CAP_DAC_OVERRIDE).
+ *
+ * Returns 0 if successful, -EACCES on error.
+ */
+static int may_open_magiclink(fmode_t upgrade_mask, int acc_mode)
{
- struct path path;
- int error = path_lookupat(nd, flags, &path);
- if (!error) {
- audit_inode(nd->name, path.dentry, 0);
- error = vfs_open(&path, file);
- path_put(&path);
- }
- return error;
+ /*
+ * We only allow for init_userns to be able to override magic-links.
+ * This is done to avoid cases where an unprivileged userns could take
+ * an O_PATH of the fd, resulting in it being very unclear whether
+ * CAP_DAC_OVERRIDE should work on the new O_PATH fd (given that it
+ * pipes through to the underlying file).
+ */
+ if (capable(CAP_DAC_OVERRIDE))
+ return 0;
+
+ if ((acc_mode & MAY_READ) &&
+ !(upgrade_mask & (FMODE_READ | FMODE_PATH_READ)))
+ goto err;
+ if ((acc_mode & MAY_WRITE) &&
+ !(upgrade_mask & (FMODE_WRITE | FMODE_PATH_WRITE)))
+ goto err;
+
+ return 0;
+
+err:
+ pr_warn_ratelimited("%s[%d]: magic-link re-open blocked ('%s%s%s' requested with an upgrade-mask of '%s%s%s%s')",
+ current->comm, task_pid_nr(current),
+ (acc_mode & MAY_READ) ? "r" : "",
+ (acc_mode & MAY_WRITE) ? "w" : "",
+ (acc_mode & MAY_EXEC) ? "x" : "",
+ (upgrade_mask & FMODE_READ) ? "r" : "",
+ (upgrade_mask & FMODE_PATH_READ) ? "R" : "",
+ (upgrade_mask & FMODE_WRITE) ? "w" : "",
+ (upgrade_mask & FMODE_PATH_WRITE) ? "W" : "");
+ return -EACCES;
+}
+
+static int trailing_magiclink(struct nameidata *nd, int acc_mode,
+ fmode_t *opath_mask)
+{
+ struct inode *inode = nd->link_inode;
+ fmode_t upgrade_mask = 0;
+
+ /* Was the trailing_symlink() a magic-link? */
+ if (!(nd->flags & LOOKUP_MAGICLINK_JUMPED))
+ return 0;
+
+ /*
+ * Figure out the upgrade-mask of the link_inode. Since these aren't
+ * strictly POSIX semantics we don't do an acl_permission_check() here,
+ * so we only care that at least one bit is set for each upgrade-mode.
+ */
+ if (inode->i_mode & S_IRUGO)
+ upgrade_mask |= FMODE_PATH_READ;
+ if (inode->i_mode & S_IWUGO)
+ upgrade_mask |= FMODE_PATH_WRITE;
+ /* Restrict the O_PATH upgrade-mask of the caller. */
+ if (opath_mask)
+ *opath_mask &= upgrade_mask;
+ return may_open_magiclink(upgrade_mask, acc_mode);
}
static struct file *path_openat(struct nameidata *nd,
@@ -3526,13 +3584,38 @@ static struct file *path_openat(struct nameidata *nd,
if (unlikely(file->f_flags & __O_TMPFILE)) {
error = do_tmpfile(nd, flags, op, file);
} else if (unlikely(file->f_flags & O_PATH)) {
- error = do_o_path(nd, flags, file);
+ /* Inlined path_lookupat() with a trailing_magiclink() check. */
+ fmode_t opath_mask = op->opath_mask;
+ const char *s = path_init(nd, flags);
+
+ while (!(error = link_path_walk(s, nd))
+ && ((error = lookup_last(nd)) > 0)) {
+ s = trailing_symlink(nd);
+ error = trailing_magiclink(nd, op->acc_mode, &opath_mask);
+ if (error)
+ s = ERR_PTR(error);
+ }
+ if (!error)
+ error = complete_walk(nd);
+
+ if (!error && nd->flags & LOOKUP_DIRECTORY)
+ if (!d_can_lookup(nd->path.dentry))
+ error = -ENOTDIR;
+ if (!error) {
+ audit_inode(nd->name, nd->path.dentry, 0);
+ error = vfs_open(&nd->path, file);
+ file->f_mode |= opath_mask;
+ }
+ terminate_walk(nd);
} else {
const char *s = path_init(nd, flags);
while (!(error = link_path_walk(s, nd)) &&
(error = do_last(nd, file, op)) > 0) {
nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
s = trailing_symlink(nd);
+ error = trailing_magiclink(nd, op->acc_mode, NULL);
+ if (error)
+ s = ERR_PTR(error);
}
terminate_walk(nd);
}
diff --git a/fs/open.c b/fs/open.c
index a59abe3c669a..806a75d685e1 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1001,8 +1001,9 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
acc_mode |= MAY_APPEND;
op->acc_mode = acc_mode;
-
op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
+ /* For O_PATH backwards-compatibility we default to an all-set mask. */
+ op->opath_mask = FMODE_PATH_READ | FMODE_PATH_WRITE;
if (flags & O_CREAT) {
op->intent |= LOOKUP_CREATE;
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 81882a13212d..9b7d8becb002 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -104,11 +104,30 @@ static void tid_fd_update_inode(struct task_struct *task, struct inode *inode,
task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
if (S_ISLNK(inode->i_mode)) {
+ /*
+ * Always set +x (depending on the fmode type), since there
+ * currently aren't FMODE_PATH_EXEC restrictions and there is
+ * no O_MAYEXEC yet. This might change in the future, in which
+ * case we will restrict +x.
+ */
unsigned i_mode = S_IFLNK;
+ if (f_mode & FMODE_PATH)
+ i_mode |= S_IXGRP;
+ else
+ i_mode |= S_IXUSR;
+ /*
+ * Construct the mode bits based on the open-mode. The u+rwx
+ * bits are for "ordinary" open modes while g+rwx are for
+ * O_PATH modes.
+ */
if (f_mode & FMODE_READ)
- i_mode |= S_IRUSR | S_IXUSR;
+ i_mode |= S_IRUSR;
if (f_mode & FMODE_WRITE)
- i_mode |= S_IWUSR | S_IXUSR;
+ i_mode |= S_IWUSR;
+ if (f_mode & FMODE_PATH_READ)
+ i_mode |= S_IRGRP;
+ if (f_mode & FMODE_PATH_WRITE)
+ i_mode |= S_IWGRP;
inode->i_mode = i_mode;
}
security_task_to_inode(task, inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 997a530ff4e9..a9ad596b28e2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -173,6 +173,10 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
/* File does not contribute to nr_files count */
#define FMODE_NOACCOUNT ((__force fmode_t)0x20000000)
+/* File is an O_PATH descriptor which can be upgraded to (read, write). */
+#define FMODE_PATH_READ ((__force fmode_t)0x40000000)
+#define FMODE_PATH_WRITE ((__force fmode_t)0x80000000)
+
/*
* Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector
* that indicates that they should check the contents of the iovec are
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 9138b4471dbf..bd6d3eb7764d 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -49,6 +49,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_ROOT 0x2000
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+#define LOOKUP_MAGICLINK_JUMPED 0x10000
extern int path_pts(struct path *path);
--
2.23.0
^ permalink raw reply related
* [PATCH v12 04/12] perf_event_open: switch to copy_struct_from_user()
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
The change is very straightforward, and takes advantage of the (very
minor) efficiency improvements in copy_struct_from_user() -- that the
memchr_inv() check is done on a buffer instead of one-at-at-time with
get_user().
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/events/core.c | 45 ++++++++------------------------------------
1 file changed, 8 insertions(+), 37 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0463c1151bae..fe5f58443ba6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10498,55 +10498,26 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
u32 size;
int ret;
- if (!access_ok(uattr, PERF_ATTR_SIZE_VER0))
- return -EFAULT;
-
- /*
- * zero the full structure, so that a short copy will be nice.
- */
+ /* Zero the full structure, so that a short copy will be nice. */
memset(attr, 0, sizeof(*attr));
ret = get_user(size, &uattr->size);
if (ret)
return ret;
- if (size > PAGE_SIZE) /* silly large */
- goto err_size;
-
- if (!size) /* abi compat */
+ /* ABI compatibility quirk: */
+ if (!size)
size = PERF_ATTR_SIZE_VER0;
-
if (size < PERF_ATTR_SIZE_VER0)
goto err_size;
- /*
- * If we're handed a bigger struct than we know of,
- * ensure all the unknown bits are 0 - i.e. new
- * user-space does not rely on any kernel feature
- * extensions we dont know about yet.
- */
- if (size > sizeof(*attr)) {
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
-
- addr = (void __user *)uattr + sizeof(*attr);
- end = (void __user *)uattr + size;
-
- for (; addr < end; addr++) {
- ret = get_user(val, addr);
- if (ret)
- return ret;
- if (val)
- goto err_size;
- }
- size = sizeof(*attr);
+ ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
+ if (ret) {
+ if (ret == -E2BIG)
+ goto err_size;
+ return ret;
}
- ret = copy_from_user(attr, uattr, size);
- if (ret)
- return -EFAULT;
-
attr->size = size;
if (attr->__reserved_1)
--
2.23.0
^ permalink raw reply related
* [PATCH v12 03/12] sched_setattr: switch to copy_struct_{to,from}_user()
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
The change is very straightforward, and takes advantage of the (very
minor) efficiency improvements in copy_struct_{to,from}_user() -- that
the memchr_inv() check is done on a buffer instead of one-at-at-time
with get_user() or put_user().
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/sched/core.c | 85 ++++++---------------------------------------
1 file changed, 10 insertions(+), 75 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 010d578118d6..2f58b07d3468 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4900,9 +4900,6 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
u32 size;
int ret;
- if (!access_ok(uattr, SCHED_ATTR_SIZE_VER0))
- return -EFAULT;
-
/* Zero the full structure, so that a short copy will be nice: */
memset(attr, 0, sizeof(*attr));
@@ -4910,45 +4907,19 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
if (ret)
return ret;
- /* Bail out on silly large: */
- if (size > PAGE_SIZE)
- goto err_size;
-
/* ABI compatibility quirk: */
if (!size)
size = SCHED_ATTR_SIZE_VER0;
-
if (size < SCHED_ATTR_SIZE_VER0)
goto err_size;
- /*
- * If we're handed a bigger struct than we know of,
- * ensure all the unknown bits are 0 - i.e. new
- * user-space does not rely on any kernel feature
- * extensions we dont know about yet.
- */
- if (size > sizeof(*attr)) {
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
-
- addr = (void __user *)uattr + sizeof(*attr);
- end = (void __user *)uattr + size;
-
- for (; addr < end; addr++) {
- ret = get_user(val, addr);
- if (ret)
- return ret;
- if (val)
- goto err_size;
- }
- size = sizeof(*attr);
+ ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
+ if (ret) {
+ if (ret == -E2BIG)
+ goto err_size;
+ return ret;
}
- ret = copy_from_user(attr, uattr, size);
- if (ret)
- return -EFAULT;
-
if ((attr->sched_flags & SCHED_FLAG_UTIL_CLAMP) &&
size < SCHED_ATTR_SIZE_VER1)
return -EINVAL;
@@ -5105,51 +5076,15 @@ SYSCALL_DEFINE2(sched_getparam, pid_t, pid, struct sched_param __user *, param)
return retval;
}
-static int sched_read_attr(struct sched_attr __user *uattr,
- struct sched_attr *attr,
- unsigned int usize)
-{
- int ret;
-
- if (!access_ok(uattr, usize))
- return -EFAULT;
-
- /*
- * If we're handed a smaller struct than we know of,
- * ensure all the unknown bits are 0 - i.e. old
- * user-space does not get uncomplete information.
- */
- if (usize < sizeof(*attr)) {
- unsigned char *addr;
- unsigned char *end;
-
- addr = (void *)attr + usize;
- end = (void *)attr + sizeof(*attr);
-
- for (; addr < end; addr++) {
- if (*addr)
- return -EFBIG;
- }
-
- attr->size = usize;
- }
-
- ret = copy_to_user(uattr, attr, attr->size);
- if (ret)
- return -EFAULT;
-
- return 0;
-}
-
/**
* sys_sched_getattr - similar to sched_getparam, but with sched_attr
* @pid: the pid in question.
* @uattr: structure containing the extended parameters.
- * @size: sizeof(attr) for fwd/bwd comp.
+ * @usize: sizeof(attr) for fwd/bwd comp.
* @flags: for future extension.
*/
SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
- unsigned int, size, unsigned int, flags)
+ unsigned int, usize, unsigned int, flags)
{
struct sched_attr attr = {
.size = sizeof(struct sched_attr),
@@ -5157,8 +5092,8 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
struct task_struct *p;
int retval;
- if (!uattr || pid < 0 || size > PAGE_SIZE ||
- size < SCHED_ATTR_SIZE_VER0 || flags)
+ if (!uattr || pid < 0 || usize > PAGE_SIZE ||
+ usize < SCHED_ATTR_SIZE_VER0 || flags)
return -EINVAL;
rcu_read_lock();
@@ -5188,7 +5123,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
rcu_read_unlock();
- retval = sched_read_attr(uattr, &attr, size);
+ retval = copy_struct_to_user(uattr, usize, &attr, sizeof(attr));
return retval;
out_unlock:
--
2.23.0
^ permalink raw reply related
* [PATCH v12 02/12] clone3: switch to copy_struct_from_user()
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Rasmus Villemoes,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
The change is very straightforward, and takes advantage of the (very
minor) efficiency improvements in copy_struct_from_user() -- that the
memchr_inv() check is done on a buffer instead of one-at-at-time with
get_user().
Additionally, explicitly define CLONE_ARGS_SIZE_VER0 to match the other
users of the struct-extension pattern.
Cc: Christian Brauner <christian@brauner.io>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
include/uapi/linux/sched.h | 2 ++
kernel/fork.c | 34 ++++++----------------------------
2 files changed, 8 insertions(+), 28 deletions(-)
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index b3105ac1381a..0945805982b4 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -47,6 +47,8 @@ struct clone_args {
__aligned_u64 tls;
};
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+
/*
* Scheduling policies
*/
diff --git a/kernel/fork.c b/kernel/fork.c
index 2852d0e76ea3..70c10d9b429a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2528,39 +2528,17 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
#ifdef __ARCH_WANT_SYS_CLONE3
noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
struct clone_args __user *uargs,
- size_t size)
+ size_t usize)
{
+ int err;
struct clone_args args;
- if (unlikely(size > PAGE_SIZE))
- return -E2BIG;
-
- if (unlikely(size < sizeof(struct clone_args)))
+ if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
return -EINVAL;
- if (unlikely(!access_ok(uargs, size)))
- return -EFAULT;
-
- if (size > sizeof(struct clone_args)) {
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
-
- addr = (void __user *)uargs + sizeof(struct clone_args);
- end = (void __user *)uargs + size;
-
- for (; addr < end; addr++) {
- if (get_user(val, addr))
- return -EFAULT;
- if (val)
- return -E2BIG;
- }
-
- size = sizeof(struct clone_args);
- }
-
- if (copy_from_user(&args, uargs, size))
- return -EFAULT;
+ err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
+ if (err)
+ return err;
*kargs = (struct kernel_clone_args){
.flags = args.flags,
--
2.23.0
^ permalink raw reply related
* [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Rasmus Villemoes, Eric Biederman, Andy Lutomirski,
Andrew Morton, Alexei Starovoitov, Kees Cook, Jann Horn,
Tycho Andersen, David Drysdale, Chanho Min, Oleg Nesterov,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Aleksa Sarai,
Linus Torvalds, containers, linux-alpha, linux-api, linux-arch
In-Reply-To: <20190904201933.10736-1-cyphar@cyphar.com>
A common pattern for syscall extensions is increasing the size of a
struct passed from userspace, such that the zero-value of the new fields
result in the old kernel behaviour (allowing for a mix of userspace and
kernel vintages to operate on one another in most cases). This is done
in both directions -- hence two helpers -- though it's more common to
have to copy user space structs into kernel space.
Previously there was no common lib/ function that implemented
the necessary extension-checking semantics (and different syscalls
implemented them slightly differently or incompletely[1]). A future
patch replaces all of the common uses of this pattern to use the new
copy_struct_{to,from}_user() helpers.
[1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
similar checks to copy_struct_from_user() while rt_sigprocmask(2)
always rejects differently-sized struct arguments.
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
include/linux/uaccess.h | 5 ++
lib/Makefile | 2 +-
lib/struct_user.c | 182 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 188 insertions(+), 1 deletion(-)
create mode 100644 lib/struct_user.c
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 34a038563d97..0ad9544a1aee 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -230,6 +230,11 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
#endif /* ARCH_HAS_NOCACHE_UACCESS */
+extern int copy_struct_to_user(void __user *dst, size_t usize,
+ const void *src, size_t ksize);
+extern int copy_struct_from_user(void *dst, size_t ksize,
+ const void __user *src, size_t usize);
+
/*
* probe_kernel_read(): safely attempt to read from a location
* @dst: pointer to the buffer that shall take the data
diff --git a/lib/Makefile b/lib/Makefile
index 29c02a924973..d86c71feaf0a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -28,7 +28,7 @@ endif
CFLAGS_string.o := $(call cc-option, -fno-stack-protector)
endif
-lib-y := ctype.o string.o vsprintf.o cmdline.o \
+lib-y := ctype.o string.o struct_user.o vsprintf.o cmdline.o \
rbtree.o radix-tree.o timerqueue.o xarray.o \
idr.o extable.o \
sha1.o chacha.o irq_regs.o argv_split.o \
diff --git a/lib/struct_user.c b/lib/struct_user.c
new file mode 100644
index 000000000000..7301ab1bbe98
--- /dev/null
+++ b/lib/struct_user.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 SUSE LLC
+ * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
+ */
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/uaccess.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+
+#define BUFFER_SIZE 64
+
+/*
+ * "memset(p, 0, size)" but for user space buffers. Caller must have already
+ * checked access_ok(p, size).
+ */
+static int __memzero_user(void __user *p, size_t s)
+{
+ const char zeros[BUFFER_SIZE] = {};
+ while (s > 0) {
+ size_t n = min(s, sizeof(zeros));
+
+ if (__copy_to_user(p, zeros, n))
+ return -EFAULT;
+
+ p += n;
+ s -= n;
+ }
+ return 0;
+}
+
+/**
+ * copy_struct_to_user: copy a struct to user space
+ * @dst: Destination address, in user space.
+ * @usize: Size of @dst struct.
+ * @src: Source address, in kernel space.
+ * @ksize: Size of @src struct.
+ *
+ * Copies a struct from kernel space to user space, in a way that guarantees
+ * backwards-compatibility for struct syscall arguments (as long as future
+ * struct extensions are made such that all new fields are *appended* to the
+ * old struct, and zeroed-out new fields have the same meaning as the old
+ * struct).
+ *
+ * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
+ * The recommended usage is something like the following:
+ *
+ * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
+ * {
+ * int err;
+ * struct foo karg = {};
+ *
+ * // do something with karg
+ *
+ * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
+ * if (err)
+ * return err;
+ *
+ * // ...
+ * }
+ *
+ * There are three cases to consider:
+ * * If @usize == @ksize, then it's copied verbatim.
+ * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
+ * older user space. In order to avoid user space getting incomplete
+ * information (new fields might be important), all trailing bytes in @src
+ * (@ksize - @usize) must be zerored, otherwise -EFBIG is returned.
+ * * If @usize > @ksize, then the kernel is "returning" an older struct to a
+ * newer user space. The trailing bytes in @dst (@usize - @ksize) will be
+ * zero-filled.
+ *
+ * Returns (in all cases, some data may have been copied):
+ * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
+ * * -EFAULT: access to user space failed.
+ */
+int copy_struct_to_user(void __user *dst, size_t usize,
+ const void *src, size_t ksize)
+{
+ size_t size = min(ksize, usize);
+ size_t rest = abs(ksize - usize);
+
+ if (unlikely(usize > PAGE_SIZE))
+ return -EFAULT;
+ if (unlikely(!access_ok(dst, usize)))
+ return -EFAULT;
+
+ /* Deal with trailing bytes. */
+ if (usize < ksize) {
+ if (memchr_inv(src + size, 0, rest))
+ return -EFBIG;
+ } else if (usize > ksize) {
+ if (__memzero_user(dst + size, rest))
+ return -EFAULT;
+ }
+ /* Copy the interoperable parts of the struct. */
+ if (__copy_to_user(dst, src, size))
+ return -EFAULT;
+ return 0;
+}
+EXPORT_SYMBOL(copy_struct_to_user);
+
+/**
+ * copy_struct_from_user: copy a struct from user space
+ * @dst: Destination address, in kernel space. This buffer must be @ksize
+ * bytes long.
+ * @ksize: Size of @dst struct.
+ * @src: Source address, in user space.
+ * @usize: (Alleged) size of @src struct.
+ *
+ * Copies a struct from user space to kernel space, in a way that guarantees
+ * backwards-compatibility for struct syscall arguments (as long as future
+ * struct extensions are made such that all new fields are *appended* to the
+ * old struct, and zeroed-out new fields have the same meaning as the old
+ * struct).
+ *
+ * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
+ * The recommended usage is something like the following:
+ *
+ * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
+ * {
+ * int err;
+ * struct foo karg = {};
+ *
+ * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
+ * if (err)
+ * return err;
+ *
+ * // ...
+ * }
+ *
+ * There are three cases to consider:
+ * * If @usize == @ksize, then it's copied verbatim.
+ * * If @usize < @ksize, then the user space has passed an old struct to a
+ * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
+ * are to be zero-filled.
+ * * If @usize > @ksize, then the user space has passed a new struct to an
+ * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
+ * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
+ *
+ * Returns (in all cases, some data may have been copied):
+ * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
+ * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
+ * * -EFAULT: access to user space failed.
+ */
+int copy_struct_from_user(void *dst, size_t ksize,
+ const void __user *src, size_t usize)
+{
+ size_t size = min(ksize, usize);
+ size_t rest = abs(ksize - usize);
+
+ if (unlikely(usize > PAGE_SIZE))
+ return -EFAULT;
+ if (unlikely(!access_ok(src, usize)))
+ return -EFAULT;
+
+ /* Deal with trailing bytes. */
+ if (usize < ksize)
+ memset(dst + size, 0, rest);
+ else if (usize > ksize) {
+ const void __user *addr = src + size;
+ char buffer[BUFFER_SIZE] = {};
+
+ while (rest > 0) {
+ size_t bufsize = min(rest, sizeof(buffer));
+
+ if (__copy_from_user(buffer, addr, bufsize))
+ return -EFAULT;
+ if (memchr_inv(buffer, 0, bufsize))
+ return -E2BIG;
+
+ addr += bufsize;
+ rest -= bufsize;
+ }
+ }
+ /* Copy the interoperable parts of the struct. */
+ if (__copy_from_user(dst, src, size))
+ return -EFAULT;
+ return 0;
+}
+EXPORT_SYMBOL(copy_struct_from_user);
--
2.23.0
^ permalink raw reply related
* [PATCH v12 00/12] namei: openat2(2) path resolution restrictions
From: Aleksa Sarai @ 2019-09-04 20:19 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Jann Horn,
David Drysdale, Tycho Andersen, Kees Cook, Linus Torvalds,
Andrew Morton, Alexei Starovoitov, Chanho Min, Oleg Nesterov,
Rasmus Villemoes, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Aleksa Sarai, containers, linux-alpha, linux-api, linux-arch
This patchset is being developed here:
<https://github.com/cyphar/linux/tree/resolveat/master>
Patch changelog:
v12:
* Remove @how->reserved field from openat2(2), and instead use the
(struct, size) design for syscall extensions.
* Implement copy_struct_{to,from}_user() to unify (struct, size)
syscall extension designs (as well as make them slightly more
efficient by using memchr_inv() as well as using buffers and
avoiding repeated access_ok() checks for trailing byte operations).
* Port sched_setattr(), perf_event_attr(), and clone3() to use the
new helpers.
v11: <https://lore.kernel.org/lkml/20190820033406.29796-1-cyphar@cyphar.com/>
<https://lore.kernel.org/lkml/20190728010207.9781-1-cyphar@cyphar.com/>
v10: <https://lore.kernel.org/lkml/20190719164225.27083-1-cyphar@cyphar.com/>
v09: <https://lore.kernel.org/lkml/20190706145737.5299-1-cyphar@cyphar.com/>
v08: <https://lore.kernel.org/lkml/20190520133305.11925-1-cyphar@cyphar.com/>
v07: <https://lore.kernel.org/lkml/20190507164317.13562-1-cyphar@cyphar.com/>
v06: <https://lore.kernel.org/lkml/20190506165439.9155-1-cyphar@cyphar.com/>
v05: <https://lore.kernel.org/lkml/20190320143717.2523-1-cyphar@cyphar.com/>
v04: <https://lore.kernel.org/lkml/20181112142654.341-1-cyphar@cyphar.com/>
v03: <https://lore.kernel.org/lkml/20181009070230.12884-1-cyphar@cyphar.com/>
v02: <https://lore.kernel.org/lkml/20181009065300.11053-1-cyphar@cyphar.com/>
v01: <https://lore.kernel.org/lkml/20180929103453.12025-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.
In line with the conclusions of the original discussion of AT_NO_JUMPS,
the flag has been split up into separate flags. However, instead of
being an openat(2) flag it is provided through a new syscall openat2(2)
which provides several other improvements to the openat(2) interface (see the
patch description for more details). The following new LOOKUP_* flags are
added:
* LOOKUP_NO_XDEV blocks all mountpoint crossings (upwards, downwards,
or through absolute links). Absolute pathnames alone in openat(2) do
not trigger this.
* LOOKUP_NO_MAGICLINKS blocks resolution through /proc/$pid/fd-style
links. This is done by blocking the usage of nd_jump_link() during
resolution in a filesystem. The term "magic-links" is used to match
with the only reference to these links in Documentation/, but I'm
happy to change the name.
It should be noted that this is different to the scope of
~LOOKUP_FOLLOW in that it applies to all path components. However,
you can do openat2(NO_FOLLOW|NO_MAGICLINKS) on a magic-link and it
will *not* fail (assuming that no parent component was a
magic-link), and you will have an fd for the magic-link.
* LOOKUP_BENEATH disallows escapes to outside the starting dirfd's
tree, using techniques such as ".." or absolute links. Absolute
paths in openat(2) are also disallowed. Conceptually this flag is to
ensure you "stay below" a certain point in the filesystem tree --
but this requires some additional to protect against various races
that would allow escape using "..".
Currently LOOKUP_BENEATH implies LOOKUP_NO_MAGICLINKS, because it
can trivially beam you around the filesystem (breaking the
protection). In future, there might be similar safety checks done as
in LOOKUP_IN_ROOT, but that requires more discussion.
In addition, two new flags are added that expand on the above ideas:
* LOOKUP_NO_SYMLINKS does what it says on the tin. No symlink
resolution is allowed at all, including magic-links. Just as with
LOOKUP_NO_MAGICLINKS this can still be used with NOFOLLOW to open an
fd for the symlink as long as no parent path had a symlink
component.
* LOOKUP_IN_ROOT is an extension of LOOKUP_BENEATH that, rather than
blocking attempts to move past the root, forces all such movements
to be scoped to the starting point. This provides chroot(2)-like
protection but without the cost of a chroot(2) for each filesystem
operation, as well as being safe against race attacks that chroot(2)
is not.
If a race is detected (as with LOOKUP_BENEATH) then an error is
generated, and similar to LOOKUP_BENEATH it is not permitted to cross
magic-links with LOOKUP_IN_ROOT.
The primary need for this is from container runtimes, which
currently need to do symlink scoping in userspace[6] when opening
paths in a potentially malicious container. There is a long list of
CVEs that could have bene mitigated by having RESOLVE_THIS_ROOT
(such as CVE-2017-1002101, CVE-2017-1002102, CVE-2018-15664, and
CVE-2019-5736, just to name a few).
And further, several semantics of file descriptor "re-opening" are now
changed to prevent attacks like CVE-2019-5736 by restricting how
magic-links can be resolved (based on their mode). This required some
other changes to the semantics of the modes of O_PATH file descriptor's
associated /proc/self/fd magic-links. openat2(2) has the ability to
further restrict re-opening of its own O_PATH fds, so that users can
make even better use of this feature.
Finally, O_EMPTYPATH was added so that users can do /proc/self/fd-style
re-opening without depending on procfs. The new restricted semantics for
magic-links are applied here too.
In order to make all of the above more usable, I'm working on
libpathrs[7] which is a C-friendly library for safe path resolution. It
features a userspace-emulated backend if the kernel doesn't support
openat2(2). Hopefully we can get userspace to switch to using it, and
thus get openat2(2) support for free once it's ready.
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: Tycho Andersen <tycho@tycho.ws>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
[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
[7]: https://github.com/openSUSE/libpathrs
Aleksa Sarai (12):
lib: introduce copy_struct_{to,from}_user helpers
clone3: switch to copy_struct_from_user()
sched_setattr: switch to copy_struct_{to,from}_user()
perf_event_open: switch to copy_struct_from_user()
namei: obey trailing magic-link DAC permissions
procfs: switch magic-link modes to be more sane
open: O_EMPTYPATH: procfs-less file descriptor re-opening
namei: O_BENEATH-style path resolution flags
namei: LOOKUP_IN_ROOT: chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
open: openat2(2) syscall
selftests: add openat2(2) selftests
Documentation/filesystems/path-lookup.rst | 12 +-
arch/alpha/include/uapi/asm/fcntl.h | 1 +
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/include/uapi/asm/fcntl.h | 39 +-
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/include/uapi/asm/fcntl.h | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/fcntl.c | 2 +-
fs/internal.h | 1 +
fs/namei.c | 270 ++++++++++--
fs/open.c | 100 ++++-
fs/proc/base.c | 20 +-
fs/proc/fd.c | 23 +-
fs/proc/namespaces.c | 2 +-
include/linux/fcntl.h | 21 +-
include/linux/fs.h | 8 +-
include/linux/namei.h | 9 +
include/linux/syscalls.h | 14 +-
include/linux/uaccess.h | 5 +
include/uapi/asm-generic/fcntl.h | 4 +
include/uapi/asm-generic/unistd.h | 5 +-
include/uapi/linux/fcntl.h | 42 ++
include/uapi/linux/sched.h | 2 +
kernel/events/core.c | 45 +-
kernel/fork.c | 34 +-
kernel/sched/core.c | 85 +---
lib/Makefile | 2 +-
lib/struct_user.c | 182 ++++++++
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/memfd/memfd_test.c | 7 +-
tools/testing/selftests/openat2/.gitignore | 1 +
tools/testing/selftests/openat2/Makefile | 8 +
tools/testing/selftests/openat2/helpers.c | 167 ++++++++
tools/testing/selftests/openat2/helpers.h | 118 +++++
.../testing/selftests/openat2/linkmode_test.c | 333 +++++++++++++++
.../testing/selftests/openat2/openat2_test.c | 106 +++++
.../selftests/openat2/rename_attack_test.c | 127 ++++++
.../testing/selftests/openat2/resolve_test.c | 402 ++++++++++++++++++
53 files changed, 1971 insertions(+), 248 deletions(-)
create mode 100644 lib/struct_user.c
create mode 100644 tools/testing/selftests/openat2/.gitignore
create mode 100644 tools/testing/selftests/openat2/Makefile
create mode 100644 tools/testing/selftests/openat2/helpers.c
create mode 100644 tools/testing/selftests/openat2/helpers.h
create mode 100644 tools/testing/selftests/openat2/linkmode_test.c
create mode 100644 tools/testing/selftests/openat2/openat2_test.c
create mode 100644 tools/testing/selftests/openat2/rename_attack_test.c
create mode 100644 tools/testing/selftests/openat2/resolve_test.c
--
2.23.0
^ permalink raw reply
* [PATCH v3 bpf-next 3/3] perf: implement CAP_TRACING
From: Alexei Starovoitov @ 2019-09-04 18:43 UTC (permalink / raw)
To: davem; +Cc: daniel, peterz, luto, netdev, bpf, kernel-team, linux-api
In-Reply-To: <20190904184335.360074-1-ast@kernel.org>
Implement permissions as stated in uapi/linux/capability.h
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
arch/powerpc/perf/core-book3s.c | 4 ++--
arch/x86/events/intel/bts.c | 2 +-
arch/x86/events/intel/core.c | 2 +-
arch/x86/events/intel/p4.c | 2 +-
kernel/events/core.c | 14 +++++++-------
kernel/events/hw_breakpoint.c | 2 +-
kernel/trace/trace_event_perf.c | 4 ++--
7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index ca92e01d0bd1..a204a3c6c68b 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -204,7 +204,7 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
*addrp = mfspr(SPRN_SDAR);
- if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+ if (perf_paranoid_kernel() && !capable_tracing() &&
is_kernel_addr(mfspr(SPRN_SDAR)))
*addrp = 0;
}
@@ -472,7 +472,7 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
* exporting it to userspace (avoid exposure of regions
* where we could have speculative execution)
*/
- if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+ if (perf_paranoid_kernel() && !capable_tracing() &&
is_kernel_addr(addr))
continue;
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 5ee3fed881d3..bd713b2dd7c2 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -550,7 +550,7 @@ static int bts_event_init(struct perf_event *event)
* users to profile the kernel.
*/
if (event->attr.exclude_kernel && perf_paranoid_kernel() &&
- !capable(CAP_SYS_ADMIN))
+ !capable_tracing())
return -EACCES;
if (x86_add_exclusive(x86_lbr_exclusive_bts))
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 648260b5f367..277b12c054fa 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3307,7 +3307,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
if (x86_pmu.version < 3)
return -EINVAL;
- if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_cpu() && !capable_tracing())
return -EACCES;
event->hw.config |= ARCH_PERFMON_EVENTSEL_ANY;
diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
index dee579efb2b2..f379a358c9cb 100644
--- a/arch/x86/events/intel/p4.c
+++ b/arch/x86/events/intel/p4.c
@@ -776,7 +776,7 @@ static int p4_validate_raw_event(struct perf_event *event)
* the user needs special permissions to be able to use it
*/
if (p4_ht_active() && p4_event_bind_map[v].shared) {
- if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_cpu() && !capable_tracing())
return -EACCES;
}
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0463c1151bae..eaba102e5d91 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4134,7 +4134,7 @@ find_get_context(struct pmu *pmu, struct task_struct *task,
if (!task) {
/* Must be root to operate on a CPU event: */
- if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_cpu() && !capable_tracing())
return ERR_PTR(-EACCES);
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
@@ -8741,7 +8741,7 @@ static int perf_kprobe_event_init(struct perf_event *event)
if (event->attr.type != perf_kprobe.type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_tracing())
return -EACCES;
/*
@@ -8801,7 +8801,7 @@ static int perf_uprobe_event_init(struct perf_event *event)
if (event->attr.type != perf_uprobe.type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_tracing())
return -EACCES;
/*
@@ -10588,7 +10588,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
}
/* privileged levels capture (kernel, hv): check permissions */
if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
- && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+ && perf_paranoid_kernel() && !capable_tracing())
return -EACCES;
}
@@ -10807,12 +10807,12 @@ SYSCALL_DEFINE5(perf_event_open,
return err;
if (!attr.exclude_kernel) {
- if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_kernel() && !capable_tracing())
return -EACCES;
}
if (attr.namespaces) {
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_tracing())
return -EACCES;
}
@@ -10826,7 +10826,7 @@ SYSCALL_DEFINE5(perf_event_open,
/* Only privileged users can get physical addresses */
if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
- perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+ perf_paranoid_kernel() && !capable_tracing())
return -EACCES;
/*
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index c5cd852fe86b..8bc4d7d8c913 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -404,7 +404,7 @@ static int hw_breakpoint_parse(struct perf_event *bp,
* Don't let unprivileged users set a breakpoint in the trap
* path to avoid trap recursion attacks.
*/
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_tracing())
return -EPERM;
}
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 0892e38ed6fb..6861307f14d6 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -46,7 +46,7 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
/* The ftrace function trace is allowed only for root. */
if (ftrace_event_is_function(tp_event)) {
- if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_tracepoint_raw() && !capable_tracing())
return -EPERM;
if (!is_sampling_event(p_event))
@@ -82,7 +82,7 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
* ...otherwise raw tracepoint data can be a severe data leak,
* only allow root to have these.
*/
- if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
+ if (perf_paranoid_tracepoint_raw() && !capable_tracing())
return -EPERM;
return 0;
--
2.20.0
^ permalink raw reply related
* [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-09-04 18:43 UTC (permalink / raw)
To: davem; +Cc: daniel, peterz, luto, netdev, bpf, kernel-team, linux-api
In-Reply-To: <20190904184335.360074-1-ast@kernel.org>
Implement permissions as stated in uapi/linux/capability.h
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
kernel/bpf/arraymap.c | 2 +-
kernel/bpf/cgroup.c | 2 +-
kernel/bpf/core.c | 4 +-
kernel/bpf/hashtab.c | 4 +-
kernel/bpf/lpm_trie.c | 2 +-
kernel/bpf/queue_stack_maps.c | 2 +-
kernel/bpf/reuseport_array.c | 2 +-
kernel/bpf/stackmap.c | 2 +-
kernel/bpf/syscall.c | 32 ++++++++------
kernel/bpf/verifier.c | 4 +-
kernel/trace/bpf_trace.c | 2 +-
net/core/bpf_sk_storage.c | 2 +-
net/core/filter.c | 10 +++--
tools/testing/selftests/bpf/test_verifier.c | 46 +++++++++++++++++----
14 files changed, 77 insertions(+), 39 deletions(-)
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 1c65ce0098a9..149f868a02dc 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -73,7 +73,7 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
bool percpu = attr->map_type == BPF_MAP_TYPE_PERCPU_ARRAY;
int ret, numa_node = bpf_map_attr_numa_node(attr);
u32 elem_size, index_mask, max_entries;
- bool unpriv = !capable(CAP_SYS_ADMIN);
+ bool unpriv = !capable_bpf();
u64 cost, array_size, mask64;
struct bpf_map_memory mem;
struct bpf_array *array;
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 6a6a154cfa7b..9c659ba5c146 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -795,7 +795,7 @@ cgroup_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_get_current_cgroup_id:
return &bpf_get_current_cgroup_id_proto;
case BPF_FUNC_trace_printk:
- if (capable(CAP_SYS_ADMIN))
+ if (capable_bpf_tracing())
return bpf_get_trace_printk_proto();
/* fall through */
default:
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 8191a7db2777..6b53c064e8e6 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -646,7 +646,7 @@ static bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
void bpf_prog_kallsyms_add(struct bpf_prog *fp)
{
if (!bpf_prog_kallsyms_candidate(fp) ||
- !capable(CAP_SYS_ADMIN))
+ !capable_bpf())
return;
spin_lock_bh(&bpf_lock);
@@ -768,7 +768,7 @@ static int bpf_jit_charge_modmem(u32 pages)
{
if (atomic_long_add_return(pages, &bpf_jit_current) >
(bpf_jit_limit >> PAGE_SHIFT)) {
- if (!capable(CAP_SYS_ADMIN)) {
+ if (!capable_bpf()) {
atomic_long_sub(pages, &bpf_jit_current);
return -EPERM;
}
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 22066a62c8c9..0fae5c45f425 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -244,9 +244,9 @@ static int htab_map_alloc_check(union bpf_attr *attr)
BUILD_BUG_ON(offsetof(struct htab_elem, fnode.next) !=
offsetof(struct htab_elem, hash_node.pprev));
- if (lru && !capable(CAP_SYS_ADMIN))
+ if (lru && !capable_bpf())
/* LRU implementation is much complicated than other
- * maps. Hence, limit to CAP_SYS_ADMIN for now.
+ * maps. Hence, limit to CAP_BPF.
*/
return -EPERM;
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index 56e6c75d354d..11da3be8a4e5 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -543,7 +543,7 @@ static struct bpf_map *trie_alloc(union bpf_attr *attr)
u64 cost = sizeof(*trie), cost_per_node;
int ret;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return ERR_PTR(-EPERM);
/* check sanity of attributes */
diff --git a/kernel/bpf/queue_stack_maps.c b/kernel/bpf/queue_stack_maps.c
index f697647ceb54..d83afac32863 100644
--- a/kernel/bpf/queue_stack_maps.c
+++ b/kernel/bpf/queue_stack_maps.c
@@ -45,7 +45,7 @@ static bool queue_stack_map_is_full(struct bpf_queue_stack *qs)
/* Called from syscall */
static int queue_stack_map_alloc_check(union bpf_attr *attr)
{
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
/* check sanity of attributes */
diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
index 50c083ba978c..b268fe4b2972 100644
--- a/kernel/bpf/reuseport_array.c
+++ b/kernel/bpf/reuseport_array.c
@@ -154,7 +154,7 @@ static struct bpf_map *reuseport_array_alloc(union bpf_attr *attr)
struct bpf_map_memory mem;
u64 array_size;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return ERR_PTR(-EPERM);
array_size = sizeof(*array);
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 052580c33d26..477063c63b27 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -90,7 +90,7 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
u64 cost, n_buckets;
int err;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_tracing())
return ERR_PTR(-EPERM);
if (attr->map_flags & ~STACK_CREATE_FLAG_MASK)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ca60eafa6922..2b832eeafda9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1176,7 +1176,7 @@ static int map_freeze(const union bpf_attr *attr)
err = -EBUSY;
goto err_put;
}
- if (!capable(CAP_SYS_ADMIN)) {
+ if (!capable_bpf()) {
err = -EPERM;
goto err_put;
}
@@ -1635,7 +1635,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
(attr->prog_flags & BPF_F_ANY_ALIGNMENT) &&
- !capable(CAP_SYS_ADMIN))
+ !capable_bpf())
return -EPERM;
/* copy eBPF program license from user space */
@@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
is_gpl = license_is_gpl_compatible(license);
if (attr->insn_cnt == 0 ||
- attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
+ attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
return -E2BIG;
if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
type != BPF_PROG_TYPE_CGROUP_SKB &&
- !capable(CAP_SYS_ADMIN))
+ !capable_bpf())
return -EPERM;
bpf_prog_load_fixup_attach_type(attr);
@@ -1803,6 +1803,9 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
char tp_name[128];
int tp_fd, err;
+ if (!capable_bpf_tracing())
+ return -EPERM;
+
if (strncpy_from_user(tp_name, u64_to_user_ptr(attr->raw_tracepoint.name),
sizeof(tp_name) - 1) < 0)
return -EFAULT;
@@ -2081,7 +2084,10 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
struct bpf_prog *prog;
int ret = -ENOTSUPP;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf_net_admin)
+ /* test_run callback is available for networking progs only.
+ * Add capable_bpf_tracing() above when tracing progs become runable.
+ */
return -EPERM;
if (CHECK_ATTR(BPF_PROG_TEST_RUN))
return -EINVAL;
@@ -2118,7 +2124,7 @@ static int bpf_obj_get_next_id(const union bpf_attr *attr,
if (CHECK_ATTR(BPF_OBJ_GET_NEXT_ID) || next_id >= INT_MAX)
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
next_id++;
@@ -2144,7 +2150,7 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_PROG_GET_FD_BY_ID))
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
spin_lock_bh(&prog_idr_lock);
@@ -2178,7 +2184,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
attr->open_flags & ~BPF_OBJ_FLAG_MASK)
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
f_flags = bpf_get_file_flag(attr->open_flags);
@@ -2353,7 +2359,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
info.run_time_ns = stats.nsecs;
info.run_cnt = stats.cnt;
- if (!capable(CAP_SYS_ADMIN)) {
+ if (!capable_bpf()) {
info.jited_prog_len = 0;
info.xlated_prog_len = 0;
info.nr_jited_ksyms = 0;
@@ -2671,7 +2677,7 @@ static int bpf_btf_load(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_BTF_LOAD))
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
return btf_new_fd(attr);
@@ -2684,7 +2690,7 @@ static int bpf_btf_get_fd_by_id(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_BTF_GET_FD_BY_ID))
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
return btf_get_fd_by_id(attr->btf_id);
@@ -2753,7 +2759,7 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
if (CHECK_ATTR(BPF_TASK_FD_QUERY))
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf_tracing())
return -EPERM;
if (attr->task_fd_query.flags != 0)
@@ -2821,7 +2827,7 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
union bpf_attr attr = {};
int err;
- if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
+ if (sysctl_unprivileged_bpf_disabled && !capable_bpf())
return -EPERM;
err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f340cfe53c6e..b2a229557602 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -987,7 +987,7 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg)
reg->umax_value = U64_MAX;
/* constant backtracking is enabled for root only for now */
- reg->precise = capable(CAP_SYS_ADMIN) ? false : true;
+ reg->precise = capable_bpf() ? false : true;
}
/* Mark a register as having a completely unknown (scalar) value. */
@@ -9233,7 +9233,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
env->insn_aux_data[i].orig_idx = i;
env->prog = *prog;
env->ops = bpf_verifier_ops[env->prog->type];
- is_priv = capable(CAP_SYS_ADMIN);
+ is_priv = capable_bpf();
/* grab the mutex to protect few globals used by verifier */
if (!is_priv)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ca1255d14576..cdf8d6c8a430 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1246,7 +1246,7 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
u32 *ids, prog_cnt, ids_len;
int ret;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf_tracing())
return -EPERM;
if (event->attr.type != PERF_TYPE_TRACEPOINT)
return -EINVAL;
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index da5639a5bd3b..aa74be21f5b6 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -616,7 +616,7 @@ static int bpf_sk_storage_map_alloc_check(union bpf_attr *attr)
!attr->btf_key_type_id || !attr->btf_value_type_id)
return -EINVAL;
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return -EPERM;
if (attr->value_size >= KMALLOC_MAX_SIZE -
diff --git a/net/core/filter.c b/net/core/filter.c
index 17bc9af8f156..fae09b4d4da4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5990,7 +5990,7 @@ bpf_base_func_proto(enum bpf_func_id func_id)
break;
}
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return NULL;
switch (func_id) {
@@ -5999,7 +5999,9 @@ bpf_base_func_proto(enum bpf_func_id func_id)
case BPF_FUNC_spin_unlock:
return &bpf_spin_unlock_proto;
case BPF_FUNC_trace_printk:
- return bpf_get_trace_printk_proto();
+ if (capable_bpf_tracing())
+ return bpf_get_trace_printk_proto();
+ /* fall through */
default:
return NULL;
}
@@ -6563,7 +6565,7 @@ static bool cg_skb_is_valid_access(int off, int size,
return false;
case bpf_ctx_range(struct __sk_buff, data):
case bpf_ctx_range(struct __sk_buff, data_end):
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return false;
break;
}
@@ -6575,7 +6577,7 @@ static bool cg_skb_is_valid_access(int off, int size,
case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
break;
case bpf_ctx_range(struct __sk_buff, tstamp):
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable_bpf())
return false;
break;
default:
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index d27fd929abb9..0d5567962c4e 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -807,10 +807,20 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
}
}
+struct libcap {
+ struct __user_cap_header_struct hdr;
+ struct __user_cap_data_struct data[2];
+};
+
static int set_admin(bool admin)
{
cap_t caps;
- const cap_value_t cap_val = CAP_SYS_ADMIN;
+ /* need CAP_BPF to load progs and CAP_NET_ADMIN to run networking progs,
+ * and CAP_TRACING to create stackmap
+ */
+ const cap_value_t cap_net_admin = CAP_NET_ADMIN;
+ const cap_value_t cap_sys_admin = CAP_SYS_ADMIN;
+ struct libcap *cap;
int ret = -1;
caps = cap_get_proc();
@@ -818,11 +828,26 @@ static int set_admin(bool admin)
perror("cap_get_proc");
return -1;
}
- if (cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_val,
+ cap = (struct libcap *)caps;
+ if (cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_sys_admin, CAP_CLEAR)) {
+ perror("cap_set_flag clear admin");
+ goto out;
+ }
+ if (cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_net_admin,
admin ? CAP_SET : CAP_CLEAR)) {
- perror("cap_set_flag");
+ perror("cap_set_flag set_or_clear net");
goto out;
}
+ /* libcap is likely old and simply ignores CAP_BPF and CAP_TRACING,
+ * so update effective bits manually
+ */
+ if (admin) {
+ cap->data[1].effective |= 1 << (38 /* CAP_BPF */ - 32);
+ cap->data[1].effective |= 1 << (39 /* CAP_TRACING */ - 32);
+ } else {
+ cap->data[1].effective &= ~(1 << (38 - 32));
+ cap->data[1].effective &= ~(1 << (39 - 32));
+ }
if (cap_set_proc(caps)) {
perror("cap_set_proc");
goto out;
@@ -1051,9 +1076,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
static bool is_admin(void)
{
+ cap_flag_value_t net_priv = CAP_CLEAR;
+ bool tracing_priv = false;
+ bool bpf_priv = false;
+ struct libcap *cap;
cap_t caps;
- cap_flag_value_t sysadmin = CAP_CLEAR;
- const cap_value_t cap_val = CAP_SYS_ADMIN;
#ifdef CAP_IS_SUPPORTED
if (!CAP_IS_SUPPORTED(CAP_SETFCAP)) {
@@ -1066,11 +1093,14 @@ static bool is_admin(void)
perror("cap_get_proc");
return false;
}
- if (cap_get_flag(caps, cap_val, CAP_EFFECTIVE, &sysadmin))
- perror("cap_get_flag");
+ cap = (struct libcap *)caps;
+ bpf_priv = cap->data[1].effective & (1 << (38/* CAP_BPF */ - 32));
+ tracing_priv = cap->data[1].effective & (1 << (39/* CAP_TRACING */ - 32));
+ if (cap_get_flag(caps, CAP_NET_ADMIN, CAP_EFFECTIVE, &net_priv))
+ perror("cap_get_flag NET");
if (cap_free(caps))
perror("cap_free");
- return (sysadmin == CAP_SET);
+ return bpf_priv && tracing_priv && net_priv == CAP_SET;
}
static void get_unpriv_disabled()
--
2.20.0
^ permalink raw reply related
* [PATCH v3 bpf-next 1/3] capability: introduce CAP_BPF and CAP_TRACING
From: Alexei Starovoitov @ 2019-09-04 18:43 UTC (permalink / raw)
To: davem; +Cc: daniel, peterz, luto, netdev, bpf, kernel-team, linux-api
Split BPF and perf/tracing operations that are allowed under
CAP_SYS_ADMIN into corresponding CAP_BPF and CAP_TRACING.
For backward compatibility include them in CAP_SYS_ADMIN as well.
The end result provides simple safety model for applications that use BPF:
- for tracing program types
BPF_PROG_TYPE_{KPROBE, TRACEPOINT, PERF_EVENT, RAW_TRACEPOINT, etc}
use CAP_BPF and CAP_TRACING
- for networking program types
BPF_PROG_TYPE_{SCHED_CLS, XDP, CGROUP_SKB, SK_SKB, etc}
use CAP_BPF and CAP_NET_ADMIN
There are few exceptions from this simple rule:
- bpf_trace_printk() is allowed in networking programs, but it's using
ftrace mechanism, hence this helper needs additional CAP_TRACING.
- cpumap is used by XDP programs. Currently it's kept under CAP_SYS_ADMIN,
but could be relaxed to CAP_NET_ADMIN in the future.
- BPF_F_ZERO_SEED flag for hash/lru map is allowed under CAP_SYS_ADMIN only
to discourage production use.
- BPF HW offload is allowed under CAP_SYS_ADMIN.
- cg_sysctl, cg_device, lirc program types are neither networking nor tracing.
They can be loaded under CAP_BPF, but attach is allowed under CAP_NET_ADMIN.
This will be cleaned up in the future.
userid=nobody + (CAP_TRACING | CAP_NET_ADMIN) + CAP_BPF is safer than
typical setup with userid=root and sudo by existing bpf applications.
It's not secure, since these capabilities:
- allow bpf progs access arbitrary memory
- let tasks access any bpf map
- let tasks attach/detach any bpf prog
bpftool, bpftrace, bcc tools binaries should not be installed with
cap_bpf+cap_tracing, since unpriv users will be able to read kernel secrets.
CAP_BPF, CAP_NET_ADMIN, CAP_TRACING are roughly equal in terms of
damage they can make to the system.
Example:
CAP_NET_ADMIN can stop network traffic. CAP_BPF can write into map
and if that map is used by firewall-like bpf prog the network traffic
may stop.
CAP_BPF allows many bpf prog_load commands in parallel. The verifier
may consume large amount of memory and significantly slow down the system.
CAP_TRACING allows many kprobes that can slow down the system.
In the future more fine-grained bpf permissions may be added.
v2->v3:
- dropped ftrace and kallsyms from CAP_TRACING description.
In the future these mechanisms can start using it too.
- added CAP_SYS_ADMIN backward compatibility.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/capability.h | 18 +++++++++++
include/uapi/linux/capability.h | 48 ++++++++++++++++++++++++++++-
security/selinux/include/classmap.h | 4 +--
3 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index ecce0f43c73a..13eb49c75797 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -247,6 +247,24 @@ static inline bool ns_capable_setid(struct user_namespace *ns, int cap)
return true;
}
#endif /* CONFIG_MULTIUSER */
+
+static inline bool capable_bpf(void)
+{
+ return capable(CAP_SYS_ADMIN) || capable(CAP_BPF);
+}
+static inline bool capable_tracing(void)
+{
+ return capable(CAP_SYS_ADMIN) || capable(CAP_TRACING);
+}
+static inline bool capable_bpf_tracing(void)
+{
+ return capable(CAP_SYS_ADMIN) || (capable(CAP_BPF) && capable(CAP_TRACING));
+}
+static inline bool capable_bpf_net_admin(void)
+{
+ return (capable(CAP_SYS_ADMIN) || capable(CAP_BPF)) && capable(CAP_NET_ADMIN);
+}
+
extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct inode *inode);
extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 240fdb9a60f6..fb71dee0ac2b 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -366,8 +366,54 @@ struct vfs_ns_cap_data {
#define CAP_AUDIT_READ 37
+/*
+ * CAP_BPF allows the following BPF operations:
+ * - Loading all types of BPF programs
+ * - Creating all types of BPF maps except:
+ * - stackmap that needs CAP_TRACING
+ * - devmap that needs CAP_NET_ADMIN
+ * - cpumap that needs CAP_SYS_ADMIN
+ * - Advanced verifier features
+ * - Indirect variable access
+ * - Bounded loops
+ * - BPF to BPF function calls
+ * - Scalar precision tracking
+ * - Larger complexity limits
+ * - Dead code elimination
+ * - And potentially other features
+ * - Use of pointer-to-integer conversions in BPF programs
+ * - Bypassing of speculation attack hardening measures
+ * - Loading BPF Type Format (BTF) data
+ * - Iterate system wide loaded programs, maps, BTF objects
+ * - Retrieve xlated and JITed code of BPF programs
+ * - Access maps and programs via id
+ * - Use bpf_spin_lock() helper
+ *
+ * CAP_BPF and CAP_TRACING together allow the following:
+ * - bpf_probe_read to read arbitrary kernel memory
+ * - bpf_trace_printk to print data to ftrace ring buffer
+ * - Attach to raw_tracepoint
+ * - Query association between kprobe/tracepoint and bpf program
+ *
+ * CAP_BPF and CAP_NET_ADMIN together allow the following:
+ * - Attach to cgroup-bpf hooks and query
+ * - skb, xdp, flow_dissector test_run command
+ *
+ * CAP_NET_ADMIN allows:
+ * - Attach networking bpf programs to xdp, tc, lwt, flow dissector
+ */
+#define CAP_BPF 38
+
+/*
+ * CAP_TRACING allows:
+ * - Full use of perf_event_open(), similarly to the effect of
+ * kernel.perf_event_paranoid == -1
+ * - Creation of [ku][ret]probe
+ * - Attach tracing bpf programs to perf events
+ */
+#define CAP_TRACING 39
-#define CAP_LAST_CAP CAP_AUDIT_READ
+#define CAP_LAST_CAP CAP_TRACING
#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..0b364e245163 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -26,9 +26,9 @@
"audit_control", "setfcap"
#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
- "wake_alarm", "block_suspend", "audit_read"
+ "wake_alarm", "block_suspend", "audit_read", "bpf", "tracing"
-#if CAP_LAST_CAP > CAP_AUDIT_READ
+#if CAP_LAST_CAP > CAP_TRACING
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif
--
2.20.0
^ permalink raw reply related
* Re: [PATCH V40 04/29] lockdown: Enforce module signatures if the kernel is locked down
From: Matthew Garrett @ 2019-09-04 16:57 UTC (permalink / raw)
To: David Howells
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
Kees Cook, Jessica Yu
In-Reply-To: <3638.1567182673@warthog.procyon.org.uk>
On Fri, Aug 30, 2019 at 9:31 AM David Howells <dhowells@redhat.com> wrote:
>
> Matthew Garrett <matthewgarrett@google.com> wrote:
>
> > enum lockdown_reason {
> > LOCKDOWN_NONE,
> > + LOCKDOWN_MODULE_SIGNATURE,
> > LOCKDOWN_INTEGRITY_MAX,
> > LOCKDOWN_CONFIDENTIALITY_MAX,
> > };
>
> Aren't you mixing disjoint sets?
The goal is to be able to check whether any given lockdown reason is a
matter of integrity or confidentiality in a straightforward way.
> > + [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
>
> Wouldn't it be better to pass this string as a parameter to
> security_locked_down()?
I thought about that, but it's not how any other LSM hooks behave. I
think it's probably easier to revisit that when we see how other LSMs
want to make use of the data.
^ permalink raw reply
* Re: [PATCH V40 03/29] security: Add a static lockdown policy LSM
From: Matthew Garrett @ 2019-09-04 16:51 UTC (permalink / raw)
To: David Howells
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
Kees Cook
In-Reply-To: <3440.1567182506@warthog.procyon.org.uk>
On Fri, Aug 30, 2019 at 9:28 AM David Howells <dhowells@redhat.com> wrote:
>
> Matthew Garrett <matthewgarrett@google.com> wrote:
>
> > +static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>
> const char *const maybe?
Seems reasonable.
> > +static enum lockdown_reason lockdown_levels[] = {LOCKDOWN_NONE,
> > + LOCKDOWN_INTEGRITY_MAX,
> > + LOCKDOWN_CONFIDENTIALITY_MAX};
> > +
>
> const?
>
> Isn't this also a 1:1 mapping?
Sorry, a 1:1 mapping to what?
> > +static int lock_kernel_down(const char *where, enum lockdown_reason level)
>
> Is the last parameter the reason or the level? You're mixing the terms.
Fair.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-09-04 15:21 UTC (permalink / raw)
To: Daniel Borkmann, nicolas.dichtel@6wind.com, Alexei Starovoitov
Cc: Alexei Starovoitov, luto@amacapital.net, davem@davemloft.net,
peterz@infradead.org, rostedt@goodmis.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team, linux-api@vger.kernel.org
In-Reply-To: <5e36a193-8ad9-77e7-e2ff-429fb521a79c@iogearbox.net>
On 9/4/19 8:16 AM, Daniel Borkmann wrote:
> opening/creating BPF maps" error="Unable to create map
> /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted"
> subsys=daemon
> 2019-09-04T14:11:47.28178666Z level=fatal msg="Error while creating
> daemon" error="Unable to create map
> /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted"
> subsys=daemon
Ok. We have to include caps in both cap_sys_admin and cap_bpf then.
> And /same/ deployment with reverted patches, hence no CAP_BPF gets it up
> and running again:
>
> # kubectl get pods --all-namespaces -o wide
Can you share what this magic commands do underneath?
What user do they pick to start under? and what caps are granted?
^ permalink raw reply
* Re: [PATCH 08/11] usb: Add USB subsystem notifications [ver #7]
From: David Howells @ 2019-09-04 15:17 UTC (permalink / raw)
To: Alan Stern
Cc: dhowells, Guenter Roeck, viro, Casey Schaufler, Stephen Smalley,
Greg Kroah-Hartman, nicolas.dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-security-module, linux-fsdevel,
linux-api, linux-block, linux-kernel
In-Reply-To: <Pine.LNX.4.44L0.1909031316130.1859-100000@iolanthe.rowland.org>
Alan Stern <stern@rowland.harvard.edu> wrote:
> > > Unfortunately, I don't know how to fix it and don't have much time to
> > > investigate it right now - and it's something that can be added back later.
> >
> > The cause of your problem is quite simple:
> >
> > static int usbdev_notify(struct notifier_block *self,
> > unsigned long action, void *dev)
> > {
> > switch (action) {
> > case USB_DEVICE_ADD:
> > + post_usb_device_notification(dev, NOTIFY_USB_DEVICE_ADD, 0);
> > break;
> > case USB_DEVICE_REMOVE:
> > + post_usb_device_notification(dev, NOTIFY_USB_DEVICE_REMOVE, 0);
> > + usbdev_remove(dev);
> > + break;
> > + case USB_BUS_ADD:
> > + post_usb_bus_notification(dev, NOTIFY_USB_BUS_ADD, 0);
> > + break;
> > + case USB_BUS_REMOVE:
> > + post_usb_bus_notification(dev, NOTIFY_USB_BUS_REMOVE, 0);
> > usbdev_remove(dev);
> > break;
> > }
> >
> > The original code had usbdev_remove(dev) under the USB_DEVICE_REMOVE
> > case. The patch mistakenly moves it, putting it under the
> ------------------------------^^^^^
>
> Sorry, I should have said "duplicates" it.
Ah, thanks. I'd already removed the USB bus notifications, so I'll leave them
out for now.
David
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Daniel Borkmann @ 2019-09-04 15:16 UTC (permalink / raw)
To: Alexei Starovoitov, nicolas.dichtel@6wind.com, Alexei Starovoitov
Cc: Alexei Starovoitov, luto@amacapital.net, davem@davemloft.net,
peterz@infradead.org, rostedt@goodmis.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team, linux-api@vger.kernel.org
In-Reply-To: <46df2c36-4276-33c0-626b-c51e77b3a04f@fb.com>
On 9/4/19 3:39 AM, Alexei Starovoitov wrote:
> On 8/30/19 8:19 AM, Nicolas Dichtel wrote:
>> Le 29/08/2019 à 19:30, Alexei Starovoitov a écrit :
>> [snip]
>>> These are the links that showing that k8 can delegates caps.
>>> Are you saying that you know of folks who specifically
>>> delegate cap_sys_admin and cap_net_admin _only_ to a container to run bpf in there?
>>>
>> Yes, we need cap_sys_admin only to load bpf:
>> tc filter add dev eth0 ingress matchall action bpf obj ./tc_test_kern.o sec test
>>
>> I'm not sure to understand why cap_net_admin is not enough to run the previous
>> command (ie why load is forbidden).
>
> because bpf syscall prog_load command requires cap_sys_admin in
> the current implementation.
>
>> I want to avoid sys_admin, thus cap_bpf will be ok. But we need to manage the
>> backward compatibility.
>
> re: backward compatibility...
> do you know of any case where task is running under userid=nobody
> with cap_sys_admin and cap_net_admin in order to do bpf ?
>
> If not then what is the concern about compatibility?
Finally managed to find some cycles to pull up a k8s cluster. Looks like it would
break deployments with the patches as-is right away; meaning, any constellation
where BPF is used inside the pod.
With CAP_BPF patches applied on bpf-next:
# kubectl apply -f ./cilium.yaml
[...]
# kubectl get pods --all-namespaces -o wide
NAMESPACE NAME READY STATUS RESTARTS AGE IP NODE NOMINATED NODE READINESS GATES
kube-system cilium-cz9qs 0/1 CrashLoopBackOff 4 2m36s 192.168.1.125 apoc <none> <none>
kube-system cilium-operator-6c7c6c788b-xcm9d 0/1 Pending 0 2m36s <none> <none> <none> <none>
kube-system coredns-5c98db65d4-6nhpg 0/1 ContainerCreating 0 4m12s <none> apoc <none> <none>
kube-system coredns-5c98db65d4-l5b94 0/1 ContainerCreating 0 4m12s <none> apoc <none> <none>
kube-system etcd-apoc 1/1 Running 0 3m26s 192.168.1.125 apoc <none> <none>
kube-system kube-apiserver-apoc 1/1 Running 0 3m32s 192.168.1.125 apoc <none> <none>
kube-system kube-controller-manager-apoc 1/1 Running 0 3m18s 192.168.1.125 apoc <none> <none>
kube-system kube-proxy-jj9kz 1/1 Running 0 4m12s 192.168.1.125 apoc <none> <none>
kube-system kube-scheduler-apoc 1/1 Running 0 3m26s 192.168.1.125 apoc <none> <none>
# kubectl -n kube-system logs --timestamps cilium-cz9qs
[...]
2019-09-04T14:11:46.399478585Z level=info msg="Cilium 1.6.90 ba0ed147b 2019-09-03T21:20:30+02:00 go version go1.12.8 linux/amd64" subsys=daemon
2019-09-04T14:11:46.410564471Z level=info msg="cilium-envoy version: b7a919ebdca3d3bbc6aae51357e78e9c603450ae/1.11.1/Modified/RELEASE/BoringSSL" subsys=daemon
2019-09-04T14:11:46.446983926Z level=info msg="clang (7.0.0) and kernel (5.3.0) versions: OK!" subsys=daemon
[...]
2019-09-04T14:11:47.27988188Z level=info msg="Mounting BPF filesystem at /run/cilium/bpffs" subsys=bpf
2019-09-04T14:11:47.279904256Z level=info msg="Detected mounted BPF filesystem at /run/cilium/bpffs" subsys=bpf
2019-09-04T14:11:47.280205098Z level=info msg="Valid label prefix configuration:" subsys=labels-filter
2019-09-04T14:11:47.280214528Z level=info msg=" - :io.kubernetes.pod.namespace" subsys=labels-filter
2019-09-04T14:11:47.28021738Z level=info msg=" - :io.cilium.k8s.namespace.labels" subsys=labels-filter
2019-09-04T14:11:47.280220836Z level=info msg=" - :app.kubernetes.io" subsys=labels-filter
2019-09-04T14:11:47.280223355Z level=info msg=" - !:io.kubernetes" subsys=labels-filter
2019-09-04T14:11:47.280225723Z level=info msg=" - !:kubernetes.io" subsys=labels-filter
2019-09-04T14:11:47.280228095Z level=info msg=" - !:.*beta.kubernetes.io" subsys=labels-filter
2019-09-04T14:11:47.280230409Z level=info msg=" - !:k8s.io" subsys=labels-filter
2019-09-04T14:11:47.280232699Z level=info msg=" - !:pod-template-generation" subsys=labels-filter
2019-09-04T14:11:47.280235569Z level=info msg=" - !:pod-template-hash" subsys=labels-filter
2019-09-04T14:11:47.28023792Z level=info msg=" - !:controller-revision-hash" subsys=labels-filter
2019-09-04T14:11:47.280240253Z level=info msg=" - !:annotation.*" subsys=labels-filter
2019-09-04T14:11:47.280242566Z level=info msg=" - !:etcd_node" subsys=labels-filter
2019-09-04T14:11:47.28026585Z level=info msg="Initializing daemon" subsys=daemon
2019-09-04T14:11:47.281344002Z level=info msg="Detected MTU 1500" subsys=mtu
2019-09-04T14:11:47.281771889Z level=error msg="Error while opening/creating BPF maps" error="Unable to create map /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted" subsys=daemon
2019-09-04T14:11:47.28178666Z level=fatal msg="Error while creating daemon" error="Unable to create map /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted" subsys=daemon
And /same/ deployment with reverted patches, hence no CAP_BPF gets it up and running again:
# kubectl get pods --all-namespaces -o wide
NAMESPACE NAME READY STATUS RESTARTS AGE IP NODE NOMINATED NODE READINESS GATES
kube-system cilium-cz9qs 1/1 Running 13 50m 192.168.1.125 apoc <none> <none>
kube-system cilium-operator-6c7c6c788b-xcm9d 0/1 Pending 0 50m <none> <none> <none> <none>
kube-system coredns-5c98db65d4-6nhpg 1/1 Running 0 52m 10.217.0.91 apoc <none> <none>
kube-system coredns-5c98db65d4-l5b94 1/1 Running 0 52m 10.217.0.225 apoc <none> <none>
kube-system etcd-apoc 1/1 Running 1 51m 192.168.1.125 apoc <none> <none>
kube-system kube-apiserver-apoc 1/1 Running 1 51m 192.168.1.125 apoc <none> <none>
kube-system kube-controller-manager-apoc 1/1 Running 1 51m 192.168.1.125 apoc <none> <none>
kube-system kube-proxy-jj9kz 1/1 Running 1 52m 192.168.1.125 apoc <none> <none>
kube-system kube-scheduler-apoc 1/1 Running 1 51m 192.168.1.125 apoc <none> <none>
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 11/11] smack: Implement the watch_key and post_notification hooks [untested] [ver #7]
From: Casey Schaufler @ 2019-09-04 14:56 UTC (permalink / raw)
To: David Howells
Cc: viro, Stephen Smalley, Greg Kroah-Hartman, nicolas.dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-security-module,
linux-fsdevel, linux-api, linux-block, linux-kernel, casey
In-Reply-To: <31205.1567598939@warthog.procyon.org.uk>
On 9/4/2019 5:08 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> I rebuilt with keys-next, updated the tests again, and now
>> the suite looks to be running trouble free.
> Can I put you down as an Acked-by or something on this patch?
I haven't done anything to see if the patch is actually useful.
I don't have much (read: anything) in the way of key tests for
Smack, so I can't say if this is what I want long term. But as
it does appear harmless, yes, you can add my Acked-by on this.
>
> Thanks,
> David
^ permalink raw reply
* Re: [PATCH 11/11] smack: Implement the watch_key and post_notification hooks [untested] [ver #7]
From: David Howells @ 2019-09-04 12:08 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, viro, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-security-module, linux-fsdevel, linux-api, linux-block,
linux-kernel
In-Reply-To: <23d61564-026e-b37a-8b16-ce68d5949f6c@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> I rebuilt with keys-next, updated the tests again, and now
> the suite looks to be running trouble free.
Can I put you down as an Acked-by or something on this patch?
Thanks,
David
^ permalink raw reply
* RE: [PATCH 08/11] usb: Add USB subsystem notifications [ver #7]
From: Yoshihiro Shimoda @ 2019-09-04 1:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: David Howells, viro@zeniv.linux.org.uk, Casey Schaufler,
Stephen Smalley, nicolas.dichtel@6wind.com, raven@themaw.net,
Christian Brauner, keyrings@vger.kernel.org,
linux-usb@vger.kernel.org, linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190903093720.GD12325@kroah.com>
Hi Greg,
> From: Greg Kroah-Hartman, Sent: Tuesday, September 3, 2019 6:37 PM
<snip>
> > > +void post_usb_bus_notification(const struct usb_bus *ubus,
> >
> > This function's argument is struct usb_bus *, but ...
> >
> > > + enum usb_notification_type subtype, u32 error)
> > > +{
> > > + post_usb_notification(ubus->bus_name, subtype, error);
> > > +}
> > > +#endif
> > > +
> > > static int usbdev_notify(struct notifier_block *self,
> > > unsigned long action, void *dev)
> > > {
> > > switch (action) {
> > > case USB_DEVICE_ADD:
> > > + post_usb_device_notification(dev, NOTIFY_USB_DEVICE_ADD, 0);
> > > break;
> > > case USB_DEVICE_REMOVE:
> > > + post_usb_device_notification(dev, NOTIFY_USB_DEVICE_REMOVE, 0);
> > > + usbdev_remove(dev);
> > > + break;
> > > + case USB_BUS_ADD:
> > > + post_usb_bus_notification(dev, NOTIFY_USB_BUS_ADD, 0);
> > > + break;
> > > + case USB_BUS_REMOVE:
> > > + post_usb_bus_notification(dev, NOTIFY_USB_BUS_REMOVE, 0);
> > > usbdev_remove(dev);
> >
> > this function calls usbdev_remove() with incorrect argument if the action
> > is USB_BUS_REMOVE. So, this seems to cause the following issue [1] on
> > my environment (R-Car H3 / r8a7795 on next-20190902) [2]. However, I have
> > no idea how to fix the issue, so I report this issue at the first step.
>
> As a few of us just discussed this on IRC, these bus notifiers should
> probably be dropped as these are the incorrect structure type as you
> found out. Thanks for the report.
Thank you for the discussion. I got it.
Best regards,
Yoshihiro Shimoda
> greg k-h
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-09-04 1:39 UTC (permalink / raw)
To: nicolas.dichtel@6wind.com, Alexei Starovoitov, Daniel Borkmann
Cc: Alexei Starovoitov, luto@amacapital.net, davem@davemloft.net,
peterz@infradead.org, rostedt@goodmis.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team, linux-api@vger.kernel.org
In-Reply-To: <59ac111e-7ce7-5e00-32c9-9b55482fe701@6wind.com>
On 8/30/19 8:19 AM, Nicolas Dichtel wrote:
> Le 29/08/2019 à 19:30, Alexei Starovoitov a écrit :
> [snip]
>> These are the links that showing that k8 can delegates caps.
>> Are you saying that you know of folks who specifically
>> delegate cap_sys_admin and cap_net_admin _only_ to a container to run bpf in there?
>>
> Yes, we need cap_sys_admin only to load bpf:
> tc filter add dev eth0 ingress matchall action bpf obj ./tc_test_kern.o sec test
>
> I'm not sure to understand why cap_net_admin is not enough to run the previous
> command (ie why load is forbidden).
because bpf syscall prog_load command requires cap_sys_admin in
the current implementation.
> I want to avoid sys_admin, thus cap_bpf will be ok. But we need to manage the
> backward compatibility.
re: backward compatibility...
do you know of any case where task is running under userid=nobody
with cap_sys_admin and cap_net_admin in order to do bpf ?
If not then what is the concern about compatibility?
^ permalink raw reply
* Re: [PATCH 11/11] smack: Implement the watch_key and post_notification hooks [untested] [ver #7]
From: David Howells @ 2019-09-03 22:39 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, viro, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-security-module, linux-fsdevel, linux-api, linux-block,
linux-kernel
In-Reply-To: <23d61564-026e-b37a-8b16-ce68d5949f6c@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> I rebuilt with keys-next, updated the tests again, and now
> the suite looks to be running trouble free.
Glad to hear that, thanks.
> I do see a message SKIP DUE TO DISABLED SELINUX which I take to mean that
> there is an SELinux specific test.
tests/bugzillas/bz1031154/runtest.sh
David
^ permalink raw reply
* Re: [PATCH 11/11] smack: Implement the watch_key and post_notification hooks [untested] [ver #7]
From: Casey Schaufler @ 2019-09-03 22:16 UTC (permalink / raw)
To: David Howells
Cc: viro, Stephen Smalley, Greg Kroah-Hartman, nicolas.dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-security-module,
linux-fsdevel, linux-api, linux-block, linux-kernel, casey
In-Reply-To: <11467.1567534014@warthog.procyon.org.uk>
On 9/3/2019 11:06 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> Built from your tree.
> What branch? keys-next?
I rebuilt with keys-next, updated the tests again, and now
the suite looks to be running trouble free. I do see a message
SKIP DUE TO DISABLED SELINUX which I take to mean that there
is an SELinux specific test.
>
>> keyctl move 483362336 1065401533 @s
>> keyctl_move: Operation not supported
> Odd. That should be unconditional if you have CONFIG_KEYS and v5.3-rc1. Can
> you try:
>
> keyctl supports
>
> or just:
>
> keyctl add user a a @s
>
> which will give you an id, say 1234, then:
>
> keyctl move 1234 @s @u
>
> see if that works.
>
> David
^ permalink raw reply
* Re: [PATCH 11/11] smack: Implement the watch_key and post_notification hooks [untested] [ver #7]
From: David Howells @ 2019-09-03 18:06 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, viro, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-security-module, linux-fsdevel, linux-api, linux-block,
linux-kernel
In-Reply-To: <87bf0363-af77-1e5a-961f-72730e39e3a6@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> Built from your tree.
What branch? keys-next?
> keyctl move 483362336 1065401533 @s
> keyctl_move: Operation not supported
Odd. That should be unconditional if you have CONFIG_KEYS and v5.3-rc1. Can
you try:
keyctl supports
or just:
keyctl add user a a @s
which will give you an id, say 1234, then:
keyctl move 1234 @s @u
see if that works.
David
^ 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