<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 9 January 2015 at 22:24, Bryce Harrington <<a href="mailto:bryce@osg.samsung.com" target="_blank">bryce@osg.samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Jan 09, 2015 at 11:33:57AM +0100, Marek Chalupa wrote: > Yeah and one more thing: Btw, any reason this is offlist? > PR_SET_PTRACER is since Linux 3.4. What happens with older kernels or with > Yama disabled? If I understand how the prctl handlers work, if Yama is not loaded then PR_SET_PTRACER will just be ignored if it is set. I'll cc author to be sure. Good point on pre-3.4 kernels. I'll #ifdef around it. > I suppose that prctl call fails but since the return code is not used, it > is ignored and everything continues like before without prctl, right? From my understanding of yama_lsm.c, there wouldn't be an error code in the case that Yama was not enabled (i.e. rc = 0). With Yama enabled, it looks like prctl will error if it can't get the tracer task, or in out of memory conditions. I suppose if the prctl call fails, we could bail out at that point. We wouldn't know whether a debugger is attached or not though, so perhaps in that case is_debugger_attached() should return 0? Looks like we return 0 in other error cases. </blockquote><div> </div>If prctl call fails, we still could get the information. It will fail on older kernels or when Yama is disabled, right? </div><div class="gmail_quote">So when either of this case is true, there are two things that could happen: </div><div class="gmail_quote"> * ptrace to parent is possible -- then everything is alright </div><div class="gmail_quote"> * permission to attach to parent is denied -- then if user will run the test in debugger, he must run it as a root, because debugger use ptrace too. But when it is running under root, than is_debugger_attached() can procceed normally, because ptrace has the permission to attach. If it is not running under root, we get this bug you found. </div><div class="gmail_quote">So the best solution IMO would be to check if ptrace in fork failed because another process is tracing the parent or because it does not have permissions. But looking to manpages of ptrace: both cases are indicated by EPERM, so this could be rather hard to do it. </div><div class="gmail_quote">So yes, probably returning 0 when prctl fails is ok. </div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Thanks for the review; will re-roll the patchset. Bryce <div class="HOEnZb"><div class="h5"> > Cheers, > Marek > > On 9 January 2015 at 11:26, Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > > > > > > > On 7 January 2015 at 21:34, Bryce Harrington <<a href="mailto:bryce@osg.samsung.com">bryce@osg.samsung.com</a>> > > 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: > >> <a href="http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2d514487faf188938a4ee4fb3464eeecfbdcf8eb" target="_blank">http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2d514487faf188938a4ee4fb3464eeecfbdcf8eb</a> > >> 2: > >> <a href="https://wiki.ubuntu.com/SecurityTeam/Roadmap/KernelHardening#ptrace_Protection" target="_blank">https://wiki.ubuntu.com/SecurityTeam/Roadmap/KernelHardening#ptrace_Protection</a> > >> > >> Signed-off-by: Bryce Harrington <<a href="mailto:bryce@osg.samsung.com">bryce@osg.samsung.com</a>> > >> --- > >> 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 > >> <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > >> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> > >> > > > > > > For both patches: > > > > Reviewed-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> > > > > </div></div></blockquote></div> </div></div>