Re: [PATCH] Add UNSUPPORTED check in elf/tst-pldd.

[Stefan Liebler <stli at linux dot ibm dot com>]

On 9/17/19 5:17 PM, Stefan Liebler wrote:
> On 9/17/19 3:31 PM, Adhemerval Zanella wrote:
> > On 02/09/2019 12:28, Stefan Liebler wrote:
> > > On 8/29/19 10:47 AM, Florian Weimer wrote:
> > > > * Stefan Liebler:
> > > >
> > > > > On 8/28/19 11:24 AM, Florian Weimer wrote:
> > > > > > * Stefan Liebler:
> > > > > >
> > > > > > >     static void
> > > > > > >     target_process (void *arg)
> > > > > > >     {
> > > > > > > +  if (ptrace_scope == 1)
> > > > > > > +    {
> > > > > > > +      /* YAMA is configured to "restricted ptrace".
> > > > > > > +     Disable the restriction for this subprocess.  */
> > > > > > > +      support_ptrace_process_set_ptracer_any ();
> > > > > > > +    }
> > > > > > > +
> > > > > > >       pause ();
> > > > > > >     }
> > > > > >
> > > > > > I think this has a race condition if pldd attaches to the process 
> > > > > > before
> > > > > > the support_ptrace_process_set_ptracer_any call.  I have no idea how
> > > > > > hard it is in practice to hit this race.  It should be possible to 
> > > > > > use a
> > > > > > process-shared barrier or some other form of synchronization to avoid
> > > > > > this issue.
> > > > > >
> > > > > > Thanks,
> > > > > > Florian
> > > > >
> > > > > I've added a synchronization with stdatomic.h on a shared memory 
> > > > > mapping.
> > > > > I've not used pthread* functions as I don't want to link against
> > > > > libpthread.so. Then further adjustments are needed.
> > > > >
> > > > > Or should I just restrict the test ptrace_scope 0 as Adhemerval has
> > > > > proposed in his post?
> > > >
> > > > Is it possible to create a process tree like this?
> > > >
> > > >
> > > >     parent (performs output checks)
> > > >       subprocess 1 (becomes pldd via execve)
> > > >         subprocess 2
> > > >
> > > > If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
> > > > ptrace scope for ptrace_scope < 2?
> > > Yes, this is possible.
> > > I've rearranged the subprocesses. See attached patch.
> > > Now we have a new function pldd_process which forks target_process,
> > > stores the pid of target_prcess to a shared memory mapping as do_test 
> > > needs to know this pid.
> > >
> > > Afterwards it execve to pldd which successfully ptrace target_process 
> > > in case of "restricted ptrace".
> > >
> > > Please review the usage of support-subprocess-functions.
> > >
> > > Bye,
> > > Stefan
> > > > Thanks,
> > > > Florian
> > >
> > >
> > > 20190902_tst-pldd.patch
> > >
> > > commit ad51263d51d12ce6ca2ce9304efe5ba05b3912b1
> > > Author: Stefan Liebler <stli@linux.ibm.com>
> > > Date:   Mon Aug 26 15:45:07 2019 +0200
> > >
> > >      Add UNSUPPORTED check in elf/tst-pldd.
> > >      The testcase forks a child process and runs pldd with PID of
> > >      this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
> > >      differs from zero, pldd will fail with
> > >      /usr/bin/pldd: cannot attach to process 3: Operation not permitted
> > >      This patch checks if ptrace_scope exists, is zero "classic 
> > > ptrace permissions"
> > >      or one "restricted ptrace".  If ptrace_scope exists and has a 
> > > higher
> > >      restriction, then the test is marked as UNSUPPORTED.
> > >      The case "restricted ptrace" is handled by rearranging the 
> > > processes involved
> > >      during the test.  Now we have the following process tree:
> > >      -parent: do_test (performs output checks)
> > >      --subprocess 1: pldd_process (becomes pldd via execve)
> > >      ---subprocess 2: target_process (ptraced via pldd)
> > >      ChangeLog:
> > >              * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
> > >              Rearrange subprocesses.
> > >              (pldd_process): New function.
> > >              * support/Makefile (libsupport-routines): Add 
> > > support_ptrace.
> > >              * support/ptrace.h: New file.
> > >              * support/support_ptrace.c: Likewise.
> >
> > LGTM with just a change below, thanks.
> >
> > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ...
> > > diff --git a/support/ptrace.h b/support/ptrace.h
> > > new file mode 100644
> > > index 0000000000..90006a6b75
> > > --- /dev/null
> > > +++ b/support/ptrace.h
> > > @@ -0,0 +1,32 @@
> > > +/* Support functions handling ptrace_scope.
> > > +   Copyright (C) 2019 Free Software Foundation, Inc.
> > > +   This file is part of the GNU C Library.
> > > +
> > > +   The GNU C Library is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU Lesser General Public
> > > +   License as published by the Free Software Foundation; either
> > > +   version 2.1 of the License, or (at your option) any later version.
> > > +
> > > +   The GNU C Library is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +   Lesser General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU Lesser General Public
> > > +   License along with the GNU C Library; if not, see
> > > +   <http://www.gnu.org/licenses/>.  */
> > > +
> > > +#ifndef SUPPORT_PTRACE_H
> > > +#define SUPPORT_PTRACE_H
> > > +
> > > +#include <sys/cdefs.h>
> > > +
> > > +__BEGIN_DECLS
> > > +
> > > +/* Return the current YAMA mode set on the machine (0 to 3) or -1
> > > +   if YAMA is not supported.  */
> > > +int support_ptrace_scope (void);
> > > +
> > > +__END_DECLS
> > > +
> > > +#endif
> >
> > I think it should named xptrace.h.
> ...
> Okay. Then I will rename it to support/xptrace.h and if nobody opposes, 
> I'll commit it tomorrow.
>
> Thanks for the review.
> Stefan
Committed with xptrace.h instead of ptrace.h.