Patchwork Add UNSUPPORTED check in elf/tst-pldd.

login
register
mail settings
Submitter Stefan Liebler
Date Aug. 28, 2019, 2:42 p.m.
Message ID <1d419974-c973-c4c1-f1cd-4bbbf8b074f8@linux.ibm.com>
Download mbox | patch
Permalink /patch/34300/
State New
Headers show

Comments

Stefan Liebler - Aug. 28, 2019, 2:42 p.m.
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?

Bye.
Stefan
Florian Weimer - Aug. 29, 2019, 8:47 a.m.
* 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?

Thanks,
Florian
Adhemerval Zanella Netto - Sept. 2, 2019, 7:37 p.m.
On 29/08/2019 05:47, 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?

Do we really need that ad-hoc support on tst-pldd to make it support 
ptrace_scope 1?

I don't oppose the support Stefan has added on latest iteration to
make it work, but this is a lot of code to support a very specific
scenario...
Stefan Liebler - Sept. 3, 2019, 6:30 a.m.
On 9/2/19 9:37 PM, Adhemerval Zanella wrote:
> 
> 
> On 29/08/2019 05:47, 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?
> 
> Do we really need that ad-hoc support on tst-pldd to make it support
> ptrace_scope 1?
> 
> I don't oppose the support Stefan has added on latest iteration to
> make it work, but this is a lot of code to support a very specific
> scenario...
> 
As there are systems where ptrace_scope is configured to 1 by default, 
we should adjust the testcase as the FAIL is misleading.
(I've just recognized that Steve Ellcey had also seen this FAIL: 
https://www.sourceware.org/ml/libc-alpha/2019-07/msg00618.html)

The minimum change should be to detect ptrace_scope == 1 and mark the 
test as UNSUPPORTED. Or we change a bit more and let the test also run 
in this scenario. (Either by support_ptrace_process_set_ptracer_any or 
adjusting the subprocess-tree)

Bye
Stefan
Adhemerval Zanella Netto - Sept. 3, 2019, 1:34 p.m.
On 03/09/2019 03:30, Stefan Liebler wrote:
> On 9/2/19 9:37 PM, Adhemerval Zanella wrote:
>>
>>
>> On 29/08/2019 05:47, 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?
>>
>> Do we really need that ad-hoc support on tst-pldd to make it support
>> ptrace_scope 1?
>>
>> I don't oppose the support Stefan has added on latest iteration to
>> make it work, but this is a lot of code to support a very specific
>> scenario...
>>
> As there are systems where ptrace_scope is configured to 1 by default, we should adjust the testcase as the FAIL is misleading.
> (I've just recognized that Steve Ellcey had also seen this FAIL: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00618.html)
> 
> The minimum change should be to detect ptrace_scope == 1 and mark the test as UNSUPPORTED. Or we change a bit more and let the test also run in this scenario. (Either by support_ptrace_process_set_ptracer_any or adjusting the subprocess-tree)

Yes, my initial suggestion was just to make it as UNSUPPORTED for ptrace_scope >= 1.
But I do not oppose adjusting it to run on ptrace_scope 1, it is just that
the required hackery lead to make it somewhat as complex than the test itself.
Carlos O'Donell - Sept. 6, 2019, 3:21 a.m.
On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
> Yes, my initial suggestion was just to make it as UNSUPPORTED for
> ptrace_scope >= 1. But I do not oppose adjusting it to run on
> ptrace_scope 1, it is just that the required hackery lead to make it
> somewhat as complex than the test itself.

The flip side of the coin is that the more "UNSUPPORTED" results we
add *implies* there is "one valid way" to setup a glibc test run
and we don't clearly document how to turn all the "UNSUPPORTED"
entries into supported tests?

Stefan's code can at least be refactored into support/ if we need
to do the same thing again in another test.
Stefan Liebler - Sept. 10, 2019, 8:46 a.m.
On 9/6/19 5:21 AM, Carlos O'Donell wrote:
> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>> ptrace_scope 1, it is just that the required hackery lead to make it
>> somewhat as complex than the test itself.
> 
> The flip side of the coin is that the more "UNSUPPORTED" results we
> add *implies* there is "one valid way" to setup a glibc test run
> and we don't clearly document how to turn all the "UNSUPPORTED"
> entries into supported tests?
> 
> Stefan's code can at least be refactored into support/ if we need
> to do the same thing again in another test.
> 

PING.

As I have already posted multiple versions of the patch, how to proceed?
1) UNSUPPORTED if support_ptrace_scope() >= 2;
Support support_ptrace_scope() == 1
by adjusting the process tree;
(see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)

2) UNSUPPORTED if support_ptrace_scope() >= 2;
Support support_ptrace_scope() == 1
by calling support_ptrace_process_set_ptracer_any();
(see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)

