* [PATCH v4 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181128165312.19500-1-lmb@cloudflare.com>
Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 7 +++++--
net/bpf/test_run.c | 15 +++++++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
{
void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
int err = -EFAULT;
+ u32 copy_size = size;
- if (data_out && copy_to_user(data_out, data, size))
+ /* Clamp copy if the user has provided a size hint, but copy the full
+ * buffer if not to retain old behaviour.
+ */
+ if (kattr->test.data_size_out &&
+ copy_size > kattr->test.data_size_out) {
+ copy_size = kattr->test.data_size_out;
+ err = -ENOSPC;
+ }
+
+ if (data_out && copy_to_user(data_out, data, copy_size))
goto out;
if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
goto out;
if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
goto out;
- err = 0;
+ if (err != -ENOSPC)
+ err = 0;
out:
return err;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v4 0/4] Fix unsafe BPF_PROG_TEST_RUN interface
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.
Changes in v4:
* Document bpf_prog_test_run and bpf_prog_test_run_xattr
* Use struct bpf_prog_test_run_attr for return values
Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing
function
Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out
Lorenz Bauer (4):
bpf: respect size hint to BPF_PROG_TEST_RUN if present
tools: sync uapi/linux/bpf.h
libbpf: add bpf_prog_test_run_xattr
selftests: add a test for bpf_prog_test_run_xattr
include/uapi/linux/bpf.h | 7 ++-
net/bpf/test_run.c | 15 ++++++-
tools/include/uapi/linux/bpf.h | 7 ++-
tools/lib/bpf/bpf.c | 23 ++++++++++
tools/lib/bpf/bpf.h | 19 ++++++++
tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
6 files changed, 119 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr
From: Lorenz Bauer @ 2018-11-28 16:52 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, linux-api, Y Song
In-Reply-To: <CAADnVQ+CJsUUG2otaLJjvnNQ55tRx6ANsFUrTWkb-k=Ztw1MRg@mail.gmail.com>
On Wed, 28 Nov 2018 at 05:05, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 26, 2018 at 4:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > That's what I had initially, but that makes re-using test_attr really
> > awkward. Either
> > you need to reset data_out_size before every call because it is used
> > to return the
> > buffer size,
>
> I think that is exactly what the user of the interface would want to do.
> Why would anyone keep reusing the same test_attr on multiple calls
> into the kernel without changing the fields?
Basically, you can only change the input part without having to reset
data_size_out to sizeof(buffer). Not a big deal, I'll change it.
>
> > It also means
> > we can't take a const struct attr, which is contrary to the other
> > xattr functions which
> > already exist.
>
> I don't see an issue with that.
>
> > I think actually inspecting the required size of the output buffer
> > will be a rare
> > occurrence, so making the user jump through the hoop of a pointer doesn't seem
> > too onerous.
>
> I think the opposite is the case.
> If the output buffer is provided the test will be comparing it
> to expected value.
Yeah, I wasn't thinking too hard on this one, sorry. User space needs to check
where the end of the buffer is.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-28 15:23 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Jann Horn, Michael Ellerman, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181128142006.GE30395-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 2056 bytes --]
On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> On 11/28, Dmitry V. Levin wrote:
> > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > On 11/28, Dmitry V. Levin wrote:
> > > >
> > > > +/*
> > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > + * to describe current syscall-stop.
> > > > + *
> > > > + * Values for these constants are chosen so that they do not appear
> > > > + * in task->ptrace_message by other means.
> > > > + */
> > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > >
> > > Again, I do not really understand the comment... Why should we care about
> > > "do not appear in task->ptrace_message by other means" ?
> > >
> > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > numbers, say, 1 and 2?
> > >
> > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > anyway after wait(status).
> >
> > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > during syscall stop is undefined, we need two different ptrace_message
> > values that cannot be set by other ptrace events to enable reliable
> > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > other ptrace events, it would be hard for userspace to find out whether
> > the kernel implements new semantics or not.
>
> Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> returns EIO then it is not implemented?
The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
PTRACE_GETEVENTMSG for syscall stops.
My concern here is the PTRACE_GETEVENTMSG interface itself. If we use
ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
it should have clear semantics.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Eric W. Biederman @ 2018-11-28 15:23 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jürg Billeter, Andrew Morton, Thomas Gleixner, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <20181128144230.GG30395@redhat.com>
Oleg Nesterov <oleg@redhat.com> writes:
> On 11/27, Jürg Billeter wrote:
>>
>> @@ -704,6 +713,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>> struct task_struct *p, *n;
>> LIST_HEAD(dead);
>>
>> + if (group_dead && tsk->signal->kill_descendants_on_exit)
>> + walk_process_tree(tsk, kill_descendant_visitor, NULL);
>
> Well, this is not exactly right, at least this is suboptimal in that
> other sub-threads can too call walk_process_tree(kill_descendant_visitor)
> later for no reason.
Oleg I think I am missing something.
Reading kernel/exit.c I see "group_dead = atomic_dec_and_test(&tsk->signal->live)".
Which seems like enough to ensure exactly one task/thread calls walk_process_tree.
Can you explain what I am missing?
Eric
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Oleg Nesterov @ 2018-11-28 14:42 UTC (permalink / raw)
To: Jürg Billeter
Cc: Andrew Morton, Thomas Gleixner, Eric Biederman, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <20181127225408.7553-2-j@bitron.ch>
On 11/27, Jürg Billeter wrote:
>
> @@ -704,6 +713,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> struct task_struct *p, *n;
> LIST_HEAD(dead);
>
> + if (group_dead && tsk->signal->kill_descendants_on_exit)
> + walk_process_tree(tsk, kill_descendant_visitor, NULL);
Well, this is not exactly right, at least this is suboptimal in that
other sub-threads can too call walk_process_tree(kill_descendant_visitor)
later for no reason.
^ permalink raw reply
* Re: [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Oleg Nesterov @ 2018-11-28 14:29 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
Michael Ellerman, linux-api, linux-kernel, strace-devel
In-Reply-To: <20181128141049.GD30395@redhat.com>
On 11/28, Oleg Nesterov wrote:
>
> On 11/28, Dmitry V. Levin wrote:
> >
> > +static unsigned long
> > +ptrace_get_syscall_info_entry(struct task_struct *child,
> > + struct ptrace_syscall_info *info)
> > +{
> > + struct pt_regs *regs = task_pt_regs(child);
> > + unsigned long args[ARRAY_SIZE(info->entry.args)];
> > + int i;
> > +
> > + info->op = PTRACE_SYSCALL_INFO_ENTRY;
> > + info->arch = syscall_get_arch(child);
> > + info->entry.nr = syscall_get_nr(child, regs);
> > + info->entry.instruction_pointer = instruction_pointer(regs);
> > + info->entry.stack_pointer = user_stack_pointer(regs);
> > + info->entry.frame_pointer = frame_pointer(regs);
> > + syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> > + for (i = 0; i < ARRAY_SIZE(args); i++)
> > + info->entry.args[i] = args[i];
>
> I must have missed something,
Yes ;) I didn't look at info->entry.args, it is __u64[6].
> but why do we need the temporary args[],
> syscall_get_arguments(..., info->entry.args) should equally work?
>
> Oleg.
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Oleg Nesterov @ 2018-11-28 14:20 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
Andy Lutomirski, linux-api, Ingo Molnar, strace-devel
In-Reply-To: <20181128140533.GF28206@altlinux.org>
On 11/28, Dmitry V. Levin wrote:
>
> On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > On 11/28, Dmitry V. Levin wrote:
> > >
> > > +/*
> > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > + * to describe current syscall-stop.
> > > + *
> > > + * Values for these constants are chosen so that they do not appear
> > > + * in task->ptrace_message by other means.
> > > + */
> > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> >
> > Again, I do not really understand the comment... Why should we care about
> > "do not appear in task->ptrace_message by other means" ?
> >
> > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > numbers, say, 1 and 2?
> >
> > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > anyway after wait(status).
>
> Given that without this patch the value returned by PTRACE_GETEVENTMSG
> during syscall stop is undefined, we need two different ptrace_message
> values that cannot be set by other ptrace events to enable reliable
> identification of syscall-enter-stop and syscall-exit-stop in userspace:
> if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> other ptrace events, it would be hard for userspace to find out whether
> the kernel implements new semantics or not.
Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
returns EIO then it is not implemented?
Oleg.
^ permalink raw reply
* Re: [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Oleg Nesterov @ 2018-11-28 14:10 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
Michael Ellerman, linux-api, linux-kernel, strace-devel
In-Reply-To: <20181128130715.GD28206@altlinux.org>
On 11/28, Dmitry V. Levin wrote:
>
> +static unsigned long
> +ptrace_get_syscall_info_entry(struct task_struct *child,
> + struct ptrace_syscall_info *info)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int i;
> +
> + info->op = PTRACE_SYSCALL_INFO_ENTRY;
> + info->arch = syscall_get_arch(child);
> + info->entry.nr = syscall_get_nr(child, regs);
> + info->entry.instruction_pointer = instruction_pointer(regs);
> + info->entry.stack_pointer = user_stack_pointer(regs);
> + info->entry.frame_pointer = frame_pointer(regs);
> + syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> + for (i = 0; i < ARRAY_SIZE(args); i++)
> + info->entry.args[i] = args[i];
I must have missed something, but why do we need the temporary args[],
syscall_get_arguments(..., info->entry.args) should equally work?
Oleg.
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-28 14:05 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
Andy Lutomirski, linux-api, Ingo Molnar, strace-devel
In-Reply-To: <20181128134913.GC30395@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]
On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> On 11/28, Dmitry V. Levin wrote:
> >
> > +/*
> > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > + * to describe current syscall-stop.
> > + *
> > + * Values for these constants are chosen so that they do not appear
> > + * in task->ptrace_message by other means.
> > + */
> > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
>
> Again, I do not really understand the comment... Why should we care about
> "do not appear in task->ptrace_message by other means" ?
>
> 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> numbers, say, 1 and 2?
>
> If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> anyway after wait(status).
Given that without this patch the value returned by PTRACE_GETEVENTMSG
during syscall stop is undefined, we need two different ptrace_message
values that cannot be set by other ptrace events to enable reliable
identification of syscall-enter-stop and syscall-exit-stop in userspace:
if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
other ptrace events, it would be hard for userspace to find out whether
the kernel implements new semantics or not.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-28 14:05 UTC (permalink / raw)
To: christian
Cc: Eric W . Biederman, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andy Lutomirski, Andrew Morton, Oleg Nesterov, cyphar,
Al Viro, Linux FS-devel Mailing List, Linux API,
Daniel Colascione, timmurray, linux-man, Kees Cook
In-Reply-To: <20181120105124.14733-1-christian@brauner.io>
On Tue, Nov 20, 2018 at 11:54 AM Christian Brauner <christian@brauner.io> wrote:
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> fs/proc/base.c | 11 ++-
> fs/proc/internal.h | 5 -
> include/linux/proc_fs.h | 12 +++
> include/linux/syscalls.h | 2 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 127 +++++++++++++++++++++++--
> 8 files changed, 151 insertions(+), 13 deletions(-)
For asm-generic:
Acked-by: Arnd Bergmann <arnd@arndb.de>
I checked that the system call wired up correctly in a way that
works on all architectures using the generic syscall table.
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Oleg Nesterov @ 2018-11-28 13:49 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
Michael Ellerman, linux-api, linux-kernel, strace-devel
In-Reply-To: <20181128130601.GC28206@altlinux.org>
On 11/28, Dmitry V. Levin wrote:
>
> +/*
> + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> + * to describe current syscall-stop.
> + *
> + * Values for these constants are chosen so that they do not appear
> + * in task->ptrace_message by other means.
> + */
> +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
Again, I do not really understand the comment... Why should we care about
"do not appear in task->ptrace_message by other means" ?
2/2 should detect ptrace_report_syscall() case correctly, so we can use any
numbers, say, 1 and 2?
If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
anyway after wait(status).
Oleg.
^ permalink raw reply
* RE: Official Linux system wrapper library?
From: David Laight @ 2018-11-28 13:18 UTC (permalink / raw)
To: 'David Newall', Florian Weimer, Daniel Colascione
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <a50dbc52-b5e8-6684-508d-b96ad8d70d4f@davidnewall.com>
From: David Newall
> Sent: 23 November 2018 14:11
>
> On 24/11/18 12:04 am, Florian Weimer wrote:
> > But socketcall does not exist on all architectures. Neither does
> > getpid, it's called getxpid on some architectures.
> > ...
> > I think it would be a poor approach to expose application developers to
> > these portability issues. We need to abstract over these differences at
> > a certain layer, and applications are too late.
>
> Interesting. I think the opposite. I think exposing the OS's
> interfaces is exactly what a c-library should do. It might also provide
> alternative interfaces that work consistently across different
> platforms, but in addition to, not instead of the OS interface.
Also, it really shouldn't implement broken workarounds for 'missing'
system calls.
At least one C library I've met converted pread() into lseek() and read().
That is just so broken it is better to fail to link or fail at runtime.
Never mind all the fun trying to read CLOCK_MONOTONIC.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [RFC PATCH RESEND v3 3/3] ptrace: add PTRACE_EVENT_SECCOMP support to PTRACE_GET_SYSCALL_INFO
From: Oleg Nesterov @ 2018-11-28 13:13 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Elvira Khabirova, Eugene Syromyatnikov, Steven Rostedt,
Andy Lutomirski, Ingo Molnar, linux-api, linux-kernel,
strace-devel
In-Reply-To: <20181128124411.GA28206@altlinux.org>
On 11/28, Dmitry V. Levin wrote:
>
> What about PTRACE_GETSIGINFO? Can it also be done lockless because
> ptrace_check_attach() has already called ptrace_freeze_traced()?
Yes, only PTRACE_INTERRUPT needs lock_task_sighand() and it can actually
fail because the tracee is not necessarily stopped.
Oleg.
^ permalink raw reply
* [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-11-28 13:07 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: Elvira Khabirova, Eugene Syromyatnikov, Steven Rostedt,
Ingo Molnar, Kees Cook, Jann Horn, Michael Ellerman, linux-api,
linux-kernel, strace-devel
In-Reply-To: <20181128130439.GB28206@altlinux.org>
From: Elvira Khabirova <lineprinter@altlinux.org>
PTRACE_GET_SYSCALL_INFO lets ptracer obtain details of the syscall
the tracee is blocked in. The request succeeds when the tracee is
in a syscall-enter-stop, syscall-exit-stop, or PTRACE_EVENT_SECCOMP,
and fails with -EINVAL otherwise.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable.
Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.
ptrace(2) man page:
long ptrace(enum __ptrace_request request, pid_t pid,
void *addr, void *data);
...
PTRACE_GET_SYSCALL_INFO
Retrieve information about the syscall that caused the stop.
The information is placed into the buffer pointed by "data"
argument, which should be a pointer to a buffer of type
"struct ptrace_syscall_info".
The "addr" argument contains the size of the buffer pointed to
by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
The return value contains the number of bytes available
to be written by the kernel.
If the size of data to be written by the kernel exceeds the size
specified by "addr" argument, the output is truncated.
This operation fails with EINVAL if the tracee is not
in a syscall-enter-stop, a syscall-exit-stop, or
a PTRACE_EVENT_SECCOMP stop.
Co-authored-by: Dmitry V. Levin <ldv@altlinux.org>
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
include/uapi/linux/ptrace.h | 34 +++++++++++
kernel/ptrace.c | 110 +++++++++++++++++++++++++++++++++++-
2 files changed, 143 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index cb138902d042..ef42149e77f4 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,40 @@ struct seccomp_metadata {
__u64 flags; /* Output: filter's flags */
};
+#define PTRACE_GET_SYSCALL_INFO 0x420f
+#define PTRACE_SYSCALL_INFO_ENTRY 0
+#define PTRACE_SYSCALL_INFO_EXIT 1
+#define PTRACE_SYSCALL_INFO_SECCOMP 2
+
+struct ptrace_syscall_info {
+ __u8 op; /* PTRACE_SYSCALL_INFO_* */
+ __u8 __pad0[3];
+ __u32 arch;
+ union {
+ struct {
+ __u64 nr;
+ __u64 instruction_pointer;
+ __u64 stack_pointer;
+ __u64 frame_pointer;
+ __u64 args[6];
+ } entry;
+ struct {
+ __s64 rval;
+ __u8 is_error;
+ __u8 __pad1[7];
+ } exit;
+ struct {
+ __u64 nr;
+ __u64 instruction_pointer;
+ __u64 stack_pointer;
+ __u64 frame_pointer;
+ __u64 args[6];
+ __u32 ret_data;
+ __u8 __pad2[4];
+ } seccomp;
+ };
+};
+
/* Read signals from a shared (process wide) queue */
#define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 80b34dffdfb9..32ff9c97f941 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -30,6 +30,10 @@
#include <linux/cn_proc.h>
#include <linux/compat.h>
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+#include <asm/syscall.h> /* For syscall_get_* */
+#endif
+
/*
* Access another process' address space via ptrace.
* Source/target buffer must be kernel space,
@@ -888,7 +892,105 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
* to ensure no machine forgets it.
*/
EXPORT_SYMBOL_GPL(task_user_regset_view);
-#endif
+
+static unsigned long
+ptrace_get_syscall_info_entry(struct task_struct *child,
+ struct ptrace_syscall_info *info)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+ unsigned long args[ARRAY_SIZE(info->entry.args)];
+ int i;
+
+ info->op = PTRACE_SYSCALL_INFO_ENTRY;
+ info->arch = syscall_get_arch(child);
+ info->entry.nr = syscall_get_nr(child, regs);
+ info->entry.instruction_pointer = instruction_pointer(regs);
+ info->entry.stack_pointer = user_stack_pointer(regs);
+ info->entry.frame_pointer = frame_pointer(regs);
+ syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
+ for (i = 0; i < ARRAY_SIZE(args); i++)
+ info->entry.args[i] = args[i];
+
+ return offsetofend(struct ptrace_syscall_info, entry);
+}
+
+static unsigned long
+ptrace_get_syscall_info_seccomp(struct task_struct *child,
+ struct ptrace_syscall_info *info)
+{
+ /*
+ * As struct ptrace_syscall_info.entry is currently a subset
+ * of struct ptrace_syscall_info.seccomp, it makes sense to
+ * initialize that subset using ptrace_get_syscall_info_entry().
+ * This can be reconsidered in the future if these structures
+ * diverge significantly enough.
+ */
+ ptrace_get_syscall_info_entry(child, info);
+ info->op = PTRACE_SYSCALL_INFO_SECCOMP;
+ info->seccomp.ret_data = child->ptrace_message;
+
+ return offsetofend(struct ptrace_syscall_info, seccomp);
+}
+
+static unsigned long
+ptrace_get_syscall_info_exit(struct task_struct *child,
+ struct ptrace_syscall_info *info)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+
+ info->op = PTRACE_SYSCALL_INFO_EXIT;
+ info->arch = syscall_get_arch(child);
+ info->exit.rval = syscall_get_error(child, regs);
+ info->exit.is_error = !!info->exit.rval;
+ if (!info->exit.is_error)
+ info->exit.rval = syscall_get_return_value(child, regs);
+
+ return offsetofend(struct ptrace_syscall_info, exit);
+}
+
+static int
+ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
+ void __user *datavp)
+{
+ unsigned long actual_size = 0;
+ unsigned long write_size;
+ struct ptrace_syscall_info info;
+
+ if (!child->last_siginfo)
+ return -EINVAL;
+
+ /*
+ * This does not need lock_task_sighand() to access
+ * child->last_siginfo because ptrace_freeze_traced()
+ * called earlier by ptrace_check_attach() ensures that
+ * the tracee cannot go away and clear its last_siginfo.
+ */
+
+ switch (child->last_siginfo->si_code) {
+ case SIGTRAP | 0x80:
+ switch (child->ptrace_message) {
+ case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+ actual_size =
+ ptrace_get_syscall_info_entry(child, &info);
+ break;
+ case PTRACE_EVENTMSG_SYSCALL_EXIT:
+ actual_size =
+ ptrace_get_syscall_info_exit(child, &info);
+ break;
+ }
+ break;
+ case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
+ actual_size = ptrace_get_syscall_info_seccomp(child, &info);
+ break;
+ }
+
+ if (!actual_size)
+ return -EINVAL;
+
+ write_size = min(actual_size, user_size);
+ return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
+}
+#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
@@ -1105,6 +1207,12 @@ int ptrace_request(struct task_struct *child, long request,
ret = seccomp_get_metadata(child, addr, datavp);
break;
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+ case PTRACE_GET_SYSCALL_INFO:
+ ret = ptrace_get_syscall_info(child, addr, datavp);
+ break;
+#endif
+
default:
break;
}
--
ldv
^ permalink raw reply related
* [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-28 13:06 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: Kees Cook, Jann Horn, Michael Ellerman, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181128130439.GB28206-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
From: Elvira Khabirova <lineprinter-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
Define two constants, PTRACE_EVENTMSG_SYSCALL_ENTRY and
PTRACE_EVENTMSG_SYSCALL_EXIT, and place them in ptrace_message
for the duration of syscall-stops.
This way ptracers can distinguish syscall-enter-stops
from syscall-exit-stops using PTRACE_GETEVENTMSG request.
Signed-off-by: Elvira Khabirova <lineprinter-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
Signed-off-by: Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
---
include/linux/tracehook.h | 9 ++++++---
include/uapi/linux/ptrace.h | 10 ++++++++++
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 40b0b4c1bf7b..633a83fe7051 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -57,13 +57,15 @@ struct linux_binprm;
/*
* ptrace report for syscall entry and exit looks identical.
*/
-static inline int ptrace_report_syscall(struct pt_regs *regs)
+static inline int ptrace_report_syscall(struct pt_regs *regs,
+ unsigned long message)
{
int ptrace = current->ptrace;
if (!(ptrace & PT_PTRACED))
return 0;
+ current->ptrace_message = message;
ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
/*
@@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
current->exit_code = 0;
}
+ current->ptrace_message = 0;
return fatal_signal_pending(current);
}
@@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
static inline __must_check int tracehook_report_syscall_entry(
struct pt_regs *regs)
{
- return ptrace_report_syscall(regs);
+ return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
}
/**
@@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
if (step)
user_single_step_report(regs);
else
- ptrace_report_syscall(regs);
+ ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
}
/**
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..cb138902d042 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -104,6 +104,16 @@ struct seccomp_metadata {
#define PTRACE_O_MASK (\
0x000000ff | PTRACE_O_EXITKILL | PTRACE_O_SUSPEND_SECCOMP)
+/*
+ * These values are stored in task->ptrace_message by tracehook_report_syscall_*
+ * to describe current syscall-stop.
+ *
+ * Values for these constants are chosen so that they do not appear
+ * in task->ptrace_message by other means.
+ */
+#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
+#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
+
#include <asm/ptrace.h>
--
ldv
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply related
* [PATCH v4 0/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-11-28 13:04 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: Kees Cook, Michael Ellerman, Eugene Syromyatnikov, Steven Rostedt,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
PTRACE_GET_SYSCALL_INFO lets ptracer obtain details of the syscall
the tracee is blocked in. The request succeeds when the tracee is in a
syscall-enter-stop, syscall-exit-stop or PTRACE_EVENT_SECCOMP stop,
and fails with -EINVAL otherwise.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable.
Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.
PTRACE_GET_SYSCALL_INFO returns the following structure:
struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
__u8 __pad0[3];
__u32 arch;
union {
struct {
__u64 nr;
__u64 instruction_pointer;
__u64 stack_pointer;
__u64 frame_pointer;
__u64 args[6];
} entry;
struct {
__s64 rval;
__u8 is_error;
__u8 __pad1[7];
} exit;
struct {
__u64 nr;
__u64 instruction_pointer;
__u64 stack_pointer;
__u64 frame_pointer;
__u64 args[6];
__u32 ret_data;
__u8 __pad2[4];
} seccomp;
};
};
The structure was chosen according to [2], except for the following
changes:
* arch is returned unconditionally to aid with tracing system calls such as
execve();
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer and frame_pointer fields were added along with
instruction_pointer field since they are readily available and can save
the tracer from extra PTRACE_GETREGSET calls;
* a boolean is_error field was added along with rval field, this way
the tracer can more reliably distinguish a return value
from an error value.
This changeset should be applied on top of [3] and [4].
[1] https://lore.kernel.org/lkml/CA+55aFzcSVmdDj9Lh_gdbz1OzHyEm6ZrGPBDAJnywm2LF_eVyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
[2] https://lore.kernel.org/lkml/CAObL_7GM0n80N7J_DFw_eQyfLyzq+sf4y2AvsCCV88Tb3AwEHA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
[3] https://lore.kernel.org/lkml/20181119210139.GA8360-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org/
[4] https://lore.kernel.org/lkml/20181120001128.GA11300-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org/
v4:
* Re-split into two commits.
* Do not introduce task_struct.ptrace_event, use child->last_siginfo->si_code
instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.
v3:
* Split into three commits.
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
* PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.
v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.
Elvira Khabirova (2):
ptrace: save the type of syscall-stop in ptrace_message
ptrace: add PTRACE_GET_SYSCALL_INFO request
include/linux/tracehook.h | 9 ++--
include/uapi/linux/ptrace.h | 44 +++++++++++++++
kernel/ptrace.c | 103 +++++++++++++++++++++++++++++++++++-
3 files changed, 152 insertions(+), 4 deletions(-)
--
ldv
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [RFC PATCH RESEND v3 3/3] ptrace: add PTRACE_EVENT_SECCOMP support to PTRACE_GET_SYSCALL_INFO
From: Dmitry V. Levin @ 2018-11-28 12:44 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, Ingo Molnar, strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181128123545.GA30395-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1270 bytes --]
On Wed, Nov 28, 2018 at 01:35:46PM +0100, Oleg Nesterov wrote:
> On 11/28, Dmitry V. Levin wrote:
> >
> > > Just like ptrace_request(PTRACE_LISTEN)
> > > does but you can do this lockless (no need to lock_task_sighand()).
> >
> > Why this can be done lockless? All other places in that file do
> > the locking,
>
> PTRACE_LISTEN too doesn't need lock_task_sighand() to access ->last_siginfo,
> this code predates ptrace_freeze_traced() which ensures that the tracee can't
> go away and clear ->last_siginfo.
>
> However, unlike ptrace_get_syscall(), PTRACE_LISTEN needs spin_lock_irq(siglock),
> it modifies ->jobctl and calls signal_wake_up().
What about PTRACE_GETSIGINFO? Can it also be done lockless because
ptrace_check_attach() has already called ptrace_freeze_traced()?
> > > Of course, debugger can do PTRACE_SETSIGINFO and confuse itself but probably we
> > > do not care?
> >
> > The only potential issue I could think of is whether PTRACE_SETSIGINFO
> > could be used this way to cause an information leak by making
> > PTRACE_GET_SYSCALL_INFO access some unrelated data.
>
> Well, afaics ptrace_get_syscall() does nothing "special", debugger can use other
> PTRACE_ requests to get the same info?
I agree.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [RFC PATCH RESEND v3 3/3] ptrace: add PTRACE_EVENT_SECCOMP support to PTRACE_GET_SYSCALL_INFO
From: Oleg Nesterov @ 2018-11-28 12:35 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Elvira Khabirova, Steven Rostedt, Ingo Molnar,
Eugene Syromyatnikov, Andy Lutomirski, strace-devel, linux-api,
linux-kernel
In-Reply-To: <20181127232753.GA18755@altlinux.org>
On 11/28, Dmitry V. Levin wrote:
>
> > Just like ptrace_request(PTRACE_LISTEN)
> > does but you can do this lockless (no need to lock_task_sighand()).
>
> Why this can be done lockless? All other places in that file do
> the locking,
PTRACE_LISTEN too doesn't need lock_task_sighand() to access ->last_siginfo,
this code predates ptrace_freeze_traced() which ensures that the tracee can't
go away and clear ->last_siginfo.
However, unlike ptrace_get_syscall(), PTRACE_LISTEN needs spin_lock_irq(siglock),
it modifies ->jobctl and calls signal_wake_up().
> > Of course, debugger can do PTRACE_SETSIGINFO and confuse itself but probably we
> > do not care?
>
> The only potential issue I could think of is whether PTRACE_SETSIGINFO
> could be used this way to cause an information leak by making
> PTRACE_GET_SYSCALL_INFO access some unrelated data.
Well, afaics ptrace_get_syscall() does nothing "special", debugger can use other
PTRACE_ requests to get the same info?
Oleg.
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Florian Weimer @ 2018-11-28 9:41 UTC (permalink / raw)
To: Christian Brauner
Cc: libc-alpha, Arnd Bergmann, Dmitry V. Levin, Albert ARIBAUD,
H. Peter Anvin, Linux API
In-Reply-To: <67B157B9-80B0-4862-87F4-F03DECBD58CC@brauner.io>
* Christian Brauner:
> The intention has always been to start a
> file descriptor process API off of that.
> If we land my procfd_signal() patchset we are in good shape for
> procfd_wait(), imho.
How does this interact with SIGCHLD and the wait system call (or any
wait function without an explicitly specified PID)?
I understand that I have somewhat conflicting requirements, but in terms
of relative priorities, launching a process without spurious signals and
wait notifications would probably offer the larger benefit.
Thanks,
Florian
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Christian Brauner @ 2018-11-28 9:36 UTC (permalink / raw)
To: Florian Weimer, Arnd Bergmann
Cc: Dmitry V. Levin, Albert ARIBAUD, H. Peter Anvin, GNU C Library,
Linux API
In-Reply-To: <87r2f5h7kk.fsf@oldenburg.str.redhat.com>
On November 28, 2018 10:31:23 PM GMT+13:00, Florian Weimer <fweimer@redhat.com> wrote:
>* Arnd Bergmann:
>
>> On Mon, Nov 26, 2018 at 4:18 PM Florian Weimer <fweimer@redhat.com>
>wrote:
>>>
>>> * Arnd Bergmann:
>>>
>>> > On Thu, Nov 15, 2018 at 7:30 AM Dmitry V. Levin <ldv@altlinux.org>
>wrote:
>>> >> On Thu, Nov 15, 2018 at 06:39:03AM -0800, Arnd Bergmann wrote:
>>> >> > On Thu, Nov 15, 2018 at 6:05 AM Dmitry V. Levin wrote:
>>> >> > > On Thu, Apr 20, 2017 at 03:20:51PM +0200, Albert ARIBAUD
>wrote:
>>> >>
>>> >> 1. strace needs a race-free invocation of wait4(2) or waitid(2)
>>> >> with a different signal mask, this cannot be achieved without
>>> >> an extended version of syscall, similar to pselect6(2) extension
>>> >> over select(2) and ppoll(2) extension over poll(2).
>>> >>
>>> >> Signal mask specification in linux requires two parameters:
>>> >> "const sigset_t *sigmask" and "size_t sigsetsize".
>>> >> Creating pwait6(2) as an extension of wait4(2) with two arguments
>>> >> is straightforward.
>>> >> Creating pwaitid(2) as an extension of waitid(2) that already has
>5
>>> >> arguments would require an indirection similar to pselect6(2).
>>> >
>>> > Getting back to this point: you could also do the same thing with
>>> > the CLONE_FD approach from Josh Triplett[1] or Casey Dahlin's
>>> > older waitfd() syscall, correct?
>>>
>>> A descriptor-based solution would not be useful to glibc because
>>> applications assume that glibc does not (persistently) open any file
>>> descriptors behind t heir back.
>>
>> Right, makes sense. What about a temporary file descriptor as
>discussed
>> in the recent procfd() mail thread then? Would that work?
The intention has always been to start a
file descriptor process API off of that.
If we land my procfd_signal() patchset we are in good shape for procfd_wait(), imho.
>>
>> /* for illustration, needs error handling and more features */
>> int pwait(pid_t id, siginfo_t *infop)
>> {
>> char waitfd_file[MAX_PROCFD_LEN];
>> struct pollfd pfd[1] = { {.events = POLLIN }};
>>
>> snprintf(waitfd_file, MAX_PROCFD_LEN, "/proc/%d/wait", pid);
>> pfd.fd = open(waitfd_file, O_RDONLY);
>> ppoll(&pfd, 1, NULL, sigmask);
>> read(fd, infop, sizeof(*infop));
>> close(fd);
>>
>> return 0;
>> }
>
>Together with an officiall supported way that allows us that the
>process
>denoted by the PID is still the same that it was, it could work, I
>think.
>
>It would also nice to have the ability to launch processes without
>making them visible to wait, and trigger SIGCHLD signals, but noticing
>process exit seems like a tricky matter under these circumstances.
>
>Thanks,
>Florian
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Florian Weimer @ 2018-11-28 9:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Dmitry V. Levin, Albert ARIBAUD, H. Peter Anvin, GNU C Library,
Linux API
In-Reply-To: <CAK8P3a3pVS3_70PQ+SeiSZtqXFcb5HON0QGRypWfu=cfLPgN=w@mail.gmail.com>
* Arnd Bergmann:
> On Mon, Nov 26, 2018 at 4:18 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Arnd Bergmann:
>>
>> > On Thu, Nov 15, 2018 at 7:30 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
>> >> On Thu, Nov 15, 2018 at 06:39:03AM -0800, Arnd Bergmann wrote:
>> >> > On Thu, Nov 15, 2018 at 6:05 AM Dmitry V. Levin wrote:
>> >> > > On Thu, Apr 20, 2017 at 03:20:51PM +0200, Albert ARIBAUD wrote:
>> >>
>> >> 1. strace needs a race-free invocation of wait4(2) or waitid(2)
>> >> with a different signal mask, this cannot be achieved without
>> >> an extended version of syscall, similar to pselect6(2) extension
>> >> over select(2) and ppoll(2) extension over poll(2).
>> >>
>> >> Signal mask specification in linux requires two parameters:
>> >> "const sigset_t *sigmask" and "size_t sigsetsize".
>> >> Creating pwait6(2) as an extension of wait4(2) with two arguments
>> >> is straightforward.
>> >> Creating pwaitid(2) as an extension of waitid(2) that already has 5
>> >> arguments would require an indirection similar to pselect6(2).
>> >
>> > Getting back to this point: you could also do the same thing with
>> > the CLONE_FD approach from Josh Triplett[1] or Casey Dahlin's
>> > older waitfd() syscall, correct?
>>
>> A descriptor-based solution would not be useful to glibc because
>> applications assume that glibc does not (persistently) open any file
>> descriptors behind t heir back.
>
> Right, makes sense. What about a temporary file descriptor as discussed
> in the recent procfd() mail thread then? Would that work?
>
> /* for illustration, needs error handling and more features */
> int pwait(pid_t id, siginfo_t *infop)
> {
> char waitfd_file[MAX_PROCFD_LEN];
> struct pollfd pfd[1] = { {.events = POLLIN }};
>
> snprintf(waitfd_file, MAX_PROCFD_LEN, "/proc/%d/wait", pid);
> pfd.fd = open(waitfd_file, O_RDONLY);
> ppoll(&pfd, 1, NULL, sigmask);
> read(fd, infop, sizeof(*infop));
> close(fd);
>
> return 0;
> }
Together with an officiall supported way that allows us that the process
denoted by the PID is still the same that it was, it could work, I
think.
It would also nice to have the ability to launch processes without
making them visible to wait, and trigger SIGCHLD signals, but noticing
process exit seems like a tricky matter under these circumstances.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr
From: Alexei Starovoitov @ 2018-11-28 5:05 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Daniel Borkmann, Alexei Starovoitov, Network Development,
Linux API, Y Song
In-Reply-To: <CACAyw98oSS6qbeNCaqukM81V6vWXdrU2HCp2-b++7E=Xnx7e=w@mail.gmail.com>
On Mon, Nov 26, 2018 at 4:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> That's what I had initially, but that makes re-using test_attr really
> awkward. Either
> you need to reset data_out_size before every call because it is used
> to return the
> buffer size,
I think that is exactly what the user of the interface would want to do.
Why would anyone keep reusing the same test_attr on multiple calls
into the kernel without changing the fields?
> It also means
> we can't take a const struct attr, which is contrary to the other
> xattr functions which
> already exist.
I don't see an issue with that.
> I think actually inspecting the required size of the output buffer
> will be a rare
> occurrence, so making the user jump through the hoop of a pointer doesn't seem
> too onerous.
I think the opposite is the case.
If the output buffer is provided the test will be comparing it
to expected value.
^ permalink raw reply
* Re: [RFC PATCH RESEND v3 3/3] ptrace: add PTRACE_EVENT_SECCOMP support to PTRACE_GET_SYSCALL_INFO
From: Dmitry V. Levin @ 2018-11-27 23:27 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, Ingo Molnar, strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181127123116.GA13284-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1969 bytes --]
On Tue, Nov 27, 2018 at 01:31:17PM +0100, Oleg Nesterov wrote:
> On 11/27, Elvira Khabirova wrote:
> > On Mon, 26 Nov 2018 15:35:24 +0100, Oleg Nesterov wrote:
> > > On 11/25, Elvira Khabirova wrote:
> > > >
> > > > Extend PTRACE_GET_SYSCALL_INFO to support PTRACE_EVENT_SECCOMP stops.
> > > > The information returned is the same as for syscall-enter-stops.
> > >
> > > Oh, this is not nice ;) there must be a better option, I hope... Plus
> > >
> > >
> > > Can't ptrace_get_syscall() check
> > >
> > > child->exit_code == (PTRACE_EVENT_SECCOMP << 8) | SIGTRAP;
> > >
> > > to detect the PTRACE_EVENT_SECCOMP case?
> >
> > Nope; looks like exit_code is zeroed after wait().
>
> Yes, thanks for correcting me,
>
> but we can use child->last_siginfo->si_code.
Yes, this approach works, thanks!
> Just like ptrace_request(PTRACE_LISTEN)
> does but you can do this lockless (no need to lock_task_sighand()).
Why this can be done lockless? All other places in that file do
the locking, so I'd rather add a comment in the new code.
> And if we require that the user of ptrace_get_syscall() should also use TRACESYSGOOD
> then ptrace_get_syscall() can probably do something like
>
> int entry;
>
> if (!child->last_siginfo)
> return -EINVAL;
> else if (child->last_siginfo->si_code == (PTRACE_EVENT_SECCOMP << 8) | SIGTRAP)
> entry = 1;
> else if (child->last_siginfo->si_code == SIGTRAP | 0x80)
> entry = child->ptrace_message == PTRACE_EVENTMSG_SYSCALL_ENTRY;
> else
> return -EINVAL;
>
> and this way PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT can't confict with seccomp or
> anything else.
>
> No?
>
> Of course, debugger can do PTRACE_SETSIGINFO and confuse itself but probably we
> do not care?
The only potential issue I could think of is whether PTRACE_SETSIGINFO
could be used this way to cause an information leak by making
PTRACE_GET_SYSCALL_INFO access some unrelated data.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Jürg Billeter @ 2018-11-27 22:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Oleg Nesterov, Thomas Gleixner, Eric Biederman, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel, Jürg Billeter
In-Reply-To: <20181127225408.7553-1-j@bitron.ch>
This introduces a new thread group flag that can be set by calling
prctl(PR_SET_KILL_DESCENDANTS_ON_EXIT, 1, 0, 0, 0)
When a thread group exits with this flag set, it will send SIGKILL to
all descendant processes. This can be used to prevent stray child
processes.
This flag is cleared on privilege gaining execve(2) to ensure an
unprivileged process cannot get a privileged process to send SIGKILL.
Descendants that are orphaned and reparented to an ancestor of the
current process before the current process exits, will not be killed.
PR_SET_CHILD_SUBREAPER can be used to contain orphaned processes.
If a descendant gained privileges, the current process may not be
allowed to kill it, and the descendant process will survive.
PR_SET_NO_NEW_PRIVS can be used to prevent descendant processes from
gaining privileges.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jürg Billeter <j@bitron.ch>
---
fs/exec.c | 6 ++++++
include/linux/sched/signal.h | 3 +++
include/uapi/linux/prctl.h | 4 ++++
kernel/exit.c | 12 ++++++++++++
kernel/sys.c | 11 +++++++++++
security/apparmor/lsm.c | 1 +
security/selinux/hooks.c | 3 +++
7 files changed, 40 insertions(+)
diff --git a/fs/exec.c b/fs/exec.c
index 1ebf6e5a521d..f48ff4333393 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1339,6 +1339,12 @@ void setup_new_exec(struct linux_binprm * bprm)
/* Make sure parent cannot signal privileged process. */
current->pdeath_signal = 0;
+ /*
+ * Do not send SIGKILL from privileged process as it may
+ * have been requested by an unprivileged process.
+ */
+ current->signal->kill_descendants_on_exit = 0;
+
/*
* For secureexec, reset the stack limit to sane default to
* avoid bad behavior from the prior rlimits. This has to
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 1be35729c2c5..3bfb71701488 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -124,6 +124,9 @@ struct signal_struct {
unsigned int is_child_subreaper:1;
unsigned int has_child_subreaper:1;
+ /* Send SIGKILL to descendant processes on exit */
+ unsigned int kill_descendants_on_exit:1;
+
#ifdef CONFIG_POSIX_TIMERS
/* POSIX.1b Interval Timers */
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index c0d7ea0bf5b6..2ac4da1f282b 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -198,6 +198,10 @@ struct prctl_mm_map {
# define PR_CAP_AMBIENT_LOWER 3
# define PR_CAP_AMBIENT_CLEAR_ALL 4
+/* Send SIGKILL to descendant processes on exit */
+#define PR_SET_KILL_DESCENDANTS_ON_EXIT 48
+#define PR_GET_KILL_DESCENDANTS_ON_EXIT 49
+
/* arm64 Scalable Vector Extension controls */
/* Flag values must be kept in sync with ptrace NT_ARM_SVE interface */
#define PR_SVE_SET_VL 50 /* set task vector length */
diff --git a/kernel/exit.c b/kernel/exit.c
index 0e21e6d21f35..7fe0c694685a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -694,6 +694,15 @@ static void forget_original_parent(struct task_struct *father,
list_splice_tail_init(&father->children, &reaper->children);
}
+static int kill_descendant_visitor(struct task_struct *p, void *data)
+{
+ /* This may fail, e.g., when a descendant process gained privileges. */
+ group_send_sig_info(SIGKILL, SEND_SIG_NOINFO, p, PIDTYPE_TGID);
+
+ /* Always continue walking the process tree. */
+ return 1;
+}
+
/*
* Send signals to all our closest relatives so that they know
* to properly mourn us..
@@ -704,6 +713,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
struct task_struct *p, *n;
LIST_HEAD(dead);
+ if (group_dead && tsk->signal->kill_descendants_on_exit)
+ walk_process_tree(tsk, kill_descendant_visitor, NULL);
+
write_lock_irq(&tasklist_lock);
forget_original_parent(tsk, &dead);
diff --git a/kernel/sys.c b/kernel/sys.c
index 123bd73046ec..8d9af81da093 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2476,6 +2476,17 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
error = arch_prctl_spec_ctrl_set(me, arg2, arg3);
break;
+ case PR_SET_KILL_DESCENDANTS_ON_EXIT:
+ if (arg3 || arg4 || arg5)
+ return -EINVAL;
+ me->signal->kill_descendants_on_exit = !!arg2;
+ break;
+ case PR_GET_KILL_DESCENDANTS_ON_EXIT:
+ if (arg3 || arg4 || arg5)
+ return -EINVAL;
+ error = put_user(me->signal->kill_descendants_on_exit,
+ (int __user *)arg2);
+ break;
default:
error = -EINVAL;
break;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..d0d8f88130fb 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -695,6 +695,7 @@ static void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
aa_inherit_files(bprm->cred, current->files);
current->pdeath_signal = 0;
+ current->signal->kill_descendants_on_exit = 0;
/* reset soft limits and set hard limits for the new label */
__aa_transition_rlimits(label, new_label);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad9a9b8e9979..313a7be43a98 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2653,6 +2653,9 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
/* Always clear parent death signal on SID transitions. */
current->pdeath_signal = 0;
+ /* Disable SIGKILL requested from before the SID transition. */
+ current->signal->kill_descendants_on_exit = 0;
+
/* Check whether the new SID can inherit resource limits from the old
* SID. If not, reset all soft limits to the lower of the current
* task's hard limit and the init task's soft limit.
--
2.19.2
^ permalink raw reply related
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