* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 8:32 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, David Howells, Eric Biederman, stable, Christian Brauner,
Serge Hallyn, dev, Linux Containers, Linux API, linux-fsdevel,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=whxNw7hYT6bJn9mVrB_a=7Y-irmpaPsp1R4xbHHkicv7g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]
On 2019-12-29, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sun, Dec 29, 2019 at 11:30 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > BUG: kernel NULL pointer dereference, address: 0000000000000000
>
> Would you mind building with debug info, and then running the oops through
>
> scripts/decode_stacktrace.sh
>
> which makes those addresses much more legible.
Will do.
> > #PF: supervisor instruction fetch in kernel mode
> > #PF: error_code(0x0010) - not-present page
>
> Somebody jumped through a NULL pointer.
>
> > RAX: 0000000000000000 RBX: ffff906d0cc3bb40 RCX: 0000000000000abc
> > RDX: 0000000000000089 RSI: ffff906d74623cc0 RDI: ffff906d74475df0
> > RBP: ffff906d74475df0 R08: ffffd70b7fb24c20 R09: ffff906d066a5000
> > R10: 0000000000000000 R11: 8080807fffffffff R12: ffff906d74623cc0
> > R13: 0000000000000089 R14: ffffb70b82963dc0 R15: 0000000000000080
> > FS: 00007fbc2a8f0540(0000) GS:ffff906dcf500000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: ffffffffffffffd6 CR3: 00000003c68f8001 CR4: 00000000003606e0
> > Call Trace:
> > __lookup_slow+0x94/0x160
>
> And "__lookup_slow()" has two indirect calls (they aren't obvious with
> retpoline, but look for something like
>
> call __x86_indirect_thunk_rax
>
> which is the modern sad way of doing "call *%rax"). One is for
> revalidatinging an old dentry, but the one I _suspect_ you trigger is
> this one:
>
> old = inode->i_op->lookup(inode, dentry, flags);
>
> but I thought we only could get here if we know it's a directory.
>
> How did we miss the "d_can_lookup()", which is what should check that
> yes, we can call that ->lookup() routine.
I'll try applying a trivial patch to add d_can_lookup() to see if it
fixes the immediate issue.
> This is why I have that suspicion that it's somehow that O_PATH fd
> opened in another process without O_PATH causes confusion...
>
> So what I think has happened is that because of the O_PATH thing,
> we've ended up with an inode that has never been truly opened (because
> O_PATH skips that part), but then with the /proc/<pid>/fd/xyz open, we
> now have a file descriptor that _looks_ like it is valid, and we're
> treating that inode as if it can be used.
I'm not sure I agree -- as I mentioned in my other mail, re-opening
through /proc/self/fd/$n works *very* well and has for a long time (in
fact, both LXC and runc depend on this working).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 1/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 8:28 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, David Howells, Eric Biederman, stable, Christian Brauner,
Serge Hallyn, dev, Linux Containers, Linux API, linux-fsdevel,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wjHPCQsMeK5bFOJQnrGPfVDXTAFQK4VsBZPj5u=ZgS-QA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5028 bytes --]
On 2019-12-29, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sun, Dec 29, 2019 at 9:21 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > + if (d_is_symlink(mp->m_dentry) ||
> > + d_is_symlink(mnt->mnt.mnt_root))
> > + return -EINVAL;
>
> So I don't hate this kind of check in general - overmounting a symlink
> sounds odd, but at the same time I get the feeling that the real issue
> is that something went wrong earlier.
>
> Yeah, the mount target kind of _is_ a path, but at the same time, we
> most definitely want to have the permission to really open the
> directory in question, don't we, and I don't see that we should accept
> a O_PATH file descriptor.
The new mount API uses O_PATH under the hood (which is a good thing
since some files you'd like to avoid actually opening -- FIFOs are the
obvious example) so I'm not sure that's something we could really avoid.
But if we block O_PATH for mounts this will achieve the same thing,
because the only way to get a file descriptor that references a symlink
is through (O_PATH | O_NOFOLLOW).
> I feel like the only valid use of "O_PATH" files is to then use them
> as the base for an openat() and friends (ie fchmodat/execveat() etc).
See below, we use this for all sorts of dirty^Wclever tricks.
> But maybe I'm completely wrong, and people really do want O_PATH
> handling exactly for mounting too. It does sound a bit odd. By
> definition, mounting wants permissions to the mount-point, so what's
> the point of using O_PATH?
When you go through O_PATH, you still get a proper 'struct path' which
means that for operations such as mount (or open) you will operate on
the *real* underlying file.
This is part of what makes magic-links so useful (but also quite
terrifying).
> For example, is the problem that when you do a proper
>
> fd = open("somepath", O_PATH);
>
> in one process, and then another thread does
>
> fd = open("/proc/<pid>/fd/<opathfd>", O_RDWR);
>
> then we get confused and do bad things on that *second* open? Because
> now the second open doesn't have O_PATH, and doesn't ghet marked
> FMODE_PATH, but the underlying file descriptor is one of those limited
> "is really only useful for openat() and friends".
Actually, this isn't true (for the same reason as above) -- when you do
a re-open through /proc/$pid/fd/$n you get a real-as-a-heart-attack file
descriptor. We make lots of use of this in container runtimes in order
to do some dirty^Wfun tricks that help us harden the runtime against
malicious container processes.
You might recall that when I was posting the earlier revisions of
openat2(), I also included a patch for O_EMPTYPATH (which basically did
a re-open of /proc/self/fd/$dfd but without needing /proc). That had
precisely the same semantics so that you could do the same operation
without procfs. That patch was dropped before Al merged openat2(), but I
am probably going to revive it for the reasons I outlined below.
> I dunno. I haven't thought through the whole thing. But the oopses you
> quote seem like we're really doing something wrong, and it really does
> feel like your patch in no way _fixes_ the wrong thing we're doing,
> it's just hiding the symptoms.
That's fair enough.
I'll be honest, the real reason why I don't want mounts over symlinks to
be possible is for an entirely different reason. I'm working on a safe
path resolution library to accompany openat2()[1] -- and one of the
things I want to do is to harden all of our uses of procfs (such that if
we are running in a context where procfs has been messed with -- such as
having files bind-mounted -- we can detect it and abort). The issue with
symlinks is that we need to be able to operate on magic-links (such as
/proc/self/fd/$n and /proc/self/exe) -- and if it's possible bind-mount
over those magic-links then we can't detect it at all.
openat2(RESOLVE_NO_XDEV) would block it, but it also blocks going
through magic-links which change your mount (which would almost always
be true). You can't trust /proc/self/mountinfo by definition -- not just
because of the TOCTOU race but also because you can't depend on /proc to
harden against a "bad" /proc. All other options such as
umount2(MNT_EXPIRE) won't help with magic-links because we cannot take
an O_PATH to a magic-link and follow it -- O_PATHs of symlinks are
completely stunted in this respect.
If allowing bind-mounts over symlinks is allowed (which I don't have a
problem with really), it just means we'll need a few more kernel pieces
to get this hardening to work. But these features would be useful
outside of the problems I'm dealing with (O_EMPTYPATH and some kind of
pidfd-based interface to grab the equivalent of /proc/self/exe and a few
other such magic-link targets).
[1]: https://github.com/openSUSE/libpathrs
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Linus Torvalds @ 2019-12-30 7:53 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, David Howells, Eric Biederman, stable, Christian Brauner,
Serge Hallyn, dev, Linux Containers, Linux API, linux-fsdevel,
Linux Kernel Mailing List
In-Reply-To: <20191230072959.62kcojxpthhdwmfa@yavin.dot.cyphar.com>
On Sun, Dec 29, 2019 at 11:30 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
Would you mind building with debug info, and then running the oops through
scripts/decode_stacktrace.sh
which makes those addresses much more legible.
> #PF: supervisor instruction fetch in kernel mode
> #PF: error_code(0x0010) - not-present page
Somebody jumped through a NULL pointer.
> RAX: 0000000000000000 RBX: ffff906d0cc3bb40 RCX: 0000000000000abc
> RDX: 0000000000000089 RSI: ffff906d74623cc0 RDI: ffff906d74475df0
> RBP: ffff906d74475df0 R08: ffffd70b7fb24c20 R09: ffff906d066a5000
> R10: 0000000000000000 R11: 8080807fffffffff R12: ffff906d74623cc0
> R13: 0000000000000089 R14: ffffb70b82963dc0 R15: 0000000000000080
> FS: 00007fbc2a8f0540(0000) GS:ffff906dcf500000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffffffffffd6 CR3: 00000003c68f8001 CR4: 00000000003606e0
> Call Trace:
> __lookup_slow+0x94/0x160
And "__lookup_slow()" has two indirect calls (they aren't obvious with
retpoline, but look for something like
call __x86_indirect_thunk_rax
which is the modern sad way of doing "call *%rax"). One is for
revalidatinging an old dentry, but the one I _suspect_ you trigger is
this one:
old = inode->i_op->lookup(inode, dentry, flags);
but I thought we only could get here if we know it's a directory.
How did we miss the "d_can_lookup()", which is what should check that
yes, we can call that ->lookup() routine.
This is why I have that suspicion that it's somehow that O_PATH fd
opened in another process without O_PATH causes confusion...
So what I think has happened is that because of the O_PATH thing,
we've ended up with an inode that has never been truly opened (because
O_PATH skips that part), but then with the /proc/<pid>/fd/xyz open, we
now have a file descriptor that _looks_ like it is valid, and we're
treating that inode as if it can be used.
But I'm handwaving.
Linus
^ permalink raw reply
* Re: [PATCH RFC 1/1] mount: universally disallow mounting over symlinks
From: Linus Torvalds @ 2019-12-30 7:34 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, David Howells, Eric Biederman, stable, Christian Brauner,
Serge Hallyn, dev, Linux Containers, Linux API, linux-fsdevel,
Linux Kernel Mailing List
In-Reply-To: <20191230052036.8765-2-cyphar@cyphar.com>
On Sun, Dec 29, 2019 at 9:21 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> + if (d_is_symlink(mp->m_dentry) ||
> + d_is_symlink(mnt->mnt.mnt_root))
> + return -EINVAL;
So I don't hate this kind of check in general - overmounting a symlink
sounds odd, but at the same time I get the feeling that the real issue
is that something went wrong earlier.
Yeah, the mount target kind of _is_ a path, but at the same time, we
most definitely want to have the permission to really open the
directory in question, don't we, and I don't see that we should accept
a O_PATH file descriptor.
I feel like the only valid use of "O_PATH" files is to then use them
as the base for an openat() and friends (ie fchmodat/execveat() etc).
But maybe I'm completely wrong, and people really do want O_PATH
handling exactly for mounting too. It does sound a bit odd. By
definition, mounting wants permissions to the mount-point, so what's
the point of using O_PATH?
So instead of saying "don't overmount symlinks", I would feel like
it's the mount system call that should use a proper file descriptor
that isn't FMODE_PATH.
Is it really the symlink that is the issue? Because if it's the
symlink that is the issue then I feel like O_NOFOLLOW should have
triggered it, but your other email seems to say that you really need
O_PATH | O_SYMLINK.
So I'm not sayng that this patch is wrong, but it really smells a bit
like it's papering over the more fundamental issue.
For example, is the problem that when you do a proper
fd = open("somepath", O_PATH);
in one process, and then another thread does
fd = open("/proc/<pid>/fd/<opathfd>", O_RDWR);
then we get confused and do bad things on that *second* open? Because
now the second open doesn't have O_PATH, and doesn't ghet marked
FMODE_PATH, but the underlying file descriptor is one of those limited
"is really only useful for openat() and friends".
I dunno. I haven't thought through the whole thing. But the oopses you
quote seem like we're really doing something wrong, and it really does
feel like your patch in no way _fixes_ the wrong thing we're doing,
it's just hiding the symptoms.
Linus
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 7:29 UTC (permalink / raw)
To: Al Viro
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20191230054913.c5avdjqbygtur2l7@yavin.dot.cyphar.com>
[-- Attachment #1.1: Type: text/plain, Size: 4818 bytes --]
On 2019-12-30, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-12-30, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:
> >
> > > A reasonably detailed explanation of the issues is provided in the patch
> > > itself, but the full traces produced by both the oopses and deadlocks is
> > > included below (it makes little sense to include them in the commit since we
> > > are disabling this feature, not directly fixing the bugs themselves).
> > >
> > > I've posted this as an RFC on whether this feature should be allowed at
> > > all (and if anyone knows of legitimate uses for it), or if we should
> > > work on fixing these other kernel bugs that it exposes.
> >
> > Umm... Are all of those traces
> > a) reproducible on mainline and
>
> This was on viro/for-next, I'll retry it on v5.5-rc4.
The NULL deref oops is reproducible on v5.5-rc4. Strangely it seems
harder to reproduce than on viro/for-next (I kept reproducing it there
by accident), but I'll double-check if that really is the case.
The simplest reproducer is (using the attached programs and .config):
ln -s . link
sudo ./umount_symlink link
There's also a few other whacky behaviours where you get -ELOOP or
-EACCES in cases where you shouldn't -- which results in MNT_DETACH
failing and the mount being impossible to get rid of. A good example is
sudo ./mount_to_symlink /proc/self/exe link
sudo ./umount_symlink link # -EACCES
Or
ln -s . link1
ln -s . link2
sudo ./mount_to_symlink link1 link2
sudo ./umount_symlink link1 # -ELOOP
sudo ./umount_symlink link2 # -ELOOP
But I am trying to find a reproducer for the "umount of a mount
triggering an Oops" issue.
On another note -- I guess this is considered a feature which should
"just work" and not a bug?
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
PGD 80000003c6fca067 P4D 80000003c6fca067 PUD 3c6f42067 PMD 0
Oops: 0010 [#1] SMP PTI
CPU: 4 PID: 4486 Comm: umount_symlink Tainted: G E 5.5.0-rc4-cyphar #126
Hardware name: LENOVO 20KHCTO1WW/20KHCTO1WW, BIOS N23ET55W (1.30 ) 08/31/2018
RIP: 0010:0x0
Code: Bad RIP value.
RSP: 0018:ffffb70b82963cc0 EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffff906d0cc3bb40 RCX: 0000000000000abc
RDX: 0000000000000089 RSI: ffff906d74623cc0 RDI: ffff906d74475df0
RBP: ffff906d74475df0 R08: ffffd70b7fb24c20 R09: ffff906d066a5000
R10: 0000000000000000 R11: 8080807fffffffff R12: ffff906d74623cc0
R13: 0000000000000089 R14: ffffb70b82963dc0 R15: 0000000000000080
FS: 00007fbc2a8f0540(0000) GS:ffff906dcf500000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 00000003c68f8001 CR4: 00000000003606e0
Call Trace:
__lookup_slow+0x94/0x160
lookup_slow+0x36/0x50
path_mountpoint+0x1be/0x360
filename_mountpoint+0xa5/0x150
? __lookup_hash+0xa0/0xa0
ksys_umount+0x78/0x490
__x64_sys_umount+0x12/0x20
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7fbc2a8274e7
Code: 09 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09
00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0
ff ff 73 01 c3 48 8b 0d 69 09 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd1da9b3f8 EFLAGS: 00000202 ORIG_RAX: 00000000000000a6
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbc2a8274e7
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000001300310
RBP: 00007ffd1da9b4c0 R08: 0000000000000000 R09: 000000000000000f
R10: 00007fbc2a92f800 R11: 0000000000000202 R12: 0000000000401090
R13: 00007ffd1da9b5a0 R14: 0000000000000000 R15: 0000000000000000
Modules linked in: [snip]
CR2: 0000000000000000
---[ end trace ae473813e34e641d ]---
RIP: 0010:0x0
Code: Bad RIP value.
RSP: 0018:ffffb70b82963cc0 EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffff906d0cc3bb40 RCX: 0000000000000abc
RDX: 0000000000000089 RSI: ffff906d74623cc0 RDI: ffff906d74475df0
RBP: ffff906d74475df0 R08: ffffd70b7fb24c20 R09: ffff906d066a5000
R10: 0000000000000000 R11: 8080807fffffffff R12: ffff906d74623cc0
R13: 0000000000000089 R14: ffffb70b82963dc0 R15: 0000000000000080
FS: 00007fbc2a8f0540(0000) GS:ffff906dcf500000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 00000003c68f8001 CR4: 00000000003606e0
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #1.2: .config --]
[-- Type: application/x-config, Size: 237462 bytes --]
[-- Attachment #1.3: mount_to_symlink.c --]
[-- Type: text/x-c, Size: 1318 bytes --]
#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#define bail(msg) \
do { printf("mount_to_symlink: %s: %m\n", msg); exit(1); } while (0)
int is_symlink(const char *path)
{
struct stat stat = {};
if (lstat(path, &stat) < 0)
bail("lstat(<path>)");
return S_ISLNK(stat.st_mode);
}
int main(int argc, char **argv)
{
struct stat stat = {};
char *src, *dst, *src_fdpath, *dst_fdpath;
int src_fd, dst_fd;
if (argc != 3)
bail("usage: mount_to_symlink <src> <dst>");
src_fdpath = src = argv[1];
dst_fdpath = dst = argv[2];
if (is_symlink(src)) {
// open source fd
src_fd = open(src, O_PATH | O_CLOEXEC | O_NOFOLLOW);
if (src_fd < 0)
bail("open(<src>, O_PATH|O_NOFOLLOW)");
// construct fd path
asprintf(&src_fdpath, "/proc/self/fd/%d", src_fd);
}
if (is_symlink(dst)) {
// open target fd
dst_fd = open(dst, O_PATH | O_CLOEXEC | O_NOFOLLOW);
if (dst_fd < 0)
bail("open(<dst>, O_PATH|O_NOFOLLOW)");
// construct fd path
asprintf(&dst_fdpath, "/proc/self/fd/%d", dst_fd);
}
// try to mount
mount(src_fdpath, dst_fdpath, "", MS_BIND, "");
printf("mount(%s, %s, MS_BIND) = %m (%d)\n", src, dst, -errno);
return 0;
}
[-- Attachment #1.4: umount_symlink.c --]
[-- Type: text/x-c, Size: 795 bytes --]
#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#define bail(msg) \
do { printf("mount_to_symlink: %s: %m\n", msg); exit(1); } while (0)
int main(int argc, char **argv)
{
struct stat stat = {};
char *mnt, *mnt_fdpath;
int mnt_fd;
if (argc != 2)
bail("need <mount> argument");
mnt = argv[1];
// open mountpoint fd
mnt_fd = open(mnt, O_PATH | O_CLOEXEC | O_NOFOLLOW);
if (mnt_fd < 0)
bail("open(<dst>, O_PATH|O_NOFOLLOW)");
// get fdpaths
asprintf(&mnt_fdpath, "/proc/self/fd/%d", mnt_fd);
// try to mount
umount2(mnt_fdpath, MNT_DETACH);
printf("umount2(%s, MNT_DETACH) = %m (%d)\n", mnt, -errno);
return 0;
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 5:49 UTC (permalink / raw)
To: Al Viro
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20191230054413.GX4203@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]
On 2019-12-30, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:
>
> > A reasonably detailed explanation of the issues is provided in the patch
> > itself, but the full traces produced by both the oopses and deadlocks is
> > included below (it makes little sense to include them in the commit since we
> > are disabling this feature, not directly fixing the bugs themselves).
> >
> > I've posted this as an RFC on whether this feature should be allowed at
> > all (and if anyone knows of legitimate uses for it), or if we should
> > work on fixing these other kernel bugs that it exposes.
>
> Umm... Are all of those traces
> a) reproducible on mainline and
This was on viro/for-next, I'll retry it on v5.5-rc4.
> b) reproducible as the first oopsen?
The NULL and garbage pointer derefs are reproducible as the first oops.
Looking at my logs, it looks like the deadlocks were always triggered
after the oops, but that might just have been a mistake on my part while
testing things.
> As it is, quite a few might be secondary results of earlier memory
> corruption...
Yeah, I thought that might be the case but decided to include them
anyway (the /proc/self/stack RCU stall is definitely the result of other
corruption and stalls).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2019-12-30 5:44 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20191230052036.8765-1-cyphar@cyphar.com>
On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:
> A reasonably detailed explanation of the issues is provided in the patch
> itself, but the full traces produced by both the oopses and deadlocks is
> included below (it makes little sense to include them in the commit since we
> are disabling this feature, not directly fixing the bugs themselves).
>
> I've posted this as an RFC on whether this feature should be allowed at
> all (and if anyone knows of legitimate uses for it), or if we should
> work on fixing these other kernel bugs that it exposes.
Umm... Are all of those traces
a) reproducible on mainline and
b) reproducible as the first oopsen?
As it is, quite a few might be secondary results of earlier memory
corruption...
^ permalink raw reply
* [PATCH RFC 1/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 5:20 UTC (permalink / raw)
To: Al Viro, David Howells, Eric Biederman, Linus Torvalds
Cc: Aleksa Sarai, stable, Christian Brauner, Serge Hallyn, dev,
containers, linux-api, linux-fsdevel, linux-kernel
In-Reply-To: <20191230052036.8765-1-cyphar@cyphar.com>
An undocumented feature of the mount interface was that it was possible
to mount over a symlink (even with the old mount API) by mounting over
/proc/self/fd/$n -- where the corresponding file descrpitor was opened
with (O_PATH|O_NOFOLLOW). This didn't work with traditional "new" mounts
(for a variety of reasons), but MS_BIND worked without issue. With the
new mount API it was even easier.
>From userspace's perspective, this capability is only really useful as
an attack vector. Until the introduction of openat2(RESOLVE_NO_XDEV),
there was no trivial way to detect if a bind-mount was present. In the
container runtime context (in a similar vein to CVE-2019-19921), this
could result in a privileged process being unable to detect that a
configuration resulted in magic-link usage operating on the wrong
magic-links. Additionally, the API to use this feature was incredibly
strange -- in order to umount, you would have go through
/proc/self/fd/$n again (umounting the path would result in the
*underlying* symlink being followed).
Which brings us to the issues on the kernel side. When umounting a mount
on top of a symlink, several oopses (both NULL and garbage kernel
address dereferences) and deadlocks could be triggered incredibly
trivially. Note that because this works in user namespaces, an
unprivileged user could trigger these oopses incredibly trivially. While
these bugs could be fixed separately, it seems much cleaner to disable a
"feature" which clearly was not intentional (and is not used --
otherwise we would've seen bug reports about it breaking on umount).
Note that because the linux-utils mount(1) helper will expand paths
containing symlinks in user-space, only users which used the mount(2)
syscall directly could possibly have seen this behaviour.
Cc: stable@vger.kernel.org # pre-git
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namespace.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index be601d3a8008..01a62bce105f 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2172,8 +2172,12 @@ static int graft_tree(struct mount *mnt, struct mount *p, struct mountpoint *mp)
if (mnt->mnt.mnt_sb->s_flags & SB_NOUSER)
return -EINVAL;
+ if (d_is_symlink(mp->m_dentry) ||
+ d_is_symlink(mnt->mnt.mnt_root))
+ return -EINVAL;
+
if (d_is_dir(mp->m_dentry) !=
- d_is_dir(mnt->mnt.mnt_root))
+ d_is_dir(mnt->mnt.mnt_root))
return -ENOTDIR;
return attach_recursive_mnt(mnt, p, mp, false);
@@ -2251,6 +2255,9 @@ static struct mount *__do_loopback(struct path *old_path, int recurse)
if (IS_MNT_UNBINDABLE(old))
return mnt;
+ if (d_is_symlink(old_path->dentry))
+ return mnt;
+
if (!check_mnt(old) && old_path->dentry->d_op != &ns_dentry_operations)
return mnt;
@@ -2635,6 +2642,10 @@ static int do_move_mount(struct path *old_path, struct path *new_path)
if (old_path->dentry != old_path->mnt->mnt_root)
goto out;
+ if (d_is_symlink(new_path->dentry) ||
+ d_is_symlink(old_path->dentry))
+ goto out;
+
if (d_is_dir(new_path->dentry) !=
d_is_dir(old_path->dentry))
goto out;
@@ -2726,10 +2737,6 @@ static int do_add_mount(struct mount *newmnt, struct path *path, int mnt_flags)
path->mnt->mnt_root == path->dentry)
goto unlock;
- err = -EINVAL;
- if (d_is_symlink(newmnt->mnt.mnt_root))
- goto unlock;
-
newmnt->mnt.mnt_flags = mnt_flags;
err = graft_tree(newmnt, parent, mp);
--
2.24.1
^ permalink raw reply related
* [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2019-12-30 5:20 UTC (permalink / raw)
To: Al Viro, David Howells, Eric Biederman, Linus Torvalds
Cc: Aleksa Sarai, stable, Christian Brauner, Serge Hallyn, dev,
containers, linux-api, linux-fsdevel, linux-kernel
An undocumented feature of the mount interface was that it was possible
to mount over a symlink (even with the old mount API) by mounting over
/proc/self/fd/$n -- where the corresponding file descrpitor was opened
with (O_PATH|O_NOFOLLOW). This didn't work with traditional "new" mounts
(for a variety of reasons), but MS_BIND worked without issue. With the
new mount API it was even easier.
A reasonably detailed explanation of the issues is provided in the patch
itself, but the full traces produced by both the oopses and deadlocks is
included below (it makes little sense to include them in the commit since we
are disabling this feature, not directly fixing the bugs themselves).
I've posted this as an RFC on whether this feature should be allowed at
all (and if anyone knows of legitimate uses for it), or if we should
work on fixing these other kernel bugs that it exposes.
Oops on NULL dereference:
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
PGD 8000000181b1f067 P4D 8000000181b1f067 PUD 24829c067 PMD 0
Oops: 0010 [#1] SMP PTI
CPU: 6 PID: 20796 Comm: mount_to_symlin Tainted: G OE 5.5.0-rc1+openat2~v18+ #123
Hardware name: LENOVO 20KHCTO1WW/20KHCTO1WW, BIOS N23ET55W (1.30 ) 08/31/2018
RIP: 0010:0x0
Code: Bad RIP value.
RSP: 0018:ffffbc7d87e1bcb0 EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffffa0c28cb633c0 RCX: 000000000000ae5a
RDX: 0000000000000089 RSI: ffffa0c0eece8840 RDI: ffffa0c0eb8843b0
RBP: ffffa0c0eb8843b0 R08: ffffdc7d7fbbb770 R09: ffffa0c0ca333000
R10: 0000000000000000 R11: 808080807fffffff R12: ffffa0c0eece8840
R13: 0000000000000089 R14: ffffbc7d87e1bdb0 R15: 0000000000000080
FS: 00007fd921508540(0000) GS:ffffa0c3cf580000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 000000018878a003 CR4: 00000000003606e0
Call Trace:
__lookup_slow+0x94/0x160
lookup_slow+0x36/0x50
path_mountpoint+0x1be/0x350
filename_mountpoint+0xa5/0x150
? __lookup_hash+0xa0/0xa0
ksys_umount+0x78/0x490
__x64_sys_umount+0x12/0x20
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7fd92143f4e7
Code: 09 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09
00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 69 09 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffe98c89cc8 EFLAGS: 00000202 ORIG_RAX: 00000000000000a6
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fd92143f4e7
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 000000000167a330
RBP: 00007ffe98c89da0 R08: 0000000000000000 R09: 000000000000000f
R10: 00000000004004c6 R11: 0000000000000202 R12: 00000000004010c0
R13: 00007ffe98c89e80 R14: 0000000000000000 R15: 0000000000000000
CR2: 0000000000000000
Oops on kernel address:
BUG: unable to handle page fault for address: ffffbc7d87e1bcc0
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 107d4a067 P4D 107d4a067 PUD 107d4b067 PMD 46d753067 PTE 0
Oops: 0002 [#2] SMP PTI
CPU: 4 PID: 20975 Comm: mount_to_symlin Tainted: G D OE 5.5.0-rc1+openat2~v18+ #123
Hardware name: LENOVO 20KHCTO1WW/20KHCTO1WW, BIOS N23ET55W (1.30 ) 08/31/2018
RIP: 0010:_raw_spin_lock_irqsave+0x28/0x50
Code: 00 00 0f 1f 44 00 00 41 54 53 48 89 fb 9c 58 0f 1f 44 00 00 49 89 c4
fa 66 0f 1f 44 00 00 e8 3f 55 82 ff 31 c0 ba 01 00 00 00 <f0> 0f b1
13 75 07 4c 89 e0 5b 41 5c c3 89 c6 48 89 df e8 01 52 77
RSP: 0018:ffffbc7d90067bd8 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffffbc7d87e1bcc0 RCX: 0000000200000000
RDX: 0000000000000001 RSI: ffffbc7d90067c50 RDI: ffffbc7d87e1bcc0
RBP: ffffbc7d87e1bcc0 R08: 0000000000000001 R09: 0000000000000003
R10: 0000000000000000 R11: 808080807fffffff R12: 0000000000000246
R13: ffffa0c28cb633c0 R14: ffffbc7d90067db0 R15: ffffa0c0eece8898
FS: 00007f4b80214540(0000) GS:ffffa0c3cf500000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffbc7d87e1bcc0 CR3: 000000026d4d0002 CR4: 00000000003606e0
Call Trace:
add_wait_queue+0x15/0x40
d_alloc_parallel+0x36d/0x480
? get_acl+0x1a/0x160
? wake_up_q+0xa0/0xa0
__lookup_slow+0x6b/0x160
lookup_slow+0x36/0x50
path_mountpoint+0x1be/0x350
filename_mountpoint+0xa5/0x150
? __lookup_hash+0xa0/0xa0
ksys_umount+0x78/0x490
__x64_sys_umount+0x12/0x20
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f4b8014b4e7
Code: 09 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09
00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 69 09 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffee8041b28 EFLAGS: 00000206 ORIG_RAX: 00000000000000a6
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f4b8014b4e7
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 00000000019c8330
RBP: 00007ffee8041c00 R08: 0000000000000000 R09: 000000000000000f
R10: 00000000004004c6 R11: 0000000000000206 R12: 00000000004010c0
R13: 00007ffee8041ce0 R14: 0000000000000000 R15: 0000000000000000
CR2: ffffbc7d87e1bcc0
Apparent deadlock in d_alloc_parallel:
watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [mount_to_symlin:21285]
CPU: 0 PID: 21285 Comm: mount_to_symlin Tainted: G D OE 5.5.0-rc1+openat2~v18+ #123
Hardware name: LENOVO 20KHCTO1WW/20KHCTO1WW, BIOS N23ET55W (1.30 ) 08/31/2018
RIP: 0010:native_queued_spin_lock_slowpath+0x5b/0x1d0
Code: 6d f0 0f ba 2f 08 0f 92 c0 0f b6 c0 c1 e0 08 89 c2 8b 07 30 e4 09 d0
a9 00 01 ff ff 75 47 85 c0 74 0e 8b 07 84 c0 74 08 f3 90 <8b> 07 84
c0 75 f8 b8 01 00 00 00 66 89 07 c3 8b 37 81 fe 00 01 00
RSP: 0018:ffffbc7d90547be8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000101 RBX: ffffffffbac7ac60 RCX: 0000000000000018
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffa0c0eece8898
RBP: ffffa0c0eece8898 R08: 00000000006f6f66 R09: 0000000000000003
R10: 0000000000000000 R11: 808080807fffffff R12: 00000000e25b3c73
R13: ffffa0c28cb633c0 R14: ffffbc7d90547db0 R15: ffffa0c0eece8898
FS: 00007fbb1fd30540(0000) GS:ffffa0c3cf400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fbb1fbd25a0 CR3: 0000000181ace005 CR4: 00000000003606f0
Call Trace:
_raw_spin_lock+0x1a/0x20
lockref_get_not_dead+0x4f/0x90
d_alloc_parallel+0x1a8/0x480
? get_acl+0x1a/0x160
__lookup_slow+0x6b/0x160
lookup_slow+0x36/0x50
path_mountpoint+0x1be/0x350
filename_mountpoint+0xa5/0x150
? __lookup_hash+0xa0/0xa0
ksys_umount+0x78/0x490
__x64_sys_umount+0x12/0x20
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7fbb1fc674e7
Code: 09 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09
00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 69 09 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd75fcb858 EFLAGS: 00000202 ORIG_RAX: 00000000000000a6
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbb1fc674e7
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000f6c330
RBP: 00007ffd75fcb930 R08: 0000000000000000 R09: 000000000000000f
R10: 00000000004004a6 R11: 0000000000000202 R12: 00000000004010b0
R13: 00007ffd75fcba10 R14: 0000000000000000 R15: 0000000000000000
RCU stall when trying to grab /proc/$pid/stack for the stuck process:
rcu: INFO: rcu_sched self-detected stall on CPU
rcu: 0-....: (15000 ticks this GP) idle=2c6/1/0x4000000000000002 softirq=1172554/1172554 fqs=6849
(t=15001 jiffies g=1935177 q=25734)
NMI backtrace for cpu 0
CPU: 0 PID: 21285 Comm: mount_to_symlin Tainted: G D OEL 5.5.0-rc1+openat2~v18+ #123
Hardware name: LENOVO 20KHCTO1WW/20KHCTO1WW, BIOS N23ET55W (1.30 ) 08/31/2018
Call Trace:
<IRQ>
dump_stack+0x8f/0xd0
? lapic_can_unplug_cpu.cold+0x3e/0x3e
nmi_cpu_backtrace.cold+0x14/0x52
nmi_trigger_cpumask_backtrace+0xf6/0xf8
rcu_dump_cpu_stacks+0x8f/0xbd
rcu_sched_clock_irq.cold+0x1b2/0x39f
update_process_times+0x24/0x50
tick_sched_handle+0x22/0x60
tick_sched_timer+0x38/0x80
? tick_sched_do_timer+0x60/0x60
__hrtimer_run_queues+0xf6/0x270
hrtimer_interrupt+0x10e/0x240
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_queued_spin_lock_slowpath+0x5b/0x1d0
Code: 6d f0 0f ba 2f 08 0f 92 c0 0f b6 c0 c1 e0 08 89 c2 8b 07 30 e4 09 d0
a9 00 01 ff ff 75 47 85 c0 74 0e 8b 07 84 c0 74 08 f3 90 <8b> 07 84 c0
75 f8 b8 01 00 00 00 66 89 07 c3 8b 37 81 fe 00 01 00
RSP: 0018:ffffbc7d90547be8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000101 RBX: ffffffffbac7ac60 RCX: 0000000000000018
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffa0c0eece8898
RBP: ffffa0c0eece8898 R08: 00000000006f6f66 R09: 0000000000000003
R10: 0000000000000000 R11: 808080807fffffff R12: 00000000e25b3c73
R13: ffffa0c28cb633c0 R14: ffffbc7d90547db0 R15: ffffa0c0eece8898
_raw_spin_lock+0x1a/0x20
lockref_get_not_dead+0x4f/0x90
d_alloc_parallel+0x1a8/0x480
? get_acl+0x1a/0x160
__lookup_slow+0x6b/0x160
lookup_slow+0x36/0x50
path_mountpoint+0x1be/0x350
filename_mountpoint+0xa5/0x150
? __lookup_hash+0xa0/0xa0
ksys_umount+0x78/0x490
__x64_sys_umount+0x12/0x20
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7fbb1fc674e7
Code: 09 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09
00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 69 09 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd75fcb858 EFLAGS: 00000202 ORIG_RAX: 00000000000000a6
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbb1fc674e7
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000f6c330
RBP: 00007ffd75fcb930 R08: 0000000000000000 R09: 000000000000000f
R10: 00000000004004a6 R11: 0000000000000202 R12: 00000000004010b0
R13: 00007ffd75fcba10 R14: 0000000000000000 R15: 0000000000000000
Deadlock on lock_mount after a successful umount(). The watchdog does trigger,
but I could only find this stall when trying to suspend the system in my logs:
Freezing of tasks failed after 20.010 seconds (2 tasks refusing to freeze, wq_busy=0):
mount_to_symlin D 0 5850 5849 0x00000004
Call Trace:
? __schedule+0x2dd/0x770
schedule+0x4a/0xb0
rwsem_down_write_slowpath+0x256/0x500
lock_mount+0x22/0xf0
do_mount+0x4b7/0x9f0
ksys_mount+0x7e/0xc0
__x64_sys_mount+0x21/0x30
do_syscall_64+0x64/0x240
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f86e6355fda
Code: Bad RIP value.
RSP: 002b:00007ffc36f952d8 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f86e6355fda
RDX: 0000000000402099 RSI: 00000000019a5310 RDI: 00007ffc36f96ee1
RBP: 00007ffc36f953b0 R08: 0000000000402099 R09: 000000000000000f
R10: 0000000000001000 R11: 0000000000000206 R12: 00000000004010c0
R13: 00007ffc36f95490 R14: 0000000000000000 R15: 0000000000000000
Cc: stable@vger.kernel.org # pre-git
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Aleksa Sarai (1):
mount: universally disallow mounting over symlinks
fs/namespace.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
base-commit: fd6988496e79a6a4bdb514a4655d2920209eb85d
--
2.24.1
^ permalink raw reply
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV
From: Sargun Dhillon @ 2019-12-29 23:42 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai,
Tycho Andersen
In-Reply-To: <20191229194318.ogsqw5pbjppbtsf7@wittgenstein>
On Sun, Dec 29, 2019 at 11:43 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Sun, Dec 29, 2019 at 11:06:25AM -0800, Sargun Dhillon wrote:
> > On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > > Does that even work if no dup() syscall has been made and trapped?
> > Yes, the first check that occurs is the check which checks if
> > seccom_notif has been
> > zeroed out. This happens before any of the other work.
>
> Ah, then sure I don't mind doing it this way. Though plumbing it
> directly into TEST(user_notification_basic) like I did below seems
> cleaner to me.
>
> >
> > > This looks like it would give you ENOENT...
> > This ioctl is a blocking ioctl. It'll block until there is a wakeup.
> > In this case, the wakeup
> > will never come, but that doesn't mean we get an ENOENT.
>
> Yeah, but that wold mean the test will hang weirdly if it bypasses the
> check. Sure it'll timeout but meh. I think I would prefer to have this
> done as part of the basic test where we know that there is an event but
> _shrug_.
>
> Christian
My one worry about this is that the behaviour should be if the input
(seccomp_notif) is invalid, it should immediately bail out, whether
or not there is an event waiting. If we add it to basic_test, then
it would hide the erroneous behaviour if bailout isn't immediate.
I'm not sure if that's a worry or not.
^ permalink raw reply
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV
From: Christian Brauner @ 2019-12-29 19:43 UTC (permalink / raw)
To: Sargun Dhillon
Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai,
Tycho Andersen
In-Reply-To: <CAMp4zn_4dN+5U2RxkpYp+m4=X9w2Wef1TuLZ2hRW+g+nK1cXGA@mail.gmail.com>
On Sun, Dec 29, 2019 at 11:06:25AM -0800, Sargun Dhillon wrote:
> On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote:
> > > Add a self-test to make sure that the kernel returns EINVAL, if any
> > > of the fields in seccomp_notif are set to non-null.
> > >
> > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > Suggested-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > ---
> > > tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++
> > > 1 file changed, 23 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > index f53f14971bff..379391a7fa41 100644
> > > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > @@ -3601,6 +3601,29 @@ TEST(user_notification_continue)
> > > }
> > > }
> > >
> > > +TEST(user_notification_garbage)
> > > +{
> > > + /*
> > > + * intentionally set pid to a garbage value to make sure the kernel
> > > + * catches it
> > > + */
> > > + struct seccomp_notif req = {
> > > + .pid = 1,
> > > + };
> > > + int ret, listener;
> > > +
> > > + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> > > + ASSERT_EQ(0, ret) {
> > > + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> > > + }
> > > +
> > > + listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> > > + ASSERT_GE(listener, 0);
> > > +
> > > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
> > > + EXPECT_EQ(EINVAL, errno);
> >
> > Does that even work if no dup() syscall has been made and trapped?
> Yes, the first check that occurs is the check which checks if
> seccom_notif has been
> zeroed out. This happens before any of the other work.
Ah, then sure I don't mind doing it this way. Though plumbing it
directly into TEST(user_notification_basic) like I did below seems
cleaner to me.
>
> > This looks like it would give you ENOENT...
> This ioctl is a blocking ioctl. It'll block until there is a wakeup.
> In this case, the wakeup
> will never come, but that doesn't mean we get an ENOENT.
Yeah, but that wold mean the test will hang weirdly if it bypasses the
check. Sure it'll timeout but meh. I think I would prefer to have this
done as part of the basic test where we know that there is an event but
_shrug_.
Christian
^ permalink raw reply
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV
From: Sargun Dhillon @ 2019-12-29 19:06 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai,
Tycho Andersen
In-Reply-To: <20191229171441.fxif7q32mv2hl3y4@wittgenstein>
On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote:
> > Add a self-test to make sure that the kernel returns EINVAL, if any
> > of the fields in seccomp_notif are set to non-null.
> >
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Suggested-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > index f53f14971bff..379391a7fa41 100644
> > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > @@ -3601,6 +3601,29 @@ TEST(user_notification_continue)
> > }
> > }
> >
> > +TEST(user_notification_garbage)
> > +{
> > + /*
> > + * intentionally set pid to a garbage value to make sure the kernel
> > + * catches it
> > + */
> > + struct seccomp_notif req = {
> > + .pid = 1,
> > + };
> > + int ret, listener;
> > +
> > + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> > + ASSERT_EQ(0, ret) {
> > + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> > + }
> > +
> > + listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> > + ASSERT_GE(listener, 0);
> > +
> > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
> > + EXPECT_EQ(EINVAL, errno);
>
> Does that even work if no dup() syscall has been made and trapped?
Yes, the first check that occurs is the check which checks if
seccom_notif has been
zeroed out. This happens before any of the other work.
> This looks like it would give you ENOENT...
This ioctl is a blocking ioctl. It'll block until there is a wakeup.
In this case, the wakeup
will never come, but that doesn't mean we get an ENOENT.
>
> If you want a simple solution just do:
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 6944b898bb53..4c73ae8679ea 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -3095,7 +3095,7 @@ TEST(user_notification_basic)
> pid_t pid;
> long ret;
> int status, listener;
> - struct seccomp_notif req = {};
> + struct seccomp_notif req;
> struct seccomp_notif_resp resp = {};
> struct pollfd pollfd;
>
> @@ -3158,6 +3158,13 @@ TEST(user_notification_basic)
> EXPECT_GT(poll(&pollfd, 1, -1), 0);
> EXPECT_EQ(pollfd.revents, POLLIN);
>
> + /* Test that we can't pass garbage to the kernel. */
> + memset(&req, 0, sizeof(req));
> + req.pid = -1;
> + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
> + EXPECT_EQ(EINVAL, errno);
> +
> + req.pid = 0;
> EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
>
> pollfd.fd = listener
>
>
> If you want a complete separate test then you can do:
I can do this, but given that the seccomp_notif datastructure should
always be copied
and checked before doing the actual evaluation of the syscall, this
test should pass
even if the trap is not triggered. The basic test should check if the
inverse holds.
If the kernel is broken the self-test harness will stall, and the
alarm timeout will
kick in.
>
> TEST(user_notification_garbage_recv)
> {
> pid_t pid;
> long ret;
> int status, listener;
> struct seccomp_notif req;
> struct seccomp_notif_resp resp = {};
> struct pollfd pollfd;
>
> ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> ASSERT_EQ(0, ret) {
> TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> }
>
> listener = user_trap_syscall(__NR_getppid,
> SECCOMP_FILTER_FLAG_NEW_LISTENER);
> ASSERT_GE(listener, 0);
>
> pid = fork();
> ASSERT_GE(pid, 0);
>
> if (pid == 0) {
> ret = syscall(__NR_getppid);
> exit(ret != USER_NOTIF_MAGIC);
> }
>
> pollfd.fd = listener;
> pollfd.events = POLLIN | POLLOUT;
>
> EXPECT_GT(poll(&pollfd, 1, -1), 0);
> EXPECT_EQ(pollfd.revents, POLLIN);
>
> memset(&req, 0, sizeof(req));
> req.pid = -1;
> EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
> EXPECT_EQ(EINVAL, errno);
>
> req.pid = 0;
> EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
>
> pollfd.fd = listener;
> pollfd.events = POLLIN | POLLOUT;
>
> EXPECT_GT(poll(&pollfd, 1, -1), 0);
> EXPECT_EQ(pollfd.revents, POLLOUT);
>
> EXPECT_EQ(req.data.nr, __NR_getppid);
>
> memset(&resp, 0, sizeof(resp));
> resp.id = req.id;
> resp.error = 0;
> resp.val = USER_NOTIF_MAGIC;
> EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
>
> EXPECT_EQ(waitpid(pid, &status, 0), pid);
> EXPECT_EQ(true, WIFEXITED(status));
> EXPECT_EQ(0, WEXITSTATUS(status));
> }
>
> Christian
^ permalink raw reply
* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Christian Brauner @ 2019-12-29 17:32 UTC (permalink / raw)
To: Sargun Dhillon
Cc: LKML, Linux Containers, Linux API, Linux FS-devel Mailing List,
Tycho Andersen, Jann Horn, Aleksa Sarai, Oleg Nesterov,
Andy Lutomirski, Al Viro, Gian-Carlo Pascutto,
Emilio Cobos Álvarez, Florian Weimer, Jed Davis,
Arnd Bergmann
In-Reply-To: <CAMp4zn9LyGw=BNiLNRgZXAbFdi87pSjy1YmDXvFvwmA=u3yDyw@mail.gmail.com>
On Sat, Dec 28, 2019 at 08:03:23AM -0500, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 5:12 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Thu, Dec 26, 2019 at 06:03:36PM +0000, Sargun Dhillon wrote:
> > > This syscall allows for the retrieval of file descriptors from other
> > > processes, based on their pidfd. This is possible using ptrace, and
> > > injection of parasitic code to inject code which leverages SCM_RIGHTS
> > > to move file descriptors between a tracee and a tracer. Unfortunately,
> > > ptrace comes with a high cost of requiring the process to be stopped,
> > > and breaks debuggers. This does not require stopping the process under
> > > manipulation.
> > >
> > > One reason to use this is to allow sandboxers to take actions on file
> > > descriptors on the behalf of another process. For example, this can be
> > > combined with seccomp-bpf's user notification to do on-demand fd
> > > extraction and take privileged actions. One such privileged action
> > > is binding a socket to a privileged port.
> > >
> > > This also adds the syscall to all architectures at the same time.
> > >
> > > /* prototype */
> > > /* flags is currently reserved and should be set to 0 */
> > > int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
> > >
> > > /* testing */
> > > Ran self-test suite on x86_64
> >
> > Fyi, I'm likely going to rewrite/add parts of/to this once I apply.
> >
> > A few comments below.
> >
> > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > index 2278e249141d..4a551f947869 100644
> > > --- a/kernel/pid.c
> > > +++ b/kernel/pid.c
> > > @@ -578,3 +578,106 @@ void __init pid_idr_init(void)
> > > init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> > > SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> > > }
> > > +
> > > +static struct file *__pidfd_fget(struct task_struct *task, int fd)
> > > +{
> > > + struct file *file;
> > > + int ret;
> > > +
> > > + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> > > + file = ERR_PTR(-EPERM);
> > > + goto out;
> > > + }
> > > +
> > > + file = fget_task(task, fd);
> > > + if (!file)
> > > + file = ERR_PTR(-EBADF);
> > > +
> > > +out:
> > > + mutex_unlock(&task->signal->cred_guard_mutex);
> > > + return file;
> > > +}
> >
> > Looking at this code now a bit closer, ptrace_may_access() and
> > fget_task() both take task_lock(task) so this currently does:
> >
> > task_lock();
> > /* check access */
> > task_unlock();
> >
> > task_lock();
> > /* get fd */
> > task_unlock();
> >
> > which doesn't seem great.
> >
> > I would prefer if we could do:
> > task_lock();
> > /* check access */
> > /* get fd */
> > task_unlock();
> >
> > But ptrace_may_access() doesn't export an unlocked variant so _shrug_.
> Right, it seems intentional that __ptrace_may_access isn't exported. We
> can always change that later?
Yeah, it's just something I noted and it's not a big deal in my book. It
just would be nicer to only have to lock once. ptrace would need to
expose an unlocked variant and fget_task() would need to be removed
completely and then grabbing the file via fget or sm. But as I said it's
ok to do it like this rn.
>
> >
> > But we can write this a little cleaner without the goto as:
> >
> > static struct file *__pidfd_fget(struct task_struct *task, int fd)
> > {
> > struct file *file;
> > int ret;
> >
> > ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> > if (ret)
> > return ERR_PTR(ret);
> >
> > if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
> > file = fget_task(task, fd);
> > else
> > file = ERR_PTR(-EPERM);
> > mutex_unlock(&task->signal->cred_guard_mutex);
> >
> > return file ?: ERR_PTR(-EBADF);
> > }
> >
> > If you don't like the ?: just do:
> >
> > if (!file)
> > return ERR_PTR(-EBADF);
> >
> > return file;
> >
> > though I prefer the shorter ?: syntax which is perfect for shortcutting
> > returns.
> >
> > > +
> > > +static int pidfd_getfd(struct pid *pid, int fd)
> > > +{
> > > + struct task_struct *task;
> > > + struct file *file;
> > > + int ret, retfd;
> > > +
> > > + task = get_pid_task(pid, PIDTYPE_PID);
> > > + if (!task)
> > > + return -ESRCH;
> > > +
> > > + file = __pidfd_fget(task, fd);
> > > + put_task_struct(task);
> > > + if (IS_ERR(file))
> > > + return PTR_ERR(file);
> > > +
> > > + retfd = get_unused_fd_flags(O_CLOEXEC);
> > > + if (retfd < 0) {
> > > + ret = retfd;
> > > + goto out;
> > > + }
> > > +
> > > + /*
> > > + * security_file_receive must come last since it may have side effects
> > > + * and cannot be reversed.
> > > + */
> > > + ret = security_file_receive(file);
> >
> > So I don't understand the comment here. Can you explain what the side
> > effects are?
> The LSM can modify the LSM blob, or emit an (audit) event, even though
> the operation as a whole failed. Smack will report that file_receive
> successfully happened even though it could not have happened,
> because we were unable to provision a file descriptor.
So this either sounds like a bug in Smack or a design choice by the LSM
framework in general and also that it might apply to a lot of other
hooks too? But I'm not qualified to assess that.
Modifying an LSM blob, emitting an audit event may very well happen but
there are places all over the kernel were security hooks are called and
they are not the last point of failure (capable hooks come to mind
right away). My point being just because an audit event that happened
from an LSM indicating that e.g. a file receive event happened cannot be
intended to be equivalent == "was successful". That is not reality right
now when looking at net/* where security_file_receive() is called too
and surely can only be guaranteed from the actual codepaths that does the
file receive.
So I'd argue let's just use the clean version where we call
security_file_receive() before allocing the new fd just like net/* does
and make the code simpler and easier to maintain.
>
> Apparmor does similar, and also manipulates the LSM blob,
> although that is undone by closing the file.
>
>
> > security_file_receive() is called in two places: net/core/scm.c and
> > net/compat.c. In both places it is called _before_ get_unused_fd_flags()
> > so I don't know what's special here that would prevent us from doing the
> > same. If there's no actual reason, please rewrite this functions as:
> >
> > static int pidfd_getfd(struct pid *pid, int fd)
> > {
> > int ret;
> > struct task_struct *task;
> > struct file *file;
> >
> > task = get_pid_task(pid, PIDTYPE_PID);
> > if (!task)
> > return -ESRCH;
> >
> > file = __pidfd_fget(task, fd);
> > put_task_struct(task);
> > if (IS_ERR(file))
> > return PTR_ERR(file);
> >
> > ret = security_file_receive(file);
> > if (ret) {
> > fput(file);
> > return ret;
> > }
> >
> > ret = get_unused_fd_flags(O_CLOEXEC);
> > if (ret < 0)
> > fput(file);
> > else
> > fd_install(ret, file);
> >
> > return ret;
> > }
^ permalink raw reply
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV
From: Christian Brauner @ 2019-12-29 17:14 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, linux-api, Jann Horn, Kees Cook, Aleksa Sarai,
Tycho Andersen
In-Reply-To: <20191229062451.9467-3-sargun@sargun.me>
On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote:
> Add a self-test to make sure that the kernel returns EINVAL, if any
> of the fields in seccomp_notif are set to non-null.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Suggested-by: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index f53f14971bff..379391a7fa41 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -3601,6 +3601,29 @@ TEST(user_notification_continue)
> }
> }
>
> +TEST(user_notification_garbage)
> +{
> + /*
> + * intentionally set pid to a garbage value to make sure the kernel
> + * catches it
> + */
> + struct seccomp_notif req = {
> + .pid = 1,
> + };
> + int ret, listener;
> +
> + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> + }
> +
> + listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + ASSERT_GE(listener, 0);
> +
> + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
> + EXPECT_EQ(EINVAL, errno);
Does that even work if no dup() syscall has been made and trapped?
This looks like it would give you ENOENT...
If you want a simple solution just do:
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 6944b898bb53..4c73ae8679ea 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3095,7 +3095,7 @@ TEST(user_notification_basic)
pid_t pid;
long ret;
int status, listener;
- struct seccomp_notif req = {};
+ struct seccomp_notif req;
struct seccomp_notif_resp resp = {};
struct pollfd pollfd;
@@ -3158,6 +3158,13 @@ TEST(user_notification_basic)
EXPECT_GT(poll(&pollfd, 1, -1), 0);
EXPECT_EQ(pollfd.revents, POLLIN);
+ /* Test that we can't pass garbage to the kernel. */
+ memset(&req, 0, sizeof(req));
+ req.pid = -1;
+ EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
+ EXPECT_EQ(EINVAL, errno);
+
+ req.pid = 0;
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
pollfd.fd = listener
If you want a complete separate test then you can do:
TEST(user_notification_garbage_recv)
{
pid_t pid;
long ret;
int status, listener;
struct seccomp_notif req;
struct seccomp_notif_resp resp = {};
struct pollfd pollfd;
ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
ASSERT_EQ(0, ret) {
TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
}
listener = user_trap_syscall(__NR_getppid,
SECCOMP_FILTER_FLAG_NEW_LISTENER);
ASSERT_GE(listener, 0);
pid = fork();
ASSERT_GE(pid, 0);
if (pid == 0) {
ret = syscall(__NR_getppid);
exit(ret != USER_NOTIF_MAGIC);
}
pollfd.fd = listener;
pollfd.events = POLLIN | POLLOUT;
EXPECT_GT(poll(&pollfd, 1, -1), 0);
EXPECT_EQ(pollfd.revents, POLLIN);
memset(&req, 0, sizeof(req));
req.pid = -1;
EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
EXPECT_EQ(EINVAL, errno);
req.pid = 0;
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
pollfd.fd = listener;
pollfd.events = POLLIN | POLLOUT;
EXPECT_GT(poll(&pollfd, 1, -1), 0);
EXPECT_EQ(pollfd.revents, POLLOUT);
EXPECT_EQ(req.data.nr, __NR_getppid);
memset(&resp, 0, sizeof(resp));
resp.id = req.id;
resp.error = 0;
resp.val = USER_NOTIF_MAGIC;
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
EXPECT_EQ(waitpid(pid, &status, 0), pid);
EXPECT_EQ(true, WIFEXITED(status));
EXPECT_EQ(0, WEXITSTATUS(status));
}
Christian
^ permalink raw reply related
* Re: [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif
From: Christian Brauner @ 2019-12-29 16:11 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, linux-api, Jann Horn, Kees Cook, Aleksa Sarai,
Tycho Andersen
In-Reply-To: <20191229062451.9467-1-sargun@sargun.me>
On Sat, Dec 28, 2019 at 10:24:49PM -0800, Sargun Dhillon wrote:
> The seccomp_notif structure should be zeroed out prior to calling the
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> whether these structures were zeroed out or not, so these worked.
>
> This patch zeroes out the seccomp_notif data structure prior to calling
> the ioctl.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
Thanks!
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Andy Lutomirski @ 2019-12-29 15:08 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Andy Lutomirski, Stephan Mueller, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <20191229144904.GB7177@mit.edu>
> On Dec 29, 2019, at 10:49 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Dec 27, 2019 at 06:06:56PM -0800, Andy Lutomirski wrote:
>>
>> I'm thinking of having a real class device and chardev for each hwrng
>> device. Authentication is entirely in userspace: whatever user code
>> is involved can look at the sysfs hierarchy and decide to what extent
>> it trusts a given source. This could be done based on bus topology or
>> based on anything else.
>
> Yes, that's what I was thinking. Another project on my "when I can
> get a round tuit" list is to change how drivers/char/random.c taps
> into the hwrng devices, mixing in a bit from each of these devies in a
> round-robin fashion, instead of just feeding from a single hwrng.
>
>> The kernel could also separately expose various noise sources, and the
>> user code can do whatever it wants with them. But these should be
>> explicitly unconditioned, un-entropy-extracted sources -- user code
>> can run its favorite algorithm to extract something it believes to be
>> useful. The only conceptually tricky bit is keeping user code like
>> this from interfering with the in-kernel RNG.
>
> The other problem is the unconditioned values of the noise sources may
> leak unacceptable amounts of information about system operation. The
> most obvious example of this would be keyboard and mouse sources,
> where today we mix in not only the timing information, but the actual
> input values (e.g., the keyboard scancodes) into the entropy pool.
> Exposing this to userspace, even if it is via a privileged system
> call, would be... unwise.
>
>
Hmm. We could give only the timing.
We could also say that the official interface for this is to use tracepoints and punt everything into userspace.
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Theodore Y. Ts'o @ 2019-12-29 14:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephan Mueller, LKML, Linux API, Kees Cook, Jason A. Donenfeld,
Ahmed S. Darwish, Lennart Poettering, Eric W. Biederman,
Alexander E. Patrakov, Michael Kerrisk, Willy Tarreau,
Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <CALCETrUyVx_qb2yYH8D_z1T2bVu5RAEr71G0MTzEksBKKM1QsA@mail.gmail.com>
On Fri, Dec 27, 2019 at 06:06:56PM -0800, Andy Lutomirski wrote:
>
> I'm thinking of having a real class device and chardev for each hwrng
> device. Authentication is entirely in userspace: whatever user code
> is involved can look at the sysfs hierarchy and decide to what extent
> it trusts a given source. This could be done based on bus topology or
> based on anything else.
Yes, that's what I was thinking. Another project on my "when I can
get a round tuit" list is to change how drivers/char/random.c taps
into the hwrng devices, mixing in a bit from each of these devies in a
round-robin fashion, instead of just feeding from a single hwrng.
> The kernel could also separately expose various noise sources, and the
> user code can do whatever it wants with them. But these should be
> explicitly unconditioned, un-entropy-extracted sources -- user code
> can run its favorite algorithm to extract something it believes to be
> useful. The only conceptually tricky bit is keeping user code like
> this from interfering with the in-kernel RNG.
The other problem is the unconditioned values of the noise sources may
leak unacceptable amounts of information about system operation. The
most obvious example of this would be keyboard and mouse sources,
where today we mix in not only the timing information, but the actual
input values (e.g., the keyboard scancodes) into the entropy pool.
Exposing this to userspace, even if it is via a privileged system
call, would be... unwise.
- Ted
^ permalink raw reply
* [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV
From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai,
Tycho Andersen, Sargun Dhillon
In-Reply-To: <20191229062451.9467-1-sargun@sargun.me>
Add a self-test to make sure that the kernel returns EINVAL, if any
of the fields in seccomp_notif are set to non-null.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Suggested-by: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Kees Cook <keescook@chromium.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index f53f14971bff..379391a7fa41 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3601,6 +3601,29 @@ TEST(user_notification_continue)
}
}
+TEST(user_notification_garbage)
+{
+ /*
+ * intentionally set pid to a garbage value to make sure the kernel
+ * catches it
+ */
+ struct seccomp_notif req = {
+ .pid = 1,
+ };
+ int ret, listener;
+
+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+ ASSERT_EQ(0, ret) {
+ TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+ }
+
+ listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req));
+ EXPECT_EQ(EINVAL, errno);
+}
+
/*
* TODO:
* - add microbenchmarks
--
2.20.1
^ permalink raw reply related
* [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user
From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai,
Tycho Andersen, Sargun Dhillon
In-Reply-To: <20191229062451.9467-1-sargun@sargun.me>
This patch is a small change in enforcement of the uapi for
SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
is passed (seccomp_notif) must be zeroed out. Previously any of its
members could be set to nonsense values, and we would ignore it.
This ensures all fields are set to their zero value.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
Acked-by: Tycho Andersen <tycho@tycho.ws>
---
kernel/seccomp.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 12d2227e5786..b6ea3dcb57bf 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
struct seccomp_notif unotif;
ssize_t ret;
+ /* Verify that we're not given garbage to keep struct extensible. */
+ ret = check_zeroed_user(buf, sizeof(unotif));
+ if (ret < 0)
+ return ret;
+ if (!ret)
+ return -EINVAL;
+
memset(&unotif, 0, sizeof(unotif));
ret = down_interruptible(&filter->notif->request);
--
2.20.1
^ permalink raw reply related
* [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif
From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai,
Tycho Andersen, Sargun Dhillon
The seccomp_notif structure should be zeroed out prior to calling the
SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
whether these structures were zeroed out or not, so these worked.
This patch zeroes out the seccomp_notif data structure prior to calling
the ioctl.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Cc: Kees Cook <keescook@chromium.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
---
samples/seccomp/user-trap.c | 2 +-
tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..3e31ec0cf4a5 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -298,7 +298,6 @@ int main(void)
req = malloc(sizes.seccomp_notif);
if (!req)
goto out_close;
- memset(req, 0, sizeof(*req));
resp = malloc(sizes.seccomp_notif_resp);
if (!resp)
@@ -306,6 +305,7 @@ int main(void)
memset(resp, 0, sizeof(*resp));
while (1) {
+ memset(req, 0, sizeof(*req));
if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
perror("ioctl recv");
goto out_resp;
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 6944b898bb53..f53f14971bff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3278,6 +3278,7 @@ TEST(user_notification_signal)
close(sk_pair[1]);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
EXPECT_EQ(kill(pid, SIGUSR1), 0);
@@ -3296,6 +3297,7 @@ TEST(user_notification_signal)
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
EXPECT_EQ(errno, ENOENT);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
resp.id = req.id;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif
From: Tycho Andersen @ 2019-12-29 0:18 UTC (permalink / raw)
To: Sargun Dhillon
Cc: LKML, Linux API, Jann Horn, Christian Brauner, Kees Cook,
Aleksa Sarai
In-Reply-To: <CAMp4zn91GoB=1eTbc_ux4eNs2-QFm+JocodgFQYUiiXL7H4m9w@mail.gmail.com>
On Sat, Dec 28, 2019 at 07:10:29PM -0500, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> > > The seccomp_notif structure should be zeroed out prior to calling the
> > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> > > whether these structures were zeroed out or not, so these worked.
> > >
> > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > ---
> > > samples/seccomp/user-trap.c | 2 +-
> > > tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> > > index 6d0125ca8af7..0ca8fb37cd79 100644
> > > --- a/samples/seccomp/user-trap.c
> > > +++ b/samples/seccomp/user-trap.c
> > > @@ -298,7 +298,6 @@ int main(void)
> > > req = malloc(sizes.seccomp_notif);
> > > if (!req)
> > > goto out_close;
> > > - memset(req, 0, sizeof(*req));
> > >
> > > resp = malloc(sizes.seccomp_notif_resp);
> > > if (!resp)
> > > @@ -306,6 +305,7 @@ int main(void)
> > > memset(resp, 0, sizeof(*resp));
> >
> > I know it's unrelated, but it's probably worth sending a patch to fix
> > this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> > the kernel is older this will over-zero things. I can do that, or you
> > can add the patch to this series, just let me know which.
>
> I was thinking about this, and initially, I chose to make the smaller
> change. I think it might make more sense to combine the patch,
> given that the memset behaviour is "incorrect" if we do it based on
> sizeof(*req), or sizeof(*resp).
>
> I'll go ahead and respin this patch with the change to call memset
> based on sizes.
I think it would be good to keep it as a separate patch, since it's an
unrelated bug fix. That way if we have to revert these because of some
breakage, we won't lose the fix.
Cheers,
Tycho
^ permalink raw reply
* Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif
From: Sargun Dhillon @ 2019-12-29 0:10 UTC (permalink / raw)
To: Tycho Andersen
Cc: LKML, Linux API, Jann Horn, Christian Brauner, Kees Cook,
Aleksa Sarai
In-Reply-To: <20191228181825.GB6746@cisco>
On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> > The seccomp_notif structure should be zeroed out prior to calling the
> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> > whether these structures were zeroed out or not, so these worked.
> >
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> > samples/seccomp/user-trap.c | 2 +-
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> > index 6d0125ca8af7..0ca8fb37cd79 100644
> > --- a/samples/seccomp/user-trap.c
> > +++ b/samples/seccomp/user-trap.c
> > @@ -298,7 +298,6 @@ int main(void)
> > req = malloc(sizes.seccomp_notif);
> > if (!req)
> > goto out_close;
> > - memset(req, 0, sizeof(*req));
> >
> > resp = malloc(sizes.seccomp_notif_resp);
> > if (!resp)
> > @@ -306,6 +305,7 @@ int main(void)
> > memset(resp, 0, sizeof(*resp));
>
> I know it's unrelated, but it's probably worth sending a patch to fix
> this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> the kernel is older this will over-zero things. I can do that, or you
> can add the patch to this series, just let me know which.
I was thinking about this, and initially, I chose to make the smaller
change. I think it might make more sense to combine the patch,
given that the memset behaviour is "incorrect" if we do it based on
sizeof(*req), or sizeof(*resp).
I'll go ahead and respin this patch with the change to call memset
based on sizes.
>
> But in any case, this patch is:
>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
>
> Cheers,
>
> Tycho
^ permalink raw reply
* Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif
From: Tycho Andersen @ 2019-12-28 18:18 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, linux-api, jannh, christian.brauner, keescook,
cyphar
In-Reply-To: <20191228014837.GA31774@ircssh-2.c.rugged-nimbus-611.internal>
On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> The seccomp_notif structure should be zeroed out prior to calling the
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> whether these structures were zeroed out or not, so these worked.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> samples/seccomp/user-trap.c | 2 +-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> index 6d0125ca8af7..0ca8fb37cd79 100644
> --- a/samples/seccomp/user-trap.c
> +++ b/samples/seccomp/user-trap.c
> @@ -298,7 +298,6 @@ int main(void)
> req = malloc(sizes.seccomp_notif);
> if (!req)
> goto out_close;
> - memset(req, 0, sizeof(*req));
>
> resp = malloc(sizes.seccomp_notif_resp);
> if (!resp)
> @@ -306,6 +305,7 @@ int main(void)
> memset(resp, 0, sizeof(*resp));
I know it's unrelated, but it's probably worth sending a patch to fix
this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
the kernel is older this will over-zero things. I can do that, or you
can add the patch to this series, just let me know which.
But in any case, this patch is:
Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Cheers,
Tycho
^ permalink raw reply
* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Sargun Dhillon @ 2019-12-28 13:03 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Linux Containers, Linux API, Linux FS-devel Mailing List,
Tycho Andersen, Jann Horn, Aleksa Sarai, Oleg Nesterov,
Andy Lutomirski, Al Viro, Gian-Carlo Pascutto,
Emilio Cobos Álvarez, Florian Weimer, Jed Davis,
Arnd Bergmann
In-Reply-To: <20191228100944.kh22bofbr5oe2kvk@wittgenstein>
On Sat, Dec 28, 2019 at 5:12 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Dec 26, 2019 at 06:03:36PM +0000, Sargun Dhillon wrote:
> > This syscall allows for the retrieval of file descriptors from other
> > processes, based on their pidfd. This is possible using ptrace, and
> > injection of parasitic code to inject code which leverages SCM_RIGHTS
> > to move file descriptors between a tracee and a tracer. Unfortunately,
> > ptrace comes with a high cost of requiring the process to be stopped,
> > and breaks debuggers. This does not require stopping the process under
> > manipulation.
> >
> > One reason to use this is to allow sandboxers to take actions on file
> > descriptors on the behalf of another process. For example, this can be
> > combined with seccomp-bpf's user notification to do on-demand fd
> > extraction and take privileged actions. One such privileged action
> > is binding a socket to a privileged port.
> >
> > This also adds the syscall to all architectures at the same time.
> >
> > /* prototype */
> > /* flags is currently reserved and should be set to 0 */
> > int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
> >
> > /* testing */
> > Ran self-test suite on x86_64
>
> Fyi, I'm likely going to rewrite/add parts of/to this once I apply.
>
> A few comments below.
>
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 2278e249141d..4a551f947869 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -578,3 +578,106 @@ void __init pid_idr_init(void)
> > init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> > SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> > }
> > +
> > +static struct file *__pidfd_fget(struct task_struct *task, int fd)
> > +{
> > + struct file *file;
> > + int ret;
> > +
> > + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> > + file = ERR_PTR(-EPERM);
> > + goto out;
> > + }
> > +
> > + file = fget_task(task, fd);
> > + if (!file)
> > + file = ERR_PTR(-EBADF);
> > +
> > +out:
> > + mutex_unlock(&task->signal->cred_guard_mutex);
> > + return file;
> > +}
>
> Looking at this code now a bit closer, ptrace_may_access() and
> fget_task() both take task_lock(task) so this currently does:
>
> task_lock();
> /* check access */
> task_unlock();
>
> task_lock();
> /* get fd */
> task_unlock();
>
> which doesn't seem great.
>
> I would prefer if we could do:
> task_lock();
> /* check access */
> /* get fd */
> task_unlock();
>
> But ptrace_may_access() doesn't export an unlocked variant so _shrug_.
Right, it seems intentional that __ptrace_may_access isn't exported. We
can always change that later?
>
> But we can write this a little cleaner without the goto as:
>
> static struct file *__pidfd_fget(struct task_struct *task, int fd)
> {
> struct file *file;
> int ret;
>
> ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> if (ret)
> return ERR_PTR(ret);
>
> if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
> file = fget_task(task, fd);
> else
> file = ERR_PTR(-EPERM);
> mutex_unlock(&task->signal->cred_guard_mutex);
>
> return file ?: ERR_PTR(-EBADF);
> }
>
> If you don't like the ?: just do:
>
> if (!file)
> return ERR_PTR(-EBADF);
>
> return file;
>
> though I prefer the shorter ?: syntax which is perfect for shortcutting
> returns.
>
> > +
> > +static int pidfd_getfd(struct pid *pid, int fd)
> > +{
> > + struct task_struct *task;
> > + struct file *file;
> > + int ret, retfd;
> > +
> > + task = get_pid_task(pid, PIDTYPE_PID);
> > + if (!task)
> > + return -ESRCH;
> > +
> > + file = __pidfd_fget(task, fd);
> > + put_task_struct(task);
> > + if (IS_ERR(file))
> > + return PTR_ERR(file);
> > +
> > + retfd = get_unused_fd_flags(O_CLOEXEC);
> > + if (retfd < 0) {
> > + ret = retfd;
> > + goto out;
> > + }
> > +
> > + /*
> > + * security_file_receive must come last since it may have side effects
> > + * and cannot be reversed.
> > + */
> > + ret = security_file_receive(file);
>
> So I don't understand the comment here. Can you explain what the side
> effects are?
The LSM can modify the LSM blob, or emit an (audit) event, even though
the operation as a whole failed. Smack will report that file_receive
successfully happened even though it could not have happened,
because we were unable to provision a file descriptor.
Apparmor does similar, and also manipulates the LSM blob,
although that is undone by closing the file.
> security_file_receive() is called in two places: net/core/scm.c and
> net/compat.c. In both places it is called _before_ get_unused_fd_flags()
> so I don't know what's special here that would prevent us from doing the
> same. If there's no actual reason, please rewrite this functions as:
>
> static int pidfd_getfd(struct pid *pid, int fd)
> {
> int ret;
> struct task_struct *task;
> struct file *file;
>
> task = get_pid_task(pid, PIDTYPE_PID);
> if (!task)
> return -ESRCH;
>
> file = __pidfd_fget(task, fd);
> put_task_struct(task);
> if (IS_ERR(file))
> return PTR_ERR(file);
>
> ret = security_file_receive(file);
> if (ret) {
> fput(file);
> return ret;
> }
>
> ret = get_unused_fd_flags(O_CLOEXEC);
> if (ret < 0)
> fput(file);
> else
> fd_install(ret, file);
>
> return ret;
> }
^ permalink raw reply
* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Christian Brauner @ 2019-12-28 10:11 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191226180334.GA29409@ircssh-2.c.rugged-nimbus-611.internal>
On Thu, Dec 26, 2019 at 06:03:36PM +0000, Sargun Dhillon wrote:
> This syscall allows for the retrieval of file descriptors from other
> processes, based on their pidfd. This is possible using ptrace, and
> injection of parasitic code to inject code which leverages SCM_RIGHTS
> to move file descriptors between a tracee and a tracer. Unfortunately,
> ptrace comes with a high cost of requiring the process to be stopped,
> and breaks debuggers. This does not require stopping the process under
> manipulation.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. One such privileged action
> is binding a socket to a privileged port.
>
> This also adds the syscall to all architectures at the same time.
>
> /* prototype */
> /* flags is currently reserved and should be set to 0 */
> int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
>
> /* testing */
> Ran self-test suite on x86_64
Fyi, I'm likely going to rewrite/add parts of/to this once I apply.
A few comments below.
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2278e249141d..4a551f947869 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -578,3 +578,106 @@ void __init pid_idr_init(void)
> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> }
> +
> +static struct file *__pidfd_fget(struct task_struct *task, int fd)
> +{
> + struct file *file;
> + int ret;
> +
> + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> + file = ERR_PTR(-EPERM);
> + goto out;
> + }
> +
> + file = fget_task(task, fd);
> + if (!file)
> + file = ERR_PTR(-EBADF);
> +
> +out:
> + mutex_unlock(&task->signal->cred_guard_mutex);
> + return file;
> +}
Looking at this code now a bit closer, ptrace_may_access() and
fget_task() both take task_lock(task) so this currently does:
task_lock();
/* check access */
task_unlock();
task_lock();
/* get fd */
task_unlock();
which doesn't seem great.
I would prefer if we could do:
task_lock();
/* check access */
/* get fd */
task_unlock();
But ptrace_may_access() doesn't export an unlocked variant so _shrug_.
But we can write this a little cleaner without the goto as:
static struct file *__pidfd_fget(struct task_struct *task, int fd)
{
struct file *file;
int ret;
ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
if (ret)
return ERR_PTR(ret);
if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
file = fget_task(task, fd);
else
file = ERR_PTR(-EPERM);
mutex_unlock(&task->signal->cred_guard_mutex);
return file ?: ERR_PTR(-EBADF);
}
If you don't like the ?: just do:
if (!file)
return ERR_PTR(-EBADF);
return file;
though I prefer the shorter ?: syntax which is perfect for shortcutting
returns.
> +
> +static int pidfd_getfd(struct pid *pid, int fd)
> +{
> + struct task_struct *task;
> + struct file *file;
> + int ret, retfd;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
> +
> + file = __pidfd_fget(task, fd);
> + put_task_struct(task);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + retfd = get_unused_fd_flags(O_CLOEXEC);
> + if (retfd < 0) {
> + ret = retfd;
> + goto out;
> + }
> +
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
So I don't understand the comment here. Can you explain what the side
effects are?
security_file_receive() is called in two places: net/core/scm.c and
net/compat.c. In both places it is called _before_ get_unused_fd_flags()
so I don't know what's special here that would prevent us from doing the
same. If there's no actual reason, please rewrite this functions as:
static int pidfd_getfd(struct pid *pid, int fd)
{
int ret;
struct task_struct *task;
struct file *file;
task = get_pid_task(pid, PIDTYPE_PID);
if (!task)
return -ESRCH;
file = __pidfd_fget(task, fd);
put_task_struct(task);
if (IS_ERR(file))
return PTR_ERR(file);
ret = security_file_receive(file);
if (ret) {
fput(file);
return ret;
}
ret = get_unused_fd_flags(O_CLOEXEC);
if (ret < 0)
fput(file);
else
fd_install(ret, file);
return ret;
}
^ 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