[PATCH wayland 2/2 v2] tests: Fix FAIL in sanity-test (*timeout*) when Yama LSM enabled

[Marek Chalupa]

Just resending to wayland-devel...

On 9 January 2015 at 11:26, Marek Chalupa <mchqwerty at gmail.com> wrote:

>
>
> On 7 January 2015 at 21:34, Bryce Harrington <bryce at osg.samsung.com>
> wrote:
>
>> This fixes a regression in the testsuite since c3653f7f, where four of
>> the timeout tests fail with "Timeouts suppressed" messages.
>>
>> The timeouts are being suppressed because the testsuite is erroneously
>> detecting that a debugger is attached.  This detection mechanism
>> (adopted from libinput) uses ptrace to test if there is a debugger
>> parent process that can be attached.  Unfortunately, this is an
>> unreliable test: Kernel security policies exist to restrict the scope of
>> ptrace to prevent processes from snooping on one another.[1] This
>> security policy is set as the default on Ubuntu, and potentially other
>> Linux distributions.[2]
>>
>> The Yama documentation suggests, "For software that has defined
>> application-specific relationships between a debugging process and its
>> inferior (crash handlers, etc), prctl(PR_SET_PTRACER, pid, ...) can be
>> used.  An inferior can declare which other process (and its descendents)
>> are allowed to call PTRACE_ATTACH against it."
>>
>> The child needs to be synchronized to the client to prevent a race
>> condition where the child might try to operate before the parent has
>> finished its prctl call.  This synchronization is done via pipes.
>>
>> 1:
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2d514487faf188938a4ee4fb3464eeecfbdcf8eb
>> 2:
>> https://wiki.ubuntu.com/SecurityTeam/Roadmap/KernelHardening#ptrace_Protection
>>
>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
>> ---
>>  tests/test-runner.c | 35 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/test-runner.c b/tests/test-runner.c
>> index b813b69..11d064e 100644
>> --- a/tests/test-runner.c
>> +++ b/tests/test-runner.c
>> @@ -34,6 +34,7 @@
>>  #include <errno.h>
>>  #include <limits.h>
>>  #include <sys/ptrace.h>
>> +#include <linux/prctl.h>
>>
>
> Better use <sys/prctl.h>. prctl() is declared in sys/prctl.h.
> linux/prctl.h only defines the values for arguments (and is included by
> sys/prctl.h)
>
>
>>
>>  #include "test-runner.h"
>>
>> @@ -255,13 +256,33 @@ is_debugger_attached(void)
>>  {
>>         int status;
>>         int rc;
>> -       pid_t pid = fork();
>> +       pid_t pid;
>> +       int pipefd[2];
>>
>> -       if (pid == -1)
>> +       if (pipe(pipefd) == -1) {
>> +               perror("pipe");
>>                 return 0;
>> +       }
>> +
>> +       pid = fork();
>> +       if (pid == -1) {
>> +               perror("fork");
>> +               close(pipefd[0]);
>> +               close(pipefd[1]);
>> +               return 0;
>> +       }
>>
>>         if (pid == 0) {
>> -               pid_t ppid = getppid();
>> +               char buf;
>> +               pid_t ppid;
>> +
>> +               /* Wait until parent is ready */
>> +               close(pipefd[1]); /* Close unused write end */
>> +               while (read(pipefd[0], &buf, 1) > 0);
>>
> +               close(pipefd[0]);
>> +
>> +               /* Attempt to ptrace the parent */
>> +               ppid = getppid();
>>                 rc = ptrace(PTRACE_ATTACH, ppid, NULL, NULL);
>>                 if (rc == 0) {
>>                         waitpid(ppid, NULL, 0);
>> @@ -273,6 +294,14 @@ is_debugger_attached(void)
>>                 }
>>                 _exit(rc);
>>         } else {
>> +               close(pipefd[0]);
>> +
>> +               /* Enable child to ptrace the parent process */
>> +               prctl(PR_SET_PTRACER, pid);
>> +
>> +               /* Signal to client that parent is ready */
>> +               close(pipefd[1]);
>>
>
> Maybe this would deserve better description, because signaling something
> by closing the pipe is not clear IMO.
>
>
>
>>                 waitpid(pid, &status, 0);
>>                 rc = WEXITSTATUS(status);
>>         }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
>
> For both patches:
>
> Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150110/a9ebf7ba/attachment.html>