On 9 January 2015 at 23:11, Kees Cook <[email protected]> wrote: > 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? > > >
No, just clicked on the wrong button (reply instead of reply to all), sorry. I just resend it. > > > 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. > > > > > Thanks for the review; will re-roll the patchset. > > > > Bryce > > > > > Cheers, > > > Marek > > > > > > On 9 January 2015 at 11:26, Marek Chalupa <[email protected]> wrote: > > > > > > > > > > > > > > > On 7 January 2015 at 21:34, Bryce Harrington <[email protected]> > > > > 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 <[email protected]> > > > >> --- > > > >> 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. > > 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). > > 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 :) > -Kees > > > > > > > > > > > > > > > > >> waitpid(pid, &status, 0); > > > >> rc = WEXITSTATUS(status); > > > >> } > > > >> -- > > > >> 1.9.1 > > > >> > > > >> _______________________________________________ > > > >> wayland-devel mailing list > > > >> [email protected] > > > >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > > >> > > > > > > > > > > > > For both patches: > > > > > > > > Reviewed-by: Marek Chalupa <[email protected]> > > > > > > > > > > -- > Kees Cook @outflux.net > Thanks, Marek
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
