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

[Stefan Liebler <stli@linux.ibm.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.