* Re: Official Linux system wrapper library?
From: Joseph Myers @ 2018-11-12 16:43 UTC (permalink / raw)
To: Florian Weimer
Cc: Michael Kerrisk (man-pages), Willy Tarreau, Daniel Colascione,
linux-kernel, Joel Fernandes, Linux API, Vlastimil Babka,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <878t1zx4gj.fsf@oldenburg.str.redhat.com>
On Sun, 11 Nov 2018, Florian Weimer wrote:
> People may have disappeared from glibc development who have objected to
> gettid. I thought this was the case with strlcpy/strlcat, but it was
> not.
Well, I know of two main people who were objecting to the notion of adding
bindings for all non-obsolescent syscalls, Linux-specific if not suitable
for adding to the OS-independent GNU API, and neither seems to have posted
in the past year.
> At present, it takes one semi-active glibc contributor to block addition
> of a system call. The process to override a sustained objection has
> never been used successfully, and it is a lot of work to get it even
> started.
We don't have such a process. (I've suggested, e.g. in conversation with
Carlos at the Cauldron, that we should have something involving a
supermajority vote of the GNU maintainers for glibc in cases where we're
unable to reach a consensus in the community as a whole.)
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Jonathan Corbet @ 2018-11-12 16:08 UTC (permalink / raw)
To: Greg KH
Cc: Carlos O'Donell, Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API
In-Reply-To: <20181112023630.GA20359@kroah.com>
On Sun, 11 Nov 2018 18:36:30 -0800
Greg KH <gregkh@linuxfoundation.org> wrote:
> We should have a checklist. That's a great idea. Now to find someone
> to write it... :)
Do we think the LPC session might have the right people to create such a
thing? If so, I can try to put together a coherent presentation of the
result.
jon
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-12 14:40 UTC (permalink / raw)
To: Theodore Y. Ts'o, Szabolcs Nagy, Daniel Colascione,
Florian Weimer, nd, Michael Kerrisk (man-pages), linux-kernel,
Joel Fernandes, Linux API, Willy Tarreau, Vlastimil Babka,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <20181112143506.GC7377@thunk.org>
On Mon, Nov 12, 2018 at 6:35 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Mon, Nov 12, 2018 at 12:45:26PM +0000, Szabolcs Nagy wrote:
>> >> A lot of the new system calls lack clear specifications or are just
>> >> somewhat misdesigned. For example, pkey_alloc
>> > [snip]
>> >> getrandom still causes boot delays
>
> I'll note that what some people consider misdesigns, others consider
> "fix CVE's".
>
> Some people may consider it more important to avoid boot delays;
> others would consider internet-wide security problems, ala
> https://factorable.net to be higher priority.
>
> It's clear this is one area where I and some glibc developers have had
> a difference of opinion. The bigger problem is that if a single glibc
> developer is able to veto any new system call, maybe we *do* need to
> have a kernel-provided library which bypasses glibc....
Historically speaking, the liberum veto has not led to good governance.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Theodore Y. Ts'o @ 2018-11-12 14:35 UTC (permalink / raw)
To: Szabolcs Nagy
Cc: Daniel Colascione, Florian Weimer, nd,
Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <45cf58e0-909e-262c-5b9f-b91d62350a79@arm.com>
On Mon, Nov 12, 2018 at 12:45:26PM +0000, Szabolcs Nagy wrote:
> >> A lot of the new system calls lack clear specifications or are just
> >> somewhat misdesigned. For example, pkey_alloc
> > [snip]
> >> getrandom still causes boot delays
I'll note that what some people consider misdesigns, others consider
"fix CVE's".
Some people may consider it more important to avoid boot delays;
others would consider internet-wide security problems, ala
https://factorable.net to be higher priority.
It's clear this is one area where I and some glibc developers have had
a difference of opinion. The bigger problem is that if a single glibc
developer is able to veto any new system call, maybe we *do* need to
have a kernel-provided library which bypasses glibc....
- Ted
^ permalink raw reply
* [PATCH v4 4/4] namei: aggressively check for nd->root escape on ".." resolution
From: Aleksa Sarai @ 2018-11-12 14:26 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Jann Horn, Eric Biederman, Andy Lutomirski,
Christian Brauner, David Drysdale, Aleksa Sarai, containers,
linux-fsdevel, linux-api, linux-kernel, linux-arch
In-Reply-To: <20181112142654.341-1-cyphar@cyphar.com>
This patch allows for O_BENEATH and O_THISROOT to safely permit ".."
resolution (in the case of O_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
O_THISROOT will chroot(2)-style scope it). "magic link" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
O_THISROOT and O_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "magic link"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of O_THISROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of path_is_under() here might seem suspect, but on further
inspection of the most important race (a path was *inside* the root but
is now *outside*), there appears to be no attack potential. If
path_is_under() occurs before the rename, then the path will be resolved
but since the path was originally inside the root there is no escape.
Subsequent ".." jumps are guaranteed to check path_is_under() (by
construction, &rename_lock or &mount_lock must have been taken by the
attacker after path_is_under() returned in the victim), and thus will
not be able to escape from the previously-inside-root path. Walking down
is still safe since the entire subtree was moved (either by rename(2) or
MS_MOVE) and because (as discussed above) walking down is safe.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 48 +++++++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 459faea5b832..b2c962f149c7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -493,7 +493,7 @@ struct nameidata {
struct path root;
struct inode *inode; /* path.dentry.d_inode */
unsigned int flags;
- unsigned seq, m_seq;
+ unsigned seq, m_seq, r_seq;
int last_type;
unsigned depth;
int total_link_count;
@@ -1741,19 +1741,35 @@ 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 | LOOKUP_CHROOT)))
- return -EXDEV;
+ int error = 0;
+
if (!nd->root.mnt)
set_root(nd);
- if (nd->flags & LOOKUP_RCU) {
- return follow_dotdot_rcu(nd);
- } else
- return follow_dotdot(nd);
+ if (nd->flags & LOOKUP_RCU)
+ error = follow_dotdot_rcu(nd);
+ else
+ error = follow_dotdot(nd);
+ if (error)
+ return error;
+
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
+ bool m_retry = read_seqretry(&mount_lock, nd->m_seq);
+ bool r_retry = read_seqretry(&rename_lock, nd->r_seq);
+
+ /*
+ * Don't bother checking unless there's a racing
+ * rename(2) or MS_MOVE.
+ */
+ if (likely(!m_retry && !r_retry))
+ return 0;
+
+ if (m_retry && !(nd->flags & LOOKUP_RCU))
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (r_retry)
+ nd->r_seq = read_seqbegin(&rename_lock);
+ if (!path_is_under(&nd->path, &nd->root))
+ return -EXDEV;
+ }
}
return 0;
}
@@ -2274,6 +2290,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->last_type = LAST_ROOT; /* if there are only slashes... */
nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
nd->depth = 0;
+
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (unlikely(flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)))
+ nd->r_seq = read_seqbegin(&rename_lock);
+
if (flags & LOOKUP_ROOT) {
struct dentry *root = nd->root.dentry;
struct inode *inode = root->d_inode;
@@ -2284,7 +2305,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
if (flags & LOOKUP_RCU) {
nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
nd->root_seq = nd->seq;
- nd->m_seq = read_seqbegin(&mount_lock);
} else {
path_get(&nd->path);
}
@@ -2295,8 +2315,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.mnt = NULL;
nd->path.dentry = NULL;
- nd->m_seq = read_seqbegin(&mount_lock);
-
if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
--
2.19.1
^ permalink raw reply related
* [PATCH v4 3/4] namei: O_THISROOT: chroot-like path resolution
From: Aleksa Sarai @ 2018-11-12 14:26 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, linux-api,
Andy Lutomirski, Jann Horn, David Drysdale, Aleksa Sarai,
containers, linux-fsdevel, linux-kernel, linux-arch
In-Reply-To: <20181112142654.341-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 O_XDEV
and O_NOMAGICLINKS[**] 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 O_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 openat(2) semantic change with O_THISROOT is that
absolute pathnames no longer cause dirfd to be ignored completely. The
rationale is that O_THISROOT 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).
Currently this is only enabled for openat(2), and similar to O_BENEATH
and family requires more discussion about extending it to more *at(2)
syscalls as well as extending AT_EMPTY_PATH support.
[1]: https://github.com/cyphar/filepath-securejoin
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: <linux-api@vger.kernel.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 6 +++---
fs/open.c | 4 +++-
include/linux/fcntl.h | 2 +-
include/linux/namei.h | 1 +
include/uapi/asm-generic/fcntl.h | 3 +++
6 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index e343618736f7..4c36c5b9fdb9 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(25 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(26 - 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 b8d2bee89b78..459faea5b832 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1097,7 +1097,7 @@ 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_CHROOT)))
return ERR_PTR(-EXDEV);
}
if (IS_ERR_OR_NULL(res))
@@ -1746,7 +1746,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
* 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_CHROOT)))
return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
@@ -2297,7 +2297,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
- if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
return ERR_PTR(error);
diff --git a/fs/open.c b/fs/open.c
index 3e73f940f56e..4ba44b07f3ff 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -960,7 +960,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* cannot have anything other than the below set of flags
*/
flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
- O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS | O_THISROOT;
acc_mode = 0;
}
@@ -997,6 +997,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_NO_MAGICLINKS;
if (flags & O_NOSYMLINKS)
lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & O_THISROOT)
+ lookup_flags |= LOOKUP_CHROOT;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 864399c2fdd2..46c92bbfce4a 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
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_BENEATH | O_XDEV | \
- O_NOMAGICLINKS | O_NOSYMLINKS)
+ O_NOMAGICLINKS | O_NOSYMLINKS | O_THISROOT)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 82b5039d27a6..b6865eda86d5 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
Implies LOOKUP_NO_MAGICLINKS. */
+#define LOOKUP_CHROOT 0x100000 /* Treat dirfd as %current->fs->root. */
extern int path_pts(struct path *path);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index b2d3811843e7..194f5de9ba51 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -113,6 +113,9 @@
#ifndef O_NOSYMLINKS
#define O_NOSYMLINKS 01000000000
#endif
+#ifndef O_THISROOT
+#define O_THISROOT 02000000000
+#endif
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
--
2.19.1
^ permalink raw reply related
* [PATCH v4 2/4] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2018-11-12 14:26 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, linux-api,
Andy Lutomirski, Jann Horn, David Drysdale, Aleksa Sarai,
containers, linux-fsdevel, linux-kernel, linux-arch
In-Reply-To: <20181112142654.341-1-cyphar@cyphar.com>
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* O_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* O_NOMAGICLINKS: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* O_NOSYMLINKS: Disallows symlink jumping *of any kind*. Implies
O_NOMAGICLINKS (obviously).
* O_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 O_NO*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2). In future it may be necessary to enable
these for *at(2) flags, as well as expanding support for AT_EMPTY_PATH.
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: <linux-api@vger.kernel.org>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 76 +++++++++++++++++++++++++++-----
fs/open.c | 11 ++++-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 +++
include/uapi/asm-generic/fcntl.h | 17 +++++++
6 files changed, 102 insertions(+), 14 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..e343618736f7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index faefca58348d..b8d2bee89b78 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1053,6 +1059,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();
@@ -1083,14 +1092,23 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ /* If we just jumped it was because of a procfs-style link. */
+ if (unlikely(nd->flags & LOOKUP_JUMPED)) {
+ if (unlikely(nd->flags & LOOKUP_NO_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 == '/'))
;
}
@@ -1271,12 +1289,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1333,6 +1355,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1377,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *inode = nd->inode;
while (1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1379,6 +1406,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1393,6 +1422,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1481,8 +1512,11 @@ static int path_parent_directory(struct path *path)
static int follow_dotdot(struct nameidata *nd)
{
while(1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -1491,6 +1525,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1705,6 +1741,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) {
@@ -2253,6 +2296,15 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ nd->root = nd->path;
+ if (!(nd->flags & LOOKUP_RCU))
+ path_get(&nd->root);
+ }
if (*s == '/') {
if (likely(!nd->root.mnt))
set_root(nd);
@@ -2261,9 +2313,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
s = ERR_PTR(error);
return s;
}
- error = dirfd_path_init(nd);
- if (unlikely(error))
- return ERR_PTR(error);
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
return s;
}
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..3e73f940f56e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -959,7 +959,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* If we have O_PATH in the open flag. Then we
* cannot have anything other than the below set of flags
*/
- flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
acc_mode = 0;
}
@@ -988,6 +989,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOMAGICLINKS)
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..864399c2fdd2 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOMAGICLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..82b5039d27a6 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* 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);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..b2d3811843e7 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,23 @@
#define O_NDELAY O_NONBLOCK
#endif
+/*
+ * These are identical to their AT_* counterparts (which affect the entireity
+ * of path resolution).
+ */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000
+#endif
+#ifndef O_NOMAGICLINKS
+#define O_NOMAGICLINKS 00200000000
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
--
2.19.1
^ permalink raw reply related
* [PATCH v4 1/4] namei: split out nd->dfd handling to dirfd_path_init
From: Aleksa Sarai @ 2018-11-12 14:26 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Jann Horn,
Christian Brauner, David Drysdale, Aleksa Sarai, containers,
linux-fsdevel, linux-api, linux-kernel, linux-arch
In-Reply-To: <20181112142654.341-1-cyphar@cyphar.com>
Previously, path_init's handling of *at(dfd, ...) was only done once,
but with O_BENEATH (and O_THISROOT) we have to parse the initial
nd->path at different times (before or after absolute path handling)
depending on whether we have been asked to scope resolution within a
root.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 103 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 59 insertions(+), 44 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index fb913148d4d1..faefca58348d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2168,9 +2168,59 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ do {
+ seq = read_seqcount_begin(&fs->seq);
+ nd->path = fs->pwd;
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
+ } while (read_seqcount_retry(&fs->seq, seq));
+ } else {
+ get_fs_pwd(current->fs, &nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ } else {
+ /* Caller must check execute permissions on the starting path component */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *dentry;
+
+ if (!f.file)
+ return -EBADF;
+
+ dentry = f.file->f_path.dentry;
+
+ if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
+ fdput(f);
+ return -ENOTDIR;
+ }
+
+ nd->path = f.file->f_path;
+ if (nd->flags & LOOKUP_RCU) {
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+ } else {
+ path_get(&nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ fdput(f);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
@@ -2204,52 +2254,17 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- do {
- seq = read_seqcount_begin(&fs->seq);
- nd->path = fs->pwd;
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
- } while (read_seqcount_retry(&fs->seq, seq));
- } else {
- get_fs_pwd(current->fs, &nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_PTR(-ENOTDIR);
- }
-
- nd->path = f.file->f_path;
- if (flags & LOOKUP_RCU) {
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
- } else {
- path_get(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- fdput(f);
+ if (likely(!nd->root.mnt))
+ set_root(nd);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
--
2.19.1
^ permalink raw reply related
* [PATCH v4 0/4] namei: O_* flags to restrict path resolution
From: Aleksa Sarai @ 2018-11-12 14:26 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Jann Horn,
Christian Brauner, David Drysdale, containers, linux-fsdevel,
linux-api, Aleksa Sarai, linux-kernel, linux-arch
Sorry for not sending a series earlier, I've been busy with assignments.
Patch changelog:
v4:
* Remove AT_* flag reservations, as they require more discussion.
* Switch to path_is_under() over __d_path() for breakout checking.
* Make O_XDEV no longer block openat("/tmp", "/", O_XDEV) -- dirfd
is now ignored for absolute paths to match other flags.
* Improve the dirfd_path_init() refactor and move it to a separate
commit.
* Remove reference to Linux-capsicum.
* Switch "proclink" name to "magic link".
v3: [resend]
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
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:
* O_XDEV blocks all mountpoint crossings (upwards, downwards, or
through absolute links). Absolute pathnames alone in openat(2) do
not trigger this.
* O_NOMAGICLINKS 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 O_NOFOLLOW
in that it applies to all path components. However, you can do
open(O_NOFOLLOW|O_NOMAGICLINKS|O_PATH) 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".
* O_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 ".." (see patch 4 for more detail).
Currently O_BENEATH implies O_NOMAGICLINKS, because it can trivially
beam you around the filesystem (breaking the protection). In future,
there might be similar safety checks as in patch 4, but that
requires more discussion.
In addition, two new flags were added that expand on the above ideas:
* O_NOSYMLINKS does what it says on the tin. No symlink resolution is
allowed at all, including "magic links". Just as with O_NOMAGICLINKS
this can still be used with (O_PATH|O_NOFOLLOW) to open an fd for
the symlink as long as no parent path had a symlink component.
* O_THISROOT is an extension of O_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 O_BENEATH) then an error is
generated, and similar to O_BENEATH it is not permitted to cross
"magic links" with O_THISROOT.
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 O_THISROOT (such as
CVE-2017-1002101, CVE-2017-1002102, CVE-2018-15664, to name a few).
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <drysdale@google.com>
Cc: <containers@lists.linux-foundation.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-api@vger.kernel.org>
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Aleksa Sarai (4):
namei: split out nd->dfd handling to dirfd_path_init
namei: O_BENEATH-style path resolution flags
namei: O_THISROOT: chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
fs/fcntl.c | 2 +-
fs/namei.c | 205 ++++++++++++++++++++++---------
fs/open.c | 13 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 8 ++
include/uapi/asm-generic/fcntl.h | 20 +++
6 files changed, 189 insertions(+), 62 deletions(-)
--
2.19.1
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-12 13:19 UTC (permalink / raw)
To: Florian Weimer
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <875zx2vhpd.fsf@oldenburg.str.redhat.com>
On Mon, Nov 12, 2018 at 12:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
> * Daniel Colascione:
>
>> If the kernel provides a system call, libc should provide a C wrapper
>> for it, even if in the opinion of the libc maintainers, that system
>> call is flawed.
>
> It's not that simple, I think. What about bdflush? socketcall?
> getxpid? osf_gettimeofday? set_robust_list?
What about them? Mentioning that these system calls exist is not in
itself an argument.
> There are quite a few
> irregularities
So?
> and some editorial discretion appears to be unavoidable.
That's an assertion, not an argument, and I strongly disagree. *Why*
do you think "editorial discretion" is unavoidable? What privileges
glibc's judgement here? What would go wrong if socketcall and
set_robust_list and so on had wrappers? If applications chose to use
these lower-level wrappers instead of higher-level facilities, they
take on responsibility for using the APIs properly.
> Even if we were to provide perfectly consistent system call wrappers
> under separate names, we'd still expose different calling conventions
> for things like off_t to applications, which would make using some of
> the system calls quite difficult and surprisingly non-portable.
We can learn something from how Windows does things. On that system,
what we think of as "libc" is actually two parts. (More, actually, but
I'm simplifying.) At the lowest level, you have the semi-documented
ntdll.dll, which contains raw system call wrappers and arcane
kernel-userland glue. On top of ntdll live the "real" libc
(msvcrt.dll, kernel32.dll, etc.) that provide conventional
application-level glue. The tight integration between ntdll.dll and
the kernel allows Windows to do very impressive things. (For example,
on x86_64, Windows has no 32-bit ABI as far as the kernel is
concerned! You can still run 32-bit programs though, and that works
via ntdll.dll essentially shimming every system call and switching the
processor between long and compatibility mode as needed.) Normally,
you'd use the higher-level capabilities, but if you need something in
ntdll (e.g., if you're Cygwin) nothing stops your calling into the
lower-level system facilities directly. ntdll is tightly bound to the
kernel; the higher-level libc, not so.
We should adopt a similar approach. Shipping a lower-level
"liblinux.so" tightly bound to the kernel would not only let the
kernel bypass glibc's "editorial discretion" in exposing new
facilities to userspace, but would also allow for tighter user-kernel
integration that one can achieve with a simplistic syscall(2)-style
escape hatch. (For example, for a long time now, I've wanted to go
beyond POSIX and improve the system's signal handling API, and this
improvement requires userspace cooperation.) The vdso is probably too
small and simplistic to serve in this role; I'd want a real library.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Szabolcs Nagy @ 2018-11-12 12:45 UTC (permalink / raw)
To: Daniel Colascione, Florian Weimer
Cc: nd, Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZues5SEESpJU=6MDTrPXTA1KTZFGNQE4Lw4t0fO-WBTU62w@mail.gmail.com>
On 11/11/18 14:22, Daniel Colascione wrote:
> On Sun, Nov 11, 2018 at 3:09 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> We had a patch for the membarrier system call, but the kernel developers
>> could not tell us what the system call does in therms of the C/C++
>> memory model
> [snip]
>> A lot of the new system calls lack clear specifications or are just
>> somewhat misdesigned. For example, pkey_alloc
> [snip]
>> getrandom still causes boot delays
> [snip]
>> For copy_file_range, we still have debates whether the system call (and
>> the glibc emulation) should preserve holes or not,
> [snip]
>
> These objections illustrate my point. glibc development is not the
> proper forum for raising post-hoc objections to system call design.
> Withholding wrappers will not un-ship these system calls. Applications
> are already using them, via syscall(2). Developers and users would be
> better served by providing access to the system as it is, with
> appropriate documentation caveats, than by holding out for some
> alternate and more ideal set of system calls that may or may not
> appear in the future. This resistance to exposing the capabilities of
> the system as they are, even in flawed and warty form, is what I meant
> by "misplaced idealism" in my previous message. If the kernel provides
> a system call, libc should provide a C wrapper for it, even if in the
> opinion of the libc maintainers, that system call is flawed.
flaws can be worked around.
it's just more work to do that, hence wrappers are delayed.
(while new flawed syscalls get added, there are missing
syscalls for implementing posix semantics or for better libc
quality, so are the priorities of linux right?)
> I agree with the proposals mentioned above to split system interface
> responsibility, having glibc handle higher-level concerns like stdio
> while punting system call wrappers and other low-level facilities to a
> kernel-provided userspace library that can move faster and more
> explicitly conform to the Linux kernel's userspace ABI.
consuming linux uapi headers is a huge problem (not just for
glibc): the libc has to repeat uapi definitions under appropriate
feature macros using proper libc types etc, this usually creates
conflict between linux and libc headers and a lot of duplicated
work at every linux release. the situation would be worse if all
new types were exposed for new syscalls when they appeared.
the proposal mentioned above does not solve this in any way.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-12 12:25 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111120910.GA4425@1wt.eu>
* Willy Tarreau:
>> > What we'd really need would be to have the libc
>> > interface as part of the operating system itself. I'm perfectly fine
>> > with glibc providing all the "high-level" stuff like strcpy(), FILE*
>> > operations etc, and all this probably is mostly system-independent.
>>
>> That's a bit messy, unfortunately.
>>
>> The kernel does not know about TCB layout, so a lot of low-level
>> threading aspects are defined by userspace.
>>
>> The kernel does not know about POSIX cancellation. Directly calling
>> system calls breaks support for that.
>>
>> A lot of multi-threaded applications assume that most high-level
>> functionality remains usable even after fork in a multi-threaded
>> process. (This is increasingly a problem today with all those direct
>> calls to clone.) Unfortunately, this introduces rather tricky
>> low-level/high-level cross-subsystem issues, too.
>
> But don't you think that moving a bit of this into the kernel
> repository could improve the situation ? The corner cases could then
> be detected when the feature is developed and be addressed either by
> adapting the kernel side of the syscall or even by changing the design
> before it's committed. Maybe a few extra syscalls are missing to
> retrieve some critial info that would make things more reliable or
> easier between userland and kernel, and that would become more obvious
> with all the relevant parts at the same place ?
>
>> > But the system interface could possibly be handled easier in the
>> > system itself, which would also provide a smoother adoption of new
>> > syscalls and API updates. It would also limit the hassle required to
>> > provide new syscalls, as if you start to have to contribute to two
>> > projects at once for a single syscall, it becomes really painful.
>>
>> Sure, the duplication is unfortunate.
>>
>> Several glibc contributors deeply care about standards compliance for
>> header files.
>
> For having suffered a lot from the libc-4 to libc-5 then libc-5 to glibc,
> I certainly can understand their concerns about standards compliance.
This is getting way off-topic, but:
The C standard does not care deeply about practical source code
compatibility. Behavior of valid syntax generally remains unchanged.
However, each revision adds many macros to existing header files, so
practical source code compatibility tends to be problematic. For glibc,
the current policy is to enable all optional features with _GNU_SOURCE,
so most projects receive the full dose of macros. (Unrelated to
standards, even new system call wrappers are problematic for source code
compatibility).)
For ABI compatibility, there are only ad-hoc standards anyway, so it's
mostly about us being careful when making changes.
>> The kernel developers care not, and the result is that we
>> copy definitions and declarations from the kernel header files, creating
>> additional problems.
>
> Probably that these standard compatibility issues should be addressed at
> their root in the kernel header definitions in fact. Working around issues
> always leads to a stall at some point, and it encourages the process not
> to change.
In the past, we couldn't even get agreement about little things such as
__u64, so I'm not hopeful. 8-(
Thanks,
Florian
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Florian Weimer @ 2018-11-12 12:00 UTC (permalink / raw)
To: Ram Pai; +Cc: linux-mm, linux-api, linuxppc-dev, Dave Hansen
In-Reply-To: <20181109180947.GF5481@ram.oc3035372033.ibm.com>
* Ram Pai:
> On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
>> * Ram Pai:
>>
>> > Florian,
>> >
>> > I can. But I am struggling to understand the requirement. Why is
>> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
>> > to be able to allocate keys that are initialied to disable-read
>> > only?
>>
>> Yes, I think that would be a natural consequence.
>>
>> However, my immediate need comes from the fact that the AMR register can
>> contain a flag combination that is not possible to represent with the
>> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
>> could write to AMR directly, so I cannot rule out that certain flag
>> combinations exist there.
>>
>> So I came up with this:
>>
>> int
>> pkey_get (int key)
>> {
>> if (key < 0 || key > PKEY_MAX)
>> {
>> __set_errno (EINVAL);
>> return -1;
>> }
>> unsigned int index = pkey_index (key);
>> unsigned long int amr = pkey_read ();
>> unsigned int bits = (amr >> index) & 3;
>>
>> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
>> currently representable. */
>> if (bits & PKEY_AMR_READ)
>
> this should be
> if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
This would return zero for PKEY_AMR_READ alone.
>> return PKEY_DISABLE_ACCESS;
>
>
>> else if (bits == PKEY_AMR_WRITE)
>> return PKEY_DISABLE_WRITE;
>> return 0;
>> }
It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
Which is why I want PKEY_DISABLE_READ.
>> And this is not ideal. I would prefer something like this instead:
>>
>> switch (bits)
>> {
>> case PKEY_AMR_READ | PKEY_AMR_WRITE:
>> return PKEY_DISABLE_ACCESS;
>> case PKEY_AMR_READ:
>> return PKEY_DISABLE_READ;
>> case PKEY_AMR_WRITE:
>> return PKEY_DISABLE_WRITE;
>> case 0:
>> return 0;
>> }
>
> yes.
> and on x86 it will be something like:
> switch (bits)
> {
> case PKEY_PKRU_ACCESS :
> return PKEY_DISABLE_ACCESS;
> case PKEY_AMR_WRITE:
> return PKEY_DISABLE_WRITE;
> case 0:
> return 0;
> }
x86 returns the PKRU bits directly, including the nonsensical case
(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).
> But for this to work, why do you need to enhance the sys_pkey_alloc()
> interface? Not that I am against it. Trying to understand if the
> enhancement is really needed.
sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
(that is, it updates the PKRU/AMR register). It makes sense to match
the behavior of the userspace implementation.
Thanks,
Florian
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Florian Weimer @ 2018-11-12 10:29 UTC (permalink / raw)
To: Ram Pai; +Cc: linux-api, dave.hansen, linuxppc-dev, linux-mm
In-Reply-To: <20181108192226.GC5481@ram.oc3035372033.ibm.com>
* Ram Pai:
> On Thu, Nov 08, 2018 at 01:05:09PM +0100, Florian Weimer wrote:
>> Would it be possible to reserve a bit for PKEY_DISABLE_READ?
>>
>> I think the POWER implementation can disable read access at the hardware
>> level, but not write access, and that cannot be expressed with the
>> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.
>
> POWER hardware can disable-read and can **also disable-write**
> at the hardware level. It can disable-execute aswell at the
> hardware level. For example if the key bits for a given key in the AMR
> register is
> 0b01 it is read-disable
> 0b10 it is write-disable
>
> To support access-disable, we make the key value 0b11.
>
> So in case if you want to know if the key is read-disable 'bitwise-and' it
> against 0x1. i.e (x & 0x1)
Not sure if we covered that alreay, but my problem is that I cannot
translate a 0b01 mask to a PKEY_DISABLE_* flag combination with the
current flags. 0b10 and 0b11 are fine.
POWER also loses the distinction between PKEY_DISABLE_ACCESS and
PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE, but that's fine. This breaks
the current glibc test case, but I have a patch for that. Arguably, the
test is wrong or at least overly strict in what it accepts.
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-12 8:11 UTC (permalink / raw)
To: Daniel Colascione
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZues5SEESpJU=6MDTrPXTA1KTZFGNQE4Lw4t0fO-WBTU62w@mail.gmail.com>
* Daniel Colascione:
> If the kernel provides a system call, libc should provide a C wrapper
> for it, even if in the opinion of the libc maintainers, that system
> call is flawed.
It's not that simple, I think. What about bdflush? socketcall?
getxpid? osf_gettimeofday? set_robust_list? There are quite a few
irregularities, and some editorial discretion appears to be unavoidable.
Even if we were to provide perfectly consistent system call wrappers
under separate names, we'd still expose different calling conventions
for things like off_t to applications, which would make using some of
the system calls quite difficult and surprisingly non-portable.
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Andy Lutomirski @ 2018-11-12 5:46 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Greg KH, Daniel Colascione, LKML, Joel Fernandes, Linux API
In-Reply-To: <cd4717c4-4950-7513-726d-2e1873aeceb5@redhat.com>
On Sun, Nov 11, 2018 at 6:24 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 11/10/18 2:20 PM, Greg KH wrote:
> > Also, what about the basic work of making sure our uapi header files can
> > actually be used untouched by a libc? That isn't the case these days as
> > the bionic maintainers like to keep reminding me. That might be a good
> > thing to do _before_ trying to add new things like syscall wrappers.
> I agree completely. There are many steps in the checklist to writing
> a new syscall, heck we should probably have a checklist!
>
> Socially the issue is difficult because the various communities only
> marginally share the same network of developers, care about different
> features, or the same features with different priorities.
>
> That doesn't mean we shouldn't try to integrate better. As was pointed
> out, various people from the userspace and toolchain communities are
> going to LPC to do just this.
>
if you all want my two cents, I think that we should approach this all
quite differently than trying to get glibc to add a wrapper for each
syscall. I think the kernel should contain a list or list of syscalls
along with parameter names, types, and numbers, and this should get
processed during the kernel build to produce a few different
artifacts:
- A machine-readable version of the same data in a stable format.
Tools like strace should be able to consume it.
- A library called, perhaps, libinux, or maybe a header-only library.
It should have a wrapper for *every* syscall, and they should be
namespaced. Instead of renameat2(), it should expose
linux_renameat2(). Ideally it would use the UAPI header types, but
void * wouldn't be so bad for pointers.
P.S. Does gcc even *have* the correct asm constraints to express
typeless syscalls? Ideally we'd want syscalls to have exactly the
same pointer escaping semantics as ordinary functions, so, if I do:
struct timeval tv;
/* typed expansion of linux_gettimeofday(&tv, NULL); */
asm volatile ("whatever" : "+m" (tv) : "D" (&tv));
it works. But if I want to use a generic wrapper that doesn't know
that the argument is a pointer, I do:
asm volatile ("whatever" :: "D" (&tv));
then gcc seems to not actually understand that the value pointed to by
&tv is modified by the syscall. glibc's syscall() function works
AFAICT because it's an external function, and gcc considers &tv to
have escaped and can't see the body of the syscall() function.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Greg KH @ 2018-11-12 2:36 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Daniel Colascione, linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <cd4717c4-4950-7513-726d-2e1873aeceb5@redhat.com>
On Sun, Nov 11, 2018 at 09:24:40PM -0500, Carlos O'Donell wrote:
> On 11/10/18 2:20 PM, Greg KH wrote:
> > Also, what about the basic work of making sure our uapi header files can
> > actually be used untouched by a libc? That isn't the case these days as
> > the bionic maintainers like to keep reminding me. That might be a good
> > thing to do _before_ trying to add new things like syscall wrappers.
> I agree completely. There are many steps in the checklist to writing
> a new syscall, heck we should probably have a checklist!
We should have a checklist. That's a great idea. Now to find someone
to write it... :)
I'll try to make it to the plumbers talk, but I think there's a
competing one I am supposed to be at at the same time.
greg k-h
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Carlos O'Donell @ 2018-11-12 2:24 UTC (permalink / raw)
To: Greg KH, Daniel Colascione; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <20181110192027.GA29892@kroah.com>
On 11/10/18 2:20 PM, Greg KH wrote:
> Also, what about the basic work of making sure our uapi header files can
> actually be used untouched by a libc? That isn't the case these days as
> the bionic maintainers like to keep reminding me. That might be a good
> thing to do _before_ trying to add new things like syscall wrappers.
I agree completely. There are many steps in the checklist to writing
a new syscall, heck we should probably have a checklist!
Socially the issue is difficult because the various communities only
marginally share the same network of developers, care about different
features, or the same features with different priorities.
That doesn't mean we shouldn't try to integrate better. As was pointed
out, various people from the userspace and toolchain communities are
going to LPC to do just this.
--
Cheers,
Carlos.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Carlos O'Donell @ 2018-11-12 2:03 UTC (permalink / raw)
To: Vlastimil Babka, Greg KH, Daniel Colascione
Cc: linux-kernel, Joel Fernandes, Linux API, Adhemerval Zanella
In-Reply-To: <6a415f31-2bce-72fa-2ff0-5cbbd572347b@suse.cz>
On 11/10/18 2:58 PM, Vlastimil Babka wrote:
> On 11/10/18 8:20 PM, Greg KH wrote:
>> On Sat, Nov 10, 2018 at 10:52:06AM -0800, Daniel Colascione wrote:
>>> Now that glibc is basically not adding any new system call wrappers,
>>
>> Why are they not doing that anymore?
>
> FYI just noticed there's a topic relevant to this in LPC Toolchain MC:
>
> https://linuxplumbersconf.org/event/2/contributions/149/
Yes, and Adhemerval put it there on purpose to continue the discussion
between glibc developers and kernel developers. Florian Weimer and I have
both provided input to that talk, so if something comes out of the talk
and you want to talk more, please just reach out.
I hope that kernel developers interested in this topic will attend
and discuss the various ways forward on certain interesting topics :-)
--
Cheers,
Carlos.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Paul Eggert @ 2018-11-12 1:44 UTC (permalink / raw)
To: Daniel Colascione, Florian Weimer
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZues5SEESpJU=6MDTrPXTA1KTZFGNQE4Lw4t0fO-WBTU62w@mail.gmail.com>
Daniel Colascione wrote:
> This resistance to exposing the capabilities of
> the system as they are, even in flawed and warty form, is what I meant
> by "misplaced idealism" in my previous message.
With my application-developer hat on I prefer some resistance to flaws and
warts, as the resistance gives me a better feel for which functions are
problematic and which can be used more reliably. If glibc is missing Linux
syscall functionality that I really need then I can use syscall (with the usual
caveats) and I've done that on occasion (and have regretted it later too :-). It
is helpful for glibc to prefer mild curation to slavishly copying an API that
can be a bit helter-skelter at times.
^ permalink raw reply
* [PATCH 3.16 330/366] fs/proc: Stop trying to report thread stacks
From: Ben Hutchings @ 2018-11-11 19:49 UTC (permalink / raw)
To: linux-kernel, stable
Cc: akpm, Linus Torvalds, Borislav Petkov, Peter Zijlstra,
Andy Lutomirski, Kees Cook, Ingo Molnar, Tycho Andersen,
Brian Gerst, Johannes Weiner, Linux API, Thomas Gleixner,
Jann Horn, Al Viro
In-Reply-To: <lsq.1541965744.387173642@decadent.org.uk>
3.16.61-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
[bwh: Backported to 3.16: Squash in the earlier commits 58cb65487e92
"proc/maps: make vm_is_stack() logic namespace-friendly" and
65376df58217 "proc: revert /proc/<pid>/maps [stack:TID] annotation",
which would introduce build failures if applied separately.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -250,13 +250,28 @@ static int do_maps_open(struct inode *in
return ret;
}
+/*
+ * Indicate if the VMA is a stack for the given task; for
+ * /proc/PID/maps that is the stack of the main task.
+ */
+static int is_stack(struct proc_maps_private *priv,
+ struct vm_area_struct *vma)
+{
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
+}
+
static void
show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
{
struct mm_struct *mm = vma->vm_mm;
struct file *file = vma->vm_file;
struct proc_maps_private *priv = m->private;
- struct task_struct *task = priv->task;
vm_flags_t flags = vma->vm_flags;
unsigned long ino = 0;
unsigned long long pgoff = 0;
@@ -304,8 +319,6 @@ show_map_vma(struct seq_file *m, struct
name = arch_vma_name(vma);
if (!name) {
- pid_t tid;
-
if (!mm) {
name = "[vdso]";
goto done;
@@ -317,22 +330,8 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- tid = vm_is_stack(task, vma, is_pid);
-
- if (tid != 0) {
- /*
- * Thread stack in /proc/PID/task/TID/maps or
- * the main process stack.
- */
- if (!is_pid || (vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack)) {
- name = "[stack]";
- } else {
- /* Thread stack in /proc/PID/maps */
- seq_pad(m, ' ');
- seq_printf(m, "[stack:%d]", tid);
- }
- }
+ if (is_stack(priv, vma))
+ name = "[stack]";
}
done:
@@ -1433,19 +1432,8 @@ static int show_numa_map(struct seq_file
seq_path(m, &file->f_path, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else {
- pid_t tid = vm_is_stack(task, vma, is_pid);
- if (tid != 0) {
- /*
- * Thread stack in /proc/PID/task/TID/maps or
- * the main process stack.
- */
- if (!is_pid || (vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack))
- seq_puts(m, " stack");
- else
- seq_printf(m, " stack:%d", tid);
- }
+ } else if (is_stack(proc_priv, vma)) {
+ seq_puts(m, " stack");
}
if (is_vm_hugetlb_page(vma))
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -123,6 +123,20 @@ unsigned long task_statm(struct mm_struc
return size;
}
+static int is_stack(struct proc_maps_private *priv,
+ struct vm_area_struct *vma)
+{
+ struct mm_struct *mm = vma->vm_mm;
+
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
+}
+
/*
* display a single VMA to a sequenced file
*/
@@ -162,21 +176,9 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_path(m, &file->f_path, "");
- } else if (mm) {
- pid_t tid = vm_is_stack(priv->task, vma, is_pid);
-
- if (tid != 0) {
- seq_pad(m, ' ');
- /*
- * Thread stack in /proc/PID/task/TID/maps or
- * the main process stack.
- */
- if (!is_pid || (vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack))
- seq_printf(m, "[stack]");
- else
- seq_printf(m, "[stack:%d]", tid);
- }
+ } else if (mm && is_stack(priv, vma)) {
+ seq_pad(m, ' ');
+ seq_printf(m, "[stack]");
}
seq_putc(m, '\n');
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1239,8 +1239,7 @@ int set_page_dirty_lock(struct page *pag
int clear_page_dirty_for_io(struct page *page);
int get_cmdline(struct task_struct *task, char *buffer, int buflen);
-extern pid_t
-vm_is_stack(struct task_struct *task, struct vm_area_struct *vma, int in_group);
+int vma_is_stack_for_task(struct vm_area_struct *vma, struct task_struct *t);
extern unsigned long move_page_tables(struct vm_area_struct *vma,
unsigned long old_addr, struct vm_area_struct *new_vma,
--- a/mm/util.c
+++ b/mm/util.c
@@ -255,43 +255,11 @@ void __vma_link_list(struct mm_struct *m
}
/* Check if the vma is being used as a stack by this task */
-static int vm_is_stack_for_task(struct task_struct *t,
- struct vm_area_struct *vma)
+int vma_is_stack_for_task(struct vm_area_struct *vma, struct task_struct *t)
{
return (vma->vm_start <= KSTK_ESP(t) && vma->vm_end >= KSTK_ESP(t));
}
-/*
- * Check if the vma is being used as a stack.
- * If is_group is non-zero, check in the entire thread group or else
- * just check in the current task. Returns the pid of the task that
- * the vma is stack for.
- */
-pid_t vm_is_stack(struct task_struct *task,
- struct vm_area_struct *vma, int in_group)
-{
- pid_t ret = 0;
-
- if (vm_is_stack_for_task(task, vma))
- return task->pid;
-
- if (in_group) {
- struct task_struct *t;
-
- rcu_read_lock();
- for_each_thread(task, t) {
- if (vm_is_stack_for_task(t, vma)) {
- ret = t->pid;
- goto done;
- }
- }
-done:
- rcu_read_unlock();
- }
-
- return ret;
-}
-
#if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
void arch_pick_mmap_layout(struct mm_struct *mm)
{
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -335,7 +335,7 @@ address perms offset dev in
a7cb1000-a7cb2000 ---p 00000000 00:00 0
a7cb2000-a7eb2000 rw-p 00000000 00:00 0
a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack:1001]
+a7eb3000-a7ed5000 rw-p 00000000 00:00 0
a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
@@ -367,40 +367,11 @@ is not associated with a file:
[heap] = the heap of the program
[stack] = the stack of the main process
- [stack:1001] = the stack of the thread with tid 1001
[vdso] = the "virtual dynamic shared object",
the kernel system call handler
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. This is a key difference from the
-content of /proc/PID/maps, where you will see all mappings that are being used
-as stack by all of those tasks. Hence, for the example above, the task-level
-map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Pavel Machek @ 2018-11-11 19:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Borislav Petkov, Yu-cheng Yu, x86, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, linux-kernel, linux-doc, linux-mm,
linux-arch, linux-api, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit
In-Reply-To: <4E917DA9-5192-48E2-8857-08C3ABE08AFE@amacapital.net>
[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]
On Sun 2018-11-11 06:59:24, Andy Lutomirski wrote:
>
>
> > On Nov 11, 2018, at 3:31 AM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> >>> +/*
> >>> + * State component 12 is Control flow Enforcement kernel states
> >>> + */
> >>> +struct cet_kernel_state {
> >>> + u64 kernel_ssp; /* kernel shadow stack */
> >>> + u64 pl1_ssp; /* ring-1 shadow stack */
> >>> + u64 pl2_ssp; /* ring-2 shadow stack */
> >>
> >> Just write "privilege level" everywhere - not "ring".
> >
> > Please just use word "ring". It is well estabilished terminology.
> >
> > Which ring is priviledge level 1, given that we have SMM and
> > virtualization support?
>
> To the contrary: CPL, DPL, and RPL are very well defined terms in the architecture manuals. “PL” is privilege level. PL 1 is very well defined.
>
"Priviledge level" is generic term. "CPL" I may recognize as
Intel-specific. "priviledge level" I would not. So I'd really use
"ring" there. "CPL 1 shadow stack" would be okay, too I guess.
> SMM is SMM, full stop (unless dual mode or whatever it’s called is on, but AFAIK no one uses it). VMX non-root CPL 1 is *still* privilege level 1.
>
> In contrast, the security community likes to call SMM “ring -1”, which is cute, but wrong from a systems programmer view. For example, SMM’s CPL can still range from 0-3.
>
Regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-11 17:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <91E8E1AA-859A-457A-8978-3EA39CBBF075@amacapital.net>
On Sun, Nov 11, 2018 at 07:14:33AM -0800, Andy Lutomirski wrote:
[...]
> >>>>>>>>>> I see two reasonable solutions:
> >>>>>>>>>>
> >>>>>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>>>>>>>>> work by itself.
> >>>>>>>>>
> >>>>>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
> >>>>>>>>> writes of already opened files. This would mean more checking in all those
> >>>>>>>>> paths (and modification of all those paths).
> >>>>>>>>>
> >>>>>>>>> Anyway going with that idea, we could
> >>>>>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >>>>>>>>> the inode::i_writecount.
> >>>>>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
> >>>>>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>>>>>>>>
> >>>>>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
> >>>>>>>>> bit about 2 not being too familiar with VFS internals, about what the
> >>>>>>>>> consequences of doing that may be.
> >>>>>>>>
> >>>>>>>> IMHO, modifying both the inode and the struct file separately is fine,
> >>>>>>>> since they mean different things. In regular filesystems, it's fine to
> >>>>>>>> have a read-write open file description for a file whose inode grants
> >>>>>>>> write permission to nobody. Speaking of which: is fchmod enough to
> >>>>>>>> prevent this attack?
> >>>>>>>
> >>>>>>> Well, yes and no. fchmod does prevent reopening the file RW, but
> >>>>>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> >>>>>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> >>>>>>> isn't sufficient by itself. While it might be good enough for Android
> >>>>>>> (in the sense that it'll prevent RW-reopens from other security
> >>>>>>> contexts to which we send an open memfd file), it's still conceptually
> >>>>>>> ugly, IMHO. Let's go with the original approach of just tweaking the
> >>>>>>> inode so that open-for-write is permanently blocked.
> >>>>>>
> >>>>>> Agreed with the idea of modifying both file and inode flags. I was thinking
> >>>>>> modifying i_mode may do the trick but as you pointed it probably could be
> >>>>>> reverted by chmod or some other attribute setting calls.
> >>>>>>
> >>>>>> OTOH, I don't think deny_write_access(file) can be reverted from any
> >>>>>> user-facing path so we could do that from the seal to prevent the future
> >>>>>> opens in write mode. I'll double check and test that out tomorrow.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> This seems considerably more complicated and more fragile than needed. Just
> >>>>> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
> >>>>> variant work exactly like it with two exceptions:
> >>>>>
> >>>>> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
> >>>>> accordingly.
> >>>>
> >>>> There's more to it than that, we also need to block future writes through
> >>>> write syscall, so we have to hook into the write path too once the seal is
> >>>> set, not just the mmap. That means we have to add code in mm/shmem.c to do
> >>>> that in all those handlers, to check for the seal (and hope we didn't miss a
> >>>> file_operations handler). Is that what you are proposing?
> >>>
> >>> The existing code already does this. That’s why I suggested grepping :)
> >>>
> >>>>
> >>>> Also, it means we have to keep CONFIG_TMPFS enabled so that the
> >>>> shmem_file_operations write handlers like write_iter are hooked up. Currently
> >>>> memfd works even with !CONFIG_TMPFS.
> >>>
> >>> If so, that sounds like it may already be a bug.
> >
> > Why shouldn't memfd work independently of CONFIG_TMPFS? In particular,
> > write(2) on tmpfs FDs shouldn't work differently. If it does, that's a
> > kernel implementation detail leaking out into userspace.
> >
> >>>>> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
> >>>>>
> >>>>> That really should be all that’s needed.
> >>>>
> >>>> It seems a fair idea what you're saying. But I don't see how its less
> >>>> complex.. IMO its far more simple to have VFS do the denial of the operations
> >>>> based on the flags of its datastructures.. and if it works (which I will test
> >>>> to be sure it will), then we should be good.
> >>>
> >>> I agree it’s complicated, but the code is already written. You should just
> >>> need to adjust some masks.
> >>>
> >>
> >> Its actually not that bad and a great idea, I did something like the
> >> following and it works pretty well. I would say its cleaner than the old
> >> approach for sure (and I also added a /proc/pid/fd/N reopen test to the
> >> selftest and made sure that issue goes away).
> >>
> >> Side note: One subtelty I discovered from the existing selftests is once the
> >> F_SEAL_WRITE are active, an mmap of PROT_READ and MAP_SHARED region is
> >> expected to fail. This is also evident from this code in mmap_region:
> >> if (vm_flags & VM_SHARED) {
> >> error = mapping_map_writable(file->f_mapping);
> >> if (error)
> >> goto allow_write_and_free_vma;
> >> }
> >>
> >
> > This behavior seems like a bug. Why should MAP_SHARED writes be denied
> > here? There's no semantic incompatibility between shared mappings and
> > the seal. And I think this change would represent an ABI break using
> > memfd seals for ashmem, since ashmem currently allows MAP_SHARED
> > mappings after changing prot_mask.
>
> Hmm. I’m guessing the intent is that the mmap count should track writable
> mappings in addition to mappings that could be made writable using
> mprotect. I think you could address this for SEAL_FUTURE in two ways:
>
> 1. In shmem_mmap, mask off VM_MAYWRITE if SEAL_FUTURE is set, or
>
> 2. Add a new vm operation that allows a vma to reject an mprotect attempt,
> like security_file_mprotect but per vma. Then give it reasonable semantics
> for shmem.
>
> (1) probably gives the semantics you want for SEAL_FUTURE: old maps can be
> mprotected, but new maps can’t.
Thanks Andy and Daniel! This occured to me too and I like the solution in (1).
I tested that now PROT_READ + MAP_SHARED works and the mrprotect is not able
to revert the protection. In fact (1) is exactly what we do in the ashmem
driver.
The updated patch now looks like the following:
---8<-----------------------
From: "Joel Fernandes" <joel@joelfernandes.org>
Subject: [PATCH] mm/memfd: implement future write seal using shmem ops
Signed-off-by: Joel Fernandes <joel@joelfernandes.org>
---
fs/hugetlbfs/inode.c | 2 +-
mm/memfd.c | 19 -------------------
mm/shmem.c | 24 +++++++++++++++++++++---
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 32920a10100e..1978581abfdf 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -530,7 +530,7 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
inode_lock(inode);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
inode_unlock(inode);
return -EPERM;
}
diff --git a/mm/memfd.c b/mm/memfd.c
index 5ba9804e9515..a9ece5fab439 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -220,25 +220,6 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
}
}
- if ((seals & F_SEAL_FUTURE_WRITE) &&
- !(*file_seals & F_SEAL_FUTURE_WRITE)) {
- /*
- * The FUTURE_WRITE seal also prevents growing and shrinking
- * so we need them to be already set, or requested now.
- */
- int test_seals = (seals | *file_seals) &
- (F_SEAL_GROW | F_SEAL_SHRINK);
-
- if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
- error = -EINVAL;
- goto unlock;
- }
-
- spin_lock(&file->f_lock);
- file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
- spin_unlock(&file->f_lock);
- }
-
*file_seals |= seals;
error = 0;
diff --git a/mm/shmem.c b/mm/shmem.c
index 446942677cd4..ef6039f1ea2a 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2163,6 +2163,23 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
{
+ struct shmem_inode_info *info = SHMEM_I(file_inode(file));
+
+ /*
+ * New PROT_READ and MAP_SHARED mmaps are not allowed when "future
+ * write" seal active.
+ */
+ if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) &&
+ (info->seals & F_SEAL_FUTURE_WRITE))
+ return -EPERM;
+
+ /*
+ * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED read-only
+ * mapping, take care to not allow mprotect to revert protections.
+ */
+ if (info->seals & F_SEAL_FUTURE_WRITE)
+ vma->vm_flags &= ~(VM_MAYWRITE);
+
file_accessed(file);
vma->vm_ops = &shmem_vm_ops;
if (IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) &&
@@ -2391,8 +2408,9 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
pgoff_t index = pos >> PAGE_SHIFT;
/* i_mutex is held by caller */
- if (unlikely(info->seals & (F_SEAL_WRITE | F_SEAL_GROW))) {
- if (info->seals & F_SEAL_WRITE)
+ if (unlikely(info->seals & (F_SEAL_GROW |
+ F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))
return -EPERM;
if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
return -EPERM;
@@ -2657,7 +2675,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(shmem_falloc_waitq);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & F_SEAL_WRITE || info->seals & F_SEAL_FUTURE_WRITE) {
error = -EPERM;
goto out;
}
--
2.19.1.930.g4563a0d9d0-goog
^ permalink raw reply related
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-11 15:14 UTC (permalink / raw)
To: Daniel Colascione
Cc: Joel Fernandes, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <CAKOZuethQ3eaV4uoEXiffVMc_S0hyk1FGPB3iQHHnv4NadW1UQ@mail.gmail.com>
> On Nov 11, 2018, at 12:30 AM, Daniel Colascione <dancol@google.com> wrote:
>
>> On Sun, Nov 11, 2018 at 12:09 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
>> On Sat, Nov 10, 2018 at 07:40:10PM -0800, Andy Lutomirski wrote:
>> [...]
>>>>>>>>>> I see two reasonable solutions:
>>>>>>>>>>
>>>>>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>>>>>>>>>> work by itself.
>>>>>>>>>
>>>>>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
>>>>>>>>> writes of already opened files. This would mean more checking in all those
>>>>>>>>> paths (and modification of all those paths).
>>>>>>>>>
>>>>>>>>> Anyway going with that idea, we could
>>>>>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>>>>>>>>> the inode::i_writecount.
>>>>>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
>>>>>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>>>>>>>>>
>>>>>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
>>>>>>>>> bit about 2 not being too familiar with VFS internals, about what the
>>>>>>>>> consequences of doing that may be.
>>>>>>>>
>>>>>>>> IMHO, modifying both the inode and the struct file separately is fine,
>>>>>>>> since they mean different things. In regular filesystems, it's fine to
>>>>>>>> have a read-write open file description for a file whose inode grants
>>>>>>>> write permission to nobody. Speaking of which: is fchmod enough to
>>>>>>>> prevent this attack?
>>>>>>>
>>>>>>> Well, yes and no. fchmod does prevent reopening the file RW, but
>>>>>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
>>>>>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
>>>>>>> isn't sufficient by itself. While it might be good enough for Android
>>>>>>> (in the sense that it'll prevent RW-reopens from other security
>>>>>>> contexts to which we send an open memfd file), it's still conceptually
>>>>>>> ugly, IMHO. Let's go with the original approach of just tweaking the
>>>>>>> inode so that open-for-write is permanently blocked.
>>>>>>
>>>>>> Agreed with the idea of modifying both file and inode flags. I was thinking
>>>>>> modifying i_mode may do the trick but as you pointed it probably could be
>>>>>> reverted by chmod or some other attribute setting calls.
>>>>>>
>>>>>> OTOH, I don't think deny_write_access(file) can be reverted from any
>>>>>> user-facing path so we could do that from the seal to prevent the future
>>>>>> opens in write mode. I'll double check and test that out tomorrow.
>>>>>>
>>>>>>
>>>>>
>>>>> This seems considerably more complicated and more fragile than needed. Just
>>>>> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
>>>>> variant work exactly like it with two exceptions:
>>>>>
>>>>> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
>>>>> accordingly.
>>>>
>>>> There's more to it than that, we also need to block future writes through
>>>> write syscall, so we have to hook into the write path too once the seal is
>>>> set, not just the mmap. That means we have to add code in mm/shmem.c to do
>>>> that in all those handlers, to check for the seal (and hope we didn't miss a
>>>> file_operations handler). Is that what you are proposing?
>>>
>>> The existing code already does this. That’s why I suggested grepping :)
>>>
>>>>
>>>> Also, it means we have to keep CONFIG_TMPFS enabled so that the
>>>> shmem_file_operations write handlers like write_iter are hooked up. Currently
>>>> memfd works even with !CONFIG_TMPFS.
>>>
>>> If so, that sounds like it may already be a bug.
>
> Why shouldn't memfd work independently of CONFIG_TMPFS? In particular,
> write(2) on tmpfs FDs shouldn't work differently. If it does, that's a
> kernel implementation detail leaking out into userspace.
>
>>>>> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
>>>>>
>>>>> That really should be all that’s needed.
>>>>
>>>> It seems a fair idea what you're saying. But I don't see how its less
>>>> complex.. IMO its far more simple to have VFS do the denial of the operations
>>>> based on the flags of its datastructures.. and if it works (which I will test
>>>> to be sure it will), then we should be good.
>>>
>>> I agree it’s complicated, but the code is already written. You should just
>>> need to adjust some masks.
>>>
>>
>> Its actually not that bad and a great idea, I did something like the
>> following and it works pretty well. I would say its cleaner than the old
>> approach for sure (and I also added a /proc/pid/fd/N reopen test to the
>> selftest and made sure that issue goes away).
>>
>> Side note: One subtelty I discovered from the existing selftests is once the
>> F_SEAL_WRITE are active, an mmap of PROT_READ and MAP_SHARED region is
>> expected to fail. This is also evident from this code in mmap_region:
>> if (vm_flags & VM_SHARED) {
>> error = mapping_map_writable(file->f_mapping);
>> if (error)
>> goto allow_write_and_free_vma;
>> }
>>
>
> This behavior seems like a bug. Why should MAP_SHARED writes be denied
> here? There's no semantic incompatibility between shared mappings and
> the seal. And I think this change would represent an ABI break using
> memfd seals for ashmem, since ashmem currently allows MAP_SHARED
> mappings after changing prot_mask.
Hmm. I’m guessing the intent is that the mmap count should track writable mappings in addition to mappings that could be made writable using mprotect. I think you could address this for SEAL_FUTURE in two ways:
1. In shmem_mmap, mask off VM_MAYWRITE if SEAL_FUTURE is set, or
2. Add a new vm operation that allows a vma to reject an mprotect attempt, like security_file_mprotect but per vma. Then give it reasonable semantics for shmem.
(1) probably gives the semantics you want for SEAL_FUTURE: old maps can be mprotected, but new maps can’t.
>
>> ---8<-----------------------
>>
>> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>> Subject: [PATCH] mm/memfd: implement future write seal using shmem ops
>>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
>> fs/hugetlbfs/inode.c | 2 +-
>> mm/memfd.c | 19 -------------------
>> mm/shmem.c | 13 ++++++++++---
>> 3 files changed, 11 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index 32920a10100e..1978581abfdf 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -530,7 +530,7 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
>> inode_lock(inode);
>>
>> /* protected by i_mutex */
>> - if (info->seals & F_SEAL_WRITE) {
>> + if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
>> inode_unlock(inode);
>> return -EPERM;
>> }
>
> Maybe we can always set F_SEAL_FUTURE_WRITE when F_SEAL_WRITE so we
> can just test one bit except where the F_SEAL_WRITE behavior differs
> from F_SEAL_FUTURE_WRITE.
This could plausibly confuse existing users that read the seal mask.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-11 14:59 UTC (permalink / raw)
To: Pavel Machek
Cc: Borislav Petkov, Yu-cheng Yu, x86, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, linux-kernel, linux-doc, linux-mm,
linux-arch, linux-api, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit
In-Reply-To: <20181111113103.GG27666@amd>
> On Nov 11, 2018, at 3:31 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
>>> +/*
>>> + * State component 12 is Control flow Enforcement kernel states
>>> + */
>>> +struct cet_kernel_state {
>>> + u64 kernel_ssp; /* kernel shadow stack */
>>> + u64 pl1_ssp; /* ring-1 shadow stack */
>>> + u64 pl2_ssp; /* ring-2 shadow stack */
>>
>> Just write "privilege level" everywhere - not "ring".
>
> Please just use word "ring". It is well estabilished terminology.
>
> Which ring is priviledge level 1, given that we have SMM and
> virtualization support?
To the contrary: CPL, DPL, and RPL are very well defined terms in the architecture manuals. “PL” is privilege level. PL 1 is very well defined.
SMM is SMM, full stop (unless dual mode or whatever it’s called is on, but AFAIK no one uses it). VMX non-root CPL 1 is *still* privilege level 1.
In contrast, the security community likes to call SMM “ring -1”, which is cute, but wrong from a systems programmer view. For example, SMM’s CPL can still range from 0-3.
>
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ 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