[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 02:19:14 PST 2015
On 9 January 2015 at 22:24, Bryce Harrington <bryce at osg.samsung.com> 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?
>
> > 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.
>
If prctl call fails, we still could get the information. It will fail on
older kernels or when Yama is disabled, right?
So when either of this case is true, there are two things that could happen:
* ptrace to parent is possible -- then everything is alright
* 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.
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.
So yes, probably returning 0 when prctl fails is ok.
>
> 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.
> > >
> > >
> > >
> > >> 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>
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150110/f16471d2/attachment.html>
More information about the wayland-devel
mailing list