Re: [PATCH] Improve ptrace-error detection on Linux targets

[Ruslan Kabatsayev <b7.10110111@gmail.com>]

Hello Sergio,

On Mon, 19 Aug 2019 at 06:29, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> In Fedora GDB, we carry the following patch:
>
>   https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb-attach-fail-reasons-5of5.patch
>
> Its purpose is to try to detect a specific scenario where SELinux's
> 'deny_ptrace' option is enabled, which prevents GDB from ptrace'ing in
> order to debug the inferior (PTRACE_ATTACH and PTRACE_ME will fail
> with EACCES in this case).
>
> I like the idea of improving error detection and providing more
> information to the user (a simple "Permission denied" can be really
> frustrating), but I don't fully agree with the way the patch was
> implemented: it makes GDB link against libselinux only for the sake of
> consulting the 'deny_ptrace' setting, and then prints a warning if
> ptrace failed and this setting is on.
>
> Instead of printing a very certain warning about a very specific
> setting, I decided to propose the following patch, which prints a
> generic warning about a possible situation (i.e., without going
> bothering to make sure that the situation is actually happening).
> What this means is that whenever a ptrace error is detected, and if
> errno is either EACCES or EPERM, a warning will be printed pointing
> the user to our documentation, where she will find more details about
> possible restrictions in place against ptrace.

Doesn't it worsen user experience compared to the current Fedora
patch? Now a user first coming across the problem will have to
navigate the documentation, then check possible scenarios – instead of
simply getting a friendly explanation what exactly is wrong.

Compare this with the patch applied by some Linux distributions
(CentOS IIRC) to make Bash emit

> bash: myprogram: /lib/ld-linux.so.2: bad ELF interpreter: No such file or directory

when ELF interpreter can't be found. By default, Bash only prints the
less than helpful message

> bash: myprogram: No such file or directory

Your proposal (if it's intended to replace Fedora's GDB patch) is
similar to replacing this default with

> bash: myprogram: No such file or directory. There might be other reasons than simply command executable not existing. Please see Bash documentation section XYZ for details.

and adding a section XYZ in Bash docs explaining that the reasons
could include lacking libraries, broken symlink to the binary, stale
hash table of PATH lookups, etc.. This is considerably worse than the
patch with explicit announcement of actual problem.

So as a user, I disfavor this approach, although it might be more
maintainable for GDB developers. But, if dependency on libselinux is
the only objection, then why not simply dlopen it as needed (and
gracefully handle failure to do so) instead of ELF-level linking to
it? Then there'll be no hard dependency on libselinux, while user
experience will not degrade too.

Regards,
Ruslan

