* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Ingo Molnar @ 2018-06-24 7:12 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-arch, Paul Eggert, Andrew Morton, Arnd Bergmann,
y2038 Mailman List, Linux API, the arch/x86 maintainers,
Linux Kernel Mailing List, Dominik Brodowski, Deepa Dinamani,
Ivan Kokshaysky, Al Viro, linux-alpha, Matt Turner,
Thomas Gleixner, Richard Henderson
In-Reply-To: <87a7rm3eb5.fsf@xmission.com>
* Eric W. Biederman <ebiederm@xmission.com> wrote:
> The trouble with attributes is that means you can't filter your system
> call arguments with seccomp. [...]
There's nothing keeping seccomp from securely fetching those arguments and
extending filtering to them as well ...
Allowing that would make sense for a lot of other system calls as well.
Thanks,
Ingo
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Jann Horn @ 2018-06-22 22:27 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Tycho Andersen, kernel list, containers,
Linux API, Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAGXu5jKWQCDzXPdTz=+bxT9+4enaBvgYfMseUiGti+t1KwZT8g@mail.gmail.com>
On Fri, Jun 22, 2018 at 11:51 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Jun 22, 2018 at 11:09 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > One possible extra issue: IIRC /proc/.../mem uses FOLL_FORCE, which is not what we want here.
Uuugh, I forgot about that.
> > How about just adding an explicit “read/write the seccomp-trapped task’s memory” primitive? That should be easier than a “open mem fd” primitive.
>
> Uuugh. Can we avoid adding another "read/write remote process memory"
> interface? The point of this series was to provide a lightweight
> approach to what should normally be possible via the existing
> seccomp+ptrace interface. I do like Jann's context idea, but I agree
> with Andy: it can't be a handle to /proc/$pid/mem, since it's
> FOLL_FORCE. Is there any other kind of process context id we can use
> for this instead of pid? There was once an idea of pid-fd but it never
> landed... This would let us get rid of the "id" in the structure too.
> And if that existed, we could make process_vm_*v() safer too (taking a
> pid-fd instead of a pid).
Or make a duplicate of /proc/$pid/mem that only differs in whether it
sets FOLL_FORCE? The code is basically already there... something like
this:
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 80aa42506b8b..e8a6a63046da 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -762,6 +762,8 @@ static int mem_open(struct inode *inode, struct file *file)
return ret;
}
+static const struct file_operations proc_mem_operations;
+
static ssize_t mem_rw(struct file *file, char __user *buf,
size_t count, loff_t *ppos, int write)
{
@@ -782,7 +784,10 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
if (!mmget_not_zero(mm))
goto free;
- flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
+ flags = (write ? FOLL_WRITE : 0);
+ if (file->f_op == &proc_mem_operations) {
+ flags |= FOLL_FORCE;
+ }
while (count > 0) {
int this_len = min_t(int, count, PAGE_SIZE);
@@ -861,6 +866,14 @@ static const struct file_operations proc_mem_operations = {
.release = mem_release,
};
+static const struct file_operations proc_mem_noforce_operations = {
+ .llseek = mem_lseek,
+ .read = mem_read,
+ .write = mem_write,
+ .open = mem_open,
+ .release = mem_release,
+};
+
static int environ_open(struct inode *inode, struct file *file)
{
return __mem_open(inode, file, PTRACE_MODE_READ);
@@ -2916,6 +2929,7 @@ static const struct pid_entry tgid_base_stuff[] = {
REG("numa_maps", S_IRUGO, proc_pid_numa_maps_operations),
#endif
REG("mem", S_IRUSR|S_IWUSR, proc_mem_operations),
+ REG("mem_noforce", S_IRUSR|S_IWUSR, proc_mem_noforce_operations),
LNK("cwd", proc_cwd_link),
LNK("root", proc_root_link),
LNK("exe", proc_exe_link),
@@ -3302,6 +3316,7 @@ static const struct pid_entry tid_base_stuff[] = {
REG("numa_maps", S_IRUGO, proc_tid_numa_maps_operations),
#endif
REG("mem", S_IRUSR|S_IWUSR, proc_mem_operations),
+ REG("mem_noforce",S_IRUSR|S_IWUSR, proc_mem_noforce_operations),
LNK("cwd", proc_cwd_link),
LNK("root", proc_root_link),
LNK("exe", proc_exe_link),
^ permalink raw reply related
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-06-22 21:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tycho Andersen, Jann Horn, kernel list, Linux Containers,
Linux API, Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Tobin C. Harding
In-Reply-To: <E8029AD7-7009-4CBF-911E-186328237176@amacapital.net>
On Fri, Jun 22, 2018 at 11:09 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> One possible extra issue: IIRC /proc/.../mem uses FOLL_FORCE, which is not what we want here.
>
> How about just adding an explicit “read/write the seccomp-trapped task’s memory” primitive? That should be easier than a “open mem fd” primitive.
Uuugh. Can we avoid adding another "read/write remote process memory"
interface? The point of this series was to provide a lightweight
approach to what should normally be possible via the existing
seccomp+ptrace interface. I do like Jann's context idea, but I agree
with Andy: it can't be a handle to /proc/$pid/mem, since it's
FOLL_FORCE. Is there any other kind of process context id we can use
for this instead of pid? There was once an idea of pid-fd but it never
landed... This would let us get rid of the "id" in the structure too.
And if that existed, we could make process_vm_*v() safer too (taking a
pid-fd instead of a pid).
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH v2 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: Davidlohr Bueso @ 2018-06-22 18:35 UTC (permalink / raw)
To: akpm, torvalds
Cc: tgraf, herbert, manfred, mhocko, guillaume.knispel, linux-api,
linux-kernel, Davidlohr Bueso, neilb
In-Reply-To: <20180622181540.5gul4lx5dteqzzk3@linux-r8p5>
On Fri, 22 Jun 2018, Davidlohr Bueso wrote:
>This slightly changes the gfp flags passed on to nested_table_alloc() as it will now
>also use GFP_ATOMIC | __GFP_NOWARN. However, I consider this a positive consequence
>as for the same reasons we want nowarn semantics in bucket_table_alloc().
If this is not acceptable, we can just keep the caller's current semantics - the
atomic flag could also be labeled 'rehash' or something considering that it comes
only from insert_rehash() when we get EAGAIN after trying to insert the first time:
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 9427b5766134..18740b052aec 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -172,17 +172,15 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
{
struct bucket_table *tbl = NULL;
size_t size, max_locks;
+ bool atomic = (gfp == GFP_ATOMIC);
int i;
size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
- if (gfp != GFP_KERNEL)
- tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
- else
- tbl = kvzalloc(size, gfp);
+ tbl = kvzalloc(size, atomic ? gfp | __GFP_NOWARN : gfp);
size = nbuckets;
- if (tbl == NULL && gfp != GFP_KERNEL) {
+ if (tbl == NULL && atomic) {
tbl = nested_bucket_table_alloc(ht, nbuckets, gfp);
nbuckets = 0;
}
^ permalink raw reply related
* Re: [PATCH v4 4/4] seccomp: add support for passing fds via USER_NOTIF
From: Andy Lutomirski @ 2018-06-22 18:21 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, kernel list, containers, Linux API,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez0HW-nScxn4G5p8UHtYy=T435ZkF3Tb1ARTyyijt_cNEg@mail.gmail.com>
> On Jun 22, 2018, at 9:23 AM, Jann Horn <jannh@google.com> wrote:
>
>> On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
>>
>> The idea here is that the userspace handler should be able to pass an fd
>> back to the trapped task, for example so it can be returned from socket().
>>
>> I've proposed one API here, but I'm open to other options. In particular,
>> this only lets you return an fd from a syscall, which may not be enough in
>> all cases. For example, if an fd is written to an output parameter instead
>> of returned, the current API can't handle this. Another case is that
>> netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
>> ever decides to install an fd and output it, we wouldn't be able to handle
>> this either.
>>
>> Still, the vast majority of interesting cases are covered by this API, so
>> perhaps it is Enough.
>>
>> I've left it as a separate commit for two reasons:
>> * It illustrates the way in which we would grow struct seccomp_notif and
>> struct seccomp_notif_resp without using netlink
>> * It shows just how little code is needed to accomplish this :)
>>
> [...]
>> @@ -1669,10 +1706,20 @@ static ssize_t seccomp_notify_write(struct file *file, const char __user *buf,
>> goto out;
>> }
>>
>> + if (resp.return_fd) {
>> + knotif->flags = resp.fd_flags;
>> + knotif->file = fget(resp.fd);
>> + if (!knotif->file) {
>> + ret = -EBADF;
>> + goto out;
>> + }
>> + }
>> +
>
> I think this is a security bug. Imagine the following scenario:
>
> - attacker creates processes A and B
> - process A installs a seccomp filter and sends the notification fd
> to process B
> - process A starts a syscall for which the filter returns
> SECCOMP_RET_USER_NOTIF
> - process B reads the notification from the notification fd
> - process B uses dup2() to copy the notification fd to file
> descriptor 1 (stdout)
> - process B executes a setuid root binary
> - the setuid root binary opens some privileged file descriptor
> (something like open("/etc/shadow", O_RDWR))
> - the setuid root binary tries to write some attacker-controlled data to stdout
> - seccomp_notify_write() interprets the start of the written data as
> a struct seccomp_notif_resp
> - seccomp_notify_write() grabs the privileged file descriptor and
> installs a copy in process A
> - process A now has access to the privileged file (e.g. /etc/shadow)
>
> It isn't clear whether it would actually be exploitable - you'd need a
> setuid binary that performs the right actions - but it's still bad.
Jann is right. ->read and ->write must not reference any of the calling task’s state except the literal memory passed in.
>
> Unless I'm missing something, can you please turn the ->read and
> ->write handlers into an ->unlocked_ioctl handler? Something like
> this:
>
> struct seccomp_user_notif_args {
> u64 buf;
> u64 size;
> };
>
> static long unlocked_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> struct seccomp_user_notif_args args;
> struct seccomp_user_notif_args __user *uargs;
>
> if (cmd != SECCOMP_USER_NOTIF_READ && cmd != SECCOMP_USER_NOTIF_WRITE)
> return -EINVAL;
>
> if (copy_from_user(&args, uargs, sizeof(args)))
> return -EFAULT;
>
> switch (cmd) {
> case SECCOMP_USER_NOTIF_READ:
> return seccomp_notify_read(file, (char __user
> *)args.buf, (size_t)args.size);
> case SECCOMP_USER_NOTIF_WRITE:
> return seccomp_notify_write(file, (char __user
> *)args.buf, (size_t)args.size);
> default:
> return -EINVAL;
> }
> }
^ permalink raw reply
* Re: [PATCH v2 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: Davidlohr Bueso @ 2018-06-22 18:16 UTC (permalink / raw)
To: akpm, torvalds
Cc: tgraf, herbert, manfred, mhocko, guillaume.knispel, linux-api,
linux-kernel, Davidlohr Bueso, neilb
In-Reply-To: <20180622181540.5gul4lx5dteqzzk3@linux-r8p5>
Cc'ing Neil.
On Fri, 22 Jun 2018, Davidlohr Bueso wrote:
>As of ce91f6ee5b3 (mm: kvmalloc does not fallback to vmalloc for incompatible gfp flag),
>we can simplify the caller and trust kvzalloc() to just do the right thing. For the
>case of the GFP_ATOMIC context, we can drop the __GFP_NORETRY flag for obvious reasons,
>and for the __GFP_NOWARN case, however, it is changed such that the caller passes the
>flag instead of making bucket_table_alloc() handle it.
>
>This slightly changes the gfp flags passed on to nested_table_alloc() as it will now
>also use GFP_ATOMIC | __GFP_NOWARN. However, I consider this a positive consequence
>as for the same reasons we want nowarn semantics in bucket_table_alloc().
>
>Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>---
>
>v2:
>- Changes based on Neil's concerns about keeping nowarn flag.
>- Better changelog.
>
>
>lib/rhashtable.c | 7 ++-----
>1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/lib/rhashtable.c b/lib/rhashtable.c
>index 9427b5766134..083f871491a1 100644
>--- a/lib/rhashtable.c
>+++ b/lib/rhashtable.c
>@@ -175,10 +175,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
> int i;
>
> size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
>- if (gfp != GFP_KERNEL)
>- tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
>- else
>- tbl = kvzalloc(size, gfp);
>+ tbl = kvzalloc(size, gfp);
>
> size = nbuckets;
>
>@@ -459,7 +456,7 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
>
> err = -ENOMEM;
>
>- new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);
>+ new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC | __GFP_NOWARN);
> if (new_tbl == NULL)
> goto fail;
>
>--
>2.16.4
>
^ permalink raw reply
* [PATCH v2 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: Davidlohr Bueso @ 2018-06-22 18:15 UTC (permalink / raw)
To: akpm, torvalds
Cc: tgraf, herbert, manfred, mhocko, guillaume.knispel, linux-api,
linux-kernel, Davidlohr Bueso
In-Reply-To: <20180621212825.3059-2-dave@stgolabs.net>
As of ce91f6ee5b3 (mm: kvmalloc does not fallback to vmalloc for incompatible gfp flag),
we can simplify the caller and trust kvzalloc() to just do the right thing. For the
case of the GFP_ATOMIC context, we can drop the __GFP_NORETRY flag for obvious reasons,
and for the __GFP_NOWARN case, however, it is changed such that the caller passes the
flag instead of making bucket_table_alloc() handle it.
This slightly changes the gfp flags passed on to nested_table_alloc() as it will now
also use GFP_ATOMIC | __GFP_NOWARN. However, I consider this a positive consequence
as for the same reasons we want nowarn semantics in bucket_table_alloc().
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
v2:
- Changes based on Neil's concerns about keeping nowarn flag.
- Better changelog.
lib/rhashtable.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 9427b5766134..083f871491a1 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -175,10 +175,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
int i;
size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
- if (gfp != GFP_KERNEL)
- tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
- else
- tbl = kvzalloc(size, gfp);
+ tbl = kvzalloc(size, gfp);
size = nbuckets;
@@ -459,7 +456,7 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
err = -ENOMEM;
- new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);
+ new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC | __GFP_NOWARN);
if (new_tbl == NULL)
goto fail;
--
2.16.4
^ permalink raw reply related
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Andy Lutomirski @ 2018-06-22 18:09 UTC (permalink / raw)
To: Tycho Andersen
Cc: Jann Horn, Kees Cook, kernel list, containers, Linux API,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180622151514.GM3992@cisco>
> On Jun 22, 2018, at 8:15 AM, Tycho Andersen <tycho@tycho.ws> wrote:
>
> Hi Jann,
>
>> On Fri, Jun 22, 2018 at 04:40:20PM +0200, Jann Horn wrote:
>>> On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
>>> This patch introduces a means for syscalls matched in seccomp to notify
>>> some other task that a particular filter has been triggered.
>>>
>>> The motivation for this is primarily for use with containers. For example,
>>> if a container does an init_module(), we obviously don't want to load this
>>> untrusted code, which may be compiled for the wrong version of the kernel
>>> anyway. Instead, we could parse the module image, figure out which module
>>> the container is trying to load and load it on the host.
>>>
>>> As another example, containers cannot mknod(), since this checks
>>> capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
>>> /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
>>> coding some whitelist in the kernel. Another example is mount(), which has
>>> many security restrictions for good reason, but configuration or runtime
>>> knowledge could potentially be used to relax these restrictions.
>>>
>>> This patch adds functionality that is already possible via at least two
>>> other means that I know about, both of which involve ptrace(): first, one
>>> could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
>>> Unfortunately this is slow, so a faster version would be to install a
>>> filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
>>> Since ptrace allows only one tracer, if the container runtime is that
>>> tracer, users inside the container (or outside) trying to debug it will not
>>> be able to use ptrace, which is annoying. It also means that older
>>> distributions based on Upstart cannot boot inside containers using ptrace,
>>> since upstart itself uses ptrace to start services.
>>>
>>> The actual implementation of this is fairly small, although getting the
>>> synchronization right was/is slightly complex.
>>>
>>> Finally, it's worth noting that the classic seccomp TOCTOU of reading
>>> memory data from the task still applies here, but can be avoided with
>>> careful design of the userspace handler: if the userspace handler reads all
>>> of the task memory that is necessary before applying its security policy,
>>> the tracee's subsequent memory edits will not be read by the tracer.
>>
>> I've been thinking about how one would actually write userspace code
>> that uses this API, and whether PID reuse is an issue here. As far as
>> I can tell, the following situation can happen:
>>
>> - seccomped process tries to perform a syscall that gets trapped
>> - notification is sent to the supervisor
>> - supervisor reads the notification
>> - seccomped process gets SIGKILLed
>> - new process appears with the PID that the seccomped process had
>> - supervisor tries to access memory of the seccomped process via
>> process_vm_{read,write}v or /proc/$pid/mem
>> - supervisor unintentionally accesses memory of the new process instead
>>
>> This could have particularly nasty consequences if the supervisor has
>> to write to memory of the seccomped process for some reason.
>> It might make sense to explicitly document how the API has to be used
>> to avoid such a scenario from occuring. AFAICS,
>> process_vm_{read,write}v are fundamentally unsafe for this;
>> /proc/$pid/mem might be safe if you do the following dance in the
>> supervisor to validate that you have a reference to the right struct
>> mm before starting to actually access memory:
>>
>> - supervisor reads a syscall notification for the seccomped process with PID $A
>> - supervisor opens /proc/$A/mem [taking a reference on the mm of the
>> process that currently has PID $A]
>> - supervisor reads all pending events from the notification FD; if
>> one of them says that PID $A was signalled, send back -ERESTARTSYS (or
>> -ERESTARTNOINTR?) and bail out
>> - [at this point, the open FD to /proc/$A/mem is known to actually
>> refer to the mm struct of the seccomped process]
>> - read and write on the open FD to /proc/$A/mem as necessary
>> - send back the syscall result
>
> Yes, this is a nasty problem :(. We have the id in the
> request/response structs to avoid this race, so perhaps we can re-use
> that? So it would look like:
>
> - supervisor gets syscall notification for $A
> - supervisor opens /proc/$A/mem or /proc/$A/map_files/... or a dir fd
> to the container's root or whatever
> - supervisor calls seccomp(SECCOMP_NOTIFICATION_IS_VALID, req->id, listener_fd)
> - supervisor knows that the fds it has open are safe
>
> That way it doesn't have to flush the whole queue? Of course this
> makes things a lot slower, but it does enable safety for more than
> just memory accesses, and also isn't required for things which
> wouldn't read memory.
>
>> It might be nice if the kernel was able to directly give the
>> supervisor an FD to /proc/$A/mem that is guaranteed to point to the
>> right struct mm, but trying to implement that would probably make this
>> patch set significantly larger?
>
> I'll take a look and see how big it is, it doesn't *seem* like it
> should be that hard. Famous last words :)
One possible extra issue: IIRC /proc/.../mem uses FOLL_FORCE, which is not what we want here.
How about just adding an explicit “read/write the seccomp-trapped task’s memory” primitive? That should be easier than a “open mem fd” primitive.
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Eric W. Biederman @ 2018-06-22 17:45 UTC (permalink / raw)
To: Ingo Molnar
Cc: Arnd Bergmann, y2038 Mailman List, Linux Kernel Mailing List,
the arch/x86 maintainers, Linux API, linux-arch, Paul Eggert,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Al Viro,
Dominik Brodowski, Thomas Gleixner, Andrew Morton, linux-alpha,
Deepa Dinamani
In-Reply-To: <20180622021636.GA11266@gmail.com>
Ingo Molnar <mingo@kernel.org> writes:
> * Arnd Bergmann <arnd@arndb.de> wrote:
>
>> However, the other question that has to be asked then is whether
>> there is anything wrong with wait4()/waitid() and getrusuage() that
>> we want to change beyond the time value passing. We have
>> answered a similar question with 'yes' for stat(), which has led
>> to the introduction of statx(),
>
> So we are thinking about adding wait5() in essence, right?
> One thing we might want to look into whether the wait4() and waitid() ABIs could
> be 'merged', by making wait4() essentially a natural special case of
> waitid().
Essentially waitid(2) not waitid(3) has already seen this merger.
In that there is nothing to wait for that you can not already
expression with waitid. status vs siginfo is a little different
but the information is encoded in both.
And waitid(2) optionally returns a struct rusage.
> This would mean that the only new system call we'd have to add is waitid2() in
> essence, which would solve both the rusage layout problem and would offer a
> unified ABI.
>
> If that makes sense (it might not!!), then I'd also modernize waitid2() by making
> it attribute structure based, have a length field and make the ABI extensible from
> now on going forward without having to introduce a new syscall variant every time
> we come up with something new...
The only part where something is not parameterized in waitid is with the
return of rusage.
What to wait for takes an explicit type parameter.
What is being returned in siginfo returns an si_code to describe how
to decode it.
If it weren't for the zombie being gone after waitid returns I don't
think it would make any sense to combine getrusage and waitid together
at all.
> I.e. how the perf syscall does ABI extensions: we've had dozens of ABI extensions,
> some of them pretty complex, and not a single time did we have to modify glibc and
> tooling was able to adapt quickly yet in a both backwards and forwards compatible
> fashion.
>
> Another, simpler example is the new sys_sched_setattr() syscall, that too is using
> the perf_copy_attr() ABI method, via sched_copy_attr(). (With a minor
> compatibility quirk of SCHED_ATTR_SIZE_VER0 that a new wait ABI wouldn't have to
> do - i.e. it could be made even simpler.)
>
> This way we only have:
>
> SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, unsigned int, flags)
>
> But even 'pid' and 'flags' could have been part of the attribute, i.e. one we pick
> up an attribute structure from user-space we can have really low argument count
> system calls. This also concentrates all the compat concerns into handling the
> attribute structure properly - no weird per-arch artifacts and quirks with 4-5-6
> system call arguments.
The trouble with attributes is that means you can't filter your system
call arguments with seccomp. Which most of the time is a pretty big
downside.
>From what I have seen the only truly interesting case for extending
waitid is something file descriptor based so the parent/child
relationship is not necessary to wait for a process to terminate.
As for getrusage. If a sane union of the rusage limits and cgroups or
something like cgroups could be devised. That would be ideal. Of
course except for the memory cgroups the similarity to the resource
usage measurments and limits really isn't there. So I don't know if
merging them would be a real possibility.
So I suspect the simplest thing to do would be to set a flag in the
idtype member of waitid that says give me rusage64 and then we would
be done. Alternately we could use the low bits of the resource usage
pointer. Assuming we don't want to introduce another syscall that is.
I really don't see much incremental extensibility potential in the wait
or rusage interface right now.
Eric
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Jann Horn @ 2018-06-22 16:24 UTC (permalink / raw)
To: tycho
Cc: keescook, linux-kernel, containers, linux-api, luto, oleg,
ebiederm, serge, christian.brauner, tyhicks, suda.akihiro, me
In-Reply-To: <20180622151514.GM3992@cisco>
On Fri, Jun 22, 2018 at 5:15 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> Hi Jann,
>
> On Fri, Jun 22, 2018 at 04:40:20PM +0200, Jann Horn wrote:
> > On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > > This patch introduces a means for syscalls matched in seccomp to notify
> > > some other task that a particular filter has been triggered.
> > >
> > > The motivation for this is primarily for use with containers. For example,
> > > if a container does an init_module(), we obviously don't want to load this
> > > untrusted code, which may be compiled for the wrong version of the kernel
> > > anyway. Instead, we could parse the module image, figure out which module
> > > the container is trying to load and load it on the host.
> > >
> > > As another example, containers cannot mknod(), since this checks
> > > capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
> > > /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
> > > coding some whitelist in the kernel. Another example is mount(), which has
> > > many security restrictions for good reason, but configuration or runtime
> > > knowledge could potentially be used to relax these restrictions.
> > >
> > > This patch adds functionality that is already possible via at least two
> > > other means that I know about, both of which involve ptrace(): first, one
> > > could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> > > Unfortunately this is slow, so a faster version would be to install a
> > > filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> > > Since ptrace allows only one tracer, if the container runtime is that
> > > tracer, users inside the container (or outside) trying to debug it will not
> > > be able to use ptrace, which is annoying. It also means that older
> > > distributions based on Upstart cannot boot inside containers using ptrace,
> > > since upstart itself uses ptrace to start services.
> > >
> > > The actual implementation of this is fairly small, although getting the
> > > synchronization right was/is slightly complex.
> > >
> > > Finally, it's worth noting that the classic seccomp TOCTOU of reading
> > > memory data from the task still applies here, but can be avoided with
> > > careful design of the userspace handler: if the userspace handler reads all
> > > of the task memory that is necessary before applying its security policy,
> > > the tracee's subsequent memory edits will not be read by the tracer.
> >
> > I've been thinking about how one would actually write userspace code
> > that uses this API, and whether PID reuse is an issue here. As far as
> > I can tell, the following situation can happen:
> >
> > - seccomped process tries to perform a syscall that gets trapped
> > - notification is sent to the supervisor
> > - supervisor reads the notification
> > - seccomped process gets SIGKILLed
> > - new process appears with the PID that the seccomped process had
> > - supervisor tries to access memory of the seccomped process via
> > process_vm_{read,write}v or /proc/$pid/mem
> > - supervisor unintentionally accesses memory of the new process instead
> >
> > This could have particularly nasty consequences if the supervisor has
> > to write to memory of the seccomped process for some reason.
> > It might make sense to explicitly document how the API has to be used
> > to avoid such a scenario from occuring. AFAICS,
> > process_vm_{read,write}v are fundamentally unsafe for this;
> > /proc/$pid/mem might be safe if you do the following dance in the
> > supervisor to validate that you have a reference to the right struct
> > mm before starting to actually access memory:
> >
> > - supervisor reads a syscall notification for the seccomped process with PID $A
> > - supervisor opens /proc/$A/mem [taking a reference on the mm of the
> > process that currently has PID $A]
> > - supervisor reads all pending events from the notification FD; if
> > one of them says that PID $A was signalled, send back -ERESTARTSYS (or
> > -ERESTARTNOINTR?) and bail out
> > - [at this point, the open FD to /proc/$A/mem is known to actually
> > refer to the mm struct of the seccomped process]
> > - read and write on the open FD to /proc/$A/mem as necessary
> > - send back the syscall result
>
> Yes, this is a nasty problem :(. We have the id in the
> request/response structs to avoid this race, so perhaps we can re-use
> that? So it would look like:
>
> - supervisor gets syscall notification for $A
> - supervisor opens /proc/$A/mem or /proc/$A/map_files/... or a dir fd
> to the container's root or whatever
(or open a dir fd to /proc/$A; then later, you can use openat()
relative to that to open whatever you need)
> - supervisor calls seccomp(SECCOMP_NOTIFICATION_IS_VALID, req->id, listener_fd)
> - supervisor knows that the fds it has open are safe
>
> That way it doesn't have to flush the whole queue? Of course this
> makes things a lot slower, but it does enable safety for more than
> just memory accesses, and also isn't required for things which
> wouldn't read memory.
That sounds good to me. :)
> > It might be nice if the kernel was able to directly give the
> > supervisor an FD to /proc/$A/mem that is guaranteed to point to the
> > right struct mm, but trying to implement that would probably make this
> > patch set significantly larger?
>
> I'll take a look and see how big it is, it doesn't *seem* like it
> should be that hard. Famous last words :)
Good luck. :D
If you do manage to implement this, it might actually make sense to
hand out an O_PATH FD to /proc/$A (or perhaps more accurately,
/proc/$A/task/$A?) instead of an FD to /proc/*/mem. Then you could
safely open whatever files you need from the process' procfs directory
in a race-free manner.
I think you'd have to add some way to tell the kernel in which procfs
instance you want the lookup to happen; so I think you'd need to
supply an FD to the root of a procfs when opening a notification fd,
and then in the read handler, you'd have to perform a lookup in
procfs.
^ permalink raw reply
* Re: [PATCH v4 4/4] seccomp: add support for passing fds via USER_NOTIF
From: Jann Horn @ 2018-06-22 16:23 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180621220416.5412-5-tycho@tycho.ws>
On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
>
> I've proposed one API here, but I'm open to other options. In particular,
> this only lets you return an fd from a syscall, which may not be enough in
> all cases. For example, if an fd is written to an output parameter instead
> of returned, the current API can't handle this. Another case is that
> netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> ever decides to install an fd and output it, we wouldn't be able to handle
> this either.
>
> Still, the vast majority of interesting cases are covered by this API, so
> perhaps it is Enough.
>
> I've left it as a separate commit for two reasons:
> * It illustrates the way in which we would grow struct seccomp_notif and
> struct seccomp_notif_resp without using netlink
> * It shows just how little code is needed to accomplish this :)
>
[...]
> @@ -1669,10 +1706,20 @@ static ssize_t seccomp_notify_write(struct file *file, const char __user *buf,
> goto out;
> }
>
> + if (resp.return_fd) {
> + knotif->flags = resp.fd_flags;
> + knotif->file = fget(resp.fd);
> + if (!knotif->file) {
> + ret = -EBADF;
> + goto out;
> + }
> + }
> +
I think this is a security bug. Imagine the following scenario:
- attacker creates processes A and B
- process A installs a seccomp filter and sends the notification fd
to process B
- process A starts a syscall for which the filter returns
SECCOMP_RET_USER_NOTIF
- process B reads the notification from the notification fd
- process B uses dup2() to copy the notification fd to file
descriptor 1 (stdout)
- process B executes a setuid root binary
- the setuid root binary opens some privileged file descriptor
(something like open("/etc/shadow", O_RDWR))
- the setuid root binary tries to write some attacker-controlled data to stdout
- seccomp_notify_write() interprets the start of the written data as
a struct seccomp_notif_resp
- seccomp_notify_write() grabs the privileged file descriptor and
installs a copy in process A
- process A now has access to the privileged file (e.g. /etc/shadow)
It isn't clear whether it would actually be exploitable - you'd need a
setuid binary that performs the right actions - but it's still bad.
Unless I'm missing something, can you please turn the ->read and
->write handlers into an ->unlocked_ioctl handler? Something like
this:
struct seccomp_user_notif_args {
u64 buf;
u64 size;
};
static long unlocked_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
struct seccomp_user_notif_args args;
struct seccomp_user_notif_args __user *uargs;
if (cmd != SECCOMP_USER_NOTIF_READ && cmd != SECCOMP_USER_NOTIF_WRITE)
return -EINVAL;
if (copy_from_user(&args, uargs, sizeof(args)))
return -EFAULT;
switch (cmd) {
case SECCOMP_USER_NOTIF_READ:
return seccomp_notify_read(file, (char __user
*)args.buf, (size_t)args.size);
case SECCOMP_USER_NOTIF_WRITE:
return seccomp_notify_write(file, (char __user
*)args.buf, (size_t)args.size);
default:
return -EINVAL;
}
}
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-06-22 15:15 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez3Ek_KG54ejR=Q=XtW_HDs8hQ+cgFODzn4rQ0nVDVpODg@mail.gmail.com>
Hi Jann,
On Fri, Jun 22, 2018 at 04:40:20PM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > This patch introduces a means for syscalls matched in seccomp to notify
> > some other task that a particular filter has been triggered.
> >
> > The motivation for this is primarily for use with containers. For example,
> > if a container does an init_module(), we obviously don't want to load this
> > untrusted code, which may be compiled for the wrong version of the kernel
> > anyway. Instead, we could parse the module image, figure out which module
> > the container is trying to load and load it on the host.
> >
> > As another example, containers cannot mknod(), since this checks
> > capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
> > /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
> > coding some whitelist in the kernel. Another example is mount(), which has
> > many security restrictions for good reason, but configuration or runtime
> > knowledge could potentially be used to relax these restrictions.
> >
> > This patch adds functionality that is already possible via at least two
> > other means that I know about, both of which involve ptrace(): first, one
> > could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> > Unfortunately this is slow, so a faster version would be to install a
> > filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> > Since ptrace allows only one tracer, if the container runtime is that
> > tracer, users inside the container (or outside) trying to debug it will not
> > be able to use ptrace, which is annoying. It also means that older
> > distributions based on Upstart cannot boot inside containers using ptrace,
> > since upstart itself uses ptrace to start services.
> >
> > The actual implementation of this is fairly small, although getting the
> > synchronization right was/is slightly complex.
> >
> > Finally, it's worth noting that the classic seccomp TOCTOU of reading
> > memory data from the task still applies here, but can be avoided with
> > careful design of the userspace handler: if the userspace handler reads all
> > of the task memory that is necessary before applying its security policy,
> > the tracee's subsequent memory edits will not be read by the tracer.
>
> I've been thinking about how one would actually write userspace code
> that uses this API, and whether PID reuse is an issue here. As far as
> I can tell, the following situation can happen:
>
> - seccomped process tries to perform a syscall that gets trapped
> - notification is sent to the supervisor
> - supervisor reads the notification
> - seccomped process gets SIGKILLed
> - new process appears with the PID that the seccomped process had
> - supervisor tries to access memory of the seccomped process via
> process_vm_{read,write}v or /proc/$pid/mem
> - supervisor unintentionally accesses memory of the new process instead
>
> This could have particularly nasty consequences if the supervisor has
> to write to memory of the seccomped process for some reason.
> It might make sense to explicitly document how the API has to be used
> to avoid such a scenario from occuring. AFAICS,
> process_vm_{read,write}v are fundamentally unsafe for this;
> /proc/$pid/mem might be safe if you do the following dance in the
> supervisor to validate that you have a reference to the right struct
> mm before starting to actually access memory:
>
> - supervisor reads a syscall notification for the seccomped process with PID $A
> - supervisor opens /proc/$A/mem [taking a reference on the mm of the
> process that currently has PID $A]
> - supervisor reads all pending events from the notification FD; if
> one of them says that PID $A was signalled, send back -ERESTARTSYS (or
> -ERESTARTNOINTR?) and bail out
> - [at this point, the open FD to /proc/$A/mem is known to actually
> refer to the mm struct of the seccomped process]
> - read and write on the open FD to /proc/$A/mem as necessary
> - send back the syscall result
Yes, this is a nasty problem :(. We have the id in the
request/response structs to avoid this race, so perhaps we can re-use
that? So it would look like:
- supervisor gets syscall notification for $A
- supervisor opens /proc/$A/mem or /proc/$A/map_files/... or a dir fd
to the container's root or whatever
- supervisor calls seccomp(SECCOMP_NOTIFICATION_IS_VALID, req->id, listener_fd)
- supervisor knows that the fds it has open are safe
That way it doesn't have to flush the whole queue? Of course this
makes things a lot slower, but it does enable safety for more than
just memory accesses, and also isn't required for things which
wouldn't read memory.
> It might be nice if the kernel was able to directly give the
> supervisor an FD to /proc/$A/mem that is guaranteed to point to the
> right struct mm, but trying to implement that would probably make this
> patch set significantly larger?
I'll take a look and see how big it is, it doesn't *seem* like it
should be that hard. Famous last words :)
Tycho
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Jann Horn @ 2018-06-22 14:40 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180621220416.5412-2-tycho@tycho.ws>
On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> This patch introduces a means for syscalls matched in seccomp to notify
> some other task that a particular filter has been triggered.
>
> The motivation for this is primarily for use with containers. For example,
> if a container does an init_module(), we obviously don't want to load this
> untrusted code, which may be compiled for the wrong version of the kernel
> anyway. Instead, we could parse the module image, figure out which module
> the container is trying to load and load it on the host.
>
> As another example, containers cannot mknod(), since this checks
> capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
> /dev/zero should be ok for containers to mknod, but we'd like to avoid hard
> coding some whitelist in the kernel. Another example is mount(), which has
> many security restrictions for good reason, but configuration or runtime
> knowledge could potentially be used to relax these restrictions.
>
> This patch adds functionality that is already possible via at least two
> other means that I know about, both of which involve ptrace(): first, one
> could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
> Unfortunately this is slow, so a faster version would be to install a
> filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
> Since ptrace allows only one tracer, if the container runtime is that
> tracer, users inside the container (or outside) trying to debug it will not
> be able to use ptrace, which is annoying. It also means that older
> distributions based on Upstart cannot boot inside containers using ptrace,
> since upstart itself uses ptrace to start services.
>
> The actual implementation of this is fairly small, although getting the
> synchronization right was/is slightly complex.
>
> Finally, it's worth noting that the classic seccomp TOCTOU of reading
> memory data from the task still applies here, but can be avoided with
> careful design of the userspace handler: if the userspace handler reads all
> of the task memory that is necessary before applying its security policy,
> the tracee's subsequent memory edits will not be read by the tracer.
I've been thinking about how one would actually write userspace code
that uses this API, and whether PID reuse is an issue here. As far as
I can tell, the following situation can happen:
- seccomped process tries to perform a syscall that gets trapped
- notification is sent to the supervisor
- supervisor reads the notification
- seccomped process gets SIGKILLed
- new process appears with the PID that the seccomped process had
- supervisor tries to access memory of the seccomped process via
process_vm_{read,write}v or /proc/$pid/mem
- supervisor unintentionally accesses memory of the new process instead
This could have particularly nasty consequences if the supervisor has
to write to memory of the seccomped process for some reason.
It might make sense to explicitly document how the API has to be used
to avoid such a scenario from occuring. AFAICS,
process_vm_{read,write}v are fundamentally unsafe for this;
/proc/$pid/mem might be safe if you do the following dance in the
supervisor to validate that you have a reference to the right struct
mm before starting to actually access memory:
- supervisor reads a syscall notification for the seccomped process with PID $A
- supervisor opens /proc/$A/mem [taking a reference on the mm of the
process that currently has PID $A]
- supervisor reads all pending events from the notification FD; if
one of them says that PID $A was signalled, send back -ERESTARTSYS (or
-ERESTARTNOINTR?) and bail out
- [at this point, the open FD to /proc/$A/mem is known to actually
refer to the mm struct of the seccomped process]
- read and write on the open FD to /proc/$A/mem as necessary
- send back the syscall result
It might be nice if the kernel was able to directly give the
supervisor an FD to /proc/$A/mem that is guaranteed to point to the
right struct mm, but trying to implement that would probably make this
patch set significantly larger?
^ permalink raw reply
* Re: [PATCH 2/4] lib/rhashtable: guarantee initial hashtable allocation
From: Herbert Xu @ 2018-06-22 6:54 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: akpm, torvalds, tgraf, manfred, mhocko, guillaume.knispel,
linux-api, linux-kernel, Davidlohr Bueso
In-Reply-To: <20180621212825.3059-3-dave@stgolabs.net>
On Thu, Jun 21, 2018 at 02:28:23PM -0700, Davidlohr Bueso wrote:
> rhashtable_init() may fail due to -ENOMEM, thus making the
> entire api unusable. This patch removes this scenario,
> however unlikely. In order to guarantee memory allocation,
> this patch always ends up doing GFP_KERNEL|__GFP_NOFAIL
> for both the tbl as well as alloc_bucket_spinlocks().
>
> Upon the first table allocation failure, we shrink the
> size to the smallest value that makes sense and retry with
> __GFP_NOFAIL semantics. With the defaults, this means that
> from 64 buckets, we retry with only 4. Any later issues
> regarding performance due to collisions or larger table
> resizing (when more memory becomes available) is the least
> of our problems.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: Davidlohr Bueso @ 2018-06-22 6:36 UTC (permalink / raw)
To: NeilBrown
Cc: akpm, torvalds, tgraf, herbert, manfred, mhocko,
guillaume.knispel, linux-api, linux-kernel, Davidlohr Bueso
In-Reply-To: <87sh5fbbma.fsf@notabene.neil.brown.name>
[-- Attachment #1: Type: text/plain, Size: 804 bytes --]
On Fri, 22 Jun 2018, NeilBrown wrote:
>On Thu, Jun 21 2018, Davidlohr Bueso wrote:
>
>> As of ce91f6ee5 (mm: kvmalloc does not fallback to vmalloc for incompatible gfp flag),
>> we can simplify the caller and trust kvzalloc() to just do the right thing.
>
>Hi,
> it isn't clear to me that this is true.
> With this change we lose __GFP_NOWARN and __GFP_NORETRY.
> I doubt the NORETRY is particularly important as this is if it
> isn't GFP_KERNEL, then it is GFP_ATOMIC which doesn't retry anyway.
> However I cannot see why this patch won't result in warnings when the
> kzalloc() fails.
> What am I missing?
You're right, it might be too agressive to get rid of the GFP_NOWARN for
the callers that do GFP_ATOMIC.
I'll send a new version of this patch along with a better changelog.
Thanks,
Davidlohr
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: NeilBrown @ 2018-06-22 6:04 UTC (permalink / raw)
To: akpm, torvalds
Cc: tgraf, herbert, manfred, mhocko, guillaume.knispel, linux-api,
linux-kernel, dave, Davidlohr Bueso
In-Reply-To: <20180621212825.3059-2-dave@stgolabs.net>
[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]
On Thu, Jun 21 2018, Davidlohr Bueso wrote:
> As of ce91f6ee5 (mm: kvmalloc does not fallback to vmalloc for incompatible gfp flag),
> we can simplify the caller and trust kvzalloc() to just do the right thing.
Hi,
it isn't clear to me that this is true.
With this change we lose __GFP_NOWARN and __GFP_NORETRY.
I doubt the NORETRY is particularly important as this is if it
isn't GFP_KERNEL, then it is GFP_ATOMIC which doesn't retry anyway.
However I cannot see why this patch won't result in warnings when the
kzalloc() fails.
What am I missing?
Thanks,
NeilBrown
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> lib/rhashtable.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 9427b5766134..26c9cd8a985a 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -175,10 +175,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
> int i;
>
> size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
> - if (gfp != GFP_KERNEL)
> - tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
> - else
> - tbl = kvzalloc(size, gfp);
> + tbl = kvzalloc(size, gfp);
>
> size = nbuckets;
>
> --
> 2.16.4
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Ingo Molnar @ 2018-06-22 2:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038 Mailman List, Linux Kernel Mailing List,
the arch/x86 maintainers, Linux API, linux-arch, Paul Eggert,
Eric W . Biederman, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Al Viro, Dominik Brodowski, Thomas Gleixner,
Andrew Morton, linux-alpha, Deepa Dinamani
In-Reply-To: <CAK8P3a1mx16S+DhEMQA6WBJ7efdhN5YaKKVHU6n+E6kfifM-Qg@mail.gmail.com>
* Arnd Bergmann <arnd@arndb.de> wrote:
> However, the other question that has to be asked then is whether
> there is anything wrong with wait4()/waitid() and getrusuage() that
> we want to change beyond the time value passing. We have
> answered a similar question with 'yes' for stat(), which has led
> to the introduction of statx(),
So we are thinking about adding wait5() in essence, right?
One thing we might want to look into whether the wait4() and waitid() ABIs could
be 'merged', by making wait4() essentially a natural special case of waitid().
This would mean that the only new system call we'd have to add is waitid2() in
essence, which would solve both the rusage layout problem and would offer a
unified ABI.
If that makes sense (it might not!!), then I'd also modernize waitid2() by making
it attribute structure based, have a length field and make the ABI extensible from
now on going forward without having to introduce a new syscall variant every time
we come up with something new...
I.e. how the perf syscall does ABI extensions: we've had dozens of ABI extensions,
some of them pretty complex, and not a single time did we have to modify glibc and
tooling was able to adapt quickly yet in a both backwards and forwards compatible
fashion.
Another, simpler example is the new sys_sched_setattr() syscall, that too is using
the perf_copy_attr() ABI method, via sched_copy_attr(). (With a minor
compatibility quirk of SCHED_ATTR_SIZE_VER0 that a new wait ABI wouldn't have to
do - i.e. it could be made even simpler.)
This way we only have:
SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, unsigned int, flags)
But even 'pid' and 'flags' could have been part of the attribute, i.e. one we pick
up an attribute structure from user-space we can have really low argument count
system calls. This also concentrates all the compat concerns into handling the
attribute structure properly - no weird per-arch artifacts and quirks with 4-5-6
system call arguments.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-06-22 1:39 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez1Wob-CrbrPfgP_OBoG7zas4bJwFdLFFQLtgQAkj+HEOQ@mail.gmail.com>
On Fri, Jun 22, 2018 at 03:28:24AM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 2:58 AM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Fri, Jun 22, 2018 at 01:21:47AM +0200, Jann Horn wrote:
> > > On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > > [...]
> > > > +
> > > > +static void seccomp_do_user_notification(int this_syscall,
> > > > + struct seccomp_filter *match,
> > > > + const struct seccomp_data *sd)
> > > > +{
> > > > + int err;
> > > > + long ret = 0;
> > > > + struct seccomp_knotif n = {};
> > > > +
> > > > + mutex_lock(&match->notify_lock);
> > > > + err = -ENOSYS;
> > > > + if (!match->has_listener)
> > > > + goto out;
> > > > +
> > > > + n.pid = task_pid(current);
> > > > + n.state = SECCOMP_NOTIFY_INIT;
> > > > + n.data = sd;
> > > > + n.id = seccomp_next_notify_id(match);
> > > > + init_completion(&n.ready);
> > > > +
> > > > + list_add(&n.list, &match->notifications);
> > > > + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> > > > +
> > > > + mutex_unlock(&match->notify_lock);
> > > > + up(&match->request);
> > > > +
> > > > + err = wait_for_completion_interruptible(&n.ready);
> > > > + mutex_lock(&match->notify_lock);
> > > > +
> > > > + /*
> > > > + * Here it's possible we got a signal and then had to wait on the mutex
> > > > + * while the reply was sent, so let's be sure there wasn't a response
> > > > + * in the meantime.
> > > > + */
> > > > + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> > > > + /*
> > > > + * We got a signal. Let's tell userspace about it (potentially
> > > > + * again, if we had already notified them about the first one).
> > > > + */
> > > > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > > > + n.state = SECCOMP_NOTIFY_INIT;
> > > > + up(&match->request);
> > > > + }
> > > > + mutex_unlock(&match->notify_lock);
> > > > + err = wait_for_completion_killable(&n.ready);
> > >
> > > Does this mean that when you get a signal that isn't SIGKILL,
> > > wait_for_completion_interruptible() will bail out with -ERESTARTSYS,
> > > but then you hang on this wait_for_completion_killable()? I don't
> > > understand what's going on here. What's the point of using
> > > wait_for_completion_interruptible() when you'll just hang on another
> > > wait on the same "struct completion"?
> >
> > This is the implementation of this suggestion by Andy:
> > https://lkml.org/lkml/2018/3/15/1122
> >
> > The idea is to alert the listener that there was a signal exactly
> > once, in case it's in the middle of processing a request it could bail
> > out and do something else. So the killable wait is intended to ignore
> > other (non-fatal) signals after the first one and wait for whatever
> > the handler decides to do with the signal it received.
>
> How can the listener tell that a signal arrived? When the first
> non-fatal signal comes in, you just set the state to
> SECCOMP_NOTIFY_INIT if it was SECCOMP_NOTIFY_SENT, right? So the
> listener will potentially see the request twice, but with no
> additional indicator that a signal arrived? And in particular, if the
> listener doesn't read the request before the signal arrives, it will
> only see the request once, just as if it was a normal request with no
> signals involved?
I was thinking just parsing /proc/pid/status (given that people are
already going to be mapping things in /proc/pid/map_files to read
arguments and stuff, I didn't think too much of it),
> Would it perhaps make sense to add a field to struct seccomp_notif
> that indicates whether the notification is for a normal syscall or a
> canceled syscall?
Sure, I'll add a __u32 signal and set it to the value of the signal if
we got one.
Thanks!
Tycho
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Jann Horn @ 2018-06-22 1:28 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180622005829.GK3992@cisco>
On Fri, Jun 22, 2018 at 2:58 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Fri, Jun 22, 2018 at 01:21:47AM +0200, Jann Horn wrote:
> > On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > >
> > > This patch introduces a means for syscalls matched in seccomp to notify
> > > some other task that a particular filter has been triggered.
> > [...]
> > > +Userspace Notification
> > > +======================
> > > +
> > > +The ``SECCOMP_RET_USER_NOTIF`` return code lets seccomp filters pass a
> > > +particular syscall to userspace to be handled. This may be useful for
> > > +applications like container managers, which whish to intercept particular
> >
> > typo: "wish"
> >
> > [...]
> > > +passed around via ``SCM_RIGHTS`` or similar. Alternativley, a filter fd can be
> >
> > typo: "Alternatively"
> >
> > [...]
> > > +It is worth noting that ``struct seccomp_data`` contains the values of register
> > > +arguments to the syscall, but does not contain pointers to memory. The task's
> > > +memory is accessiable to suitably privileged traces via via ``ptrace()`` or
> >
> > Typo: "accessible"
>
> Thanks!
>
> > [...]
> > > +
> > > +static void seccomp_do_user_notification(int this_syscall,
> > > + struct seccomp_filter *match,
> > > + const struct seccomp_data *sd)
> > > +{
> > > + int err;
> > > + long ret = 0;
> > > + struct seccomp_knotif n = {};
> > > +
> > > + mutex_lock(&match->notify_lock);
> > > + err = -ENOSYS;
> > > + if (!match->has_listener)
> > > + goto out;
> > > +
> > > + n.pid = task_pid(current);
> > > + n.state = SECCOMP_NOTIFY_INIT;
> > > + n.data = sd;
> > > + n.id = seccomp_next_notify_id(match);
> > > + init_completion(&n.ready);
> > > +
> > > + list_add(&n.list, &match->notifications);
> > > + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> > > +
> > > + mutex_unlock(&match->notify_lock);
> > > + up(&match->request);
> > > +
> > > + err = wait_for_completion_interruptible(&n.ready);
> > > + mutex_lock(&match->notify_lock);
> > > +
> > > + /*
> > > + * Here it's possible we got a signal and then had to wait on the mutex
> > > + * while the reply was sent, so let's be sure there wasn't a response
> > > + * in the meantime.
> > > + */
> > > + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> > > + /*
> > > + * We got a signal. Let's tell userspace about it (potentially
> > > + * again, if we had already notified them about the first one).
> > > + */
> > > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > > + n.state = SECCOMP_NOTIFY_INIT;
> > > + up(&match->request);
> > > + }
> > > + mutex_unlock(&match->notify_lock);
> > > + err = wait_for_completion_killable(&n.ready);
> >
> > Does this mean that when you get a signal that isn't SIGKILL,
> > wait_for_completion_interruptible() will bail out with -ERESTARTSYS,
> > but then you hang on this wait_for_completion_killable()? I don't
> > understand what's going on here. What's the point of using
> > wait_for_completion_interruptible() when you'll just hang on another
> > wait on the same "struct completion"?
>
> This is the implementation of this suggestion by Andy:
> https://lkml.org/lkml/2018/3/15/1122
>
> The idea is to alert the listener that there was a signal exactly
> once, in case it's in the middle of processing a request it could bail
> out and do something else. So the killable wait is intended to ignore
> other (non-fatal) signals after the first one and wait for whatever
> the handler decides to do with the signal it received.
How can the listener tell that a signal arrived? When the first
non-fatal signal comes in, you just set the state to
SECCOMP_NOTIFY_INIT if it was SECCOMP_NOTIFY_SENT, right? So the
listener will potentially see the request twice, but with no
additional indicator that a signal arrived? And in particular, if the
listener doesn't read the request before the signal arrives, it will
only see the request once, just as if it was a normal request with no
signals involved?
Would it perhaps make sense to add a field to struct seccomp_notif
that indicates whether the notification is for a normal syscall or a
canceled syscall?
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-06-22 0:58 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez2-vK57HfbRs94YLiz3SVJ7u4tTrxtAmbRepDqk6m-c_A@mail.gmail.com>
On Fri, Jun 22, 2018 at 01:21:47AM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > This patch introduces a means for syscalls matched in seccomp to notify
> > some other task that a particular filter has been triggered.
> [...]
> > +Userspace Notification
> > +======================
> > +
> > +The ``SECCOMP_RET_USER_NOTIF`` return code lets seccomp filters pass a
> > +particular syscall to userspace to be handled. This may be useful for
> > +applications like container managers, which whish to intercept particular
>
> typo: "wish"
>
> [...]
> > +passed around via ``SCM_RIGHTS`` or similar. Alternativley, a filter fd can be
>
> typo: "Alternatively"
>
> [...]
> > +It is worth noting that ``struct seccomp_data`` contains the values of register
> > +arguments to the syscall, but does not contain pointers to memory. The task's
> > +memory is accessiable to suitably privileged traces via via ``ptrace()`` or
>
> Typo: "accessible"
Thanks!
> [...]
> > +
> > +static void seccomp_do_user_notification(int this_syscall,
> > + struct seccomp_filter *match,
> > + const struct seccomp_data *sd)
> > +{
> > + int err;
> > + long ret = 0;
> > + struct seccomp_knotif n = {};
> > +
> > + mutex_lock(&match->notify_lock);
> > + err = -ENOSYS;
> > + if (!match->has_listener)
> > + goto out;
> > +
> > + n.pid = task_pid(current);
> > + n.state = SECCOMP_NOTIFY_INIT;
> > + n.data = sd;
> > + n.id = seccomp_next_notify_id(match);
> > + init_completion(&n.ready);
> > +
> > + list_add(&n.list, &match->notifications);
> > + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> > +
> > + mutex_unlock(&match->notify_lock);
> > + up(&match->request);
> > +
> > + err = wait_for_completion_interruptible(&n.ready);
> > + mutex_lock(&match->notify_lock);
> > +
> > + /*
> > + * Here it's possible we got a signal and then had to wait on the mutex
> > + * while the reply was sent, so let's be sure there wasn't a response
> > + * in the meantime.
> > + */
> > + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> > + /*
> > + * We got a signal. Let's tell userspace about it (potentially
> > + * again, if we had already notified them about the first one).
> > + */
> > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > + n.state = SECCOMP_NOTIFY_INIT;
> > + up(&match->request);
> > + }
> > + mutex_unlock(&match->notify_lock);
> > + err = wait_for_completion_killable(&n.ready);
>
> Does this mean that when you get a signal that isn't SIGKILL,
> wait_for_completion_interruptible() will bail out with -ERESTARTSYS,
> but then you hang on this wait_for_completion_killable()? I don't
> understand what's going on here. What's the point of using
> wait_for_completion_interruptible() when you'll just hang on another
> wait on the same "struct completion"?
This is the implementation of this suggestion by Andy:
https://lkml.org/lkml/2018/3/15/1122
The idea is to alert the listener that there was a signal exactly
once, in case it's in the middle of processing a request it could bail
out and do something else. So the killable wait is intended to ignore
other (non-fatal) signals after the first one and wait for whatever
the handler decides to do with the signal it received.
Tycho
^ permalink raw reply
* Re: [PATCH v4 4/4] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-06-22 0:51 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez0d+tRsfvstU3Kd7T=kkdKbbfpNS+19NLTchhSL1fUyNg@mail.gmail.com>
On Fri, Jun 22, 2018 at 01:34:18AM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > The idea here is that the userspace handler should be able to pass an fd
> > back to the trapped task, for example so it can be returned from socket().
> [...]
> > +Userspace can also return file descriptors. For example, one may decide to
> > +intercept ``socket()`` syscalls, and return some file descriptor from those
> > +based on some policy. To return a file descriptor, the ``return_fd`` member
> > +should be non-zero, the ``fd`` argument should be the fd in the listener's
> > +table to send to the tracee (similar to how ``SCM_RIGHTS`` works), and
> > +``fd_flags`` should be the flags that the fd in the tracee's table is opened
> > +with (e.g. ``O_EXCL`` or similar).
>
> fd_flags only contains file descriptor flags (meaning only O_CLOEXEC).
> O_EXCL is a file creation flag, so setting it here wouldn't make sense.
> Setting file status flags like O_APPEND does make sense, but those are
> stored in the `struct file` and don't need to be passed separately;
> the caller can e.g. set them via fcntl(fd, F_SETFD, flags) or on
> open().
> (The fcntl.2 manpage explains these.)
Ugh, yes, O_CLOEXEC is what I meant. Thanks, I'll clarify.
Tycho
^ permalink raw reply
* Re: [PATCH v4 4/4] seccomp: add support for passing fds via USER_NOTIF
From: Jann Horn @ 2018-06-21 23:34 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180621220416.5412-5-tycho@tycho.ws>
On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
[...]
> +Userspace can also return file descriptors. For example, one may decide to
> +intercept ``socket()`` syscalls, and return some file descriptor from those
> +based on some policy. To return a file descriptor, the ``return_fd`` member
> +should be non-zero, the ``fd`` argument should be the fd in the listener's
> +table to send to the tracee (similar to how ``SCM_RIGHTS`` works), and
> +``fd_flags`` should be the flags that the fd in the tracee's table is opened
> +with (e.g. ``O_EXCL`` or similar).
fd_flags only contains file descriptor flags (meaning only O_CLOEXEC).
O_EXCL is a file creation flag, so setting it here wouldn't make sense.
Setting file status flags like O_APPEND does make sense, but those are
stored in the `struct file` and don't need to be passed separately;
the caller can e.g. set them via fcntl(fd, F_SETFD, flags) or on
open().
(The fcntl.2 manpage explains these.)
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Jann Horn @ 2018-06-21 23:21 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180621220416.5412-2-tycho@tycho.ws>
On Fri, Jun 22, 2018 at 12:05 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> This patch introduces a means for syscalls matched in seccomp to notify
> some other task that a particular filter has been triggered.
[...]
> +Userspace Notification
> +======================
> +
> +The ``SECCOMP_RET_USER_NOTIF`` return code lets seccomp filters pass a
> +particular syscall to userspace to be handled. This may be useful for
> +applications like container managers, which whish to intercept particular
typo: "wish"
[...]
> +passed around via ``SCM_RIGHTS`` or similar. Alternativley, a filter fd can be
typo: "Alternatively"
[...]
> +It is worth noting that ``struct seccomp_data`` contains the values of register
> +arguments to the syscall, but does not contain pointers to memory. The task's
> +memory is accessiable to suitably privileged traces via via ``ptrace()`` or
Typo: "accessible"
[...]
> +
> +static void seccomp_do_user_notification(int this_syscall,
> + struct seccomp_filter *match,
> + const struct seccomp_data *sd)
> +{
> + int err;
> + long ret = 0;
> + struct seccomp_knotif n = {};
> +
> + mutex_lock(&match->notify_lock);
> + err = -ENOSYS;
> + if (!match->has_listener)
> + goto out;
> +
> + n.pid = task_pid(current);
> + n.state = SECCOMP_NOTIFY_INIT;
> + n.data = sd;
> + n.id = seccomp_next_notify_id(match);
> + init_completion(&n.ready);
> +
> + list_add(&n.list, &match->notifications);
> + wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> +
> + mutex_unlock(&match->notify_lock);
> + up(&match->request);
> +
> + err = wait_for_completion_interruptible(&n.ready);
> + mutex_lock(&match->notify_lock);
> +
> + /*
> + * Here it's possible we got a signal and then had to wait on the mutex
> + * while the reply was sent, so let's be sure there wasn't a response
> + * in the meantime.
> + */
> + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> + /*
> + * We got a signal. Let's tell userspace about it (potentially
> + * again, if we had already notified them about the first one).
> + */
> + if (n.state == SECCOMP_NOTIFY_SENT) {
> + n.state = SECCOMP_NOTIFY_INIT;
> + up(&match->request);
> + }
> + mutex_unlock(&match->notify_lock);
> + err = wait_for_completion_killable(&n.ready);
Does this mean that when you get a signal that isn't SIGKILL,
wait_for_completion_interruptible() will bail out with -ERESTARTSYS,
but then you hang on this wait_for_completion_killable()? I don't
understand what's going on here. What's the point of using
wait_for_completion_interruptible() when you'll just hang on another
wait on the same "struct completion"?
> + mutex_lock(&match->notify_lock);
> + if (err < 0)
> + goto remove_list;
> + }
> +
> + ret = n.val;
> + err = n.error;
> +
> +remove_list:
> + list_del(&n.list);
> +out:
> + mutex_unlock(&match->notify_lock);
> + syscall_set_return_value(current, task_pt_regs(current),
> + err, ret);
> +}
^ permalink raw reply
* Re: [PATCH v4 3/4] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-06-21 23:07 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com>
Hi Jann,
On Fri, Jun 22, 2018 at 12:48:09AM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 12:04 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
> > version which can acquire filters is useful. There are at least two reasons
> > this is preferable, even though it uses ptrace:
> >
> > 1. You can control tasks that aren't cooperating with you
> > 2. You can control tasks whose filters block sendmsg() and socket(); if the
> > task installs a filter which blocks these calls, there's no way with
> > SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.
> [...]
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index bbc24938c51d..b68a5d4a15cd 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1743,6 +1743,34 @@ static struct file *init_listener(struct task_struct *task,
> >
> > return ret;
> > }
> > +
> > +long seccomp_new_listener(struct task_struct *task,
> > + unsigned long filter_off)
> > +{
> > + struct seccomp_filter *filter;
> > + struct file *listener;
> > + int fd;
> > +
> > + filter = get_nth_filter(task, filter_off);
> > + if (IS_ERR(filter))
> > + return PTR_ERR(filter);
> > +
> > + fd = get_unused_fd_flags(0);
> > + if (fd < 0) {
> > + __put_seccomp_filter(filter);
> > + return fd;
> > + }
> > +
> > + listener = init_listener(task, task->seccomp.filter);
> > + __put_seccomp_filter(filter);
> > + if (IS_ERR(listener)) {
> > + put_unused_fd(fd);
> > + return PTR_ERR(listener);
> > + }
> > +
> > + fd_install(fd, listener);
> > + return fd;
> > +}
>
> I think there's a security problem here. Imagine the following scenario:
>
> 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> 2. task A forks off a child B
> 3. task B uses setuid(1) to drop its privileges
> 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> or via execve()
> 5. task C (the attacker, uid==1) attaches to task B via ptrace
> 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> 7. because the seccomp filter is shared by task A and task B, task C
> is now able to influence syscall results for syscalls performed by
> task A
>
> Unless I'm missing something, you might have to add some extra
> security check here: Either a check to ensure that no other task is
> using the same seccomp filter, or (as a last resort) a check for
> capable(CAP_SYS_ADMIN).
I guess my first thought is "don't do that". But I am also not opposed
to adding a check for capable(CAP_SYS_ADMIN) to prevent the footgun,
so I can do that for v5. I think checking whether other tasks are
using a filter would be hard without adding some additional counter
logic or something, and at least for the use cases I know of,
capable(CAP_SYS_ADMIN) is fine.
Tycho
^ permalink raw reply
* Re: [PATCH v4 3/4] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-06-21 22:48 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180621220416.5412-4-tycho@tycho.ws>
On Fri, Jun 22, 2018 at 12:04 AM Tycho Andersen <tycho@tycho.ws> wrote:
> As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
> version which can acquire filters is useful. There are at least two reasons
> this is preferable, even though it uses ptrace:
>
> 1. You can control tasks that aren't cooperating with you
> 2. You can control tasks whose filters block sendmsg() and socket(); if the
> task installs a filter which blocks these calls, there's no way with
> SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.
[...]
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index bbc24938c51d..b68a5d4a15cd 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1743,6 +1743,34 @@ static struct file *init_listener(struct task_struct *task,
>
> return ret;
> }
> +
> +long seccomp_new_listener(struct task_struct *task,
> + unsigned long filter_off)
> +{
> + struct seccomp_filter *filter;
> + struct file *listener;
> + int fd;
> +
> + filter = get_nth_filter(task, filter_off);
> + if (IS_ERR(filter))
> + return PTR_ERR(filter);
> +
> + fd = get_unused_fd_flags(0);
> + if (fd < 0) {
> + __put_seccomp_filter(filter);
> + return fd;
> + }
> +
> + listener = init_listener(task, task->seccomp.filter);
> + __put_seccomp_filter(filter);
> + if (IS_ERR(listener)) {
> + put_unused_fd(fd);
> + return PTR_ERR(listener);
> + }
> +
> + fd_install(fd, listener);
> + return fd;
> +}
I think there's a security problem here. Imagine the following scenario:
1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
2. task A forks off a child B
3. task B uses setuid(1) to drop its privileges
4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
or via execve()
5. task C (the attacker, uid==1) attaches to task B via ptrace
6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
7. because the seccomp filter is shared by task A and task B, task C
is now able to influence syscall results for syscalls performed by
task A
Unless I'm missing something, you might have to add some extra
security check here: Either a check to ensure that no other task is
using the same seccomp filter, or (as a last resort) a check for
capable(CAP_SYS_ADMIN).
^ 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