* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-05 14:12 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CACAyw99ar0Wokyg4jx8FPyr1Z=7Cf+=WVgF6sLL1FK9JtHROjw@mail.gmail.com>
On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> >
> > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > check in here, right? I doubt it matters, but I don't really like
> > seeing something like this exposed to unprivileged userspace just
> > because you need it for kernel testing.
>
> That would mean all tests have to run as root / with CAP_SYS_ADMIN
> which isn't ideal.
This patch basically means that it becomes easier for a local user to
construct a BPF hash table that has all of its values stuffed into a
single hash bucket, correct? Which makes it easier to create a BPF
program that generates unusually large RCU stalls by performing ~40000
BPF map lookups, each of which has to walk through the entire linked
list of the hash map bucket? I dislike exposing something like that to
unprivileged userspace.
And if you want to run the whole BPF test suite with all its tests,
don't you already need root privileges? Or is this a different test
suite?
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-05 14:21 UTC (permalink / raw)
To: jannh; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAG48ez3wwGgc40uAg5nq9dnC3qsAD04jQZTWtPi7jg_VDws-fg@mail.gmail.com>
On Fri, 5 Oct 2018 at 15:12, Jann Horn <jannh@google.com> wrote:
>
> On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> > >
> > > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > > check in here, right? I doubt it matters, but I don't really like
> > > seeing something like this exposed to unprivileged userspace just
> > > because you need it for kernel testing.
> >
> > That would mean all tests have to run as root / with CAP_SYS_ADMIN
> > which isn't ideal.
>
> This patch basically means that it becomes easier for a local user to
> construct a BPF hash table that has all of its values stuffed into a
> single hash bucket, correct? Which makes it easier to create a BPF
> program that generates unusually large RCU stalls by performing ~40000
> BPF map lookups, each of which has to walk through the entire linked
> list of the hash map bucket? I dislike exposing something like that to
> unprivileged userspace.
That's a good point, for which I don't have an answer. You could argue that
this was the status quo until the seed was randomised, so it seems
like this hasn't been a worry so far. Should it be going forward?
> And if you want to run the whole BPF test suite with all its tests,
> don't you already need root privileges? Or is this a different test
> suite?
No, I'm thinking about third parties that want to test their own BPF.
If you enable unprivileged BPF you can use BPF_PROG_TEST_RUN to
test your programs without root, if I'm not mistaken.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-10-05 14:27 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, netdev, linux-api
In-Reply-To: <59ef80ab-4f28-5c75-c394-55fcfd9bc8ca@iogearbox.net>
On Mon, 1 Oct 2018 at 20:12, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/01/2018 12:45 PM, Lorenz Bauer wrote:
> > This patch set adds a new flag BPF_F_ZERO_SEED, which allows
> > forcing the seed used by hash maps to zero. This makes
> > it possible to write deterministic tests.
> >
> > Based on an off-list conversation with Alexei Starovoitov and
> > Daniel Borkmann.
> >
> > Lorenz Bauer (3):
> > bpf: allow zero-initializing hash map seed
> > tools: sync linux/bpf.h
> > tools: add selftest for BPF_F_ZERO_SEED
> >
> > include/uapi/linux/bpf.h | 2 +
> > kernel/bpf/hashtab.c | 8 ++-
> > tools/include/uapi/linux/bpf.h | 2 +
> > tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
> > 4 files changed, 66 insertions(+), 13 deletions(-)
> >
>
> Please respin with proper SoB for each patch and non-empty commit
> description.
What does SoB mean? Point taken about the empty commit message.
> I think patch 1 should also have a more elaborate
> commit description on the use case for BPF_F_ZERO_SEED, and I
This came out of the off-list discussion we had about map hash functions,
where Alexei expressed concern that your change to randomise the seed
might catch users off-guard. I personally don't have a use case, but decided
to tackle it since it seemed a simple-ish fix to get acquainted with
the code base.
Maybe this isn't needed after all?
> think also a better comment in the uapi header that this is only
> meant for testing and not production use.
Will do, if you decide that this is worth having in the first place.
>
> Thanks,
> Daniel
Lorenz
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-05 14:27 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CACAyw9-JcORMGDb3wm-E4VYmjVmvWqB5vVh+=qTvXmpFWye_QA@mail.gmail.com>
On Fri, Oct 5, 2018 at 4:21 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> On Fri, 5 Oct 2018 at 15:12, Jann Horn <jannh@google.com> wrote:
> > On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> > > > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > > > check in here, right? I doubt it matters, but I don't really like
> > > > seeing something like this exposed to unprivileged userspace just
> > > > because you need it for kernel testing.
> > >
> > > That would mean all tests have to run as root / with CAP_SYS_ADMIN
> > > which isn't ideal.
> >
> > This patch basically means that it becomes easier for a local user to
> > construct a BPF hash table that has all of its values stuffed into a
> > single hash bucket, correct? Which makes it easier to create a BPF
> > program that generates unusually large RCU stalls by performing ~40000
> > BPF map lookups, each of which has to walk through the entire linked
> > list of the hash map bucket? I dislike exposing something like that to
> > unprivileged userspace.
>
> That's a good point, for which I don't have an answer. You could argue that
> this was the status quo until the seed was randomised, so it seems
> like this hasn't been a worry so far. Should it be going forward?
I don't think that local DoS bugs, or bugs that locally degrade
performance, are a big deal, but I also think that the kernel should
try to avoid having such issues.
> > And if you want to run the whole BPF test suite with all its tests,
> > don't you already need root privileges? Or is this a different test
> > suite?
>
> No, I'm thinking about third parties that want to test their own BPF.
Ah. That wasn't clear to me from your patch description.
Can you please describe exactly why something that is not a kernel
unit test needs deterministic BPF hash map behavior?
> If you enable unprivileged BPF you can use BPF_PROG_TEST_RUN to
> test your programs without root, if I'm not mistaken.
^ permalink raw reply
* Re: [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Jann Horn @ 2018-10-05 14:29 UTC (permalink / raw)
To: lmb; +Cc: Daniel Borkmann, Alexei Starovoitov, Network Development,
Linux API
In-Reply-To: <CACAyw99h58NX6tE1KrgoNEWwX4U8WW3JaRnJGJ0EGXEoeqeMDw@mail.gmail.com>
On Fri, Oct 5, 2018 at 4:27 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Mon, 1 Oct 2018 at 20:12, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 10/01/2018 12:45 PM, Lorenz Bauer wrote:
> > > This patch set adds a new flag BPF_F_ZERO_SEED, which allows
> > > forcing the seed used by hash maps to zero. This makes
> > > it possible to write deterministic tests.
> > >
> > > Based on an off-list conversation with Alexei Starovoitov and
> > > Daniel Borkmann.
> > >
> > > Lorenz Bauer (3):
> > > bpf: allow zero-initializing hash map seed
> > > tools: sync linux/bpf.h
> > > tools: add selftest for BPF_F_ZERO_SEED
> > >
> > > include/uapi/linux/bpf.h | 2 +
> > > kernel/bpf/hashtab.c | 8 ++-
> > > tools/include/uapi/linux/bpf.h | 2 +
> > > tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
> > > 4 files changed, 66 insertions(+), 13 deletions(-)
> > >
> >
> > Please respin with proper SoB for each patch and non-empty commit
> > description.
>
> What does SoB mean? Point taken about the empty commit message.
SoB is the Signed-off-by line. See
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-05 15:07 UTC (permalink / raw)
To: Jann Horn
Cc: Eric W. Biederman, jlayton, Bruce Fields, Al Viro, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <CAG48ez2u--Wac7d9n3idC92Pd-M40QzYtR6FKR8PAYr=VQHUBQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10352 bytes --]
On 2018-10-04, Jann Horn <jannh@google.com> wrote:
> On Thu, Oct 4, 2018 at 6:26 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > > Something else concurrently moves /A/B/C to /A/C. This can result in
> > > the following:
> > >
> > > 1. You start the path walk and reach /A/B/C.
> > > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > > 3. Your path walk follows the first ".." up into /A. This is outside
> > > the process root, but you never actually encountered the process root,
> > > so you don't notice.
> > > 4. Your path walk follows the second ".." up to /. Again, this is
> > > outside the process root, but you don't notice.
> > > 5. Your path walk walks down to /etc/passwd, and the open completes
> > > successfully. You now have an fd pointing outside your chroot.
> >
> > I've been playing with this and I have the following patch, which
> > according to my testing protects against attacks where ".." skips over
> > nd->root. It abuses __d_path to figure out if nd->path can be resolved
> > from nd->root (obviously a proper version of this patch would refactor
> > __d_path so it could be used like this -- and would not return
> > -EMULTIHOP).
> >
> > I've also attached my reproducer. With it, I was seeing fairly constant
> > breakouts before this patch and after it I didn't see a single breakout
> > after running it overnight. Obviously this is not conclusive, but I'm
> > hoping that it can show what my idea for protecting against ".." was.
> >
> > Does this patch make sense? Or is there something wrong with it that I'm
> > not seeing?
> >
> > --8<-------------------------------------------------------------------
> >
> > There is a fairly easy-to-exploit race condition with chroot(2) (and
> > thus by extension AT_THIS_ROOT and AT_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". 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).
> >
> > The use of __d_path here might seem suspect, however we don't mind if a
> > path is moved from within the chroot to outside the chroot and we
> > incorrectly decide it is safe (because at that point we are still within
> > the set of files which were accessible at the beginning of resolution).
> > However, we can fail resolution on the next path component if it remains
> > outside of the root. A path which has always been outside nd->root
> > during resolution will never be resolveable from nd->root and thus will
> > always be blocked.
> >
> > DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
> > purely as a debugging measure (so that you can see that
> > the protection actually does something). Obviously in the
> > proper patch this will return -EXDEV.
> >
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > fs/namei.c | 32 ++++++++++++++++++++++++++++++--
> > 1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 6f995e6de6b1..c8349693d47b 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -53,8 +53,8 @@
> > * The new code replaces the old recursive symlink resolution with
> > * an iterative one (in case of non-nested symlink chains). It does
> > * this with calls to <fs>_follow_link().
> > - * As a side effect, dir_namei(), _namei() and follow_link() are now
> > - * replaced with a single function lookup_dentry() that can handle all
> > + * As a side effect, dir_namei(), _namei() and follow_link() are now
> > + * replaced with a single function lookup_dentry() that can handle all
> > * the special cases of the former code.
> > *
> > * With the new dcache, the pathname is stored at each inode, at least as
> > @@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -EXDEV;
> > break;
> > }
> > + if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
> > + char *pathbuf, *pathptr;
> > +
> > + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> > + if (!pathbuf)
> > + return -ECHILD;
> > + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> > + kfree(pathbuf);
> > + if (IS_ERR_OR_NULL(pathptr)) {
> > + if (!pathptr)
> > + pathptr = ERR_PTR(-EMULTIHOP);
> > + return PTR_ERR(pathptr);
> > + }
> > + }
>
> One somewhat problematic thing about this approach is that if someone
> tries to lookup
> "a/a/a/a/a/a/a/a/a/a/[...]/../../../../../../../../../.." for some
> reason, you'll have quadratic runtime: For each "..", you'll have to
> walk up to the root.
What if we took rename_lock (call it nd->r_seq) at the start of the
resolution, and then only tried the __d_path-style check
if (read_seqretry(&rename_lock, nd->r_seq) ||
read_seqretry(&mount_lock, nd->m_seq))
/* do the __d_path lookup. */
That way you would only hit the slow path if there were concurrent
renames or mounts *and* you are doing a path resolution with
AT_THIS_ROOT or AT_BENEATH. I've attached a modified patch that does
this (and after some testing it also appears to work).
I'm not sure if there's a way to always avoid the quadratic lookup
without (significantly and probably unreasonably) changing how dcache
invalidation works. And obviously using this slow path if there was
_any_ rename on the _entire_ system is suboptimal, but I think it is a
significant improvement.
Another possibility is to expand on Andy's suggestion to use
/proc/$pid/root, and instead require AT_THIS_ROOT to use the root of a
namespace as its dirfd (I'm not sure if there's a trivial way to detect
this though). This wouldn't help with AT_BENEATH, but it should protect
against ".." shenanigans without any ".." handling changes. (This is
less ideal because it requires a container process, but it is another
way of dealing with the issue.)
---
fs/namei.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 6f995e6de6b1..12c9be175cb4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -493,7 +493,7 @@ struct nameidata {
struct path root;
struct inode *inode; /* path.dentry.d_inode */
unsigned int flags;
- unsigned seq, m_seq;
+ unsigned seq, m_seq, r_seq;
int last_type;
unsigned depth;
int total_link_count;
@@ -1375,6 +1375,27 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -EXDEV;
break;
}
+ if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
+ (read_seqretry(&rename_lock, nd->r_seq) ||
+ read_seqretry(&mount_lock, nd->m_seq)))) {
+ char *pathbuf, *pathptr;
+
+ nd->r_seq = read_seqbegin(&rename_lock);
+ /* Cannot take m_seq here. */
+
+ pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
+ if (!pathbuf)
+ return -ECHILD;
+ pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+ kfree(pathbuf);
+ if (IS_ERR_OR_NULL(pathptr)) {
+ int error = PTR_ERR_OR_ZERO(pathptr);
+
+ if (!error)
+ error = nd_jump_root(nd);
+ return error;
+ }
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1510,6 +1531,27 @@ static int follow_dotdot(struct nameidata *nd)
return -EXDEV;
break;
}
+ if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
+ (read_seqretry(&rename_lock, nd->r_seq) ||
+ read_seqretry(&mount_lock, nd->m_seq)))) {
+ char *pathbuf, *pathptr;
+
+ nd->r_seq = read_seqbegin(&rename_lock);
+ /* Cannot take m_seq here. */
+
+ pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (!pathbuf)
+ return -ENOMEM;
+ pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+ kfree(pathbuf);
+ if (IS_ERR_OR_NULL(pathptr)) {
+ int error = PTR_ERR_OR_ZERO(pathptr);
+
+ if (!error)
+ error = nd_jump_root(nd);
+ return error;
+ }
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -2269,6 +2311,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->last_type = LAST_ROOT; /* if there are only slashes... */
nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
nd->depth = 0;
+ nd->m_seq = read_seqbegin(&mount_lock);
+ nd->r_seq = read_seqbegin(&rename_lock);
+
if (flags & LOOKUP_ROOT) {
struct dentry *root = nd->root.dentry;
struct inode *inode = root->d_inode;
@@ -2279,7 +2324,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);
}
@@ -2290,7 +2334,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(flags & (LOOKUP_CHROOT | LOOKUP_XDEV))) {
error = dirfd_path_init(nd);
if (unlikely(error))
--
2.19.0
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Jann Horn @ 2018-10-05 15:55 UTC (permalink / raw)
To: cyphar, Eric W. Biederman, Al Viro
Cc: jlayton, Bruce Fields, Arnd Bergmann, shuah, David Howells,
Andy Lutomirski, christian, Tycho Andersen, kernel list,
linux-fsdevel, linux-arch, linux-kselftest, dev, containers,
Linux API
In-Reply-To: <20181005150728.mgqnpkbukpeu3bsm@ryuk>
On Fri, Oct 5, 2018 at 5:07 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-10-04, Jann Horn <jannh@google.com> wrote:
> > On Thu, Oct 4, 2018 at 6:26 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > > > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > > > Something else concurrently moves /A/B/C to /A/C. This can result in
> > > > the following:
> > > >
> > > > 1. You start the path walk and reach /A/B/C.
> > > > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > > > 3. Your path walk follows the first ".." up into /A. This is outside
> > > > the process root, but you never actually encountered the process root,
> > > > so you don't notice.
> > > > 4. Your path walk follows the second ".." up to /. Again, this is
> > > > outside the process root, but you don't notice.
> > > > 5. Your path walk walks down to /etc/passwd, and the open completes
> > > > successfully. You now have an fd pointing outside your chroot.
> > >
> > > I've been playing with this and I have the following patch, which
> > > according to my testing protects against attacks where ".." skips over
> > > nd->root. It abuses __d_path to figure out if nd->path can be resolved
> > > from nd->root (obviously a proper version of this patch would refactor
> > > __d_path so it could be used like this -- and would not return
> > > -EMULTIHOP).
> > >
> > > I've also attached my reproducer. With it, I was seeing fairly constant
> > > breakouts before this patch and after it I didn't see a single breakout
> > > after running it overnight. Obviously this is not conclusive, but I'm
> > > hoping that it can show what my idea for protecting against ".." was.
> > >
> > > Does this patch make sense? Or is there something wrong with it that I'm
> > > not seeing?
> > >
> > > --8<-------------------------------------------------------------------
> > >
> > > There is a fairly easy-to-exploit race condition with chroot(2) (and
> > > thus by extension AT_THIS_ROOT and AT_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". 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).
> > >
> > > The use of __d_path here might seem suspect, however we don't mind if a
> > > path is moved from within the chroot to outside the chroot and we
> > > incorrectly decide it is safe (because at that point we are still within
> > > the set of files which were accessible at the beginning of resolution).
> > > However, we can fail resolution on the next path component if it remains
> > > outside of the root. A path which has always been outside nd->root
> > > during resolution will never be resolveable from nd->root and thus will
> > > always be blocked.
> > >
> > > DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
> > > purely as a debugging measure (so that you can see that
> > > the protection actually does something). Obviously in the
> > > proper patch this will return -EXDEV.
> > >
> > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > ---
> > > fs/namei.c | 32 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 6f995e6de6b1..c8349693d47b 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -53,8 +53,8 @@
> > > * The new code replaces the old recursive symlink resolution with
> > > * an iterative one (in case of non-nested symlink chains). It does
> > > * this with calls to <fs>_follow_link().
> > > - * As a side effect, dir_namei(), _namei() and follow_link() are now
> > > - * replaced with a single function lookup_dentry() that can handle all
> > > + * As a side effect, dir_namei(), _namei() and follow_link() are now
> > > + * replaced with a single function lookup_dentry() that can handle all
> > > * the special cases of the former code.
> > > *
> > > * With the new dcache, the pathname is stored at each inode, at least as
> > > @@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > > return -EXDEV;
> > > break;
> > > }
> > > + if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
> > > + char *pathbuf, *pathptr;
> > > +
> > > + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> > > + if (!pathbuf)
> > > + return -ECHILD;
> > > + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> > > + kfree(pathbuf);
> > > + if (IS_ERR_OR_NULL(pathptr)) {
> > > + if (!pathptr)
> > > + pathptr = ERR_PTR(-EMULTIHOP);
> > > + return PTR_ERR(pathptr);
> > > + }
> > > + }
> >
> > One somewhat problematic thing about this approach is that if someone
> > tries to lookup
> > "a/a/a/a/a/a/a/a/a/a/[...]/../../../../../../../../../.." for some
> > reason, you'll have quadratic runtime: For each "..", you'll have to
> > walk up to the root.
>
> What if we took rename_lock (call it nd->r_seq) at the start of the
> resolution, and then only tried the __d_path-style check
>
> if (read_seqretry(&rename_lock, nd->r_seq) ||
> read_seqretry(&mount_lock, nd->m_seq))
> /* do the __d_path lookup. */
>
> That way you would only hit the slow path if there were concurrent
> renames or mounts *and* you are doing a path resolution with
> AT_THIS_ROOT or AT_BENEATH. I've attached a modified patch that does
> this (and after some testing it also appears to work).
Yeah, I think that might do the job.
> I'm not sure if there's a way to always avoid the quadratic lookup
> without (significantly and probably unreasonably) changing how dcache
> invalidation works. And obviously using this slow path if there was
> _any_ rename on the _entire_ system is suboptimal, but I think it is a
> significant improvement.
Yeah, I think this is much better.
> Another possibility is to expand on Andy's suggestion to use
> /proc/$pid/root, and instead require AT_THIS_ROOT to use the root of a
> namespace as its dirfd (I'm not sure if there's a trivial way to detect
> this though). This wouldn't help with AT_BENEATH, but it should protect
> against ".." shenanigans without any ".." handling changes. (This is
> less ideal because it requires a container process, but it is another
> way of dealing with the issue.)
(For container usecases, but not for a web server that uses AT_BENEATH.)
> ---
> fs/namei.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 6f995e6de6b1..12c9be175cb4 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -493,7 +493,7 @@ struct nameidata {
> struct path root;
> struct inode *inode; /* path.dentry.d_inode */
> unsigned int flags;
> - unsigned seq, m_seq;
> + unsigned seq, m_seq, r_seq;
> int last_type;
> unsigned depth;
> int total_link_count;
> @@ -1375,6 +1375,27 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> return -EXDEV;
> break;
> }
> + if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
> + (read_seqretry(&rename_lock, nd->r_seq) ||
> + read_seqretry(&mount_lock, nd->m_seq)))) {
> + char *pathbuf, *pathptr;
> +
> + nd->r_seq = read_seqbegin(&rename_lock);
> + /* Cannot take m_seq here. */
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> + if (!pathbuf)
> + return -ECHILD;
> + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> + kfree(pathbuf);
You're doing this check before actually looking up the parent, right?
So as long as I don't trigger the "path_equal(&nd->path, &nd->root)"
check that you do for O_BENEATH, escaping up by one level is possible,
right? You should probably move this check so that it happens after
following "..".
(Also: I assume that you're going to get rid of that memory allocation
in a future version.)
> + if (IS_ERR_OR_NULL(pathptr)) {
> + int error = PTR_ERR_OR_ZERO(pathptr);
> +
> + if (!error)
> + error = nd_jump_root(nd);
> + return error;
> + }
> + }
> if (nd->path.dentry != nd->path.mnt->mnt_root) {
> struct dentry *old = nd->path.dentry;
> struct dentry *parent = old->d_parent;
> @@ -1510,6 +1531,27 @@ static int follow_dotdot(struct nameidata *nd)
> return -EXDEV;
> break;
> }
> + if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
> + (read_seqretry(&rename_lock, nd->r_seq) ||
> + read_seqretry(&mount_lock, nd->m_seq)))) {
> + char *pathbuf, *pathptr;
> +
> + nd->r_seq = read_seqbegin(&rename_lock);
> + /* Cannot take m_seq here. */
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (!pathbuf)
> + return -ENOMEM;
> + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> + kfree(pathbuf);
> + if (IS_ERR_OR_NULL(pathptr)) {
> + int error = PTR_ERR_OR_ZERO(pathptr);
> +
> + if (!error)
> + error = nd_jump_root(nd);
> + return error;
> + }
> + }
Same problem as in the RCU case above.
> if (nd->path.dentry != nd->path.mnt->mnt_root) {
> int ret = path_parent_directory(&nd->path);
> if (ret)
> @@ -2269,6 +2311,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> nd->last_type = LAST_ROOT; /* if there are only slashes... */
> nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
> nd->depth = 0;
> + nd->m_seq = read_seqbegin(&mount_lock);
> + nd->r_seq = read_seqbegin(&rename_lock);
This means that now, attempting to perform a lookup while something is
holding the rename_lock will spin on the lock. I don't know whether
that's a problem in practice though. Does anyone on this thread know
whether this is problematic?
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Yu-cheng Yu @ 2018-10-05 16:13 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181003195702.GF32759@asgard.redhat.com>
On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
> On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> > Indirect branch tracking provides an optional legacy code bitmap
> > that indicates locations of non-IBT compatible code. When set,
> > each bit in the bitmap represents a page in the linear address is
> > legacy code.
> >
> > We allocate the bitmap only when the application requests it.
> > Most applications do not need the bitmap.
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> > arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> > index 6adfe795d692..a65d9745af08 100644
> > --- a/arch/x86/kernel/cet.c
> > +++ b/arch/x86/kernel/cet.c
> > @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> > wrmsrl(MSR_IA32_U_CET, r);
> > current->thread.cet.ibt_enabled = 0;
> > }
> > +
> > +int cet_setup_ibt_bitmap(void)
> > +{
> > + u64 r;
> > + unsigned long bitmap;
> > + unsigned long size;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > + return -EOPNOTSUPP;
> > +
> > + if (!current->thread.cet.ibt_bitmap_addr) {
> > + /*
> > + * Calculate size and put in thread header.
> > + * may_expand_vm() needs this information.
> > + */
> > + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
>
> TASK_SIZE_MAX is likely needed here, as an application can easily switch
> between long an 32-bit protected mode. And then the case of a CPU that
> doesn't support 5LPT.
If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would have
failed the allocation for bitmap size > TASK_SIZE. Please see values below,
which is printed from the current code.
Yu-cheng
x64:
TASK_SIZE_MAX = 0000 7fff ffff f000
TASK_SIZE = 0000 7fff ffff f000
bitmap size = 0000 0000 ffff ffff
x32:
TASK_SIZE_MAX = 0000 7fff ffff f000
TASK_SIZE = 0000 0000 ffff e000
bitmap size = 0000 0000 0001 ffff
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Andy Lutomirski @ 2018-10-05 16:28 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Eugene Syromiatnikov, 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, Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook, Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <fc2f98ab46240c0498bdf4d7458b4373c1f02bf8.camel@intel.com>
> On Oct 5, 2018, at 9:13 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
>> On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
>>> On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
>>> Indirect branch tracking provides an optional legacy code bitmap
>>> that indicates locations of non-IBT compatible code. When set,
>>> each bit in the bitmap represents a page in the linear address is
>>> legacy code.
>>>
>>> We allocate the bitmap only when the application requests it.
>>> Most applications do not need the bitmap.
>>>
>>> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
>>> ---
>>> arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 45 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
>>> index 6adfe795d692..a65d9745af08 100644
>>> --- a/arch/x86/kernel/cet.c
>>> +++ b/arch/x86/kernel/cet.c
>>> @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
>>> wrmsrl(MSR_IA32_U_CET, r);
>>> current->thread.cet.ibt_enabled = 0;
>>> }
>>> +
>>> +int cet_setup_ibt_bitmap(void)
>>> +{
>>> + u64 r;
>>> + unsigned long bitmap;
>>> + unsigned long size;
>>> +
>>> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (!current->thread.cet.ibt_bitmap_addr) {
>>> + /*
>>> + * Calculate size and put in thread header.
>>> + * may_expand_vm() needs this information.
>>> + */
>>> + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
>>
>> TASK_SIZE_MAX is likely needed here, as an application can easily switch
>> between long an 32-bit protected mode. And then the case of a CPU that
>> doesn't support 5LPT.
>
> If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would have
> failed the allocation for bitmap size > TASK_SIZE. Please see values below,
> which is printed from the current code.
>
> Yu-cheng
>
>
> x64:
> TASK_SIZE_MAX = 0000 7fff ffff f000
> TASK_SIZE = 0000 7fff ffff f000
> bitmap size = 0000 0000 ffff ffff
>
> x32:
> TASK_SIZE_MAX = 0000 7fff ffff f000
> TASK_SIZE = 0000 0000 ffff e000
> bitmap size = 0000 0000 0001 ffff
>
I haven’t followed all the details here, but I have a general policy of objecting to any new use of TASK_SIZE. If you really really need to depend on 32-bitness in new code, please figure out what exactly you mean by “32-bit” and use an explicit check.
Some day I would love to delete TASK_SIZE.
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Yu-cheng Yu @ 2018-10-05 16:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eugene Syromiatnikov, 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, Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook, Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <5BF3AE8F-CC2A-4160-9FF6-FEA171A76371@amacapital.net>
On Fri, 2018-10-05 at 09:28 -0700, Andy Lutomirski wrote:
> > On Oct 5, 2018, at 9:13 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > > On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
> > > > On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> > > > Indirect branch tracking provides an optional legacy code bitmap
> > > > that indicates locations of non-IBT compatible code. When set,
> > > > each bit in the bitmap represents a page in the linear address is
> > > > legacy code.
> > > >
> > > > We allocate the bitmap only when the application requests it.
> > > > Most applications do not need the bitmap.
> > > >
> > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > > > ---
> > > > arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 45 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> > > > index 6adfe795d692..a65d9745af08 100644
> > > > --- a/arch/x86/kernel/cet.c
> > > > +++ b/arch/x86/kernel/cet.c
> > > > @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> > > > wrmsrl(MSR_IA32_U_CET, r);
> > > > current->thread.cet.ibt_enabled = 0;
> > > > }
> > > > +
> > > > +int cet_setup_ibt_bitmap(void)
> > > > +{
> > > > + u64 r;
> > > > + unsigned long bitmap;
> > > > + unsigned long size;
> > > > +
> > > > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > + return -EOPNOTSUPP;
> > > > +
> > > > + if (!current->thread.cet.ibt_bitmap_addr) {
> > > > + /*
> > > > + * Calculate size and put in thread header.
> > > > + * may_expand_vm() needs this information.
> > > > + */
> > > > + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
> > >
> > > TASK_SIZE_MAX is likely needed here, as an application can easily switch
> > > between long an 32-bit protected mode. And then the case of a CPU that
> > > doesn't support 5LPT.
> >
> > If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would
> > have
> > failed the allocation for bitmap size > TASK_SIZE. Please see values below,
> > which is printed from the current code.
> >
> > Yu-cheng
> >
> >
> > x64:
> > TASK_SIZE_MAX = 0000 7fff ffff f000
> > TASK_SIZE = 0000 7fff ffff f000
> > bitmap size = 0000 0000 ffff ffff
> >
> > x32:
> > TASK_SIZE_MAX = 0000 7fff ffff f000
> > TASK_SIZE = 0000 0000 ffff e000
> > bitmap size = 0000 0000 0001 ffff
> >
>
> I haven’t followed all the details here, but I have a general policy of
> objecting to any new use of TASK_SIZE. If you really really need to depend on
> 32-bitness in new code, please figure out what exactly you mean by “32-bit”
> and use an explicit check.
The explicit check would be:
test_thread_flag(TIF_ADDR32) ? IA32_PAGE_OFFSET : TASK_SIZE_MAX
which is the same as TASK_SIZE.
Or, do we want a new macro?
#define IBT_BITMAP_SIZE (test_thread_flag(TIF_ADDR32) ? \
(IA32_PAGE_OFFSET / PAGE_SIZE / BITS_PER_BYTE) : \
(TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE))
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Andy Lutomirski @ 2018-10-05 17:07 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Eugene Syromiatnikov, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <aa5a061c159471f410d677af6a609793906cece1.camel@intel.com>
On Fri, Oct 5, 2018 at 10:03 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> On Fri, 2018-10-05 at 09:28 -0700, Andy Lutomirski wrote:
> > > On Oct 5, 2018, at 9:13 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > >
> > > > On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
> > > > > On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> > > > > Indirect branch tracking provides an optional legacy code bitmap
> > > > > that indicates locations of non-IBT compatible code. When set,
> > > > > each bit in the bitmap represents a page in the linear address is
> > > > > legacy code.
> > > > >
> > > > > We allocate the bitmap only when the application requests it.
> > > > > Most applications do not need the bitmap.
> > > > >
> > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > > > > ---
> > > > > arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 45 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> > > > > index 6adfe795d692..a65d9745af08 100644
> > > > > --- a/arch/x86/kernel/cet.c
> > > > > +++ b/arch/x86/kernel/cet.c
> > > > > @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> > > > > wrmsrl(MSR_IA32_U_CET, r);
> > > > > current->thread.cet.ibt_enabled = 0;
> > > > > }
> > > > > +
> > > > > +int cet_setup_ibt_bitmap(void)
> > > > > +{
> > > > > + u64 r;
> > > > > + unsigned long bitmap;
> > > > > + unsigned long size;
> > > > > +
> > > > > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > > + return -EOPNOTSUPP;
> > > > > +
> > > > > + if (!current->thread.cet.ibt_bitmap_addr) {
> > > > > + /*
> > > > > + * Calculate size and put in thread header.
> > > > > + * may_expand_vm() needs this information.
> > > > > + */
> > > > > + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
> > > >
> > > > TASK_SIZE_MAX is likely needed here, as an application can easily switch
> > > > between long an 32-bit protected mode. And then the case of a CPU that
> > > > doesn't support 5LPT.
> > >
> > > If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would
> > > have
> > > failed the allocation for bitmap size > TASK_SIZE. Please see values below,
> > > which is printed from the current code.
> > >
> > > Yu-cheng
> > >
> > >
> > > x64:
> > > TASK_SIZE_MAX = 0000 7fff ffff f000
> > > TASK_SIZE = 0000 7fff ffff f000
> > > bitmap size = 0000 0000 ffff ffff
> > >
> > > x32:
> > > TASK_SIZE_MAX = 0000 7fff ffff f000
> > > TASK_SIZE = 0000 0000 ffff e000
> > > bitmap size = 0000 0000 0001 ffff
> > >
> >
> > I haven’t followed all the details here, but I have a general policy of
> > objecting to any new use of TASK_SIZE. If you really really need to depend on
> > 32-bitness in new code, please figure out what exactly you mean by “32-bit”
> > and use an explicit check.
>
> The explicit check would be:
>
> test_thread_flag(TIF_ADDR32) ? IA32_PAGE_OFFSET : TASK_SIZE_MAX
>
> which is the same as TASK_SIZE.
But this is only ever done in response to a syscall, right? So
wouldn't in_compat_syscall() be the right check?
Also, this whole thing makes me extremely nervous. The MSR only
contains the start address, not the size, right? So what prevents
some goof from causing the CPU to read way past the end of the bitmap
if the bitmap is short because the kernel thought it was supposed to
be 32-bit?
I'm inclined to suggest something awful-ish: always allocate the
bitmap as though it's for a 64-bit process, and just let it be at a
high address. And add a syscall or arch_prctl() to manipulate it for
the benefit of 32-bit programs that can't address it directly.
>
> Or, do we want a new macro?
>
> #define IBT_BITMAP_SIZE (test_thread_flag(TIF_ADDR32) ? \
> (IA32_PAGE_OFFSET / PAGE_SIZE / BITS_PER_BYTE) : \
> (TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE))
No. I don't like hiding magic like this in a macro that looks like a constant.
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Eugene Syromiatnikov @ 2018-10-05 17:26 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit
In-Reply-To: <CALCETrXVdYsJsVy=QWruDYdRc6wb44b=0J3OK3zjR_fT1fQH7w@mail.gmail.com>
On Fri, Oct 05, 2018 at 10:07:46AM -0700, Andy Lutomirski wrote:
> On Fri, Oct 5, 2018 at 10:03 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > On Fri, 2018-10-05 at 09:28 -0700, Andy Lutomirski wrote:
> > > > On Oct 5, 2018, at 9:13 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > >
> > > > > On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
> > > > > > On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> > > > > > Indirect branch tracking provides an optional legacy code bitmap
> > > > > > that indicates locations of non-IBT compatible code. When set,
> > > > > > each bit in the bitmap represents a page in the linear address is
> > > > > > legacy code.
> > > > > >
> > > > > > We allocate the bitmap only when the application requests it.
> > > > > > Most applications do not need the bitmap.
> > > > > >
> > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > > > > > ---
> > > > > > arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 45 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> > > > > > index 6adfe795d692..a65d9745af08 100644
> > > > > > --- a/arch/x86/kernel/cet.c
> > > > > > +++ b/arch/x86/kernel/cet.c
> > > > > > @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> > > > > > wrmsrl(MSR_IA32_U_CET, r);
> > > > > > current->thread.cet.ibt_enabled = 0;
> > > > > > }
> > > > > > +
> > > > > > +int cet_setup_ibt_bitmap(void)
> > > > > > +{
> > > > > > + u64 r;
> > > > > > + unsigned long bitmap;
> > > > > > + unsigned long size;
> > > > > > +
> > > > > > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > > > + return -EOPNOTSUPP;
> > > > > > +
> > > > > > + if (!current->thread.cet.ibt_bitmap_addr) {
> > > > > > + /*
> > > > > > + * Calculate size and put in thread header.
> > > > > > + * may_expand_vm() needs this information.
> > > > > > + */
> > > > > > + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
> > > > >
> > > > > TASK_SIZE_MAX is likely needed here, as an application can easily switch
> > > > > between long an 32-bit protected mode. And then the case of a CPU that
> > > > > doesn't support 5LPT.
> > > >
> > > > If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would
> > > > have
> > > > failed the allocation for bitmap size > TASK_SIZE. Please see values below,
> > > > which is printed from the current code.
> > > >
> > > > Yu-cheng
> > > >
> > > >
> > > > x64:
> > > > TASK_SIZE_MAX = 0000 7fff ffff f000
> > > > TASK_SIZE = 0000 7fff ffff f000
> > > > bitmap size = 0000 0000 ffff ffff
> > > >
> > > > x32:
> > > > TASK_SIZE_MAX = 0000 7fff ffff f000
> > > > TASK_SIZE = 0000 0000 ffff e000
> > > > bitmap size = 0000 0000 0001 ffff
> > > >
> > >
> > > I haven’t followed all the details here, but I have a general policy of
> > > objecting to any new use of TASK_SIZE. If you really really need to depend on
> > > 32-bitness in new code, please figure out what exactly you mean by “32-bit”
> > > and use an explicit check.
> >
> > The explicit check would be:
> >
> > test_thread_flag(TIF_ADDR32) ? IA32_PAGE_OFFSET : TASK_SIZE_MAX
> >
> > which is the same as TASK_SIZE.
>
> But this is only ever done in response to a syscall, right? So
> wouldn't in_compat_syscall() be the right check?
>
> Also, this whole thing makes me extremely nervous. The MSR only
> contains the start address, not the size, right? So what prevents
> some goof from causing the CPU to read way past the end of the bitmap
> if the bitmap is short because the kernel thought it was supposed to
> be 32-bit?
That's what I've mentioned initially: every syscall made with int 0x80
is interpreted as compat, even if it was made from long mode.
> I'm inclined to suggest something awful-ish: always allocate the
> bitmap as though it's for a 64-bit process, and just let it be at a
> high address. And add a syscall or arch_prctl() to manipulate it for
> the benefit of 32-bit programs that can't address it directly.
That's likely the only way to go.
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Alexei Starovoitov @ 2018-10-05 21:07 UTC (permalink / raw)
To: Jann Horn
Cc: lmb, Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CAG48ez0+1k90oK2qrwbg0HAWF=eSocZee=Mi75E_7UY0u7TGJg@mail.gmail.com>
On Fri, Oct 05, 2018 at 04:27:58PM +0200, Jann Horn wrote:
>
> Can you please describe exactly why something that is not a kernel
> unit test needs deterministic BPF hash map behavior?
my use case for deterministic hashing is performance analysis.
Both while developing and tuning bpf program and while optimizing
kernel side implementation.
Local dos is a valid concern, so requiring root for this flag makes sense.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-06 2:10 UTC (permalink / raw)
To: Jann Horn
Cc: Eric W. Biederman, Al Viro, jlayton, Bruce Fields, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, cyphar@cyphar.com Linux API
In-Reply-To: <CAG48ez3FG0axt92F=+vDe1Q=kj-YngmCgFqDET7E2xCE1mOgeg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3987 bytes --]
On 2018-10-05, Jann Horn <jannh@google.com> wrote:
> > What if we took rename_lock (call it nd->r_seq) at the start of the
> > resolution, and then only tried the __d_path-style check
> >
> > if (read_seqretry(&rename_lock, nd->r_seq) ||
> > read_seqretry(&mount_lock, nd->m_seq))
> > /* do the __d_path lookup. */
> >
> > That way you would only hit the slow path if there were concurrent
> > renames or mounts *and* you are doing a path resolution with
> > AT_THIS_ROOT or AT_BENEATH. I've attached a modified patch that does
> > this (and after some testing it also appears to work).
>
> Yeah, I think that might do the job.
*phew* I was all out of other ideas. :P
> > ---
> > fs/namei.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 6f995e6de6b1..12c9be175cb4 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -493,7 +493,7 @@ struct nameidata {
> > struct path root;
> > struct inode *inode; /* path.dentry.d_inode */
> > unsigned int flags;
> > - unsigned seq, m_seq;
> > + unsigned seq, m_seq, r_seq;
> > int last_type;
> > unsigned depth;
> > int total_link_count;
> > @@ -1375,6 +1375,27 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -EXDEV;
> > break;
> > }
> > + if (unlikely((nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)) &&
> > + (read_seqretry(&rename_lock, nd->r_seq) ||
> > + read_seqretry(&mount_lock, nd->m_seq)))) {
> > + char *pathbuf, *pathptr;
> > +
> > + nd->r_seq = read_seqbegin(&rename_lock);
> > + /* Cannot take m_seq here. */
> > +
> > + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> > + if (!pathbuf)
> > + return -ECHILD;
> > + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> > + kfree(pathbuf);
>
> You're doing this check before actually looking up the parent, right?
> So as long as I don't trigger the "path_equal(&nd->path, &nd->root)"
> check that you do for O_BENEATH, escaping up by one level is possible,
> right? You should probably move this check so that it happens after
> following "..".
Yup, you're right. I'll do that.
> (Also: I assume that you're going to get rid of that memory allocation
> in a future version.)
Sure. Would you prefer adding some scratch space in nameidata, or that I
change __d_path so it accepts NULL as the buffer (and thus it doesn't
actually do any string operations)?
> > if (nd->path.dentry != nd->path.mnt->mnt_root) {
> > int ret = path_parent_directory(&nd->path);
> > if (ret)
> > @@ -2269,6 +2311,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> > nd->last_type = LAST_ROOT; /* if there are only slashes... */
> > nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
> > nd->depth = 0;
> > + nd->m_seq = read_seqbegin(&mount_lock);
> > + nd->r_seq = read_seqbegin(&rename_lock);
>
> This means that now, attempting to perform a lookup while something is
> holding the rename_lock will spin on the lock. I don't know whether
> that's a problem in practice though. Does anyone on this thread know
> whether this is problematic?
I could make it so that we only take &rename_lock
if (nd->flags & (FOLLOW_BENEATH | FOLLOW_CHROOT)),
since it's not used outside of that path.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC v3 1/1] ns: add binfmt_misc to the user namespace
From: Andrei Vagin @ 2018-10-06 6:04 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Andrei Vagin, Jann Horn, linux-api, containers,
Dmitry Safonov, James Bottomley, Alexander Viro, linux-fsdevel,
Eric Biederman
In-Reply-To: <20181003225022.32033-2-laurent@vivier.eu>
On Thu, Oct 04, 2018 at 12:50:22AM +0200, Laurent Vivier wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the host, but if the binfmt_misc filesystem is mounted
> in the new namespace a new empty binfmt instance is created and used
> in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> fs/binfmt_misc.c | 85 +++++++++++++++++++++++-----------
> include/linux/user_namespace.h | 15 ++++++
> kernel/user.c | 14 ++++++
> kernel/user_namespace.c | 9 ++++
> 4 files changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..78780bc87506 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -38,9 +38,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +57,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -85,13 +79,13 @@ static int entry_count;
> * if we do, return the node, else NULL
> * locking is done in load_misc_binary
> */
> -static Node *check_file(struct linux_binprm *bprm)
> +static Node *check_file(struct user_namespace *ns, struct linux_binprm *bprm)
> {
> char *p = strrchr(bprm->interp, '.');
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &ns->binfmt_ns->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +127,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct user_namespace *ns = current_user_ns();
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->binfmt_ns->enabled)
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> - fmt = check_file(bprm);
> + read_lock(&ns->binfmt_ns->entries_lock);
It looks like ns->binfmt_ns isn't protected by any lock and
ns->binfmt_ns can be changed between read_lock() and read_unlock().
This can be fixed if ns->binfmt_ns will be dereferenced only once in
this function:
struct binfmt_namespace *binfmt_ns = ns->binfmt_ns;
> + fmt = check_file(ns ,bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->binfmt_ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +604,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct user_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->binfmt_ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&ns->binfmt_ns->entries_lock);
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->binfmt_ns->bm_mnt, &ns->binfmt_ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +646,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> struct dentry *root;
> Node *e = file_inode(file)->i_private;
> int res = parse_command(buffer, count);
> + struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
>
> switch (res) {
> case 1:
> @@ -667,7 +663,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> inode_lock(d_inode(root));
>
> if (!list_empty(&e->list))
> - kill_node(e);
> + kill_node(ns, e);
>
> inode_unlock(d_inode(root));
> break;
> @@ -693,6 +689,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> struct inode *inode;
> struct super_block *sb = file_inode(file)->i_sb;
> struct dentry *root = sb->s_root, *dentry;
> + struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +713,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + err = simple_pin_fs(&bm_fs_type, &ns->binfmt_ns->bm_mnt,
> + &ns->binfmt_ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +723,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>
> if (e->flags & MISC_FMT_OPEN_FILE) {
> struct file *f;
> + const struct cred *old_cred;
>
> + old_cred = override_creds(file->f_cred);
> f = open_exec(e->interpreter);
> + revert_creds(old_cred);
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->binfmt_ns->bm_mnt,
> + &ns->binfmt_ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +745,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&ns->binfmt_ns->entries_lock);
> + list_add(&e->list, &ns->binfmt_ns->entries);
> + write_unlock(&ns->binfmt_ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +772,8 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
> + char *s = ns->binfmt_ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +781,27 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> + struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->binfmt_ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + ns->binfmt_ns->enabled = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&ns->binfmt_ns->entries))
> + kill_node(ns, list_first_entry(&ns->binfmt_ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -838,7 +843,30 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (ns != &init_user_ns && ns->binfmt_ns == &init_binfmt_ns) {
> + struct binfmt_namespace *binfmt_ns;
> +
> + binfmt_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (binfmt_ns == NULL) {
> + return ERR_PTR(-ENOMEM);
> + }
> + INIT_LIST_HEAD(&binfmt_ns->entries);
> + binfmt_ns->enabled = 1;
> + rwlock_init(&binfmt_ns->entries_lock);
> + binfmt_ns->bm_mnt = NULL;
> + binfmt_ns->entry_count = 0;
> + ns->binfmt_ns = binfmt_ns;
> + }
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +877,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d6b74b91096b..319141da5315 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -52,6 +52,18 @@ enum ucount_type {
> UCOUNT_COUNTS,
> };
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace {
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> +} __randomize_layout;
> +
> +extern struct binfmt_namespace init_binfmt_ns;
> +#endif
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -76,6 +88,9 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> int ucount_max[UCOUNT_COUNTS];
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + struct binfmt_namespace *binfmt_ns;
> +#endif
> } __randomize_layout;
>
> struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index 0df9b1640b2a..220ab2053d44 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -19,6 +19,17 @@
> #include <linux/user_namespace.h>
> #include <linux/proc_ns.h>
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace init_binfmt_ns = {
> + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> + .enabled = 1,
> + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> + .bm_mnt = NULL,
> + .entry_count = 0,
> +};
> +EXPORT_SYMBOL_GPL(init_binfmt_ns);
> +#endif
> +
> /*
> * userns count is 1 for root user, 1 for init_uts_ns,
> * and 1 for... ?
> @@ -66,6 +77,9 @@ struct user_namespace init_user_ns = {
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> #endif
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + .binfmt_ns = &init_binfmt_ns,
> +#endif
> };
> EXPORT_SYMBOL_GPL(init_user_ns);
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..dec0ab4a729a 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
> if (!setup_userns_sysctls(ns))
> goto fail_keyring;
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + ns->binfmt_ns = &init_binfmt_ns;
User namespaces are hierarchical, so I think binfmt_ns should be
inherited from a parent userns.
I suggest to initialize ns->binfmt_ns by NULL for a new namespace
ns->binfmt_ns = NULL;
And create a helper to get a current binfmt_ns:
struct binfmt_namespace current_binfmt_ns()
{
struct user_namespace *ns = current_user_ns();
struct binfmt_namespace *binfmt_ns;
while (ns) {
if (ns->binfmt_ns) {
binfmt_ns = ns->binfmt_ns;
break;
}
}
return ns;
}
In bm_mount, a new binfmt_ns should be created if ns->binfmt_ns is
NULL
> +#endif
> +
> set_cred_user_ns(new, ns);
> return 0;
> fail_keyring:
> @@ -195,6 +199,11 @@ static void free_user_ns(struct work_struct *work)
> kfree(ns->projid_map.forward);
> kfree(ns->projid_map.reverse);
> }
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + if (ns->binfmt_ns != &init_binfmt_ns) {
> + kfree(ns->binfmt_ns);
> + }
> +#endif
> retire_userns_sysctls(ns);
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> key_put(ns->persistent_keyring_register);
> --
> 2.17.1
>
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* [RFC v4 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-06 19:35 UTC (permalink / raw)
To: linux-kernel
Cc: Andrei Vagin, linux-fsdevel, Eric Biederman, linux-api,
Dmitry Safonov, containers, Alexander Viro, Jann Horn,
James Bottomley, Laurent Vivier
v4: first user namespace is initialized with &init_binfmt_ns,
all new user namespaces are initialized with a NULL and use
the one of the first parent that is not NULL. The pointer
is initialized to a valid value the first time the binfmt_misc
fs is mounted in the current user namespace.
This allows to not change the way it was working before:
new ns inherits values from its parent, and if parent values are
modified (or parent creates its own binfmt entry by mounting the fs)
child inherits it (unless it has itself mounted the fs).
v3: create a structure to store binfmt_misc data,
add a pointer to this structure in the user_namespace structure,
in init_user_ns structure this pointer points to an init_binfmt_ns
structure. And all new user namespaces point to this init structure.
A new binfmt namespace structure is allocated if the binfmt_misc
filesystem is mounted in a user namespace that is not the initial
one but its binfmt namespace pointer points to the initial one.
add override_creds()/revert_creds() around open_exec() in
bm_register_write()
v2: no new namespace, binfmt_misc data are now part of
the mount namespace
I put this in mount namespace instead of user namespace
because the mount namespace is already needed and
I don't want to force to have the user namespace for that.
As this is a filesystem, it seems logic to have it here.
This allows to define a new interpreter for each new container.
But the main goal is to be able to chroot to a directory
using a binfmt_misc interpreter without being root.
I have a modified version of unshare at:
git@github.com:vivier/util-linux.git branch unshare-chroot
with some new options to unshare binfmt_misc namespace and to chroot
to a directory.
If you have a directory /chroot/powerpc/jessie containing debian for powerpc
binaries and a qemu-ppc interpreter, you can do for instance:
$ uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
$ ./unshare --map-root-user --fork --pid \
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
--root=/chroot/powerpc/jessie /bin/bash -l
# uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
# id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
# ls -l
total 5940
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
-rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
If you want to use the qemu binary provided by your distro, you can use
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
With the 'F' flag, qemu-ppc-static will be then loaded from the main root
filesystem before switching to the chroot.
Laurent Vivier (1):
ns: add binfmt_misc to the user namespace
fs/binfmt_misc.c | 99 ++++++++++++++++++++++++----------
include/linux/user_namespace.h | 13 +++++
kernel/user.c | 13 +++++
kernel/user_namespace.c | 7 +++
4 files changed, 104 insertions(+), 28 deletions(-)
--
2.17.1
^ permalink raw reply
* [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-06 19:35 UTC (permalink / raw)
To: linux-kernel
Cc: Andrei Vagin, linux-fsdevel, Eric Biederman, linux-api,
Dmitry Safonov, containers, Alexander Viro, Jann Horn,
James Bottomley, Laurent Vivier
In-Reply-To: <20181006193546.29340-1-laurent@vivier.eu>
This patch allows to have a different binfmt_misc configuration
for each new user namespace. By default, the binfmt_misc configuration
is the one of the previous level, but if the binfmt_misc filesystem is
mounted in the new namespace a new empty binfmt instance is created and
used in this namespace.
For instance, using "unshare" we can start a chroot of an another
architecture and configure the binfmt_misc interpreter without being root
to run the binaries in this chroot.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
fs/binfmt_misc.c | 99 ++++++++++++++++++++++++----------
include/linux/user_namespace.h | 13 +++++
kernel/user.c | 13 +++++
kernel/user_namespace.c | 7 +++
4 files changed, 104 insertions(+), 28 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..1beefafcb416 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -38,9 +38,6 @@ enum {
VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
};
-static LIST_HEAD(entries);
-static int enabled = 1;
-
enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
@@ -60,10 +57,7 @@ typedef struct {
struct file *interp_file;
} Node;
-static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
/*
* Max length of the register string. Determined by:
@@ -80,18 +74,28 @@ static int entry_count;
*/
#define MAX_REGISTER_LENGTH 1920
+static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
+{
+ while (ns) {
+ if (ns->binfmt_ns)
+ return ns->binfmt_ns;
+ ns = ns->parent;
+ }
+ return NULL;
+}
+
/*
* Check if we support the binfmt
* if we do, return the node, else NULL
* locking is done in load_misc_binary
*/
-static Node *check_file(struct linux_binprm *bprm)
+static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
{
char *p = strrchr(bprm->interp, '.');
struct list_head *l;
/* Walk all the registered handlers. */
- list_for_each(l, &entries) {
+ list_for_each(l, &ns->entries) {
Node *e = list_entry(l, Node, list);
char *s;
int j;
@@ -133,17 +137,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
struct file *interp_file = NULL;
int retval;
int fd_binary = -1;
+ struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
retval = -ENOEXEC;
- if (!enabled)
+ if (!ns->enabled)
return retval;
/* to keep locking time low, we copy the interpreter string */
- read_lock(&entries_lock);
- fmt = check_file(bprm);
+ read_lock(&ns->entries_lock);
+ fmt = check_file(ns, bprm);
if (fmt)
dget(fmt->dentry);
- read_unlock(&entries_lock);
+ read_unlock(&ns->entries_lock);
if (!fmt)
return retval;
@@ -609,19 +614,19 @@ static void bm_evict_inode(struct inode *inode)
kfree(e);
}
-static void kill_node(Node *e)
+static void kill_node(struct binfmt_namespace *ns, Node *e)
{
struct dentry *dentry;
- write_lock(&entries_lock);
+ write_lock(&ns->entries_lock);
list_del_init(&e->list);
- write_unlock(&entries_lock);
+ write_unlock(&ns->entries_lock);
dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt, &ns->entry_count);
}
/* /<entry> */
@@ -651,6 +656,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
struct dentry *root;
Node *e = file_inode(file)->i_private;
int res = parse_command(buffer, count);
+ struct binfmt_namespace *ns;
+
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
switch (res) {
case 1:
@@ -667,7 +675,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
inode_lock(d_inode(root));
if (!list_empty(&e->list))
- kill_node(e);
+ kill_node(ns, e);
inode_unlock(d_inode(root));
break;
@@ -693,6 +701,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
struct inode *inode;
struct super_block *sb = file_inode(file)->i_sb;
struct dentry *root = sb->s_root, *dentry;
+ struct binfmt_namespace *ns;
int err = 0;
e = create_entry(buffer, count);
@@ -716,7 +725,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
+ err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
+ &ns->entry_count);
if (err) {
iput(inode);
inode = NULL;
@@ -725,12 +736,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (e->flags & MISC_FMT_OPEN_FILE) {
struct file *f;
+ const struct cred *old_cred;
+ old_cred = override_creds(file->f_cred);
f = open_exec(e->interpreter);
+ revert_creds(old_cred);
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt,
+ &ns->entry_count);
iput(inode);
inode = NULL;
goto out2;
@@ -743,9 +758,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
inode->i_fop = &bm_entry_operations;
d_instantiate(dentry, inode);
- write_lock(&entries_lock);
- list_add(&e->list, &entries);
- write_unlock(&entries_lock);
+ write_lock(&ns->entries_lock);
+ list_add(&e->list, &ns->entries);
+ write_unlock(&ns->entries_lock);
err = 0;
out2:
@@ -770,7 +785,9 @@ static const struct file_operations bm_register_operations = {
static ssize_t
bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- char *s = enabled ? "enabled\n" : "disabled\n";
+ struct binfmt_namespace *ns =
+ binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
+ char *s = ns->enabled ? "enabled\n" : "disabled\n";
return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
}
@@ -778,25 +795,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
static ssize_t bm_status_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
+ struct binfmt_namespace *ns;
int res = parse_command(buffer, count);
struct dentry *root;
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
switch (res) {
case 1:
/* Disable all handlers. */
- enabled = 0;
+ ns->enabled = 0;
break;
case 2:
/* Enable all handlers. */
- enabled = 1;
+ ns->enabled = 1;
break;
case 3:
/* Delete all handlers. */
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));
- while (!list_empty(&entries))
- kill_node(list_first_entry(&entries, Node, list));
+ while (!list_empty(&ns->entries))
+ kill_node(ns, list_first_entry(&ns->entries,
+ Node, list));
inode_unlock(d_inode(root));
break;
@@ -838,7 +858,29 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
static struct dentry *bm_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- return mount_single(fs_type, flags, data, bm_fill_super);
+ struct user_namespace *ns = current_user_ns();
+
+ /* create a new binfmt namespace
+ * if we are not in the first user namespace
+ * but the binfmt namespace is the first one
+ */
+ if (ns->binfmt_ns == NULL) {
+ struct binfmt_namespace *new_ns;
+
+ new_ns = kmalloc(sizeof(struct binfmt_namespace),
+ GFP_KERNEL);
+ if (new_ns == NULL)
+ return ERR_PTR(-ENOMEM);
+ INIT_LIST_HEAD(&new_ns->entries);
+ new_ns->enabled = 1;
+ rwlock_init(&new_ns->entries_lock);
+ new_ns->bm_mnt = NULL;
+ new_ns->entry_count = 0;
+ ns->binfmt_ns = new_ns;
+ }
+
+ return mount_ns(fs_type, flags, data, ns, ns,
+ bm_fill_super);
}
static struct linux_binfmt misc_format = {
@@ -849,6 +891,7 @@ static struct linux_binfmt misc_format = {
static struct file_system_type bm_fs_type = {
.owner = THIS_MODULE,
.name = "binfmt_misc",
+ .fs_flags = FS_USERNS_MOUNT,
.mount = bm_mount,
.kill_sb = kill_litter_super,
};
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index d6b74b91096b..5c6e7e63b97e 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -52,6 +52,16 @@ enum ucount_type {
UCOUNT_COUNTS,
};
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace {
+ struct list_head entries;
+ rwlock_t entries_lock;
+ int enabled;
+ struct vfsmount *bm_mnt;
+ int entry_count;
+} __randomize_layout;
+#endif
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -76,6 +86,9 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
int ucount_max[UCOUNT_COUNTS];
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ struct binfmt_namespace *binfmt_ns;
+#endif
} __randomize_layout;
struct ucounts {
diff --git a/kernel/user.c b/kernel/user.c
index 0df9b1640b2a..912916d435aa 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -19,6 +19,16 @@
#include <linux/user_namespace.h>
#include <linux/proc_ns.h>
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+static struct binfmt_namespace init_binfmt_ns = {
+ .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
+ .enabled = 1,
+ .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
+ .bm_mnt = NULL,
+ .entry_count = 0,
+};
+#endif
+
/*
* userns count is 1 for root user, 1 for init_uts_ns,
* and 1 for... ?
@@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
#endif
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ .binfmt_ns = &init_binfmt_ns,
+#endif
};
EXPORT_SYMBOL_GPL(init_user_ns);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index e5222b5fb4fe..da4950282ea1 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
if (!setup_userns_sysctls(ns))
goto fail_keyring;
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ ns->binfmt_ns = NULL;
+#endif
+
set_cred_user_ns(new, ns);
return 0;
fail_keyring:
@@ -195,6 +199,9 @@ static void free_user_ns(struct work_struct *work)
kfree(ns->projid_map.forward);
kfree(ns->projid_map.reverse);
}
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ kfree(ns->binfmt_ns);
+#endif
retire_userns_sysctls(ns);
#ifdef CONFIG_PERSISTENT_KEYRINGS
key_put(ns->persistent_keyring_register);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Florian Weimer @ 2018-10-06 20:56 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Andy Lutomirski, Jann Horn, Eric W. Biederman, jlayton,
Bruce Fields, Al Viro, Arnd Bergmann, shuah, David Howells,
Andy Lutomirski, christian, Tycho Andersen, kernel list,
linux-fsdevel, linux-arch, linux-kselftest, dev, containers,
Linux API
In-Reply-To: <20181002073220.7mzndna4tdnxdvdt@ryuk>
* Aleksa Sarai:
> On 2018-10-01, Andy Lutomirski <luto@amacapital.net> wrote:
>> >>> 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 which is *very* costly if necessary for every
>> >>> filesystem operation involving a container.
>> >>
>> >> Wait. fork() I understand, but why exec? And actually, you don't need
>> >> a full fork() either, clone() lets you do this with some process parts
>> >> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
>> >> the file descriptor table shared. And why chroot()/pivot_root(),
>> >> wouldn't you want to use setns()?
>> >
>> > You're right about this -- for C runtimes. In Go we cannot do a raw
>> > clone() or fork() (if you do it manually with RawSyscall you'll end with
>> > broken runtime state). So you're forced to do fork+exec (which then
>> > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
>> > for CLONE_VFORK.
>>
>> I must admit that I’m not very sympathetic to the argument that “Go’s
>> runtime model is incompatible with the simpler solution.”
>
> Multi-threaded programs have a similar issue (though with Go it's much
> worse). If you fork a multi-threaded C program then you can only safely
> use AS-Safe glibc functions (those that are safe within a signal
> handler). But if you're just doing three syscalls this shouldn't be as
> big of a problem as Go where you can't even do said syscalls.
The situation is a bit more complicated. There are many programs out
there which use malloc and free (at least indirectly) after a fork,
and we cannot break them. In glibc, we have a couple of subsystems
which are put into a known state before calling the fork/clone system
call if the application calls fork. The price we pay for that is a
fork which is not POSIX-compliant because it is not async-signal-safe.
Admittedly, other libcs chose different trade-offs.
However, what is the same across libcs is this: You cannot call the
clone system call directly and get a fully working new process. Some
things break. For example, for recursive mutexes, we need to know the
TID of the current thread, and we cannot perform a system call to get
it for performance reasons. So everyone has a TID cache for that.
But the TID cache does not get reset when you bypass the fork
implementation in libc, so you end up with subtle corruption bugs on
TID reuse.
So I'd say that in most cases, the C situation is pretty much the same
as the Go situation. If I recall correctly, the problem for Go is
that it cannot call setns from Go code because it fails in the kernel
for multi-threaded processes, and Go processes are already
multi-threaded when user Go code runs.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-06 21:49 UTC (permalink / raw)
To: fw
Cc: Aleksa Sarai, luto, Jann Horn, Eric W . Biederman, Jeff Layton,
J. Bruce Fields, Al Viro, Arnd Bergmann, Shuah Khan,
David Howells, Andy Lutomirski, Tycho Andersen, LKML,
linux-fsdevel, linux-arch, linux-kselftest, dev, Linux Containers,
Linux API
In-Reply-To: <875zyeg5fs.fsf@mid.deneb.enyo.de>
On Sat, Oct 6, 2018 at 10:56 PM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Aleksa Sarai:
>
> > On 2018-10-01, Andy Lutomirski <luto@amacapital.net> wrote:
> >> >>> 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 which is *very* costly if necessary for every
> >> >>> filesystem operation involving a container.
> >> >>
> >> >> Wait. fork() I understand, but why exec? And actually, you don't need
> >> >> a full fork() either, clone() lets you do this with some process parts
> >> >> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> >> >> the file descriptor table shared. And why chroot()/pivot_root(),
> >> >> wouldn't you want to use setns()?
> >> >
> >> > You're right about this -- for C runtimes. In Go we cannot do a raw
> >> > clone() or fork() (if you do it manually with RawSyscall you'll end with
> >> > broken runtime state). So you're forced to do fork+exec (which then
> >> > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> >> > for CLONE_VFORK.
> >>
> >> I must admit that I’m not very sympathetic to the argument that “Go’s
> >> runtime model is incompatible with the simpler solution.”
> >
> > Multi-threaded programs have a similar issue (though with Go it's much
> > worse). If you fork a multi-threaded C program then you can only safely
> > use AS-Safe glibc functions (those that are safe within a signal
> > handler). But if you're just doing three syscalls this shouldn't be as
> > big of a problem as Go where you can't even do said syscalls.
>
> The situation is a bit more complicated. There are many programs out
> there which use malloc and free (at least indirectly) after a fork,
> and we cannot break them. In glibc, we have a couple of subsystems
> which are put into a known state before calling the fork/clone system
> call if the application calls fork. The price we pay for that is a
> fork which is not POSIX-compliant because it is not async-signal-safe.
> Admittedly, other libcs chose different trade-offs.
>
> However, what is the same across libcs is this: You cannot call the
> clone system call directly and get a fully working new process. Some
> things break. For example, for recursive mutexes, we need to know the
> TID of the current thread, and we cannot perform a system call to get
> it for performance reasons. So everyone has a TID cache for that.
> But the TID cache does not get reset when you bypass the fork
> implementation in libc, so you end up with subtle corruption bugs on
> TID reuse.
Sure, but recursive mutexes etc. are very specific use-case.
I'd even go so far to say that if you use mutexes + threads and then
also fork in those threads you're hosed anyway. If you don't things get a little
cleaner assuming you don't call library functions that use mutexes
internally.
Event then you might (sometimes at least) still get around most problems
with atfork handlers (thought I really don't like him). But you know more
about this then I do. :)
>
> So I'd say that in most cases, the C situation is pretty much the same
> as the Go situation. If I recall correctly, the problem for Go is
> that it cannot call setns from Go code because it fails in the kernel
> for multi-threaded processes, and Go processes are already
> multi-threaded when user Go code runs.
That is true for *some* namespaces (user, mount) but not for all.
For example, setns(CLONE_NEWNET) would be fine from go.
But the go runtime thinks it's clever to clone a new thread in between
entry and exit of a syscall. If you switch namespaces you might end
up with a new thread that belongs to the wrong namespace which is
very problematic.
So you can either rely on calling some go magic that locks
you to a specific os thread but that does only work in later go versions or
you go the constructor route, i.e. you e.g. implement a (dummy)
subcommand that you can call and that triggers the execution of a
C function that is marked with __attribute__((constructor)) that runs
before the go runtime and in which you can do setns(), fork() and
friends (somewhat) safely. This has very
bad performance and is a nasty hack but it's really unavoidable.
^ permalink raw reply
* Re: [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Serge E. Hallyn @ 2018-10-07 5:02 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Andrei Vagin, linux-fsdevel, Eric Biederman,
linux-api, Dmitry Safonov, containers, Alexander Viro, Jann Horn,
James Bottomley
In-Reply-To: <20181006193546.29340-2-laurent@vivier.eu>
On Sat, Oct 06, 2018 at 09:35:46PM +0200, Laurent Vivier wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Hi,
quick question below,
> ---
> fs/binfmt_misc.c | 99 ++++++++++++++++++++++++----------
> include/linux/user_namespace.h | 13 +++++
> kernel/user.c | 13 +++++
> kernel/user_namespace.c | 7 +++
> 4 files changed, 104 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..1beefafcb416 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -38,9 +38,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +57,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -80,18 +74,28 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + while (ns) {
> + if (ns->binfmt_ns)
> + return ns->binfmt_ns;
> + ns = ns->parent;
> + }
> + return NULL;
> +}
> +
> /*
> * Check if we support the binfmt
> * if we do, return the node, else NULL
> * locking is done in load_misc_binary
> */
> -static Node *check_file(struct linux_binprm *bprm)
> +static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
> {
> char *p = strrchr(bprm->interp, '.');
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &ns->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +137,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->enabled)
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> - fmt = check_file(bprm);
> + read_lock(&ns->entries_lock);
> + fmt = check_file(ns, bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +614,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct binfmt_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&ns->entries_lock);
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt, &ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +656,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> struct dentry *root;
> Node *e = file_inode(file)->i_private;
> int res = parse_command(buffer, count);
> + struct binfmt_namespace *ns;
> +
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
>
> switch (res) {
> case 1:
> @@ -667,7 +675,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> inode_lock(d_inode(root));
>
> if (!list_empty(&e->list))
> - kill_node(e);
> + kill_node(ns, e);
>
> inode_unlock(d_inode(root));
> break;
> @@ -693,6 +701,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> struct inode *inode;
> struct super_block *sb = file_inode(file)->i_sb;
> struct dentry *root = sb->s_root, *dentry;
> + struct binfmt_namespace *ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +725,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +736,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>
> if (e->flags & MISC_FMT_OPEN_FILE) {
> struct file *f;
> + const struct cred *old_cred;
>
> + old_cred = override_creds(file->f_cred);
What exactly is this aiming to do?
> f = open_exec(e->interpreter);
> + revert_creds(old_cred);
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt,
> + &ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +758,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&ns->entries_lock);
> + list_add(&e->list, &ns->entries);
> + write_unlock(&ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +785,9 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + struct binfmt_namespace *ns =
> + binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + char *s = ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +795,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> + struct binfmt_namespace *ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + ns->enabled = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&ns->entries))
> + kill_node(ns, list_first_entry(&ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -838,7 +858,29 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (ns->binfmt_ns == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return ERR_PTR(-ENOMEM);
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + ns->binfmt_ns = new_ns;
> + }
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +891,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d6b74b91096b..5c6e7e63b97e 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -52,6 +52,16 @@ enum ucount_type {
> UCOUNT_COUNTS,
> };
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace {
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> +} __randomize_layout;
> +#endif
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -76,6 +86,9 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> int ucount_max[UCOUNT_COUNTS];
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + struct binfmt_namespace *binfmt_ns;
> +#endif
> } __randomize_layout;
>
> struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index 0df9b1640b2a..912916d435aa 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -19,6 +19,16 @@
> #include <linux/user_namespace.h>
> #include <linux/proc_ns.h>
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +static struct binfmt_namespace init_binfmt_ns = {
> + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> + .enabled = 1,
> + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> + .bm_mnt = NULL,
> + .entry_count = 0,
> +};
> +#endif
> +
> /*
> * userns count is 1 for root user, 1 for init_uts_ns,
> * and 1 for... ?
> @@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> #endif
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + .binfmt_ns = &init_binfmt_ns,
> +#endif
> };
> EXPORT_SYMBOL_GPL(init_user_ns);
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..da4950282ea1 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
> if (!setup_userns_sysctls(ns))
> goto fail_keyring;
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + ns->binfmt_ns = NULL;
> +#endif
> +
> set_cred_user_ns(new, ns);
> return 0;
> fail_keyring:
> @@ -195,6 +199,9 @@ static void free_user_ns(struct work_struct *work)
> kfree(ns->projid_map.forward);
> kfree(ns->projid_map.reverse);
> }
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + kfree(ns->binfmt_ns);
> +#endif
> retire_userns_sysctls(ns);
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> key_put(ns->persistent_keyring_register);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-07 9:11 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: linux-kernel, Andrei Vagin, linux-fsdevel, Eric Biederman,
linux-api, Dmitry Safonov, containers, Alexander Viro, Jann Horn,
James Bottomley
In-Reply-To: <20181007050224.GA3035@mail.hallyn.com>
Le 07/10/2018 à 07:02, Serge E. Hallyn a écrit :
> On Sat, Oct 06, 2018 at 09:35:46PM +0200, Laurent Vivier wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of an another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>
> Hi,
>
> quick question below,
>
>> ---
>> fs/binfmt_misc.c | 99 ++++++++++++++++++++++++----------
>> include/linux/user_namespace.h | 13 +++++
>> kernel/user.c | 13 +++++
>> kernel/user_namespace.c | 7 +++
>> 4 files changed, 104 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
>> index aa4a7a23ff99..1beefafcb416 100644
>> --- a/fs/binfmt_misc.c
...
>> @@ -725,12 +736,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>>
>> if (e->flags & MISC_FMT_OPEN_FILE) {
>> struct file *f;
>> + const struct cred *old_cred;
>>
>> + old_cred = override_creds(file->f_cred);
>
> What exactly is this aiming to do?
See comment from the version 1:
https://lkml.org/lkml/2018/10/1/377
"This looks wrong. A write handler's behavior should not depend on the
namespace of the process that is using it.
Ideally, the affected namespace should depend on the file you're writing to.
If that's not possible, the affected namespace should at least be the
namespace of the process that opened the file." -- Jann Horn
And from version 2:
https://lkml.org/lkml/2018/10/3/872
"Something else: bm_register_write() currently calls into open_exec(),
which uses the credentials of current. That's not really allowed in
this context - but so far, it's not a big deal because only
init-namespace root can reach this code. Before you expose this stuff
to unprivileged userspace, this needs to get fixed; perhaps by
wrapping the open_exec() call in override_creds(file->f_cred) and
revert_creds()." -- Jann Horn
Thanks,
Laurent
^ permalink raw reply
* Re: [RFC v4 1/1] ns: add binfmt_misc to the user namespace
From: Andrei Vagin @ 2018-10-08 1:11 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, linux-fsdevel, Eric Biederman, linux-api,
Dmitry Safonov, containers, Alexander Viro, Jann Horn,
James Bottomley
In-Reply-To: <20181006193546.29340-2-laurent@vivier.eu>
On Sat, Oct 06, 2018 at 09:35:46PM +0200, Laurent Vivier wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> fs/binfmt_misc.c | 99 ++++++++++++++++++++++++----------
> include/linux/user_namespace.h | 13 +++++
> kernel/user.c | 13 +++++
> kernel/user_namespace.c | 7 +++
> 4 files changed, 104 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..1beefafcb416 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -38,9 +38,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +57,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -80,18 +74,28 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + while (ns) {
> + if (ns->binfmt_ns)
> + return ns->binfmt_ns;
> + ns = ns->parent;
> + }
> + return NULL;
> +}
> +
> /*
> * Check if we support the binfmt
> * if we do, return the node, else NULL
> * locking is done in load_misc_binary
> */
> -static Node *check_file(struct linux_binprm *bprm)
> +static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
> {
> char *p = strrchr(bprm->interp, '.');
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &ns->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +137,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->enabled)
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> - fmt = check_file(bprm);
> + read_lock(&ns->entries_lock);
> + fmt = check_file(ns, bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +614,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct binfmt_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&ns->entries_lock);
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt, &ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +656,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> struct dentry *root;
> Node *e = file_inode(file)->i_private;
> int res = parse_command(buffer, count);
> + struct binfmt_namespace *ns;
> +
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
>
> switch (res) {
> case 1:
> @@ -667,7 +675,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> inode_lock(d_inode(root));
>
> if (!list_empty(&e->list))
> - kill_node(e);
> + kill_node(ns, e);
>
> inode_unlock(d_inode(root));
> break;
> @@ -693,6 +701,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> struct inode *inode;
> struct super_block *sb = file_inode(file)->i_sb;
> struct dentry *root = sb->s_root, *dentry;
> + struct binfmt_namespace *ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +725,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +736,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>
> if (e->flags & MISC_FMT_OPEN_FILE) {
> struct file *f;
> + const struct cred *old_cred;
>
> + old_cred = override_creds(file->f_cred);
> f = open_exec(e->interpreter);
> + revert_creds(old_cred);
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt,
> + &ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +758,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&ns->entries_lock);
> + list_add(&e->list, &ns->entries);
> + write_unlock(&ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +785,9 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + struct binfmt_namespace *ns =
> + binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + char *s = ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +795,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> + struct binfmt_namespace *ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + ns->enabled = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&ns->entries))
> + kill_node(ns, list_first_entry(&ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -838,7 +858,29 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (ns->binfmt_ns == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return ERR_PTR(-ENOMEM);
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
I think we need a memory barrier here to be sure that new_ns is
comletely initialized before someone will be able to see it.
smp_wmb();
> + ns->binfmt_ns = new_ns;
> + }
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +891,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d6b74b91096b..5c6e7e63b97e 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -52,6 +52,16 @@ enum ucount_type {
> UCOUNT_COUNTS,
> };
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace {
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> +} __randomize_layout;
> +#endif
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -76,6 +86,9 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> int ucount_max[UCOUNT_COUNTS];
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + struct binfmt_namespace *binfmt_ns;
> +#endif
> } __randomize_layout;
>
> struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index 0df9b1640b2a..912916d435aa 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -19,6 +19,16 @@
> #include <linux/user_namespace.h>
> #include <linux/proc_ns.h>
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +static struct binfmt_namespace init_binfmt_ns = {
> + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> + .enabled = 1,
> + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> + .bm_mnt = NULL,
> + .entry_count = 0,
> +};
> +#endif
> +
> /*
> * userns count is 1 for root user, 1 for init_uts_ns,
> * and 1 for... ?
> @@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> #endif
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + .binfmt_ns = &init_binfmt_ns,
> +#endif
> };
> EXPORT_SYMBOL_GPL(init_user_ns);
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..da4950282ea1 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
> if (!setup_userns_sysctls(ns))
> goto fail_keyring;
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + ns->binfmt_ns = NULL;
> +#endif
> +
> set_cred_user_ns(new, ns);
> return 0;
> fail_keyring:
> @@ -195,6 +199,9 @@ static void free_user_ns(struct work_struct *work)
> kfree(ns->projid_map.forward);
> kfree(ns->projid_map.reverse);
> }
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + kfree(ns->binfmt_ns);
> +#endif
> retire_userns_sysctls(ns);
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> key_put(ns->persistent_keyring_register);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf-next 1/6] bpf: introduce BPF_PROG_TYPE_FILE_FILTER
From: Mickaël Salaün @ 2018-10-08 9:06 UTC (permalink / raw)
To: Alexei Starovoitov, Jann Horn
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Andy Lutomirski, Al Viro, Network Development, kernel list,
kernel-team, Kernel Hardening, linux-security-module, Linux API
In-Reply-To: <20181008022210.33ljgryhodzunf5l@ast-mbp>
[-- Attachment #1.1: Type: text/plain, Size: 9383 bytes --]
On 10/8/18 04:22, Alexei Starovoitov wrote:
> On Mon, Oct 08, 2018 at 02:56:15AM +0200, Jann Horn wrote:
>> +cc kernel-hardening because this is related to sandboxing
>> +cc Mickaël Salaün because this looks related to his Landlock proposal
Thanks for the CC. Because this patch series is an access-control
mechanism, it will undoubtedly interest LSM folks as well.
Here is the full thread:
https://lore.kernel.org/lkml/20181004025750.498303-1-ast@kernel.org/
>
> It may seem that this work overlaps with landlock, but the goals
> are different. Landlock is LSM based to act as _security_ framework
> with end goal being available to unpriv users.
Right, one of the end goal of Landlock is to make most of its
access-control features available to unprivileged processes. However,
some of them may require higher privileges, but we should minimize them
as much as possible to not end up with the SUID-binary (or
CAP_SYS_ADMIN) syndrome.
> While this cgroup-bpf hook is expected to stay root only,
> since we're trying to restrict what containers can do in a trusted environment.
> 'sandboxing' is overloaded word.
> Sandboxing for security and sandboxing of trusted root are different.
Sure we must treat them in a different manner but we must take care of
which capability this sandboxing mechanism really need, and hopefully
not root-like ones, especially for a container.
As a reminder, using cgroups to sandbox processes is definitely a use
case for Landlock, as well as the process hierarchy one (seccomp-like).
I implemented sandboxing with cgroups in a previous patch series, but
put it aside for now to minimize the number of patches.
>
>> On Mon, Oct 8, 2018 at 2:30 AM Alexei Starovoitov <ast@kernel.org> wrote:
>>> Similar to networking sandboxing programs and cgroup-v2 based hooks
>>> (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc)
>>> introduce basic per-container sandboxing for file access via
>>> new BPF_PROG_TYPE_FILE_FILTER program type that attaches after
>>> security_file_open() LSM hook and works as additional file_open filter.
>>> The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.
>>
>> Why do_dentry_open() specifically, and nothing else? If you want to
>> filter open, wouldn't you also want to filter a bunch of other
>> filesystem operations with LSM hooks, like rename, unlink, truncate
>> and so on? Landlock benefits there from re-using the existing security
>> hooks.
>
> It may make sense to extend in the future, but we don't have clear
> user cases for rename/unlink/truncate at this point.
I guess the users who ask for the current features will come with new
ones, which will move towards the same goal as Landlock.
>
> As you can see the amount of pushback even for basic file access is high.
> Hence I don't think the landlock can be upstreamed in the current form,
> since it touches VFS layer a lot more than this patch.
That is not true for the main part. The only patch from Landlock
touching the FS subsystem is to add a new LSM hook. This was NAKed by Al
but I guess mostly because of misunderstanding. I'll send a new specific
patch to the FS subsystem soon to properly explain the impact on path
lookup and why it doesn't expose more that userspace have already access.
Regarding the upstreaming concern, if I remember correctly, the net/BPF
and the LSM maintainers are (mostly ?) OK. The feedback I got (you
included) was good.
I can definitely create a lighter version of Landlock with your use
cases in mind. There is already all the necessary building blocks: the
BPF (un-)privileged handling, the file checks (see the FS_PICK subtype)
and the cgroups handling. I may even be easier to upstream (and answer
Casey's request) to make the patch series smaller.
> It's intrusive to LSM, and adds new concepts to BPF as well.
> This work fits into existing BPF machinery and minimally intrusive to VFS.
> I hope we can find a common ground with Al regarding what file
> access primitives are exposed to BPF side.
> Once we agree on that the landlock can piggy back on this work
> and extend it to all file-based LSM hooks.
Can you explain your thought about piggy backing? If your patch is
included, Landlock will still need to add new BPF prog types, and will
still be an LSM (which is designed to handle access-control).
> The first step for everyone interested in bpf-based 'sandboxing'
> is to figure out VFS<->BPF interface.
> If you or Mickael have suggestions on what bpf progs should and should not
> see at these hooks, it's a good time to discuss.
> I believe the fields proposed are the obvious minimum.
In a nutshell, Landlock rely on an inode map, which contains references
to inodes. These references/handles can then be used to "compare" with a
requested inode. This way, we can limit the checks to what is already
available to the process which loaded the map. I suggest to not use
these hardcoded fields but use a BFP helper with a file handle as
argument and dedicated flags, as I did in a previous version of Landlock
(which I planned to upstream as a new future feature).
>
>>> Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type
>>> is only available to root.
>>>
>>> This program type has access to single argument 'struct bpf_file_info'
>>> that contains standard sys_stat fields:
>>> struct bpf_file_info {
>>> __u64 inode;
>>> __u32 dev_major;
>>> __u32 dev_minor;
>>> __u32 fs_magic;
>>> __u32 mnt_id;
>>> __u32 nlink;
>>> __u32 mode; /* file mode S_ISDIR, S_ISLNK, 0755, etc */
>>> __u32 flags; /* open flags O_RDWR, O_CREAT, etc */
>>> };
>>> Other file attributes can be added in the future to the end of this struct
>>> without breaking bpf programs.
>>>
>>> For debugging introduce bpf_get_file_path() helper that returns
>>> NUL-terminated full path of the file. It should never be used for sandboxing.
Well, I'm convinced it *will* be used for sandboxing. Once a feature is
available, it is too late to forbid users to use it the way they want,
even if you warn them about security issues…
>>>
>>> Use cases:
>>> - disallow certain FS types within containers (fs_magic == CGROUP2_SUPER_MAGIC)
>>> - restrict permissions in particular mount (mnt_id == X && (flags & O_RDWR))
>>> - disallow access to hard linked sensitive files (nlink > 1 && mode == 0700)
>>> - disallow access to world writeable files (mode == 0..7)
>>> - disallow access to given set of files (dev_major == X && dev_minor == Y && inode == Z)
These use cases are interesting and should help to harden containers.
They are definitely in the scope of what Landlock can do.
However, I would like to see which capabilities are needed to access
these properties.
>>
>> That last one sounds weird. It doesn't work if you want to ban access
>> to a whole directory at once. And in general, highly specific
>> blocklists make me nervous, because if you add anything new and forget
>> to put it on the list, you have a problem.
>
> In the upcoming V2 of the patches the direct exposure to dev and inode will be removed.
> And instead the opaque 'struct file_handle' will be available to bpf progs.
> The use case is indeed to restrict access to specific blacklist of files.
> The user space will collect the set of files via sys_name_to_handle_at(),
> then store the fhandles in bpf map, and bpf prog will consult the map
> to deny the access.
> It's not a replacement for ACLs, directory permissions, etc. The use case
> is to prevent trusted containers messing up the environment.
> The continuous integration system needs to run some containers (and tests inside them)
> with root privs. When these jobs mess up the system the subsequent jobs may incorrectly
> fail. We believe that this cgroup based container enforcement will solve this use case
> and similar other use cases when containers are trusted, but could be buggy when
> it comes to file access.
>
> To recap what I'm implementing in V2:
> 1.
> struct bpf_file_info {
> __u32 fs_magic; // file->f_inode->i_sb->s_magic
> __u32 mnt_id; // real_mount(file->f_path.mnt)->mnt_id
> __u32 nlink; // file->f_inode->i_nlink
> __u32 mode; // file->f_inode->i_mode
> __u32 flags; // file->f_flags
> };
> I double checked what VFS layer does with above fields and I think
> there is no additional user space exposure will be made when such
> fields are seen by bpf progs.
> But since I'm not a VFS expert, I'd like Al to confirm.
>
> 2.
> bpf_get_file_handle(struct bpf_file_info *ctx, struct file_handle *fh, int fh_size);
> helper that bpf prog will use to obtain fh of the file about to be open.
This looks like Landlock. :)
>
> 3.
> bpf_get_file_statx(struct bpf_file_info *ctx, struct statx *sx, int size, int flags);
> Though struct statx is 256 bytes, and the helper would have to touch all bytes
> I couldn't figure out the faster way to get to inode/dev/uid of the given file
> that will work on all underlying FSes.
This looks like a previous helper I implemented, but tried to avoid
because of security issues (mostly for unpriv processes).
>
> Thoughts?
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-08 9:48 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jann Horn, Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181005210732.ft77r5qqcoqpiwu7@ast-mbp.dhcp.thefacebook.com>
On Fri, 5 Oct 2018 at 22:07, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Oct 05, 2018 at 04:27:58PM +0200, Jann Horn wrote:
> >
> > Can you please describe exactly why something that is not a kernel
> > unit test needs deterministic BPF hash map behavior?
>
> my use case for deterministic hashing is performance analysis.
> Both while developing and tuning bpf program and while optimizing
> kernel side implementation.
> Local dos is a valid concern, so requiring root for this flag makes sense.
>
Ok, I'll respin and address the comments.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* [PATCH v2 0/3] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-10-08 10:32 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
Allow forcing the seed of a hash table to zero, for deterministic
execution during benchmarking and testing.
Comments adressed from v1:
* Add comment to discourage production use to linux/bpf.h
* Require CAP_SYS_ADMIN
Lorenz Bauer (3):
bpf: allow zero-initializing hash map seed
tools: sync linux/bpf.h
tools: add selftest for BPF_F_ZERO_SEED
include/uapi/linux/bpf.h | 2 +
kernel/bpf/hashtab.c | 13 ++++-
tools/include/uapi/linux/bpf.h | 2 +
tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
4 files changed, 72 insertions(+), 13 deletions(-)
--
2.17.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox