From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2151 invoked by alias); 17 Sep 2019 13:31:36 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 2140 invoked by uid 89); 17 Sep 2019 13:31:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Adhemerval, Zanella, zanella, sk:adhemer X-HELO: mail-qk1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:openpgp:autocrypt:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=9fvpo3sCy1mXguWIGW55MdxS+Fm1XFVZD4dH1fyUffU=; b=CQrxHKevh42i/Z/N9/7etEXTUivzZveOsX2mc4mmQ0KAKXiLct36/Z96sFBzhbt9UX EdPY8k/Rdrwhm1+zlwGCJjEZaN+2ECFplFmqmw1Dy53Z/OJaOhJWF0VGIP98Xt5UZx7e t7KaoIdB+Dy5WoA6U/zo7sRGmXBgk1aLWd6z3twBDoew+p+XPsf/YszIXzi1ogwGMEnl fIH185N/40AgmVzK5NASxClEeuw+yF05NVGytTA720Tgo/HKeFY7jsBLJ1di5kyrm6g8 qo0XKOZxLOuYtcfc7a4hnix9XIiNw/YZY5Awh//g1QHEhdMtpsEnADqMKw8eK19jbVSo vZBQ== Return-Path: To: Stefan Liebler , Florian Weimer Cc: libc-alpha@sourceware.org References: <877e6yiqsn.fsf@oldenburg2.str.redhat.com> <3b9cb5e4-7c9a-c9a4-449e-43ba98a6ad01@linaro.org> <32961942-e67b-c356-a92d-b6e45c8aaf9d@linux.ibm.com> <87blw9fxrp.fsf@oldenburg2.str.redhat.com> <1d419974-c973-c4c1-f1cd-4bbbf8b074f8@linux.ibm.com> <87tva08ijw.fsf@oldenburg2.str.redhat.com> <21f1131e-5756-3c56-d41f-a37f172c48e8@linux.ibm.com> From: Adhemerval Zanella Openpgp: preference=signencrypt Subject: Re: [PATCH] Add UNSUPPORTED check in elf/tst-pldd. Message-ID: <303c145e-c9da-3061-48df-76b00ee5b10e@linaro.org> Date: Tue, 17 Sep 2019 13:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <21f1131e-5756-3c56-d41f-a37f172c48e8@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2019-09/txt/msg00240.txt.bz2 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 > 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 > > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c > index 6b7c94a1c0..eb1b9cb5f5 100644 > --- a/elf/tst-pldd.c > +++ b/elf/tst-pldd.c > @@ -30,6 +30,12 @@ > #include > #include > #include > +#include > +#include > +#include > +#include > +#include > + > > static void > target_process (void *arg) > @@ -37,6 +43,34 @@ target_process (void *arg) > pause (); > } > > +static void > +pldd_process (void *arg) > +{ > + pid_t *target_pid_ptr = (pid_t *) arg; > + > + /* Create a copy of current test to check with pldd. As the > + target_process is a child of this pldd_process, pldd is also able > + to attach to target_process if YAMA is configured to 1 = > + "restricted ptrace". */ > + struct support_subprocess target = support_subprocess (target_process, NULL); > + > + /* Store the pid of target-process as do_test needs it in order to > + e.g. terminate it at end of the test. */ > + *target_pid_ptr = target.pid; > + > + /* Three digits per byte plus null terminator. */ > + char pid[3 * sizeof (uint32_t) + 1]; > + snprintf (pid, array_length (pid), "%d", target.pid); > + > + char *prog = xasprintf ("%s/pldd", support_bindir_prefix); > + > + /* Run pldd and use the pid of target_process as argument. */ > + execve (prog, (char *const []) { (char *) prog, pid, NULL }, > + (char *const []) { NULL }); > + > + FAIL_EXIT1 ("Returned from execve: errno=%d=%m\n", errno); > +} > + Ok. > /* The test runs in a container because pldd does not support tracing > a binary started by the loader iself (as with testrun.sh). */ > > @@ -52,25 +86,22 @@ in_str_list (const char *libname, const char *const strlist[]) > static int > do_test (void) > { > - /* Create a copy of current test to check with pldd. */ > - struct support_subprocess target = support_subprocess (target_process, NULL); > - > - /* Run 'pldd' on test subprocess. */ > - struct support_capture_subprocess pldd; > + /* Check if our subprocess can be debugged with ptrace. */ > { > - /* Three digits per byte plus null terminator. */ > - char pid[3 * sizeof (uint32_t) + 1]; > - snprintf (pid, array_length (pid), "%d", target.pid); > - > - char *prog = xasprintf ("%s/pldd", support_bindir_prefix); > - > - pldd = support_capture_subprogram (prog, > - (char *const []) { (char *) prog, pid, NULL }); > + int ptrace_scope = support_ptrace_scope (); > + if (ptrace_scope >= 2) > + FAIL_UNSUPPORTED ("/proc/sys/kernel/yama/ptrace_scope >= 2"); > + } > Ok. > - support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout); > + pid_t *target_pid_ptr = (pid_t *) xmmap (NULL, sizeof (pid_t), > + PROT_READ | PROT_WRITE, > + MAP_SHARED | MAP_ANONYMOUS, -1); > > - free (prog); > - } Ok. > + /* Run 'pldd' on test subprocess which will be created in pldd_process. > + The pid of the subprocess will be written to target_pid_ptr. */ > + struct support_capture_subprocess pldd; > + pldd = support_capture_subprocess (pldd_process, target_pid_ptr); > + support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout); Ok, this is will updated by the pldd_process. > > /* Check 'pldd' output. The test is expected to be linked against only > loader and libc. */ > @@ -85,7 +116,7 @@ do_test (void) > /* First line is in the form of : */ > TEST_COMPARE (fscanf (out, "%u: " STRINPUT (512), &pid, buffer), 2); > > - TEST_COMPARE (pid, target.pid); > + TEST_COMPARE (pid, *target_pid_ptr); > TEST_COMPARE (strcmp (basename (buffer), "tst-pldd"), 0); > > /* It expects only one loader and libc loaded by the program. */ > @@ -93,7 +124,7 @@ do_test (void) > while (fgets (buffer, array_length (buffer), out) != NULL) > { > /* Ignore vDSO. */ > - if (buffer[0] != '/') > + if (buffer[0] != '/') > continue; > Ok. > /* Remove newline so baseline (buffer) can compare against the > @@ -128,7 +159,9 @@ do_test (void) > } > > support_capture_subprocess_free (&pldd); > - support_process_terminate (&target); > + if (kill (*target_pid_ptr, SIGKILL) != 0) > + FAIL_EXIT1 ("Unable to kill target_process: errno=%d=%m\n", errno); > + xmunmap (target_pid_ptr, sizeof (pid_t)); > > return 0; > } Ok. > 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 \ Ok. > 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 > + . */ > + > +#ifndef SUPPORT_PTRACE_H > +#define SUPPORT_PTRACE_H > + > +#include > + > +__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. > diff --git a/support/support_ptrace.c b/support/support_ptrace.c > new file mode 100644 > index 0000000000..2084e5a189 > --- /dev/null > +++ b/support/support_ptrace.c > @@ -0,0 +1,44 @@ > +/* 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 > + . */ > + > +#include > +#include > +#include > +#include > + > +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; > +} > Ok.