<div dir="ltr"><div><div><div><div><div><div><div><div>Thx for the review. Indeed I double checked and prctl should be blocked. It seems in chromium gpu process and plugin process, they enable it that why we did too. Though there is a todo saying to block it :)<br></div>In our case we can directly block it.<br><br></div>For my patch it means the condition<span class="im"> "prctl(PR_GET_SECCOMP"</span> should go away and the todo too.<br><br></div>I am also wondering why pulseaudio block signals "pthread_sigmask(SIG_BLOCK, &mask, NULL);"<br><br></div>AFAIK glib does not do that. Also is pa_threaded_mainloop_start called in pa sever too ? Maybe it should be applied to the server only. Worth to mention that pthread_sigmask(SIG_BLOCK, &mask, NULL); has last been touched in 2006. I am not saying this a reason at all but just in case :)<br><br></div>Also note that glib only does it for explicit unix source: "g_unix_signal_source_new". Though they do it differently, see "g_get_worker_context", they block everything and then restore previous mask. Note that "pthread_sigmask (SIG_SETMASK" is similar to "pthread_sigmask(SIG_BLOCK", see man for the details.<br></div><div>Maybe pulseaudio should restore the previous mask too ?<br></div><div><br></div>In chromium the block everything by default and unblock SIGSYG whenever a handler is attached to a system call, see: chromium/src/sandbox/linux/seccomp-bpf/trap.cc::Trap::Trap<br><br></div>Anyway I think for the pulse audio client, the mask should be inherited from application.<br></div>So that the patch would become:<br><br> sigset_t mask;<br> sigset_t prev_mask;<br><br> /* Make sure that signals are delivered to the main thread */<br> sigfillset(&mask);<br> pthread_sigmask(SIG_BLOCK, &mask, &prev_mask);<br><br> if (!sigismember(&prev_mask, SIGSYS)) { // <- check if parent has unblocked the SIGSYS signal .<br> int r = sigemptyset(&mask) || sigaddset(&mask, SIGSYS) || pthread_sigmask(SIG_UNBLOCK, &mask, NULL);<br> pa_assert(!r);<br> }<br><div><div><div><div><div> <br><div><div>Problem is that if I replace SIGSYS by any other signal, it still goes into the block. So it means no signal are blocked in prev_mask which is wrong. Any idea ?<br><br></div><div>Also I'll add what you suggested header presence or not, and configure check.<br><br></div><div>Thx<br></div><div>Julien<br></div><div><br><br><br><br></div></div></div></div></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 19 October 2015 at 04:35, Arun Raghavan <span dir="ltr"><<a href="mailto:arun@accosted.net" target="_blank">arun@accosted.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Sat, 2015-10-10 at 20:11 +0100, Julien Isorce wrote:<br>
> Seccomp-BPF actually uses SIGSYS to trigger<br>
> the trap handler attached to sys_open.<br>
> If the signal is blocked then the kernel kills<br>
> the process whenever pulse audio calls 'open'.<br>
> The result backtrace is terminating in sys_open.<br>
><br>
> This is required to have pulse audio working<br>
> in a sandbox.<br>
> ---<br>
> src/pulse/thread-mainloop.c | 10 ++++++++++<br>
> 1 file changed, 10 insertions(+)<br>
><br>
> diff --git a/src/pulse/thread-mainloop.c b/src/pulse/thread<br>
> -mainloop.c<br>
> index afd0581..93582d2 100644<br>
> --- a/src/pulse/thread-mainloop.c<br>
> +++ b/src/pulse/thread-mainloop.c<br>
> @@ -28,6 +28,8 @@<br>
><br>
> #include <signal.h><br>
> #include <stdio.h><br>
> +#include <sys/prctl.h><br>
<br>
</span>This needs to be in a #ifdef HAVE_SYS_PRCTL_H (which is already<br>
defined).<br>
<br>
> +#include <linux/seccomp.h><br>
<br>
You need to add a configure-time header check for this one and then<br>
make the include conditional, as we need to make sure we build on<br>
machines without seccomp (which includes non-Linux systems too).<br>
<span class=""><br>
><br>
> #include <pulse/xmalloc.h><br>
> #include <pulse/mainloop.h><br>
> @@ -81,6 +83,14 @@ static void thread(void *userdata) {<br>
> /* Make sure that signals are delivered to the main thread */<br>
> sigfillset(&mask);<br>
> pthread_sigmask(SIG_BLOCK, &mask, NULL);<br>
> +<br>
> + /* If seccomp is in use, only filter mode has a chance to work.<br>
> + * Because pa requires sys_open. */<br>
> + if (prctl(PR_GET_SECCOMP, SECCOMP_MODE_FILTER, NULL) == 2) {<br>
<br>
</span>Is the second argument of PR_GET_SETCOMP ever used? The man page<br>
suggests that it takes no arguments.<br>
<br>
Also, I see that if prctl() is not allowed, the process will be killed.<br>
Is there any condition where prctl() might not be allowed by seccomp,<br>
but we might still be able to function correctly?<br>
<span class=""><br>
> + /* TODO: unblock SIGSYS only if a trap is attached to<br>
> sys_open. */<br>
<br>
</span>Could you clarify what needs to be done for this TODO to go away?<br>
<span class=""><br>
> + int r = sigemptyset(&mask) || sigaddset(&mask, SIGSYS) ||<br>
> pthread_sigmask(SIG_UNBLOCK, &mask, NULL);<br>
> + pa_assert(!r);<br>
> + }<br>
<br>
</span>This entire condition would then be within a conditional for the<br>
presence of prctl.h and seccomp.h.<br>
<br>
> #endif<br>
><br>
> pa_mutex_lock(m->mutex);<br>
<br>
-- Arun<br>
_______________________________________________<br>
pulseaudio-discuss mailing list<br>
<a href="mailto:pulseaudio-discuss@lists.freedesktop.org">pulseaudio-discuss@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss</a><br>
</blockquote></div><br></div>