From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 2Si8CV7TpmcLtycAWB0awg (envelope-from ) for ; Fri, 07 Feb 2025 22:45:34 -0500 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=aN0P2kDr; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 10C411E105; Fri, 7 Feb 2025 22:45:34 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 57A611E05C for ; Fri, 7 Feb 2025 22:45:31 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C2D213857829 for ; Sat, 8 Feb 2025 03:45:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C2D213857829 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=aN0P2kDr Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by sourceware.org (Postfix) with ESMTPS id CE0443858D1E for ; Sat, 8 Feb 2025 03:44:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CE0443858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CE0443858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::1036 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1738986298; cv=none; b=JpaCpnnC2tcMhl7mY3vG2ki+yYa84VabgNDP+CyHMLyKQAaGqSqeV+hPbCxDF7Crwskjf4JNBL0jvWlgD2iAliqAno7RJaiBVpwvY6aq+tSGshzB0hlLddjKAWuJT53yvv2HfLQmxdN/9LhbC9mrqOJlNnbYbAe9p5/mOHN6yqw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1738986298; c=relaxed/simple; bh=tISR9A5LDKscFFD7YEygOjw+RpoFMeSiASH5+GGlcds=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=JFfZln5ruerTutwm7tkecCTOqai1F9WVPikr/NoFsez/uRslRlp31j6LcT8gbsOmBKHW7hEqMGqcibS5jiSR9TBRCIeX7ypcLWARIPBN6ehtast+5ospkli0wapyFuNMBmVamBxyVJie6JQEPNcBUnsogw6TaqCbBN8uK4/O5Oc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CE0443858D1E Received: by mail-pj1-x1036.google.com with SMTP id 98e67ed59e1d1-2fa488351ffso216562a91.3 for ; Fri, 07 Feb 2025 19:44:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1738986297; x=1739591097; darn=sourceware.org; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to; bh=fvtwvnRF9v0JUYmIO55UAxkOTwOhpWZb7kvy4NhJyAA=; b=aN0P2kDrvXIN+lDkqwR2ZxURLs5oZ3rvpuilcQ/0/J62SMIumNOK8qJ3IR49WMkmfS pQwr4hafxOcqB1QEJKWj4BVtAlcLWlYCUAWqtx+gFMZba0cQwzn9SO/K31l/UKr14CK2 tiNmkfKxo+3yx6YvzkY3vA5zNvdybCX3MoUJUW/k0FUIC/7+P74UzppMAfSyyZI5SX1k tXmh601+k5Azff8o/Hf347vT+zoMyY1O3+1omcVMIk2b8bvqvWNEfwAzh8PN4KwNNhB5 AFhZcOyqYDhhkLAFBlH6TsoUCofu0yKulSp4sjwAQeTvjd/7BeHCew/SMCZdEzbsMSZ1 EUjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738986297; x=1739591097; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=fvtwvnRF9v0JUYmIO55UAxkOTwOhpWZb7kvy4NhJyAA=; b=pIuCppUioBmg79hLxrDvie7J8skoKKWwnWmY0QsYHrpiTVoXwd1Seos1+P6DqYn2BW +DXzWCDRx0zb+qIFJrZtdPvT7unCyED5VLG0v7HJ0LzhxQahNGOO5zdFcyb1RqB2LvTr utJrR4znC89gq1Gf+5GQs0+tFEYx19KCoIhqTLPhLRteIwO9d3X4VrgdTYZSrQmdFXqy dDq1NXQuT5Ihg74PKrCEks6/BDZlAOjsmZ6pFy2ileACFRpYv9sI5wJxCDmZE/xa0vir LRP7fGo2ZvgVUoXOtHKZcPeUN4JpBj/ir4vUHWTeQZOW0ckrcBDebv4hnJE6nNKf8jNM u7rg== X-Gm-Message-State: AOJu0YwAAxM4h4VjnUs70vRQ/PpN6KYCjGqgFaXK6xqGuHF+jCDCubtl YKfuvOivbvlXuUPLeeVNle4kiYjgsyo/CC+OZ+dmWU6O9TF+CX+AfZXsXVQqHzrUgfw/YgoeKAu i X-Gm-Gg: ASbGncu5+bTPvPqnYfyGixKA1o4Y2zyw7mPb9FQTq1blp9nV35E+m3Jc3+Ezto1CcYo NyyjUXYdacsy4n/d5uo4SS4Y34b3XS397RckL2oYVrtNPcta6enKdPWUPIJZfJKZF6c+uE8ZYdJ QyZpoT9CpmBvmIeL2DOPlxIDHim41KXJQ1JaC1uDrQOE9/ZBYBG9vUs3/C+65UG22bj6KF5WL5z ZfRgIZp/G3QGW45K3DgPjj4KtkxO+pwaUfNHPRX1IL/BgFKrMCFOlhymwakOtKZafo1ZysCxYGK ovl7ys/XyTQPIegqtMb+MlY= X-Google-Smtp-Source: AGHT+IHw0DPGSVd1CSyHWSqWcnldJ5GMRGgIavwj7Qp16twhvKK8KqQeIvF4S0uf9/QxHHc/TsiB9w== X-Received: by 2002:a05:6a00:4fc7:b0:727:3ac3:7f9c with SMTP id d2e1a72fcca58-7305d44a432mr7679433b3a.10.1738986296695; Fri, 07 Feb 2025 19:44:56 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:b53a:98d4:8895:d0]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-73048bf143dsm3736584b3a.118.2025.02.07.19.44.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Feb 2025 19:44:55 -0800 (PST) From: Thiago Jung Bauermann To: "Schimpe, Christina" Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack pointer register. In-Reply-To: (Christina Schimpe's message of "Thu, 6 Feb 2025 14:33:43 +0000") References: <20241220200501.324191-1-christina.schimpe@intel.com> <20241220200501.324191-7-christina.schimpe@intel.com> <877c63ix3x.fsf@linaro.org> User-Agent: mu4e 1.12.8; emacs 29.4 Date: Sat, 08 Feb 2025 00:44:52 -0300 Message-ID: <877c61rtfv.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org "Schimpe, Christina" writes: > Hi Thiago, > > Thank you for the review. Please see my comments to your feedback below. > >> -----Original Message----- >> From: Thiago Jung Bauermann >> Sent: Thursday, February 6, 2025 4:14 AM >> To: Schimpe, Christina >> Cc: gdb-patches@sourceware.org >> Subject: Re: [PATCH 06/12] gdb, gdbserver: Add support of Intel shadow stack >> pointer register. >> >> >> "Schimpe, Christina" writes: >> >> > diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c index >> > 4a39028a472..59daaa4c583 100644 >> > --- a/gdb/arch/i386.c >> > +++ b/gdb/arch/i386.c >> > @@ -28,6 +28,7 @@ >> > #include "../features/i386/32bit-avx512.c" >> > #include "../features/i386/32bit-segments.c" >> > #include "../features/i386/pkeys.c" >> > +#include "../features/i386/32bit-ssp.c" >> > >> > /* See i386.h. */ >> > >> > @@ -66,5 +67,8 @@ i386_create_target_description (uint64_t >> xstate_bv_mask, bool is_linux, >> > if (xstate_bv_mask & X86_XSTATE_PKRU) >> > regnum = create_feature_i386_pkeys (tdesc.get (), regnum); >> > >> > + if (xstate_bv_mask & X86_XSTATE_CET_U) >> > + regnum = create_feature_i386_32bit_ssp (tdesc.get (), regnum); >> > + >> > return tdesc.release (); >> > } >> >> The patch description mentions that "32 bit support is not covered due to missing >> linux kernel support". Is this change useful, or is it unreachable code? > > I think I consistently did not implement 32 bit linux support, but added the code > for 32 bit support in locations which are not linux dependent, as preparation for > other OS. But for now yes, this should be unreachable code. > Should I (1) Remove the code or (2) improve the commit message ? > I'd be in favour of (2), would that be acceptable? In the general case, I lean towards (1), but in this case it's just two simple lines, and also in an arch-specific corner of GDB so it's not really a problem. It's certainly acceptable in my view. >> > diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c index >> > d1fece717a7..5bbd4640e30 100644 >> > --- a/gdb/x86-linux-nat.c >> > +++ b/gdb/x86-linux-nat.c >> > @@ -41,6 +41,7 @@ >> > #include "nat/x86-linux.h" >> > #include "nat/x86-linux-dregs.h" >> > #include "nat/linux-ptrace.h" >> > +#include "x86-tdep.h" >> > #include "nat/x86-linux-tdesc.h" >> > >> > /* linux_nat_target::low_new_fork implementation. */ @@ -97,11 >> > +98,10 @@ const struct target_desc * >> > x86_linux_nat_target::read_description () { >> > /* The x86_linux_tdesc_for_tid call only reads xcr0 the first time it is >> > - called. The mask is stored in XSTATE_BV_STORAGE and reused on >> > - subsequent calls. Note that GDB currently supports features for user >> > - state components only. However, once supervisor state components are >> > - supported in GDB XSTATE_BV_STORAGE will not be configured based on >> > - xcr0 only. */ >> > + called. Also it checks the enablement state of features which are >> > + not configured in xcr0, such as CET shadow stack. Once the >> > + supported >> >> The "not" above should be removed. > > You mean the "not" in this sentence? > "Also it checks the enablement state of features which are not configured in xcr0, such as > CET shadow stack. " > > This should be correct in that context. CET shadow stack is not configured in xcr0. Ah, I see. Sorry, I had misread the comment. >> > + features are identified, the XSTATE_BV_STORAGE value is configured >> > + accordingly and preserved for subsequent calls of this function. >> > + */ >> > static uint64_t xstate_bv_storage; >> > >> > if (inferior_ptid == null_ptid) >> > @@ -215,6 +215,46 @@ x86_linux_get_thread_area (pid_t pid, void *addr, >> > unsigned int *base_addr) } >> > >> >> >> > >> > +/* See x86-linux-nat.h. */ >> > + >> > +void >> > +x86_linux_fetch_ssp (regcache *regcache, const int tid) { >> > + uint64_t ssp = 0x0; >> > + iovec iov {&ssp, sizeof (ssp)}; >> > + >> > + /* The shadow stack may be enabled and disabled at runtime. Reading the >> > + ssp might fail as shadow stack was not activated for the current >> > + thread. We don't want to show a warning but silently return. The >> > + register will be shown as unavailable for the user. */ if >> > + (ptrace (PTRACE_GETREGSET, tid, NT_X86_SHSTK, &iov) != 0) >> > + return; >> >> In case the ptrace fails and there is already an old value for ssp in regcache, >> shouldn't it be removed from it? > > Hm, it doesn't seem to be a problem. I ran a test using inline enablement of shadow stack: > ~~~ > (gdb) p $pl3_ssp > $1 = (void *) 0x7ffff7c00000 > (gdb) n > 54 if (ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK)) { > (gdb) n > 58 return ret; > (gdb) p $pl3_ssp > $2 = > ~~~ > So it should be fine from my perspective. Or do you see another potential issue? Each time the inferior continues, GDB drops the existing regcaches and builds new ones, so the values from $1 and $2 in the output above come from different regcache instances. I made the comment thinking that if GDB knows that ssp is unavailable then it should strive to make sure that is reflected in the regcache. But come to think of it, if the inferior is stopped then whether ssp is available or not won't change until it runs again and the scenario I was considering can't happen. Sorry about the false alarm. -- Thiago