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

[Derek Foreman]

On 05/02/15 07:39 PM, Bryce Harrington 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>
> 
> v4: Allow parent to communicate error state to child to prevent leaving
> child in zombie state if parent hits an error.
> 
> v5: Check errno instead of rc for error.  Don't waitpid on ppid.
> (I guess these two coding errors canceled out for me or something.)

FWIW, the waitpid thing was never a bug, I errantly called it one in an
off list conversation.  It'll work fine the way it is now, or the way it
was.

Either way you choose:
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>


> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  tests/test-runner.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 48e9a22..f031a42 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,29 +253,65 @@ 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 (pid == -1)
> +	if (pipe(pipefd) == -1) {
> +		perror("pipe");
>  		return 0;
> +	}
>  
> -	if (pid == 0) {
> +	pid = fork();
> +	if (pid == -1) {
> +		perror("fork");
> +		close(pipefd[0]);
> +		close(pipefd[1]);
> +		return 0;
> +	} else if (pid == 0) {
> +		char buf;
>  		pid_t ppid = getppid();
> -		if (ptrace(PTRACE_ATTACH, ppid, NULL, NULL) == 0) {
> -			waitpid(ppid, NULL, 0);
> -			ptrace(PTRACE_CONT, NULL, NULL);
> -			ptrace(PTRACE_DETACH, ppid, NULL, NULL);
> -			rc = 0;
> +
> +		/* Wait until parent is ready */
> +		close(pipefd[1]);  /* Close unused write end */
> +		read(pipefd[0], &buf, 1);
> +		close(pipefd[0]);
> +		if (buf == '-')
> +			_exit(1);
> +		if (ptrace(PTRACE_ATTACH, ppid, NULL, NULL) != 0)
> +			_exit(1);
> +		if (!waitpid(-1, NULL, 0))
> +			_exit(1);
> +		ptrace(PTRACE_CONT, NULL, NULL);
> +		ptrace(PTRACE_DETACH, ppid, NULL, NULL);
> +		_exit(0);
> +	} else {
> +		close(pipefd[0]);
> +
> +		/* Enable child to ptrace the parent process */
> +		rc = prctl(PR_SET_PTRACER, pid);
> +		if (rc != 0 && errno != 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.
> +			 * Then flag the error state to the client by sending '-'.
> +			 */
> +			perror("prctl");
> +			write(pipefd[1], "-", 1);
>  		} else {
> -			rc = 1;
> +			/* Signal to client that parent is ready by passing '+' */
> +			write(pipefd[1], "+", 1);
>  		}
> -		_exit(rc);
> -	} else {
> +		close(pipefd[1]);
> +
>  		waitpid(pid, &status, 0);
>  		rc = WEXITSTATUS(status);
>  	}
>