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

Tom Tromey
2019-09-24T20:40:48Z

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Changes from v3:

Thanks for doing this. I like this change quite a bit. For one thing I
think it would have saved me some time over the years, as I randomly
forget about ptrace security measures on new machines.

Sergio> +++ b/gdb/gdbserver/server.c
Sergio> @@ -2913,9 +2913,21 @@ handle_v_attach (char *own_buf)
Sergio> {
Sergio> client_state &cs = get_client_state ();
Sergio> int pid;
Sergio> + int ret;

Sergio> pid = strtol (own_buf + 8, NULL, 16);
Sergio> - if (pid != 0 && attach_inferior (pid) == 0)
Sergio> +
Sergio> + try
Sergio> + {
Sergio> + ret = attach_inferior (pid);
Sergio> + }
Sergio> + catch (const gdb_exception_error &e)
Sergio> + {
Sergio> + sprintf (own_buf, "E.%s", e.what ());

Unrestricted sprintf gives me pause. Do we know own_buf is large
enough? Or can/should we truncate the text instead?

Sergio> -gdb_dlopen (const char *filename)
Sergio> +gdb_dlopen (const char *filename, bool dont_throw)

This parameter is only used in one spot, and it's on an error path that
is computing the string to show to the user. So, I think it's better to
just remove the parameter and use try/catch in that one caller.

Sergio> +static std::string
Sergio> +linux_ptrace_restricted_fail_reason (int err)
Sergio> +{
...
Sergio> + std::string ret;
Sergio> + gdb_dlhandle_up handle = gdb_dlopen ("libselinux.so.1", true);
Sergio> +
Sergio> + if (handle != nullptr)
Sergio> + {
Sergio> + selinux_ftype selinux_get_bool
Sergio> + = (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
Sergio> +
Sergio> + if (selinux_get_bool != NULL
Sergio> + && (*selinux_get_bool) ("deny_ptrace") == 1)
Sergio> + string_appendf (ret,
Sergio> + _("\n\
Sergio> +The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
Sergio> +from using 'ptrace'. You can disable it by executing (as root):\n\
Sergio> +\n\
Sergio> + setsebool deny_ptrace off\n"));
Sergio> + }
Sergio> +
Sergio> + gdb_file_up yama_ptrace_scope
Sergio> + = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
...

If SELinux was the problem, is it also possible that ptrace_scope is the
problem?

I was wondering if each case should just return, or if checking each one
is the correct thing to do here.

thanks,
Tom