3) UNSUPPORTED if support_ptrace_scope() != 0
(patch would use support_ptrace_scope() of one of the patches above in 
order to trigger FAIL_UNSUPPORTED)

Bye,
Stefan
Adhemerval Zanella Netto - Sept. 10, 2019, 1:32 p.m.
On 10/09/2019 05:46, Stefan Liebler wrote:
> On 9/6/19 5:21 AM, Carlos O'Donell wrote:
>> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>>> ptrace_scope 1, it is just that the required hackery lead to make it
>>> somewhat as complex than the test itself.
>>
>> The flip side of the coin is that the more "UNSUPPORTED" results we
>> add *implies* there is "one valid way" to setup a glibc test run
>> and we don't clearly document how to turn all the "UNSUPPORTED"
>> entries into supported tests?
>>
>> Stefan's code can at least be refactored into support/ if we need
>> to do the same thing again in another test.
>>
> 
> PING.
> 
> As I have already posted multiple versions of the patch, how to proceed?
> 1) UNSUPPORTED if support_ptrace_scope() >= 2;
> Support support_ptrace_scope() == 1
> by adjusting the process tree;
> (see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)
> 
> 2) UNSUPPORTED if support_ptrace_scope() >= 2;
> Support support_ptrace_scope() == 1
> by calling support_ptrace_process_set_ptracer_any();
> (see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)
> 
> 3) UNSUPPORTED if support_ptrace_scope() != 0
> (patch would use support_ptrace_scope() of one of the patches above in order to trigger FAIL_UNSUPPORTED)

My view is although 2) is way complex that I would like, I think it should
the more complete solution. Does still need review or is it ready to land?
Stefan Liebler - Sept. 11, 2019, 7:05 a.m.
On 9/10/19 3:32 PM, Adhemerval Zanella wrote:
> 
> 
> On 10/09/2019 05:46, Stefan Liebler wrote:
>> On 9/6/19 5:21 AM, Carlos O'Donell wrote:
>>> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>>>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>>>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>>>> ptrace_scope 1, it is just that the required hackery lead to make it
>>>> somewhat as complex than the test itself.
>>>
>>> The flip side of the coin is that the more "UNSUPPORTED" results we
>>> add *implies* there is "one valid way" to setup a glibc test run
>>> and we don't clearly document how to turn all the "UNSUPPORTED"
>>> entries into supported tests?
>>>
>>> Stefan's code can at least be refactored into support/ if we need
>>> to do the same thing again in another test.
>>>
>>
>> PING.
>>
>> As I have already posted multiple versions of the patch, how to proceed?
>> 1) UNSUPPORTED if support_ptrace_scope() >= 2;
>> Support support_ptrace_scope() == 1
>> by adjusting the process tree;
>> (see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)
>>
>> 2) UNSUPPORTED if support_ptrace_scope() >= 2;
>> Support support_ptrace_scope() == 1
>> by calling support_ptrace_process_set_ptracer_any();
>> (see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)
>>
>> 3) UNSUPPORTED if support_ptrace_scope() != 0
>> (patch would use support_ptrace_scope() of one of the patches above in order to trigger FAIL_UNSUPPORTED)
> 
> My view is although 2) is way complex that I would like, I think it should
> the more complete solution. Does still need review or is it ready to land?
> 
You've already reviewed the support part on a previous version of "patch 
2)" (the support part was not changed in the latest version; see 
https://www.sourceware.org/ml/libc-alpha/2019-08/msg00703.html).

But it needs review for the synchronization part between the 
target_process and do_test in tst-pldd.c.

Patch

commit e2aed097192c423dcf1882cb1c39567676ef2255
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".  In case of "restricted ptrace", we can
    effectively disable the restriction by using prctl (PR_SET_PTRACER_ANY).
    
    If ptrace_scope exists and has a higher restriction, then the test
    is marked as UNSUPPORTED.
    
    ChangeLog:
    
            * elf/tst-pldd.c (ptrace_scope, target_barrier): New variable.
            (do_test): Add UNSUPPORTED check.
            (target_process): Disable restricted ptrace.
            * support/Makefile (libsupport-routines): Add support_ptrace.
            * support/ptrace.h: New file.
            * support/support_ptrace.c: Likewise.

diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
index 6b7c94a1c0..130d1f51db 100644
--- a/elf/tst-pldd.c
+++ b/elf/tst-pldd.c
@@ -30,10 +30,27 @@ 
 #include <support/capture_subprocess.h>
 #include <support/check.h>
 #include <support/support.h>
+#include <support/ptrace.h>
+#include <support/xunistd.h>
+#include <sys/mman.h>
+#include <stdatomic.h>
+
+static int ptrace_scope;
+static atomic_int *target_barrier;
 
 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 ();
+    }
+
+  /* Ensure that pldd is called after ptrace restriction has been disabled.  */
+  *target_barrier = 1;
+
   pause ();
 }
 
@@ -52,9 +69,24 @@  in_str_list (const char *libname, const char *const strlist[])
 static int
 do_test (void)
 {
+  /* Check if our subprocess can be debugged with ptrace.  */
+  ptrace_scope = support_ptrace_scope ();
+  if (ptrace_scope >= 2)
+    FAIL_UNSUPPORTED ("/proc/sys/kernel/yama/ptrace_scope >= 2");
+
+  target_barrier = (atomic_int *) xmmap (NULL, sizeof (int), PROT_READ | PROT_WRITE,
+				  MAP_SHARED | MAP_ANONYMOUS, -1);
+  atomic_init (target_barrier, 0);
+
   /* Create a copy of current test to check with pldd.  */
   struct support_subprocess target = support_subprocess (target_process, NULL);
 
+  /* Ensure that pldd is called after ptrace restriction has been disabled in
+     target_process.  Do not use pthread synchronization functions here as the
+     test below expects not to be linked against libpthread.  */
+  while (*target_barrier == 0)
+    ;
+
   /* Run 'pldd' on test subprocess.  */
   struct support_capture_subprocess pldd;
   {
@@ -129,6 +161,7 @@  do_test (void)
 
   support_capture_subprocess_free (&pldd);
   support_process_terminate (&target);
+  xmunmap (target_barrier, sizeof (int));
 
   return 0;
 }
diff --git a/support/Makefile b/support/Makefile
index ab66913a02..34d14eb1bb 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -56,6 +56,7 @@  libsupport-routines = \
   support_format_hostent \
   support_format_netent \
   support_isolate_in_subprocess \
+  support_ptrace \
   support_openpty \
   support_paths \
   support_quote_blob \
diff --git a/support/ptrace.h b/support/ptrace.h
new file mode 100644
index 0000000000..82f79ff25c
--- /dev/null
+++ b/support/ptrace.h
@@ -0,0 +1,36 @@ 
+/* 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);
+
+/* Effectively disables YAMA restriction for the calling process
+   if support_ptrace_scope returns 1 "restricted ptrace".  */
+void support_ptrace_process_set_ptracer_any (void);
+
+__END_DECLS
+
+#endif
diff --git a/support/support_ptrace.c b/support/support_ptrace.c
new file mode 100644
index 0000000000..e9410384b5
--- /dev/null
+++ b/support/support_ptrace.c
@@ -0,0 +1,58 @@ 
+/* 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/>.  */
+
+#include <support/check.h>
+#include <support/xstdio.h>
+#include <support/ptrace.h>
+#include <sys/prctl.h>
+
+int
+support_ptrace_scope (void)
+{
+  int ptrace_scope = -1;
+
+#ifdef __linux__
+  /* YAMA may be not enabled.  Otherwise it contains a value from 0 to 3:
+     - 0 classic ptrace permissions
+     - 1 restricted ptrace
+     - 2 admin-only attach
+     - 3 no attach  */
+  FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
+  if (f != NULL)
+    {
+      TEST_COMPARE (fscanf (f, "%d", &ptrace_scope), 1);
+      xfclose (f);
+    }
+#endif
+
+  return ptrace_scope;
+}
+
+void
+support_ptrace_process_set_ptracer_any (void)
+{
+#ifdef __linux__
+  int ret = prctl (PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
+  if (ret != 0)
+    FAIL_EXIT1 ("Failed to disable YAMA restriction. (prctl returned %d: %m)",
+		ret);
+#else
+  FAIL_UNSUPPORTED ("prctl (PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) "
+		    "not supported!");
+#endif
+}