From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 78289 invoked by alias); 25 Sep 2019 14:14:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 78198 invoked by uid 89); 25 Sep 2019 14:14:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=worried X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 25 Sep 2019 14:14:11 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 08B561895A43; Wed, 25 Sep 2019 14:14:10 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5552C60C5D; Wed, 25 Sep 2019 14:14:08 +0000 (UTC) From: Sergio Durigan Junior To: Tom Tromey Cc: GDB Patches , Pedro Alves , Eli Zaretskii , Ruslan Kabatsayev 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> Date: Wed, 25 Sep 2019 14:14:00 -0000 In-Reply-To: <87h851mnqt.fsf@tromey.com> (Tom Tromey's message of "Tue, 24 Sep 2019 14:40:42 -0600") Message-ID: <87mueszcnk.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00481.txt.bz2 On Tuesday, September 24 2019, Tom Tromey wrote: >>>>>> "Sergio" == Sergio Durigan Junior 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/