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

[Marek Chalupa]

On 10 January 2015 at 01:05, 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."  This prctl call has no
> effect if Yama LSM is not loaded.
>
> 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.
>
> This patch can be tested by running sanity-test with
> /proc/sys/kernel/yama/ptrace_scope set to 0 or 1; the test must pass for
> either value.
>
> 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>
> Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  tests/test-runner.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 48e9a22..4750038 100644
> --- a/tests/test-runner.c
> +++ b/tests/test-runner.c
> @@ -34,6 +34,10 @@
>  #include <errno.h>
>  #include <limits.h>
>  #include <sys/ptrace.h>
> +#include <sys/prctl.h>
> +#ifndef PR_SET_PTRACER
> +# define PR_SET_PTRACER 0x59616d61
> +#endif
>
>  #include "test-runner.h"
>
> @@ -249,16 +253,30 @@ stderr_reset_color(void)
>
>  /* this function is taken from libinput/test/litest.c
>   * (rev 028513a0a723e97941c39)
> + *
> + * Returns: 1 if a debugger is confirmed present; 0 if no debugger is
> + * present or if it can't be determined.
>   */
>  static int
>  is_debugger_attached(void)
>  {
>         int status;
>         int rc;
> -       pid_t pid = fork();
> +       pid_t pid;
> +       int pipefd[2];
> +
> +       if (pipe(pipefd) == -1) {
> +               perror("pipe");
> +               return 0;
> +       }
>
> -       if (pid == -1)
> +       pid = fork();
> +       if (pid == -1) {
> +               perror("fork");
> +               close(pipefd[0]);
> +               close(pipefd[1]);
>                 return 0;
> +       }
>
>         if (pid == 0) {
>                 pid_t ppid = getppid();
> @@ -272,6 +290,25 @@ is_debugger_attached(void)
>                 }
>                 _exit(rc);
>         } else {
> +               close(pipefd[0]);
> +
> +               /* Enable child to ptrace the parent process */
> +               rc = prctl(PR_SET_PTRACER, pid);
> +               if (rc != 0 && rc != EINVAL) {
> +                       /* An error prevents us from telling if a debugger
> is attached.
> +                        * Instead of propagating the error, assume no
> debugger present.
> +                        * But note the error to the log as a clue for
> troubleshooting.
> +                        */
> +                       perror("prctl");
> +                       close(pipefd[1]);
> +                       return 0;
>

Here we're creating a zombie, aren't we?


> +               }
> +
> +               /* Signal to client that parent is ready.  No need to write
> +                * anything to the child; the closure of the pipe will be
> +                * sufficient to signal we're ready. */
> +               close(pipefd[1]);
> +
>                 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
>


Thanks for the added comments :)

Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150110/ef0c61b4/attachment-0001.html>