[pulseaudio-discuss] [PATCH 2/2] thread-mainloop: unblock SIGSYS on sandbox

[Julien Isorce]

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 :)
In our case we can directly block it.

For my patch it means the condition "prctl(PR_GET_SECCOMP" should go away
and the todo too.

I am also wondering why pulseaudio block signals
"pthread_sigmask(SIG_BLOCK, &mask, NULL);"

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 :)

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.
Maybe pulseaudio should restore the previous mask too ?

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

Anyway I think for the pulse audio client, the mask should be inherited
from application.
So that the patch would become:

    sigset_t mask;
    sigset_t prev_mask;

    /* Make sure that signals are delivered to the main thread */
    sigfillset(&mask);
    pthread_sigmask(SIG_BLOCK, &mask, &prev_mask);

    if (!sigismember(&prev_mask, SIGSYS)) {  // <- check if parent has
unblocked the SIGSYS signal .
        int r = sigemptyset(&mask) || sigaddset(&mask, SIGSYS) ||
pthread_sigmask(SIG_UNBLOCK, &mask, NULL);
        pa_assert(!r);
    }

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 ?

Also I'll add what you suggested header presence or not, and configure
check.

Thx
Julien





On 19 October 2015 at 04:35, Arun Raghavan <arun at accosted.net> wrote:

> On Sat, 2015-10-10 at 20:11 +0100, Julien Isorce wrote:
> > Seccomp-BPF actually uses SIGSYS to trigger
> > the trap handler attached to sys_open.
> > If the signal is blocked then the kernel kills
> > the process whenever pulse audio calls 'open'.
> > The result backtrace is terminating in sys_open.
> >
> > This is required to have pulse audio working
> > in a sandbox.
> > ---
> >  src/pulse/thread-mainloop.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/src/pulse/thread-mainloop.c b/src/pulse/thread
> > -mainloop.c
> > index afd0581..93582d2 100644
> > --- a/src/pulse/thread-mainloop.c
> > +++ b/src/pulse/thread-mainloop.c
> > @@ -28,6 +28,8 @@
> >
> >  #include <signal.h>
> >  #include <stdio.h>
> > +#include <sys/prctl.h>
>
> This needs to be in a #ifdef HAVE_SYS_PRCTL_H (which is already
> defined).
>
> > +#include <linux/seccomp.h>
>
> You need to add a configure-time header check for this one and then
> make the include conditional, as we need to make sure we build on
> machines without seccomp (which includes non-Linux systems too).
>
> >
> >  #include <pulse/xmalloc.h>
> >  #include <pulse/mainloop.h>
> > @@ -81,6 +83,14 @@ static void thread(void *userdata) {
> >      /* Make sure that signals are delivered to the main thread */
> >      sigfillset(&mask);
> >      pthread_sigmask(SIG_BLOCK, &mask, NULL);
> > +
> > +    /* If seccomp is in use, only filter mode has a chance to work.
> > +     * Because pa requires sys_open. */
> > +    if (prctl(PR_GET_SECCOMP, SECCOMP_MODE_FILTER, NULL) == 2) {
>
> Is the second argument of PR_GET_SETCOMP ever used? The man page
> suggests that it takes no arguments.
>
> Also, I see that if prctl() is not allowed, the process will be killed.
> Is there any condition where prctl() might not be allowed by seccomp,
> but we might still be able to function correctly?
>
> > +        /* TODO: unblock SIGSYS only if a trap is attached to
> > sys_open. */
>
> Could you clarify what needs to be done for this TODO to go away?
>
> > +        int r = sigemptyset(&mask) || sigaddset(&mask, SIGSYS) ||
> > pthread_sigmask(SIG_UNBLOCK, &mask, NULL);
> > +        pa_assert(!r);
> > +    }
>
> This entire condition would then be within a conditional for the
> presence of prctl.h and seccomp.h.
>
> >  #endif
> >
> >      pa_mutex_lock(m->mutex);
>
> -- Arun
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20151019/5f29c713/attachment-0001.html>