[PATCH wayland 2/2 v2] tests: Fix FAIL in sanity-test (*timeout*) when Yama LSM enabled
Marek Chalupa
mchqwerty at gmail.com
Sat Jan 10 01:51:26 PST 2015
On 9 January 2015 at 23:11, Kees Cook <kees at outflux.net> 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 <mchqwerty at gmail.com> wrote:
> > >
> > > >
> > > >
> > > > On 7 January 2015 at 21:34, 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."
> > > >>
> > > >> 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 <bryce at osg.samsung.com>
> > > >> ---
> > > >> 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
> > > >> wayland-devel at lists.freedesktop.org
> > > >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > > >>
> > > >
> > > >
> > > > For both patches:
> > > >
> > > > Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>
> > > >
> > > >
>
> --
> Kees Cook @outflux.net
>
Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150110/05a76d66/attachment-0001.html>
More information about the wayland-devel
mailing list