>
> The patch obviously expands our documentation and creates a new
> appendix section named "Linux kernel ptrace restrictions", with
> sub-sections for each possible restriction that might be in place.
>
> The current list of possible restrictions is:
>
>   - SELinux's 'deny_ptrace' option.
>
>   - YAMA's /proc/sys/kernel/yama/ptrace_scope setting.
>
>   - seccomp on Docker containers.
>
> It's important to mention that all of this is Linux-specific; as far
> as I know, SELinux, YAMA and seccomp are Linux-only features.
>
> For the PTRACE_ATTACH scenario, I borrowed the original patch's idea
> and hacked the warning into 'linux_ptrace_attach_fail_reason'.  For
> PTRACE_ME, I opted to implement a new target method called
> 'ptrace_me_fail_reason' which is then be called inside inf-ptrace.c's
> 'inf_ptrace_me'.  This method is currently only implemented by the
> Linux target.
>
> I tested this patch locally, on my Fedora 30 machine (actually, a
> Fedora Rawhide VM), but I'm not proposing a testcase for it because of
> the difficulty of writing one.  Maybe something like using
> dlopen (RTLD_NEXT... and wrapping ptrace could work, but I don't know
> offhand.
>
> WDYT?
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
>         * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
>         section.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>             Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         * inf-ptrace.c (inf_ptrace_me): Update call to
>         'trace_start_error_with_name'.
>         * linux-nat.c (linux_nat_target::ptrace_me_fail_reason): New
>         method.
>         (linux_nat_target::attach): Update call to
>         'linux_ptrace_attach_fail_reason'.
>         * linux-nat.h (linux_nat_target) <ptrace_me_fail_reason>: New
>         method.
>         * nat/fork-inferior.c (trace_start_error_with_name): Add
>         optional 'append' argument.
>         * nat/fork-inferior.h (trace_start_error_with_name): Update
>         prototype.
>         * nat/linux-ptrace.c (linux_ptrace_attach_fail_reason): Add
>         optional 'err' argument.
>         * nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): Update
>         prototype.
>         * target-delegates.c: Regenerate.
>         * target.h (struct target_ops) <ptrace_me_fail_reason>: New
>         method.
>         (target_ptrace_me_fail_reason): New define.
> ---
>  gdb/doc/gdb.texinfo     | 90 +++++++++++++++++++++++++++++++++++++++++
>  gdb/inf-ptrace.c        |  3 +-
>  gdb/linux-nat.c         | 19 ++++++++-
>  gdb/linux-nat.h         |  2 +
>  gdb/nat/fork-inferior.c |  4 +-
>  gdb/nat/fork-inferior.h |  7 ++--
>  gdb/nat/linux-ptrace.c  | 11 +++--
>  gdb/nat/linux-ptrace.h  |  6 ++-
>  gdb/target-delegates.c  | 28 +++++++++++++
>  gdb/target.h            | 15 +++++++
>  10 files changed, 174 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index bcf0420779..d2aab9be45 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -182,6 +182,9 @@ software in general.  We will miss him.
>                                  @value{GDBN}
>  * Operating System Information:: Getting additional information from
>                                   the operating system
> +* Linux kernel @code{ptrace} restrictions:: Restrictions sometimes
> +                                            imposed by the Linux
> +                                            kernel on @code{ptrace}
>  * Trace File Format::          GDB trace file format
>  * Index Section Format::        .gdb_index section format
>  * Man Pages::                  Manual pages
> @@ -44672,6 +44675,93 @@ should contain a comma-separated list of cores that this process
>  is running on.  Target may provide additional columns,
>  which @value{GDBN} currently ignores.
>
> +@node Linux kernel @code{ptrace} restrictions
> +@appendix Linux kernel @code{ptrace} restrictions
> +@cindex linux kernel ptrace restrictions, attach
> +
> +The @code{ptrace} system call is used by @value{GDBN} to, among other
> +things, attach to a new or existing inferior in order to start
> +debugging it.  Due to security concerns, some distributions and
> +vendors disable or severily restrict the ability to perform these
> +operations, which can make @value{GDBN} malfunction.  In this section,
> +we will expand on how this malfunction can manifest, and how to modify
> +the system's settings in order to be able to use @value{GDBN}
> +properly.
> +
> +@menu
> +* SELinux's @code{deny_ptrace}::        SELinux and the @code{deny_ptrace} option
> +* Yama's @code{ptrace_scope}::          Yama and the @code{ptrace_scope} setting
> +* Docker and @code{seccomp}::           Docker and the @code{seccomp}
> +                                        infrastructure
> +@end menu
> +
> +@node SELinux's @code{deny_ptrace}
> +@appendixsection SELinux's @code{deny_ptrace}
> +@cindex selinux, deny_ptrace
> +
> +If you are using SELinux, you might want to check whether the
> +@code{deny_ptrace} option is enabled by doing:
> +
> +@smallexample
> +$ getsebool deny_ptrace
> +deny_ptrace --> on
> +@end smallexample
> +
> +If the option is enabled, you can disable it by doing, as root:
> +
> +@smallexample
> +# setsebool deny_ptrace off
> +@end smallexample
> +
> +The option will be disabled until the next reboot.  If you would like
> +to disable it permanently, you can do:
> +
> +@smallexample
> +# setsebool -P deny_ptrace off
> +@end smallexample
> +
> +@node Yama's @code{ptrace_scope}
> +@appendixsection Yama's @code{ptrace_scope}
> +@cindex yama, ptrace_scope
> +
> +If your system has Yama enabled, you might want to check whether the
> +@code{ptrace_scope} setting is enabled by checking the value of
> +@file{/proc/sys/kernel/yama/ptrace_scope}:
> +
> +@smallexample
> +$ cat /proc/sys/kernel/yama/ptrace_scope
> +0
> +@end smallexample
> +
> +If you see anything other than @code{0}, @value{GDBN} can be affected
> +by it.  You can temporarily disable the feature by doing:
> +
> +@smallexample
> +# sysctl kernel.yama.ptrace_scope=0
> +kernel.yama.ptrace_scope = 0
> +@end smallexample
> +
> +You can make this permanent by doing:
> +
> +@smallexample
> +# sysctl -w kernel.yama.ptrace_scope=0
> +kernel.yama.ptrace_scope = 0
> +@end smallexample
> +
> +@node Docker and @code{seccomp}
> +@appendixsection Docker and @code{seccomp}
> +@cindex docker, seccomp
> +
> +If you are using Docker (@uref{https://www.docker.com/}) containers,
> +you will probably have to disable its @code{seccomp} protections in
> +order to be able to use @value{GDBN}.  To do that, you can use the
> +options @code{--cap-add=SYS_PTRACE --security-opt seccomp=unconfined}
> +when invoking Docker:
> +
> +@smallexample
> +$ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined
> +@end smallexample
> +
>  @node Trace File Format
>  @appendix Trace File Format
>  @cindex trace file format
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index 4a8e732373..4e21f677a2 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -101,7 +101,8 @@ inf_ptrace_me (void)
>  {
>    /* "Trace me, Dr. Memory!"  */
>    if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0)
> -    trace_start_error_with_name ("ptrace");
> +    trace_start_error_with_name ("ptrace",
> +                                target_ptrace_me_fail_reason (errno).c_str ());
>  }
>
>  /* Start a new inferior Unix child process.  EXEC_FILE is the file to
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 945c19f666..053d7630b8 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -614,6 +614,22 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
>    return 0;
>  }
>
> +/* See the declaration on target.h.  */
> +
> +std::string
> +linux_nat_target::ptrace_me_fail_reason (int err)
> +{
> +  std::string ret;
> +
> +  if (err == EACCES || err == EPERM)
> +    ret = _("There might be restrictions preventing ptrace from\n"
> +           "working.  Please see the appendix "
> +           "\"Linux kernel ptrace restrictions\"\n"
> +           "in the GDB documentation for more details.");
> +
> +  return ret;
> +}
> +
>
>  int
>  linux_nat_target::insert_fork_catchpoint (int pid)
> @@ -1191,8 +1207,9 @@ linux_nat_target::attach (const char *args, int from_tty)
>      }
>    catch (const gdb_exception_error &ex)
>      {
> +      int saved_errno = errno;
>        pid_t pid = parse_pid_to_attach (args);
> -      std::string reason = linux_ptrace_attach_fail_reason (pid);
> +      std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno);
>
>        if (!reason.empty ())
>         throw_error (ex.error, "warning: %s\n%s", reason.c_str (),
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index 0c1695ad10..1aa9b50130 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -134,6 +134,8 @@ public:
>
>    int follow_fork (int, int) override;
>
> +  std::string ptrace_me_fail_reason (int err) override;
> +
>    std::vector<static_tracepoint_marker>
>      static_tracepoint_markers_by_strid (const char *id) override;
>
> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> index 68b51aa814..72ac623e20 100644
> --- a/gdb/nat/fork-inferior.c
> +++ b/gdb/nat/fork-inferior.c
> @@ -591,7 +591,7 @@ trace_start_error (const char *fmt, ...)
>  /* See nat/fork-inferior.h.  */
>
>  void
> -trace_start_error_with_name (const char *string)
> +trace_start_error_with_name (const char *string, const char *append)
>  {
> -  trace_start_error ("%s: %s", string, safe_strerror (errno));
> +  trace_start_error ("%s: %s%s", string, safe_strerror (errno), append);
>  }
> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> index 1d0519fb26..7e6b889210 100644
> --- a/gdb/nat/fork-inferior.h
> +++ b/gdb/nat/fork-inferior.h
> @@ -98,9 +98,10 @@ extern void trace_start_error (const char *fmt, ...)
>    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>
>  /* Like "trace_start_error", but the error message is constructed by
> -   combining STRING with the system error message for errno.  This
> -   function does not return.  */
> -extern void trace_start_error_with_name (const char *string)
> +   combining STRING with the system error message for errno, and
> +   (optionally) with APPEND.  This function does not return.  */
> +extern void trace_start_error_with_name (const char *string,
> +                                        const char *append = "")
>    ATTRIBUTE_NORETURN;
>
>  #endif /* NAT_FORK_INFERIOR_H */
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index c1ebc0a032..fa5584829e 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -30,11 +30,10 @@
>     of 0 means there are no supported features.  */
>  static int supported_ptrace_options = -1;
>
> -/* Find all possible reasons we could fail to attach PID and return these
> -   as a string.  An empty string is returned if we didn't find any reason.  */
> +/* See declaration in linux-ptrace.h.  */
>
>  std::string
> -linux_ptrace_attach_fail_reason (pid_t pid)
> +linux_ptrace_attach_fail_reason (pid_t pid, int err)
>  {
>    pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid);
>    std::string result;
> @@ -50,6 +49,12 @@ linux_ptrace_attach_fail_reason (pid_t pid)
>                       "terminated"),
>                     (int) pid);
>
> +  if (err == EACCES || err == EPERM)
> +    string_appendf (result,
> +                   _("There might be restrictions preventing ptrace from\n"
> +                     "working.  Please see the appendix "
> +                     "\"Linux kernel ptrace restrictions\"\n"
> +                     "in the GDB documentation for more details."));
>    return result;
>  }
>
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index fd2f12a342..99a7780ac7 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -176,7 +176,11 @@ struct buffer;
>  # define TRAP_HWBKPT 4
>  #endif
>
> -extern std::string linux_ptrace_attach_fail_reason (pid_t pid);
> +/* Find all possible reasons we could fail to attach PID and return
> +   these as a string.  An empty string is returned if we didn't find
> +   any reason.  If ERR is EACCES or EPERM, we also add a warning about
> +   possible restrictions to use ptrace.  */
> +extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err = -1);
>
>  /* Find all possible reasons we could have failed to attach to PTID
>     and return them as a string.  ERR is the error PTRACE_ATTACH failed
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index 52034fe436..7fe45412cb 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -52,6 +52,7 @@ struct dummy_target : public target_ops
>    void kill () override;
>    void load (const char *arg0, int arg1) override;
>    void post_startup_inferior (ptid_t arg0) override;
> +  std::string ptrace_me_fail_reason (int arg0) override;
>    int insert_fork_catchpoint (int arg0) override;
>    int remove_fork_catchpoint (int arg0) override;
>    int insert_vfork_catchpoint (int arg0) override;
> @@ -220,6 +221,7 @@ struct debug_target : public target_ops
>    void kill () override;
>    void load (const char *arg0, int arg1) override;
>    void post_startup_inferior (ptid_t arg0) override;
> +  std::string ptrace_me_fail_reason (int arg0) override;
>    int insert_fork_catchpoint (int arg0) override;
>    int remove_fork_catchpoint (int arg0) override;
>    int insert_vfork_catchpoint (int arg0) override;
> @@ -1400,6 +1402,32 @@ debug_target::post_startup_inferior (ptid_t arg0)
>    fputs_unfiltered (")\n", gdb_stdlog);
>  }
>
> +std::string
> +target_ops::ptrace_me_fail_reason (int arg0)
> +{
> +  return this->beneath ()->ptrace_me_fail_reason (arg0);
> +}
> +
> +std::string
> +dummy_target::ptrace_me_fail_reason (int arg0)
> +{
> +  return std::string ();
> +}
> +
> +std::string
> +debug_target::ptrace_me_fail_reason (int arg0)
> +{
> +  std::string result;
> +  fprintf_unfiltered (gdb_stdlog, "-> %s->ptrace_me_fail_reason (...)\n", this->beneath ()->shortname ());
> +  result = this->beneath ()->ptrace_me_fail_reason (arg0);
> +  fprintf_unfiltered (gdb_stdlog, "<- %s->ptrace_me_fail_reason (", this->beneath ()->shortname ());
> +  target_debug_print_int (arg0);
> +  fputs_unfiltered (") = ", gdb_stdlog);
> +  target_debug_print_std_string (result);
> +  fputs_unfiltered ("\n", gdb_stdlog);
> +  return result;
> +}
> +
>  int
>  target_ops::insert_fork_catchpoint (int arg0)
>  {
> diff --git a/gdb/target.h b/gdb/target.h
> index 4e2e75cb80..8e6cc75d8e 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -608,6 +608,18 @@ struct target_ops
>                                   char **, int);
>      virtual void post_startup_inferior (ptid_t)
>        TARGET_DEFAULT_IGNORE ();
> +    /* When the call to ptrace fails, and we have already forked, this
> +       function can be called in order to try to obtain the reason why
> +       ptrace failed.  It takes one integer argument, which should be
> +       the ERRNO value returned by ptrace.
> +
> +       This function will return a 'std::string' containing the fail
> +       reason, or an empty string otherwise.
> +
> +       The reason this is a target method is because different targets
> +       can compute the reason in different ways.  */
> +    virtual std::string ptrace_me_fail_reason (int)
> +      TARGET_DEFAULT_RETURN (std::string ());
>      virtual int insert_fork_catchpoint (int)
>        TARGET_DEFAULT_RETURN (1);
>      virtual int remove_fork_catchpoint (int)
> @@ -1624,6 +1636,9 @@ extern void target_load (const char *arg, int from_tty);
>  #define target_remove_vfork_catchpoint(pid) \
>       (current_top_target ()->remove_vfork_catchpoint) (pid)
>
> +#define target_ptrace_me_fail_reason(err) \
> +     (current_top_target ()->ptrace_me_fail_reason) (err)
> +
>  /* If the inferior forks or vforks, this function will be called at
>     the next resume in order to perform any bookkeeping and fiddling
>     necessary to continue debugging either the parent or child, as
> --
> 2.21.0
>