This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4] Improve ptrace-error detection on Linux targets
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>, Eli Zaretskii <eliz at gnu dot org>, Ruslan Kabatsayev <b7 dot 10110111 at gmail dot com>
- Date: Wed, 25 Sep 2019 10:14:07 -0400
- Subject: Re: [PATCH v4] Improve ptrace-error detection on Linux targets
- References: <20190819032918.3536-1-sergiodj@redhat.com> <20190911011103.12774-1-sergiodj@redhat.com> <87h851mnqt.fsf@tromey.com>
On Tuesday, September 24 2019, Tom Tromey wrote:
>>>>>> "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.
:-)
Thanks for the review.
> 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?
I was also worried about this. I found other cases using sprintf, but
their strings are not as big. I know own_buf comes from struct
client_state, and its size of PBUFSIZ + 1. One option would be to use
snprintf with this value. WDYT?
> 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.
OK, I will do that.
> 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?
Not at the same time, but if both restrictions are active, we can be
sure that both will have an impact when using GDB.
> I was wondering if each case should just return, or if checking each one
> is the correct thing to do here.
I don't have a strong opinion here, but I think it's more useful for the
user if we print everything wrong in the first pass. I'm thinking of
cases when starting GDB may take a while, for example: if we tell the
user a list of problems that need to be solved, then she won't need to
keep retrying.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/