* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-29 20:14 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Florian Weimer, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181129195551.woe2bl3z3yaysqb6@brauner.io>
> On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
>
>> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
>>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
>>>
>>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>>
>>>>> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>> Disclaimer: I'm looking at this patch because Christian requested it.
>>>>> I'm not a kernel developer.
>>>>>
>>>>> * Christian Brauner:
>>>>>
>>>>>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
>>>> b/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> index 3cf7b533b3d1..3f27ffd8ae87 100644
>>>>>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> @@ -398,3 +398,4 @@
>>>>>> 384 i386 arch_prctl sys_arch_prctl
>>>> __ia32_compat_sys_arch_prctl
>>>>>> 385 i386 io_pgetevents sys_io_pgetevents
>>>> __ia32_compat_sys_io_pgetevents
>>>>>> 386 i386 rseq sys_rseq __ia32_sys_rseq
>>>>>> +387 i386 procfd_signal sys_procfd_signal
>>>> __ia32_compat_sys_procfd_signal
>>>>>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
>>>> b/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> index f0b1709a5ffb..8a30cde82450 100644
>>>>>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> @@ -343,6 +343,7 @@
>>>>>> 332 common statx __x64_sys_statx
>>>>>> 333 common io_pgetevents __x64_sys_io_pgetevents
>>>>>> 334 common rseq __x64_sys_rseq
>>>>>> +335 64 procfd_signal __x64_sys_procfd_signal
>>>>>>
>>>>>> #
>>>>>> # x32-specific system call numbers start at 512 to avoid cache
>>>> impact
>>>>>> @@ -386,3 +387,4 @@
>>>>>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
>>>>>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
>>>>>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
>>>>>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
>>>>>
>>>>> Is there a reason why these numbers have to be different?
>>>>>
>>>>> (See the recent discussion with Andy Lutomirski.)
>>>>
>>>> Hah, I missed this part of the patch. Let’s not add new x32 syscall
>>>> numbers.
>>>>
>>>> Also, can we perhaps rework this a bit to get rid of the compat entry
>>>> point? The easier way would be to check in_compat_syscall(). The nicer
>>>> way IMO would be to use the 64-bit structure for 32-bit as well.
>>>
>>> Do you have a syscall which set precedence/did this before I could look at?
>>> Just if you happen to remember one.
>>> Fwiw, I followed the other signal syscalls.
>>> They all introduce compat syscalls.
>>>
>>
>> Not really.
>>
>> Let me try to explain. I have three issues with the approach in your patchset:
>>
>> 1. You're introducing a new syscall, and it behaves differently on
>> 32-bit and 64-bit because the structure you pass in is different.
>> This is necessary for old syscalls where compatibility matters, but
>> maybe we can get rid of it for new syscalls. Could we define a
>> siginfo64_t that is identical to the 64-bit siginfo_t and just use
>> that in all cases?
>>
>> 2. Assuming that #1 doesn't work, then we need compat support. But
>> you're doing it by having two different entry points. Instead, you
>> could have a single entry point that calls in_compat_syscall() to
>> decide which structure to read. This would simplify things because
>> x86 doesn't really support the separate compat entry points, which
>> leads me to #3.
>>
>> 3. The separate x32 numbers are a huge turd that may have security
>> holes and certainly have comprehensibility holes. I will object to
>> any patch that adds a new one (like yours). Fixing #1 or #2 makes
>> this problem go away.
>>
>> Does that make any sense? The #2 fix would be something like:
>>
>> if (in_compat_syscall)
>> copy...user32();
>> else
>> copy_from_user();
>>
>> The #1 fix would add a copy_siginfo_from_user64() or similar.
>
> Thanks very much! That all helped a bunch already! I'll try to go the
> copy_siginfo_from_user64() way first and see if I can make this work. If
> we do this I would however only want to use it for the new syscall first
> and not change all other signal syscalls over to it too. I'd rather keep
> this patchset focussed and small and do such conversions caused by the
> new approach later. Does that sound reasonable?
Absolutely. I don’t think we can change old syscalls — the ABI is set in stone. But for new syscalls, I think the always-64-bit behavior makes sense.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-29 21:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: christian, Andy Lutomirski, Florian Weimer, Eric W . Biederman,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <6E21165F-2C76-4877-ABD9-0C86D55FD6AA@amacapital.net>
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
> >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
> >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
> >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
> >>
> >> The #1 fix would add a copy_siginfo_from_user64() or similar.
> >
> > Thanks very much! That all helped a bunch already! I'll try to go the
> > copy_siginfo_from_user64() way first and see if I can make this work. If
> > we do this I would however only want to use it for the new syscall first
> > and not change all other signal syscalls over to it too. I'd rather keep
> > this patchset focussed and small and do such conversions caused by the
> > new approach later. Does that sound reasonable?
>
> Absolutely. I don’t think we can change old syscalls — the ABI is set in stone.
> But for new syscalls, I think the always-64-bit behavior makes sense.
It looks like we already have a 'struct signalfd_siginfo' that is defined in a
sane architecture-independent way, so I'd suggest we use that.
We may then also want to make sure that any system call that takes a
siginfo has a replacement that takes a signalfd_siginfo, and that this
replacement can be used to implement the old version purely in
user space.
Is the current procfd_signal() proposal (under whichever name) sufficient
to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()?
Can we implement sys_rt_sigtimedwait() based on signalfd()?
If yes, that would leave waitid(), which already needs a replacement
for y2038, and that should then also return a signalfd_siginfo.
My current preference for waitid() would be to do a version that
closely resembles the current interface, but takes a signalfd_siginfo
and a __kernel_timespec based rusage replacement (possibly
two of them to let us map wait6), but does not operate on procfd or
take a signal mask. That would require yet another syscall, but I
don't think I can do that before we want to have the set of y2038
safe syscalls.
Arnd
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-29 21:10 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: <20181129144742.GB10645-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 2450 bytes --]
On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
> On 11/29, Dmitry V. Levin wrote:
> >
> > 2. Document these values
>
> sure, they should be documented and live in include/uapi/,
>
> > chosen to avoid collisions with ptrace_message values
> > set by other ptrace events
>
> this is what I can't understand. But to clarify, I don't really care and
> won't argue.
>
> If an application wants to use PTRACE_GETEVENTMSG to distinguish entry/exit
> (without PTRACE_GET_SYSCALL_INFO) it needs to do wait(status) and check status
> anyway, otherwise PTRACE_GETEVENTMSG is simply pointless (wrt syscall entry/
> exit). So we do not care if PTRACE_EVENTMSG_SYSCALL_ENTRY conflicts with, say,
> SECCOMP_RET_DATA.
Yes, once the application has verified that the kernel implements this
feature, there is no risk of collision.
> > so that PTRACE_GETEVENTMSG users can easily tell
> > whether this new semantics is supported by the kernel or not.
>
> Yes. And how much this can help? Again, an application can trivially detect
> if this feature implemented or not, and it should do this anyway if it wants
> to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
How an application can easily detect whether this feature is implemented?
By invoking PTRACE_GETEVENTMSG after the first syscall stop reported by
wait and checking whether the returned value is either
PTRACE_EVENTMSG_SYSCALL_ENTRY or PTRACE_EVENTMSG_SYSCALL_EXIT.
So the question is, how can this value be equal to one of these constants
when this feature is not implemented? Can a value saved to ptrace_message
earlier by one of ptrace events be equal to one of these constants?
Imagine an application attaches to already existing process, enables
PTRACE_O_TRACESECCOMP, and a PTRACE_EVENT_SECCOMP arrives with
ptrace_message set to 1. If this application then exits and a new invocation
of the same application attaches to the same process, it will very likely see
this 1 returned by PTRACE_GETEVENTMSG if the feature is not implemented
in the kernel.
To avoid that kind of collisions, kernel should use different ptrace_message
values for syscall stops.
> Again, I won't reallly argue. But if you insist that these values must
> be unique then you probably need to add
>
> BUILD_BUG_ON(PTRACE_EVENTMSG_SYSCALL_ENTRY <= PID_MAX_LIMIT);
Yes, it's a good idea. What is the proper place for this check?
--
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 v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-29 21:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Eric W . Biederman, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a2i9DFaS9-3A9V_pKwFfwVR0bDqgLosmivge3Cx2VyiuQ@mail.gmail.com>
On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
> On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > > On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
> > >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
> > >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
> > >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
> > >>
> > >> The #1 fix would add a copy_siginfo_from_user64() or similar.
> > >
> > > Thanks very much! That all helped a bunch already! I'll try to go the
> > > copy_siginfo_from_user64() way first and see if I can make this work. If
> > > we do this I would however only want to use it for the new syscall first
> > > and not change all other signal syscalls over to it too. I'd rather keep
> > > this patchset focussed and small and do such conversions caused by the
> > > new approach later. Does that sound reasonable?
> >
> > Absolutely. I don’t think we can change old syscalls — the ABI is set in stone.
> > But for new syscalls, I think the always-64-bit behavior makes sense.
>
> It looks like we already have a 'struct signalfd_siginfo' that is defined in a
> sane architecture-independent way, so I'd suggest we use that.
Just so that I understand you correctly: swapping out struct signinfo
for struct signalfd_siginfo in procfd_<whatever-suffix>? If so that
sounds great to me!
>
> We may then also want to make sure that any system call that takes a
> siginfo has a replacement that takes a signalfd_siginfo, and that this
> replacement can be used to implement the old version purely in
> user space.
Sounds good but is unrelated to this patchset I take it. :)
>
> Is the current procfd_signal() proposal (under whichever name) sufficient
> to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()?
Yes, I see no reason why not. My idea is to extend it - after we have a
basic version in - to also work with:
/proc/<pid>/task/<tid>
If I'm not mistaken this should be sufficient to get rt_tgsigqueueinfo.
The thread will be uniquely identified by the tid descriptor and no
combination of /proc/<pid> and /proc/<pid>/task/<tid> is needed. Does
that sound reasonable?
> Can we implement sys_rt_sigtimedwait() based on signalfd()?
> If yes, that would leave waitid(), which already needs a replacement
> for y2038, and that should then also return a signalfd_siginfo.
> My current preference for waitid() would be to do a version that
> closely resembles the current interface, but takes a signalfd_siginfo
> and a __kernel_timespec based rusage replacement (possibly
> two of them to let us map wait6), but does not operate on procfd or
> take a signal mask. That would require yet another syscall, but I
> don't think I can do that before we want to have the set of y2038
> safe syscalls.
All sounds reasonable to me but that's not a blocker for the current
syscall though, is it?
Christian
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-29 21:40 UTC (permalink / raw)
To: christian
Cc: Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Eric W . Biederman, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181129213458.4h44dpg6ltqow4k4@brauner.io>
On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner <christian@brauner.io> wrote:
> On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
> > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Is the current procfd_signal() proposal (under whichever name) sufficient
> > to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()?
>
> Yes, I see no reason why not. My idea is to extend it - after we have a
> basic version in - to also work with:
> /proc/<pid>/task/<tid>
> If I'm not mistaken this should be sufficient to get rt_tgsigqueueinfo.
> The thread will be uniquely identified by the tid descriptor and no
> combination of /proc/<pid> and /proc/<pid>/task/<tid> is needed. Does
> that sound reasonable?
Yes. So it would currently replace rt_gsigqueueinfo() but
not rt_tgsigqueueinfo(), and could be extended to do both
afterwards, without making the interface ugly in any form?
I suppose we can always add more flags if needed, and you
already ensure that flags is zero for the moment.
> > Can we implement sys_rt_sigtimedwait() based on signalfd()?
> >
> > If yes, that would leave waitid(), which already needs a replacement
> > for y2038, and that should then also return a signalfd_siginfo.
> > My current preference for waitid() would be to do a version that
> > closely resembles the current interface, but takes a signalfd_siginfo
> > and a __kernel_timespec based rusage replacement (possibly
> > two of them to let us map wait6), but does not operate on procfd or
> > take a signal mask. That would require yet another syscall, but I
> > don't think I can do that before we want to have the set of y2038
> > safe syscalls.
>
> All sounds reasonable to me but that's not a blocker for the current
> syscall though, is it?
I'd like to at least understand about sys_rt_sigtimedwait() before
we go on, so we all know what's coming, and document the
plans in the changelog.
waitid() probably remains on my plate anyway, and I hope understand
where we're at with it.
Arnd
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-29 23:08 UTC (permalink / raw)
To: Kees Cook, Andy Lutomirski
Cc: Oleg Nesterov, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181029224031.29809-2-tycho@tycho.ws>
On Mon, Oct 29, 2018 at 04:40:30PM -0600, Tycho Andersen wrote:
> + resp.id = req.id;
> + resp.error = -512; /* -ERESTARTSYS */
> + resp.val = 0;
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
So, it turns out this *doesn't* work, and the reason this test was
passing is because of poor hygiene on my part.
Per the documentation in include/linux/errno.h,
/*
* These should never be seen by user programs. To return one of ERESTART*
* codes, signal_pending() MUST be set. Note that ptrace can observe these
* at syscall exit tracing, but they will never be left for the debugged user
* process to see.
*/
#define ERESTARTSYS 512
So basically, if you respond with -ERESTARTSYS with no signal pending, you'll
leak it to userspace. It turns out this is already possible with
SECCOMP_RET_TRAP (and probably ptrace alone, although I didn't try it out),
see the program below.
The question is: do we care? If so, it seems like we may need to handle the
-ERESTARTSYS-style cases even when there is no signal pending. If we don't,
there's precedent for us to just do the same thing as what happens for
SECCOMP_RET_TRACE, but we should probably at least fix the docs.
Tycho
#include <stdio.h>
#include <stddef.h>
#include <stdbool.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <signal.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <sys/ptrace.h>
#include <sys/ioctl.h>
#include <inttypes.h>
#include <linux/filter.h>
#include <linux/seccomp.h>
#include <linux/ptrace.h>
#include <linux/elf.h>
#include <pthread.h>
static int filter_syscall(int syscall_nr)
{
struct sock_filter filter[] = {
BPF_STMT(BPF_LD+BPF_W+BPF_ABS, offsetof(struct seccomp_data, nr)),
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, syscall_nr, 0, 1),
BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_TRACE),
BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
};
struct sock_fprog bpf_prog = {
.len = (unsigned short)(sizeof(filter)/sizeof(filter[0])),
.filter = filter,
};
int ret;
ret = syscall(__NR_seccomp, SECCOMP_SET_MODE_FILTER, 0, &bpf_prog);
if (ret < 0) {
perror("prctl failed");
return -1;
}
return ret;
}
typedef struct {
uint64_t r15;
uint64_t r14;
uint64_t r13;
uint64_t r12;
uint64_t bp;
uint64_t bx;
uint64_t r11;
uint64_t r10;
uint64_t r9;
uint64_t r8;
uint64_t ax;
uint64_t cx;
uint64_t dx;
uint64_t si;
uint64_t di;
uint64_t orig_ax;
uint64_t ip;
uint64_t cs;
uint64_t flags;
uint64_t sp;
uint64_t ss;
uint64_t fs_base;
uint64_t gs_base;
uint64_t ds;
uint64_t es;
uint64_t fs;
uint64_t gs;
} user_regs_struct64;
int main(int argc, char **argv)
{
pid_t pid;
user_regs_struct64 regs;
struct iovec iov = {.iov_base = ®s, .iov_len = sizeof(regs)};
int status;
pid = fork();
if (pid == 0) {
if (signal(SIGUSR1, signal_handler) == SIG_ERR) {
perror("signal");
exit(1);
}
if (filter_syscall(__NR_getpid) < 0)
exit(1);
/* i'm lazy, so sue me :) */
sleep(1);
errno = 0;
pid = syscall(__NR_getpid);
/*
* we get:
* getpid(): -1, errno: 512
* probably should get
* getpid(): <pid> errno: 0
*/
printf("getpid(): %d, errno: %d\n", pid, errno);
exit(errno);
}
if (ptrace(PTRACE_ATTACH, pid, NULL, 0) < 0) {
perror("ptrace attach");
goto out;
}
if (waitpid(pid, NULL, 0) != pid) {
perror("waitpid");
goto out;
}
if (ptrace(PTRACE_SETOPTIONS, pid, NULL, PTRACE_O_TRACESECCOMP) < 0) {
perror("ptrace setoptions");
goto out;
}
if (ptrace(PTRACE_CONT, pid, NULL, 0) != 0) {
perror("ptrace cont");
goto out;
}
if (waitpid(pid, &status, 0) != pid) {
perror("wait for trace");
goto out;
}
if (status >> 8 != (SIGTRAP | (PTRACE_EVENT_SECCOMP<<8))) {
printf("got bad trap event?\n");
goto out;
}
if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov) < 0) {
perror("getregset");
goto out;
}
/*
* Tell the syscall to restart. Per include/linux/errno.h this should
* only be set when signal_pending() is set. But we just won't send
* any signals to the process, and we'll see this in userspace.
*/
regs.ax = -512; /* -ERESTARTSYS */
/*
* This makes the this_syscall < 0 check in __seccomp_filter()
* trigger, so we skip the syscall and return whatever is in ax
*/
regs.orig_ax = -512; /* -ERESTARTSYS */
if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov) < 0) {
perror("setregset");
goto out;
}
if (ptrace(PTRACE_CONT, pid, NULL, 0) < 0) {
perror("cont after setregset");
goto out;
}
while (1) {
if (waitpid(pid, &status, 0) != pid) {
perror("wait for death");
goto out;
}
if (!WIFSTOPPED(status)) {
break;
}
if (ptrace(PTRACE_CONT, pid, NULL, 0) < 0) {
perror("cont after setregset");
goto out;
}
}
if (WIFSIGNALED(status)) {
printf("didn't exit: %d\n", WTERMSIG(status));
return 1;
}
if (WEXITSTATUS(status)) {
printf("exited: %d\n", WEXITSTATUS(status));
return 1;
}
return 0;
out:
kill(pid, SIGKILL);
return 1;
}
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Aleksa Sarai @ 2018-11-30 2:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: christian, Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Eric W . Biederman, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a1_if7+Ak2eefU6JeZT9KW827gkLHaObX-QOsHrB5ZwfA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 475 bytes --]
On 2018-11-29, Arnd Bergmann <arnd@arndb.de> wrote:
> waitid() probably remains on my plate anyway, and I hope understand
> where we're at with it.
Having a way to wait on a processfd is something we'll eventually need,
though the semantics of zombies might get a little bit hairy. I propose
we work through that rewrite in a future series once this one goes in.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-11-30 5:13 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andy Lutomirski, christian, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a2i9DFaS9-3A9V_pKwFfwVR0bDqgLosmivge3Cx2VyiuQ@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> writes:
> On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> > On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
>> >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
>> >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
>> >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>> >>
>> >> The #1 fix would add a copy_siginfo_from_user64() or similar.
>> >
>> > Thanks very much! That all helped a bunch already! I'll try to go the
>> > copy_siginfo_from_user64() way first and see if I can make this work. If
>> > we do this I would however only want to use it for the new syscall first
>> > and not change all other signal syscalls over to it too. I'd rather keep
>> > this patchset focussed and small and do such conversions caused by the
>> > new approach later. Does that sound reasonable?
>>
>> Absolutely. I don’t think we can change old syscalls — the ABI is set in stone.
>> But for new syscalls, I think the always-64-bit behavior makes sense.
>
> It looks like we already have a 'struct signalfd_siginfo' that is defined in a
> sane architecture-independent way, so I'd suggest we use that.
Unfortunately it isn't maintained very well. What you can
express with signalfd_siginfo is a subset what you can express with
siginfo. Many of the linux extensions to siginfo for exception
information add pointers and have integers right after those pointers.
Not all of those linux specific extentions for exceptions are handled
by signalfd_siginfo (it needs new fields).
As originally defined siginfo had the sigval_t union at the end so it
was perfectly fine on both 32bit and 64bit as it only had a single
pointer in the structure and the other fields were 32bits in size.
Although I do feel the pain as x86_64 has to deal with 3 versions
of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
with a 64bit si_clock_t.
> We may then also want to make sure that any system call that takes a
> siginfo has a replacement that takes a signalfd_siginfo, and that this
> replacement can be used to implement the old version purely in
> user space.
If you are not implementing CRIU and reinserting exceptions to yourself.
At most user space wants the ability to implement sigqueue:
AKA:
sigqueue(pid_t pid, int sig, const union sigval value);
Well sigqueue with it's own si_codes so the following would cover all
the use cases I know of:
int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
The si_code could even be set to SI_USER to request that the normal
trusted SI_USER values be filled in. si_code values of < 0 if not
recognized could reasonably safely be treated as the _rt member of
the siginfo union. Or perhaps better we error out in such a case.
If we want to be flexible and not have N kinds of system calls that
is the direction I would go. That is simple, and you don't need any of
the rest.
Alternatively we abandon the mistake of sigqueueinfo and not allow
a signal number in the arguments that differs from the si_signo in the
siginfo and allow passing the entire thing unchanged from sender to
receiver. That is maximum flexibility.
signalfd_siginfo just sucks in practice. It is larger that siginfo 104
bytes versus 48. We must deliver it to userspace as a siginfo so it
must be translated. Because of the translation signalfd_siginfo adds
no flexiblity in practice, because it can not just be passed through.
Finallay signalfd_siginfo does not have encodings for all of the
siginfo union members, so it fails to be fully general.
Personally if I was to define signalfd_siginfo today I would make it:
struct siginfo_subset {
__u32 sis_signo;
__s32 sis_errno;
__s32 sis_code;
__u32 sis_pad;
__u32 sis_pid;
__u32 sis_uid;
__u64 sis_data (A pointer or integer data field);
};
That is just 32bytes, and is all that is needed for everything
except for synchronous exceptions. Oh and that happens to be a proper
subset of a any sane siginfo layout, on both 32bit and 64bit.
This is one of those rare times where POSIX is sane and what linux
has implemented is not.
Eric
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-30 6:56 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Arnd Bergmann, Andy Lutomirski, Andy Lutomirski, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <87y39b2lm2.fsf@xmission.com>
On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
>
> > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >> > On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
> >> >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
> >> >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
> >> >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
> >> >>
> >> >> The #1 fix would add a copy_siginfo_from_user64() or similar.
> >> >
> >> > Thanks very much! That all helped a bunch already! I'll try to go the
> >> > copy_siginfo_from_user64() way first and see if I can make this work. If
> >> > we do this I would however only want to use it for the new syscall first
> >> > and not change all other signal syscalls over to it too. I'd rather keep
> >> > this patchset focussed and small and do such conversions caused by the
> >> > new approach later. Does that sound reasonable?
> >>
> >> Absolutely. I don’t think we can change old syscalls — the ABI is set in stone.
> >> But for new syscalls, I think the always-64-bit behavior makes sense.
> >
> > It looks like we already have a 'struct signalfd_siginfo' that is defined in a
> > sane architecture-independent way, so I'd suggest we use that.
>
> Unfortunately it isn't maintained very well. What you can
> express with signalfd_siginfo is a subset what you can express with
> siginfo. Many of the linux extensions to siginfo for exception
> information add pointers and have integers right after those pointers.
> Not all of those linux specific extentions for exceptions are handled
> by signalfd_siginfo (it needs new fields).
>
> As originally defined siginfo had the sigval_t union at the end so it
> was perfectly fine on both 32bit and 64bit as it only had a single
> pointer in the structure and the other fields were 32bits in size.
>
> Although I do feel the pain as x86_64 has to deal with 3 versions
> of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
> with a 64bit si_clock_t.
>
> > We may then also want to make sure that any system call that takes a
> > siginfo has a replacement that takes a signalfd_siginfo, and that this
> > replacement can be used to implement the old version purely in
> > user space.
>
> If you are not implementing CRIU and reinserting exceptions to yourself.
> At most user space wants the ability to implement sigqueue:
>
> AKA:
> sigqueue(pid_t pid, int sig, const union sigval value);
>
> Well sigqueue with it's own si_codes so the following would cover all
> the use cases I know of:
> int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
>
> The si_code could even be set to SI_USER to request that the normal
> trusted SI_USER values be filled in. si_code values of < 0 if not
> recognized could reasonably safely be treated as the _rt member of
> the siginfo union. Or perhaps better we error out in such a case.
>
> If we want to be flexible and not have N kinds of system calls that
> is the direction I would go. That is simple, and you don't need any of
> the rest.
>
>
> Alternatively we abandon the mistake of sigqueueinfo and not allow
> a signal number in the arguments that differs from the si_signo in the
> siginfo and allow passing the entire thing unchanged from sender to
> receiver. That is maximum flexibility.
>
> signalfd_siginfo just sucks in practice. It is larger that siginfo 104
> bytes versus 48. We must deliver it to userspace as a siginfo so it
> must be translated. Because of the translation signalfd_siginfo adds
> no flexiblity in practice, because it can not just be passed through.
> Finallay signalfd_siginfo does not have encodings for all of the
> siginfo union members, so it fails to be fully general.
>
> Personally if I was to define signalfd_siginfo today I would make it:
> struct siginfo_subset {
> __u32 sis_signo;
> __s32 sis_errno;
> __s32 sis_code;
> __u32 sis_pad;
> __u32 sis_pid;
> __u32 sis_uid;
> __u64 sis_data (A pointer or integer data field);
> };
>
> That is just 32bytes, and is all that is needed for everything
> except for synchronous exceptions. Oh and that happens to be a proper
> subset of a any sane siginfo layout, on both 32bit and 64bit.
>
> This is one of those rare times where POSIX is sane and what linux
> has implemented is not.
Thanks for the in-depth explanation. So your point is that we are better
off if we stick with siginfo_t instead of struct signalfd_siginfo in
procfd_signal()?
^ permalink raw reply
* [PATCH v2 0/1] Add prctl to kill descendants on exit
From: Jürg Billeter @ 2018-11-30 8:00 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-2-j@bitron.ch>
This patch adds a new prctl to kill all descendant processes on exit.
See commit message for details of the prctl.
This is a replacement of PR_SET_PDEATHSIG_PROC I proposed last year [1].
In the following discussion, Oleg suggested this approach.
The motivation for this is to provide a lightweight mechanism to prevent
stray processes. There is also a related Bugzilla entry [2].
PID namespaces can also be used to prevent stray processes, of course.
However, they are not quite as lightweight as they typically also
require a new mount namespace to be able to mount a new /proc. And they
require CAP_SYS_ADMIN. User namespaces can help to gain CAP_SYS_ADMIN,
however, that further increases the overhead and the other effects of
the user namespace may not be desired.
PID 1 in PID namespaces also exhibits non-standard signal behavior
(SIGNAL_UNKILLABLE) [3].
Changes in v2:
- Use bool instead of bitfield to avoid race with
PR_SET_CHILD_SUBREAPER
[1] https://lkml.kernel.org/lkml/20170929123058.48924-1-j@bitron.ch/
[2] https://bugzilla.kernel.org/show_bug.cgi?id=43300
[3] https://lkml.kernel.org/lkml/20180803144021.56920-2-j@bitron.ch/
Jürg Billeter (1):
prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
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(+)
--
2.19.2
^ permalink raw reply
* [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Jürg Billeter @ 2018-11-30 8:00 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: <20181130080004.23635-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 044e296f2381..1c9520d83d6b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1343,6 +1343,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 = false;
+
/*
* 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 0c3e396dca04..91ed7f480b60 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 */
+ bool kill_descendants_on_exit;
+
#ifdef CONFIG_POSIX_TIMERS
/* POSIX.1b Interval Timers */
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index b17201edfa09..a31141236064 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 7027c7aee00d..5de086d7546c 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -798,6 +798,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 = false;
/* 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 67f1344e728a..4c041212702c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2657,6 +2657,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 = false;
+
/* 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
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-11-30 10:17 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181129230826.GB4676@cisco>
On 11/29, Tycho Andersen wrote:
>
> /*
> * These should never be seen by user programs. To return one of ERESTART*
> * codes, signal_pending() MUST be set. Note that ptrace can observe these
> * at syscall exit tracing, but they will never be left for the debugged user
> * process to see.
> */
> #define ERESTARTSYS 512
>
> So basically, if you respond with -ERESTARTSYS with no signal pending, you'll
> leak it to userspace.
Yes,
> It turns out this is already possible with
> SECCOMP_RET_TRAP (and probably ptrace alone,
Yes,
> The question is: do we care?
I think we do not care, debugger can do anything with the tracee.
Oleg.
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Oleg Nesterov @ 2018-11-30 10:33 UTC (permalink / raw)
To: Jürg Billeter
Cc: Eric W. Biederman, Andrew Morton, Thomas Gleixner, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <f55261dbc41544363b4348820f67fb01c6d7aaec.camel@bitron.ch>
On 11/29, Jürg Billeter wrote:
>
> On Thu, 2018-11-29 at 13:34 +0100, Oleg Nesterov wrote:
> > To me it would be more clean to call walk_process_tree(kill_descendant_visitor)
> > unconditionally in find_new_reaper() right before "if (has_child_subreaper)", but
> > then we will need to shift read_lock(tasklist) from walk_process_tree().
>
> Yes, that's the reason why I added the call before the tasklist lock.
> Let me know if you want me to move the read lock from
> walk_process_tree() to PR_SET_CHILD_SUBREAPER (the only caller)
> instead.
I am fine either way. We can do this later, lets keep your patch simple.
> > So I think the patch is mostly fine, the only problem I can see is that
> > PR_SET_KILL_DESCENDANTS_ON_EXIT can race with PR_SET_CHILD_SUBREAPER, they both
> > need to update the bits in the same word.
>
> Good point. I'll make it a regular bool instead of a bitfield for v2,
Agreed,
> unless you have another approach in mind to fix this.
Well, I think that is_child_subreaper/has_child_subreaper and the new
kill_descendants_on_exit should live in signal->flags, but we need some
cleanups to make this possible, so I agree with the boolean.
Oleg.
^ permalink raw reply
* Re: [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Oleg Nesterov @ 2018-11-30 11:22 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: <20181130080004.23635-2-j@bitron.ch>
On 11/30, Jürg Billeter wrote:
>
> 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.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Oleg Nesterov @ 2018-11-30 11:29 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: <20181129211044.GA20529@altlinux.org>
On 11/30, Dmitry V. Levin wrote:
>
> On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
>
> > > so that PTRACE_GETEVENTMSG users can easily tell
> > > whether this new semantics is supported by the kernel or not.
> >
> > Yes. And how much this can help? Again, an application can trivially detect
> > if this feature implemented or not, and it should do this anyway if it wants
> > to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
>
> How an application can easily detect whether this feature is implemented?
As I already said, it can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL) ?
If it returns -EIO then this feature is not implemented. Any other error
code (actually EINVAL or EFAULT) means it is implemented.
Oleg.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-30 11:41 UTC (permalink / raw)
To: christian
Cc: Eric W . Biederman, Andy Lutomirski, Andy Lutomirski,
Florian Weimer, Linux Kernel Mailing List, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181130065606.kmilbbq46oeycjp5@brauner.io>
On Fri, Nov 30, 2018 at 7:56 AM Christian Brauner <christian@brauner.io> wrote:
> On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
> > Arnd Bergmann <arnd@arndb.de> writes:
> > > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > >
> > > It looks like we already have a 'struct signalfd_siginfo' that is defined in a
> > > sane architecture-independent way, so I'd suggest we use that.
> >
> > Unfortunately it isn't maintained very well. What you can
> > express with signalfd_siginfo is a subset what you can express with
> > siginfo. Many of the linux extensions to siginfo for exception
> > information add pointers and have integers right after those pointers.
> > Not all of those linux specific extentions for exceptions are handled
> > by signalfd_siginfo (it needs new fields).
Would those fit in the 30 remaining padding bytes?
> > As originally defined siginfo had the sigval_t union at the end so it
> > was perfectly fine on both 32bit and 64bit as it only had a single
> > pointer in the structure and the other fields were 32bits in size.
The problem with sigval_t of course is that it is incompatible between
32-bit and 64-bit. We can add the same information, but at least on
the syscall level that would have to be a __u64.
> > Although I do feel the pain as x86_64 has to deal with 3 versions
> > of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
> > with a 64bit si_clock_t.
At least you and Al have managed to get it down to a single source-level
definition across all architectures, but there is also the (lesser) problem
that the structure has a slightly different binary layout on each of the
classic architectures.
If we go with Andy's suggestion of having only a single binary layout
on x86 for the new call, I'd argue that we should also make it have
the exact same layout on all other architectures.
> > > We may then also want to make sure that any system call that takes a
> > > siginfo has a replacement that takes a signalfd_siginfo, and that this
> > > replacement can be used to implement the old version purely in
> > > user space.
> >
> > If you are not implementing CRIU and reinserting exceptions to yourself.
> > At most user space wants the ability to implement sigqueue:
> >
> > AKA:
> > sigqueue(pid_t pid, int sig, const union sigval value);
> >
> > Well sigqueue with it's own si_codes so the following would cover all
> > the use cases I know of:
> > int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
> >
> > The si_code could even be set to SI_USER to request that the normal
> > trusted SI_USER values be filled in. si_code values of < 0 if not
> > recognized could reasonably safely be treated as the _rt member of
> > the siginfo union. Or perhaps better we error out in such a case.
> >
> > If we want to be flexible and not have N kinds of system calls that
> > is the direction I would go. That is simple, and you don't need any of
> > the rest.
I'm not sure I understand what you are suggesting here. Would the
low-level implementation of this be based on procfd, or do you
mean that would be done for pid_t at the kernel level, plus another
syscall for procfd?
> > Alternatively we abandon the mistake of sigqueueinfo and not allow
> > a signal number in the arguments that differs from the si_signo in the
> > siginfo and allow passing the entire thing unchanged from sender to
> > receiver. That is maximum flexibility.
This would be regardless of which flavor of siginfo (today's arch
specific one, signalfd_siginfo, or a new one) we pass, right?
> > signalfd_siginfo just sucks in practice. It is larger that siginfo 104
> > bytes versus 48. We must deliver it to userspace as a siginfo so it
> > must be translated. Because of the translation signalfd_siginfo adds
> > no flexiblity in practice, because it can not just be passed through.
> > Finallay signalfd_siginfo does not have encodings for all of the
> > siginfo union members, so it fails to be fully general.
> >
> > Personally if I was to define signalfd_siginfo today I would make it:
> > struct siginfo_subset {
> > __u32 sis_signo;
> > __s32 sis_errno;
> > __s32 sis_code;
> > __u32 sis_pad;
> > __u32 sis_pid;
> > __u32 sis_uid;
> > __u64 sis_data (A pointer or integer data field);
> > };
> >
> > That is just 32bytes, and is all that is needed for everything
> > except for synchronous exceptions. Oh and that happens to be a proper
> > subset of a any sane siginfo layout, on both 32bit and 64bit.
And that would work for signalfd and waitid_time64, but not for
procfd_signal/kill/sendsiginfo?
> > This is one of those rare times where POSIX is sane and what linux
> > has implemented is not.
>
> Thanks for the in-depth explanation. So your point is that we are better
> off if we stick with siginfo_t instead of struct signalfd_siginfo in
> procfd_signal()?
I think it means we still need more discussion. Using signalfd_siginfo
without further changes doesn't seem sufficient because of the missing
sigval and the excessive length adds some cost.
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
Arnd
^ permalink raw reply
* Re: [PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Florian Weimer @ 2018-11-30 13:40 UTC (permalink / raw)
To: Jürg Billeter
Cc: Andrew Morton, Oleg Nesterov, Thomas Gleixner, Eric Biederman,
Kees Cook, Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <20181130080004.23635-2-j@bitron.ch>
* Jürg Billeter:
> 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.
So this is inherited across regular execve? I'm not sure that's a good
idea.
> 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.
For double- or triple-forking daemons, the reparenting will be racy, if
I understand things correctly.
Thanks,
Florian
^ permalink raw reply
* Security modules and sending signals within the same process
From: Florian Weimer @ 2018-11-30 15:14 UTC (permalink / raw)
To: apparmor-nLRlyDuq1AZFpShjVBNYrg,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
selinux-+05T5uksL2qpZYMLLGbcSA, linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Arnd Bergmann, H. Peter Anvin
Is it guaranteed that tasks in the same thread group can always send
signals to each other, irrespective of their respective credentials
structs?
It's not clear to me whether this is always possible based on the
security_task_kill implementations I've examined.
I want to support per-thread setresuid/setresgid, but we also use
signals for inter-thread communication. This is mainly for thread
cancellation; the setxgid stuff isn't needed for threads with private
credentials. I wonder if I need to disable cancellation for threads
with such credentials.
Thanks,
Florian
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
^ permalink raw reply
* Re: Security modules and sending signals within the same process
From: Stephen Smalley @ 2018-11-30 16:02 UTC (permalink / raw)
To: Florian Weimer, apparmor-nLRlyDuq1AZFpShjVBNYrg,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
selinux-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Arnd Bergmann, H. Peter Anvin
In-Reply-To: <87lg5asilo.fsf-fjB847h8rq0pB0kWxzfTigCJwEvxM/w9@public.gmane.org>
On 11/30/18 10:14 AM, Florian Weimer wrote:
> Is it guaranteed that tasks in the same thread group can always send
> signals to each other, irrespective of their respective credentials
> structs?
>
> It's not clear to me whether this is always possible based on the
> security_task_kill implementations I've examined.
>
> I want to support per-thread setresuid/setresgid, but we also use
> signals for inter-thread communication. This is mainly for thread
> cancellation; the setxgid stuff isn't needed for threads with private
> credentials. I wonder if I need to disable cancellation for threads
> with such credentials.
(fixed selinux list address, which moved to vger)
Looks like commit 065add3941bd ("signals: check_kill_permission(): don't
check creds if same_thread_group()") skipped the uid-based checks if the
sender and target were in the same thread group, but not the security
hook call. One could argue that the security hook call ought to be
skipped in that case as well using the same rationale given in that
commit. Nothing appears to guarantee the property you state above for
security_task_kill implementations, although none of the in-tree users
are based on uids or gids so setresuid/setresgid shouldn't affect them.
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-30 16:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christian Brauner, Eric W. Biederman, Andrew Lutomirski,
Florian Weimer, LKML, Serge E. Hallyn, Jann Horn, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAK8P3a0kqPii5TwFAo_JHLX=o_FDMFVKXxgzzbDjLFZ7OQ5QCQ@mail.gmail.com>
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> siginfo_t as it is now still has a number of other downsides, and Andy in
> particular didn't like the idea of having three new variants on x86
> (depending on how you count). His alternative suggestion of having
> a single syscall entry point that takes a 'signfo_t __user *' but interprets
> it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> should work correctly, but feels wrong to me, or at least inconsistent
> with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants on
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
Obviously, I'd prefer a variant where the structure that's passed in
is always the same.
BTW, do we consider siginfo_t to be extensible? If so, and if we pass
in a pointer, presumably we should pass a length as well.
^ permalink raw reply
* Re: Security modules and sending signals within the same process
From: Casey Schaufler @ 2018-11-30 17:54 UTC (permalink / raw)
To: Florian Weimer, apparmor-nLRlyDuq1AZFpShjVBNYrg,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
selinux-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Arnd Bergmann, H. Peter Anvin
In-Reply-To: <87lg5asilo.fsf-fjB847h8rq0pB0kWxzfTigCJwEvxM/w9@public.gmane.org>
On 11/30/2018 7:14 AM, Florian Weimer wrote:
> Is it guaranteed that tasks in the same thread group can always send
> signals to each other, irrespective of their respective credentials
> structs?
No. An LSM may chose to disallow this based on just about any
criteria it desires.
> It's not clear to me whether this is always possible based on the
> security_task_kill implementations I've examined.
SELinux, Smack and AppArmor make their decisions based on
the task_struct credential, so if it's possible to change
the LSM attributes at the task granularity, it's possible
to have a process that can't always talk to itself.
> I want to support per-thread setresuid/setresgid,
That's pretty dangerous in its own right. Effectively
the process containing the threads has multiple UIDs.
That complicates the DAC model significantly.
> but we also use
> signals for inter-thread communication.
It's unfortunate that no one has seriously proposed
mode bits on processes for signal delivery. The UID
matching policy is inconvenient in a lot of cases.
Hmmm...
> This is mainly for thread
> cancellation; the setxgid stuff isn't needed for threads with private
> credentials. I wonder if I need to disable cancellation for threads
> with such credentials.
>
> Thanks,
> Florian
>
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
^ permalink raw reply
* Re: Security modules and sending signals within the same process
From: Florian Weimer @ 2018-11-30 18:00 UTC (permalink / raw)
To: Casey Schaufler
Cc: Arnd Bergmann, selinux-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, apparmor-nLRlyDuq1AZFpShjVBNYrg,
linux-security-module-u79uwXL29TY76Z2rM5mHXA, H. Peter Anvin
In-Reply-To: <2c3e813c-f56a-3354-1299-30b0646f40e1-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>
* Casey Schaufler:
> On 11/30/2018 7:14 AM, Florian Weimer wrote:
>> Is it guaranteed that tasks in the same thread group can always send
>> signals to each other, irrespective of their respective credentials
>> structs?
>
> No. An LSM may chose to disallow this based on just about any
> criteria it desires.
Hmm.
>> It's not clear to me whether this is always possible based on the
>> security_task_kill implementations I've examined.
>
> SELinux, Smack and AppArmor make their decisions based on
> the task_struct credential, so if it's possible to change
> the LSM attributes at the task granularity, it's possible
> to have a process that can't always talk to itself.
Yes, we already have this today for LSM attributes. We only paper over
this for the UIDs/GIDs, using a really awkward mechanism. Maybe we will
get rid of that one day, when the kernel supports a proper per-process
attribute, but I don't count on it.
>> I want to support per-thread setresuid/setresgid,
>
> That's pretty dangerous in its own right. Effectively
> the process containing the threads has multiple UIDs.
> That complicates the DAC model significantly.
Sure. But I think it's better to do this explicitly in glibc, rather
than file server authors calling the system calls directly. And it only
exposes what the kernel does anyway.
Thanks,
Florian
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
^ permalink raw reply
* Re: [PATCH v4 3/4] libbpf: add bpf_prog_test_run_xattr
From: Alexei Starovoitov @ 2018-11-30 21:21 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: ast, daniel, netdev, linux-api, ys114321
In-Reply-To: <20181128165312.19500-4-lmb@cloudflare.com>
On Wed, Nov 28, 2018 at 04:53:11PM +0000, Lorenz Bauer wrote:
> Add a new function, which encourages safe usage of the test interface.
> bpf_prog_test_run continues to work as before, but should be considered
> unsafe.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
..
> +
> +LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
it fails to be build due to:
LINK tools/testing/selftests/bpf/test_libbpf
(117) does NOT match with num of versioned symbols in testing/selftests/bpf/libbpf.so (116).
Please make sure all LIBBPF_API symbols are versioned in libbpf.map.
Please fixup and respin.
The rest looks good.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-30 21:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric W. Biederman, Andrew Lutomirski, Florian Weimer, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrW2aphWwEY4=hUwo_JBCvkQyMjxzxGd9FCW017kMLaMOQ@mail.gmail.com>
On December 1, 2018 5:35:45 AM GMT+13:00, Andy Lutomirski <luto@kernel.org> wrote:
>On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> siginfo_t as it is now still has a number of other downsides, and
>Andy in
>> particular didn't like the idea of having three new variants on x86
>> (depending on how you count). His alternative suggestion of having
>> a single syscall entry point that takes a 'signfo_t __user *' but
>interprets
>> it as compat_siginfo depending on
>in_compat_syscall()/in_x32_syscall()
>> should work correctly, but feels wrong to me, or at least
>inconsistent
>> with how we do this elsewhere.
>
>If everyone else is okay with it, I can get on board with three
>variants on x86. What I can't get on board with is *five* variants on
Thanks Andy, that helps a lot.
I'm okay with it. Does this require any additional changes to how the syscall
is currently hooked up?
>x86, which would be:
>
>procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
>
>syscall64 with nr == 335 (presumably): 64-bit
>
>syscall64 with nr == 548 | 0x40000000: x32
>
>syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
>true, behavior is arbitrary
>
>syscall64 with nr == 335 | 0x40000000: x32 entry, but
>in_compat_syscall() == false, behavior is arbitrary
>
>This mess isn't really Christian's fault -- it's been there for a
>while, but it's awful and I don't think we want to perpetuate it.
>
>Obviously, I'd prefer a variant where the structure that's passed in
>is always the same.
>
>BTW, do we consider siginfo_t to be extensible? If so, and if we pass
I would prefer if we could consider it extensible.
If so I would prefer if we could pass in a size argument.
>in a pointer, presumably we should pass a length as well.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Arnd Bergmann @ 2018-11-30 22:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: christian, Eric W . Biederman, Florian Weimer,
Linux Kernel Mailing List, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, cyphar, Al Viro,
Linux FS-devel Mailing List, Linux API, Daniel Colascione,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrW2aphWwEY4=hUwo_JBCvkQyMjxzxGd9FCW017kMLaMOQ@mail.gmail.com>
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > siginfo_t as it is now still has a number of other downsides, and Andy in
> > particular didn't like the idea of having three new variants on x86
> > (depending on how you count). His alternative suggestion of having
> > a single syscall entry point that takes a 'signfo_t __user *' but interprets
> > it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
> > should work correctly, but feels wrong to me, or at least inconsistent
> > with how we do this elsewhere.
>
> If everyone else is okay with it, I can get on board with three
> variants on x86. What I can't get on board with is *five* variants on
> x86, which would be:
>
> procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
>
> syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
> syscall64 with nr == 548 | 0x40000000: x32
>
> syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
> true, behavior is arbitrary
>
> syscall64 with nr == 335 | 0x40000000: x32 entry, but
> in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
> This mess isn't really Christian's fault -- it's been there for a
> while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
Arnd
^ 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