<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 9 January 2015 at 23:11, Kees Cook <<a href="mailto:kees@outflux.net" target="_blank">kees@outflux.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Bryce, On Fri, Jan 09, 2015 at 01:24:39PM -0800, Bryce Harrington wrote: > On Fri, Jan 09, 2015 at 11:33:57AM +0100, Marek Chalupa wrote: > > Yeah and one more thing: > > Btw, any reason this is offlist? > </blockquote><div> </div><div>No, just clicked on the wrong button (reply instead of reply to all), sorry. I just resend it. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > 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. The prctl will return EINVAL if Yama is not loaded. (i.e. just ignore the prctl). > > Good point on pre-3.4 kernels. I'll #ifdef around it. I recommend: #include <sys/prctl.h> #ifndef PR_SET_PTRACER # define PR_SET_PTRACER 0x59616d61 #endif > > > 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. If the prctl fails, there's no Yama, so no need for the prctl, so it can be ignored. <div><div class="h5"> > > Thanks for the review; will re-roll the patchset. > > Bryce > > > 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. </div></div>Since the pipe is blocking, it should be sufficient. I don't think it's needed to actually perform a write. The read will block until the close, and on close, the read will return 0, which is what it's waiting for (or for an error). </blockquote><div>Yep, I understand how it works, but I had to go thourgh the code back and forth to make myself sure that I get it. Therefore I suggested to add a comment that explains how the signaling works :) </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> -Kees <div class=""><div class="h5"> > > > > > > > > > > > >> 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>-- Kees Cook @<a href="http://outflux.net" target="_blank">outflux.net</a> </blockquote><div> </div><div>Thanks, </div><div>Marek </div></div> </div></div>