From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Hw7jGDTLnWh0QAMAWB0awg (envelope-from ) for ; Thu, 14 Aug 2025 07:40:36 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=JMbWzYlB; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 55F331E0B3; Thu, 14 Aug 2025 07:40:36 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 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 44FAB1E04C for ; Thu, 14 Aug 2025 07:40:34 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B2F003858C74 for ; Thu, 14 Aug 2025 11:40:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B2F003858C74 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=JMbWzYlB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id C5D123858C62 for ; Thu, 14 Aug 2025 11:39:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C5D123858C62 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C5D123858C62 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755171591; cv=none; b=pbf9eTOO/4w6995Cc9XKSC/1gjEvXD0V0DRHtPjpQABGvmWwPz+l/bUHwCQutMNjbsZZ79/AU9brSmnKhsCDs9Lx5a4l/wbfaJy8XyfnZ3yR49xxoUnMDNBEgVkdJ7nXEZvICR31Tqb0VNzvmdIiZHu/EQgfLNY+CEAEtDq+5ZQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755171591; c=relaxed/simple; bh=CG9oy/fcCQN+00l3w3dkrkZWsTug3sk2f9GObO+HY8E=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=JSNI7eZLBpB4dcIec19A32Ujh59+FRJlCbQhqjhJxgBAwapFj0l4geSgHHe5xWhsqsuBx0E9bI5Y5au8jhufW1w1Vs2DDbNPAzVcOeqWGnP9LvUFFOC/pvbGui3+Ju1qcg9LwbeElfHu8oxb+IM1cDcLyb8NcMxdlv//lcTeAuA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C5D123858C62 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1755171591; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/8NoX1hBXsmkewULJ7Od9glP/gn58XRmJFvwt7JG14s=; b=JMbWzYlBot7OH1tX/joJRGMd3UCGprYxo9ZlEhb4moCcJexiPZhJUYksra1EUIpHNJcQPY EG5R5rrBCKnov0YNezXRfuJ6GkCOeksrHMNvvlaWct2zTHGIvZuxyxMcV8R6Y+xrwe60kh aj3s14etF0MZ09M/TiwILPmUBcUQdtM= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-503-W57XyC13OBaUnMoEVSFj1w-1; Thu, 14 Aug 2025 07:39:50 -0400 X-MC-Unique: W57XyC13OBaUnMoEVSFj1w-1 X-Mimecast-MFC-AGG-ID: W57XyC13OBaUnMoEVSFj1w_1755171589 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-3b9edf332faso357879f8f.3 for ; Thu, 14 Aug 2025 04:39:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1755171589; x=1755776389; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=u/GLipQH1Il8Rgyo+loh+b7q2eLdnmK0ACupFd5nfaE=; b=QApJTscJ6vs3lf7gcjQwpJ4rP8/XjOoPT1+yDgaeGKu5s5DUkzpPzOhhQ9XAxEa/0/ Dnus+mo9efAaByjvtL8J8WkoRi/e4fngc4krFmC1CHDgl+ttzIQtrWGAOWYMDO+NpHcY W4/oZhiU+zMPWvT9BBMbYcfWesM5oZ66JJRTiQRk6lmlcywNQbAOVl3mBpK7MK31xuqD HmjX++hY6SaFF0a/HDLVaEHLTtEJ5laXgoEAvwAzGnNTNPPLpxONkA03KDvumzOCGPvo udTSj/7tKB6dXou072FnttIfG/TfGolxmrDJEhJSOTOQscJYomPeWahvNve9k/W0Y+c4 P11w== X-Forwarded-Encrypted: i=1; AJvYcCUxuwDP/lb+tPMKptMa6ygJUlGWYMut9+AgJV8Qb9x/Lvr06s+CgDbSyhU5zs34MmU4dNb6stK2N/erKA==@sourceware.org X-Gm-Message-State: AOJu0YzzDt+26wttDCDbShSr3xoktoCHFRzGVAum4QnHg0RcIcZGNLaL mAoqN4CBWm8cHAl0ukafcF8/ZX6lvjCGFlb6oyUpIB/XAzE1iY+0/1R6LiOJ0c63EuPvro3CGfg aIL3t/jhUOs7NAFQnkFTZJwCD7sck/2U4hukQTNYWV4jRRaD8L4Kxe9PdXAXaCOE= X-Gm-Gg: ASbGnctlZdydpxVRFUBp9P9IFGPNtol8NojFB6kk2bzkxrKWsere2IKzNWc+lc9HgUS tf2CngKwap5R0w2tYgx77UqVpJMuLU2S5dXpV9gsXb2N16DSVi4vnVOzmFcHwgZ1slm1gNapeWz dB3PmaGI9/QlEo26lWptt2dmvirfDYbXw6yTuutB6XD8CcIlaBcp0r7sjvvbne6NpY1l4U7X0is Cd6+iJVh5eqig7BMNmyUK3wQicE/I7xGFyNPjWfJGW4NL0uNgi0yuobSM+uAC/p62UaNvtIOYr2 aYY9uuzyPCoiksI2sU3k6BbSWZ5xYPp5DZBSpA6YfXbYaGdz47LCNzL+7z0= X-Received: by 2002:a05:6000:1449:b0:3b8:d79a:6a35 with SMTP id ffacd0b85a97d-3b9edfb6f3emr2525793f8f.20.1755171588615; Thu, 14 Aug 2025 04:39:48 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHKlcS4OlqcdP5OO9ggGU4d9coxfGglu7XQdmDzU2C1+lTq5xM2uGdynEMpL34a6TydA2+Z8A== X-Received: by 2002:a05:6000:1449:b0:3b8:d79a:6a35 with SMTP id ffacd0b85a97d-3b9edfb6f3emr2525767f8f.20.1755171588102; Thu, 14 Aug 2025 04:39:48 -0700 (PDT) Received: from localhost (13.81.93.209.dyn.plus.net. [209.93.81.13]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3b8f8b1bc81sm28618540f8f.69.2025.08.14.04.39.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Aug 2025 04:39:47 -0700 (PDT) From: Andrew Burgess To: "Schimpe, Christina" , "gdb-patches@sourceware.org" , "thiago.bauermann@linaro.org" Cc: "luis.machado@arm.com" Subject: RE: [PATCH v5 06/12] gdb, gdbserver: Add support of Intel shadow stack pointer register. In-Reply-To: References: <20250628082810.332526-1-christina.schimpe@intel.com> <20250628082810.332526-7-christina.schimpe@intel.com> <87cy9oe8pc.fsf@redhat.com> <87qzxpan2h.fsf@redhat.com> Date: Thu, 14 Aug 2025 12:39:47 +0100 Message-ID: <87qzxe873w.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Q4WyRjYznrDDJAl6K5QU5s0XJQufaOtnnFmtfyiu32o_1755171589 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 Andrew, > >> -----Original Message----- >> From: Andrew Burgess >> Sent: Tuesday, August 5, 2025 3:57 PM >> To: Schimpe, Christina ; gdb- >> patches@sourceware.org >> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com >> Subject: RE: [PATCH v5 06/12] gdb, gdbserver: Add support of Intel shado= w >> stack pointer register. >>=20 >> "Schimpe, Christina" writes: >>=20 >> > Hi Andrew, >> > >> > Thanks a lot for the review! I have some questions for your feedback, >> > please find my comments below. >> > >> >> -----Original Message----- >> >> From: Andrew Burgess >> >> Sent: Friday, July 25, 2025 2:50 PM >> >> To: Schimpe, Christina ; gdb- >> >> patches@sourceware.org >> >> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com >> >> Subject: Re: [PATCH v5 06/12] gdb, gdbserver: Add support of Intel >> >> shadow stack pointer register. >> >> >> >> Christina Schimpe writes: >> >> >> >> > This patch adds the user mode register PL3_SSP which is part of the >> >> > Intel(R) Control-Flow Enforcement Technology (CET) feature for >> >> > support of shadow stack. >> >> > For now, only native and remote debugging support for shadow stack >> >> > userspace on amd64 linux are covered by this patch including 64 bit >> >> > and >> >> > x32 support. 32 bit support is not covered due to missing Linux >> >> > kernel support. >> >> > >> >> > This patch requires fixing the test >> >> > gdb.base/inline-frame-cycle-unwind >> >> > which is failing in case the shadow stack pointer is unavailable. >> >> > Such a state is possible if shadow stack is disabled for the >> >> > current thread but supported by HW. >> >> > >> >> > This test uses the Python unwinder inline-frame-cycle-unwind.py >> >> > which fakes the cyclic stack cycle by reading the pending frame's >> >> > registers and adding them to the unwinder: >> >> > >> >> > ~~~ >> >> > for reg in pending_frame.architecture().registers("general"): >> >> > val =3D pending_frame.read_register(reg) >> >> > unwinder.add_saved_register(reg, val) >> >> > return unwinder >> >> > ~~~ >> >> > >> >> > However, in case the python unwinder is used we add a register >> >> > (pl3_ssp) that is unavailable. This leads to a NOT_AVAILABLE_ERROR >> >> > caught in gdb/frame-unwind.c:frame_unwind_try_unwinder and it is >> >> > continued with standard unwinders. This destroys the faked cyclic >> >> > behavior and the stack is further unwinded after frame 5. >> >> > >> >> > In the working scenario an error should be triggered: >> >> > ~~~ >> >> > bt >> >> > 0 inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:49^M >> >> > 1 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M >> >> > 2 0x000055555555516e in inline_func () at >> >> > /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M >> >> > 3 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M >> >> > 4 0x000055555555516e in inline_func () at >> >> > /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M >> >> > 5 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M >> >> > Backtrace stopped: previous frame identical to this frame (corrupt >> >> > stack?) >> >> > (gdb) PASS: gdb.base/inline-frame-cycle-unwind.exp: cycle at level = 5: >> >> > backtrace when the unwind is broken at frame 5 ~~~ >> >> > >> >> > To fix the Python unwinder, we simply skip the unavailable register= s. >> >> > >> >> > Reviewed-by: Thiago Jung Bauermann >> >> > Reviewed-By: Eli Zaretskii >> >> > Reviewed-By: Luis Machado >> >> > --- >> >> > gdb/NEWS | 3 + >> >> > gdb/amd64-linux-nat.c | 17 +++++ >> >> > gdb/amd64-linux-tdep.c | 1 + >> >> > gdb/amd64-tdep.c | 6 +- >> >> > gdb/amd64-tdep.h | 1 + >> >> > gdb/arch/amd64.c | 10 +++ >> >> > gdb/arch/i386.c | 4 ++ >> >> > gdb/arch/x86-linux-tdesc-features.c | 1 + >> >> > gdb/doc/gdb.texinfo | 4 ++ >> >> > gdb/features/Makefile | 2 + >> >> > gdb/features/i386/32bit-ssp.c | 14 ++++ >> >> > gdb/features/i386/32bit-ssp.xml | 11 +++ >> >> > gdb/features/i386/64bit-ssp.c | 14 ++++ >> >> > gdb/features/i386/64bit-ssp.xml | 11 +++ >> >> > gdb/i386-tdep.c | 22 +++++- >> >> > gdb/i386-tdep.h | 4 ++ >> >> > gdb/nat/x86-linux-tdesc.c | 2 + >> >> > gdb/nat/x86-linux.c | 57 +++++++++++++++ >> >> > gdb/nat/x86-linux.h | 4 ++ >> >> > gdb/testsuite/gdb.arch/amd64-shadow-stack.c | 22 ++++++ >> >> > gdb/testsuite/gdb.arch/amd64-ssp.exp | 50 +++++++++++++ >> >> > .../gdb.base/inline-frame-cycle-unwind.py | 4 ++ >> >> > gdb/testsuite/lib/gdb.exp | 70 +++++++++++++++= ++++ >> >> > gdb/x86-linux-nat.c | 49 +++++++++++-- >> >> > gdb/x86-linux-nat.h | 11 +++ >> >> > gdb/x86-tdep.c | 21 ++++++ >> >> > gdb/x86-tdep.h | 9 +++ >> >> > gdbserver/linux-x86-low.cc | 28 +++++++- >> >> > gdbsupport/x86-xstate.h | 5 +- >> >> > 29 files changed, 447 insertions(+), 10 deletions(-) create mode >> >> > 100644 gdb/features/i386/32bit-ssp.c create mode 100644 >> >> > gdb/features/i386/32bit-ssp.xml create mode 100644 >> >> > gdb/features/i386/64bit-ssp.c create mode 100644 >> >> > gdb/features/i386/64bit-ssp.xml create mode 100644 >> >> > gdb/testsuite/gdb.arch/amd64-shadow-stack.c >> >> > create mode 100644 gdb/testsuite/gdb.arch/amd64-ssp.exp >> >> > >> >> >> >> >> >> > diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c index >> >> > dbb9b3223cb..4df99ccca54 100644 >> >> > --- a/gdb/amd64-linux-nat.c >> >> > +++ b/gdb/amd64-linux-nat.c >> >> > @@ -32,6 +32,7 @@ >> >> > #include "amd64-tdep.h" >> >> > #include "amd64-linux-tdep.h" >> >> > #include "i386-linux-tdep.h" >> >> > +#include "x86-tdep.h" >> >> > #include "gdbsupport/x86-xstate.h" >> >> > >> >> > #include "x86-linux-nat.h" >> >> > @@ -237,6 +238,14 @@ amd64_linux_nat_target::fetch_registers >> >> > (struct regcache *regcache, int regnum) >> >> > >> >> > if (have_ptrace_getregset =3D=3D TRIBOOL_TRUE) >> >> > =09{ >> >> > +=09 if ((regnum =3D=3D -1 && tdep->ssp_regnum > 0) >> >> > +=09 || (regnum !=3D -1 && regnum =3D=3D tdep->ssp_regnum)) >> >> >> >> It's really nit-picking, but I don't think the '>' here check is >> >> correct. You're checking that tdep->ssp_regnum has been assigned a >> >> value, right? And it's default value is -1, so, shouldn't the check = really be >> '>=3D 0' or '!=3D -1' maybe? >> >> >> >> The same pattern is repeated below in ::store_registers. >> >> >> >> Ahh, reading further on, I see that (not just you), but all the >> >> *_regnum fields in i386_gdbarch_tdep start set to 0 (which is a valid >> >> register number), but are set to -1 if the feature is not found. >> >> Maybe I'm missing something incredibly clever about this ... but I >> >> suspect this code is just showing its age a little. I still think th= e validity >> check should be against -1. >> > >> > Yes I agree, it seems to me that there is something inconsistent in GD= B. >> > >> > The current default value is 0, we set all regnums to 0 initially in >> > gdb/i386-tdep.h And we set it to -1 to indicate the absence of that sp= ecific >> register. >> > >> > But on the other hand the enums (enum amd64_regnum/enum >> i386_regnum) >> > defining the register numbers start at 0. >> > >> > So that seems to be a separate issue one could consider to fix. >> > Maybe the default should be simply -1, instead of 0 ? >> > >> > But for my specific patch here the best would be to compare against >> against -1, I agree and will fix. >> > >> >> > +=09 { >> >> > +=09 x86_linux_fetch_ssp (regcache, tid); >> >> > +=09 if (regnum !=3D -1) >> >> > +=09=09return; >> >> > +=09 } >> >> > + >> >> > =09 /* Pre-4.14 kernels have a bug (fixed by commit 0852b374173b >> >> > =09 "x86/fpu: Add FPU state copying quirk to handle XRSTOR fai= lure >> on >> >> > =09 Intel Skylake CPUs") that sometimes causes the mxcsr >> >> > location in @@ -302,6 +311,14 @@ >> >> > amd64_linux_nat_target::store_registers (struct >> >> regcache *regcache, int regnum) >> >> > if (have_ptrace_getregset =3D=3D TRIBOOL_TRUE) >> >> > =09{ >> >> > =09 gdb::byte_vector xstateregs (tdep->xsave_layout.sizeof_xsave)= ; >> >> > +=09 if ((regnum =3D=3D -1 && tdep->ssp_regnum > 0) >> >> > +=09 || (regnum !=3D -1 && regnum =3D=3D tdep->ssp_regnum)) >> >> > +=09 { >> >> > +=09 x86_linux_store_ssp (regcache, tid); >> >> > +=09 if (regnum !=3D -1) >> >> > +=09=09return; >> >> > +=09 } >> >> > + >> >> > =09 struct iovec iov; >> >> > >> >> > =09 iov.iov_base =3D xstateregs.data (); >> >> >> >> >> >> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index >> >> > 4ef640698bd..0881ac4aee5 100644 >> >> > --- a/gdb/doc/gdb.texinfo >> >> > +++ b/gdb/doc/gdb.texinfo >> >> > @@ -49959,6 +49959,10 @@ The @samp{org.gnu.gdb.i386.pkeys} >> feature >> >> is >> >> > optional. It should describe a single register, @samp{pkru}. It >> >> > is a 32-bit register valid for i386 and amd64. >> >> > >> >> > +The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional. It >> >> > +should describe the user mode register @samp{pl3_ssp} which has 64 >> >> > +bits on amd64. Following the restriction of the Linux kernel, >> >> > +only amd64 is >> >> supported for now. >> >> >> >> You mention a couple of times in this patch that the shadow stack is >> >> only supported (for now) on amd64 (& x32), but you have added an i386 >> >> version of the org.gnu.gdb.i386.pl3_ssp feature with a 32-bit pl3_ssp >> register. >> > >> > We need the 32-bit pl3_ssp register for x32, since for x32 the shadow >> stack pointer has 32 bits only. >> > >> >> This feature is (as far as I can tell) instantiated for 32-bit >> >> targets, which means gdbserver will send out target descriptions >> containing this feature. >> >> >> >> I think either (a) you should drop the i386 xml file and feature, or >> >> (b) my preferred choice, document the i386 version here. It's still >> >> fine to say that the 32-bit (i386) register is ignored by GDB for now >> >> though. I had a go at rewording the paragraph, but I'm not 100% sure= its >> OK yet: >> >> >> >> The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional. It should >> >> describe the user mode register @samp{pl3_ssp} which has 64 bits on >> >> amd64 and 32 bits on i386. Following the restriction of the Linux >> >> kernel, only GDB for amd64 targets makes use of this feature for no= w. >> > >> > I missed to describe x32 here, actually. So I'd write it as follows: >> > >> > The @samp{org.gnu.gdb.i386.pl3_ssp} feature is optional. It should >> > describe the user mode register @samp{pl3_ssp} which has 64 bits on >> > amd64, 32 bits on amd64 with 32-bit pointer size (X32) and 32 bits on >> i386. >> > Following the restriction of the Linux kernel, only GDB for amd64 >> > targets makes use of this feature for now. >> > >> > What do you think? >>=20 >> Sounds great. >>=20 >> > >> >> >> >> > + >> >> > @node LoongArch Features >> >> > @subsection LoongArch Features >> >> > @cindex target descriptions, LoongArch Features diff --git >> >> > a/gdb/features/Makefile b/gdb/features/Makefile index >> >> > 7a8c7999733..2afda1ccd00 100644 >> >> > --- a/gdb/features/Makefile >> >> > +++ b/gdb/features/Makefile >> >> > @@ -225,6 +225,7 @@ FEATURE_XMLFILES =3D aarch64-core.xml \ >> >> > =09i386/32bit-avx.xml \ >> >> > =09i386/32bit-avx512.xml \ >> >> > =09i386/32bit-segments.xml \ >> >> > +=09i386/32bit-ssp.xml \ >> >> > =09i386/64bit-avx512.xml \ >> >> > =09i386/64bit-core.xml \ >> >> > =09i386/64bit-segments.xml \ >> >> > @@ -232,6 +233,7 @@ FEATURE_XMLFILES =3D aarch64-core.xml \ >> >> > =09i386/64bit-linux.xml \ >> >> > =09i386/64bit-sse.xml \ >> >> > =09i386/pkeys.xml \ >> >> > +=09i386/64bit-ssp.xml \ >> >> > =09i386/x32-core.xml \ >> >> > =09loongarch/base32.xml \ >> >> > =09loongarch/base64.xml \ >> >> > diff --git a/gdb/features/i386/32bit-ssp.c >> >> > b/gdb/features/i386/32bit-ssp.c new file mode 100644 index >> >> > 00000000000..991bae3c1e6 >> >> > --- /dev/null >> >> > +++ b/gdb/features/i386/32bit-ssp.c >> >> > @@ -0,0 +1,14 @@ >> >> > +/* THIS FILE IS GENERATED. -*- buffer-read-only: t -*- vi:set ro: >> >> > + Original: 32bit-ssp.xml */ >> >> > + >> >> > +#include "gdbsupport/tdesc.h" >> >> > + >> >> > +static int >> >> > +create_feature_i386_32bit_ssp (struct target_desc *result, long >> >> > +regnum) { >> >> > + struct tdesc_feature *feature; >> >> > + >> >> > + feature =3D tdesc_create_feature (result, >> >> > +"org.gnu.gdb.i386.pl3_ssp"); >> >> > + tdesc_create_reg (feature, "pl3_ssp", regnum++, 1, NULL, 32, >> >> > +"data_ptr"); >> >> > + return regnum; >> >> > +} >> >> > diff --git a/gdb/features/i386/32bit-ssp.xml >> >> > b/gdb/features/i386/32bit-ssp.xml new file mode 100644 index >> >> > 00000000000..d17e7004eec >> >> > --- /dev/null >> >> > +++ b/gdb/features/i386/32bit-ssp.xml >> >> > @@ -0,0 +1,11 @@ >> >> > + >> >> > + >> >> > + >> >> > + > >> > +name=3D"org.gnu.gdb.i386.pl3_ssp"> >> >> > + >> >> > diff --git a/gdb/features/i386/64bit-ssp.c >> >> > b/gdb/features/i386/64bit-ssp.c new file mode 100644 index >> >> > 00000000000..5468099ddf6 >> >> > --- /dev/null >> >> > +++ b/gdb/features/i386/64bit-ssp.c >> >> > @@ -0,0 +1,14 @@ >> >> > +/* THIS FILE IS GENERATED. -*- buffer-read-only: t -*- vi:set ro: >> >> > + Original: 64bit-ssp.xml */ >> >> > + >> >> > +#include "gdbsupport/tdesc.h" >> >> > + >> >> > +static int >> >> > +create_feature_i386_64bit_ssp (struct target_desc *result, long >> >> > +regnum) { >> >> > + struct tdesc_feature *feature; >> >> > + >> >> > + feature =3D tdesc_create_feature (result, >> >> > +"org.gnu.gdb.i386.pl3_ssp"); >> >> > + tdesc_create_reg (feature, "pl3_ssp", regnum++, 1, NULL, 64, >> >> > +"data_ptr"); >> >> > + return regnum; >> >> > +} >> >> > diff --git a/gdb/features/i386/64bit-ssp.xml >> >> > b/gdb/features/i386/64bit-ssp.xml new file mode 100644 index >> >> > 00000000000..a0688d018a5 >> >> > --- /dev/null >> >> > +++ b/gdb/features/i386/64bit-ssp.xml >> >> > @@ -0,0 +1,11 @@ >> >> > + >> >> > + >> >> > + >> >> > + > >> > +name=3D"org.gnu.gdb.i386.pl3_ssp"> >> >> > + >> >> > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index >> >> > 90ff0c5c706..8eb5b4fac86 100644 >> >> > --- a/gdb/i386-tdep.c >> >> > +++ b/gdb/i386-tdep.c >> >> > @@ -8403,7 +8403,8 @@ i386_validate_tdesc_p (i386_gdbarch_tdep >> *tdep, >> >> > const struct tdesc_feature *feature_core; >> >> > >> >> > const struct tdesc_feature *feature_sse, *feature_avx, >> *feature_avx512, >> >> > -=09=09=09 *feature_pkeys, *feature_segments; >> >> > +=09=09=09 *feature_pkeys, *feature_segments, >> >> > +=09=09=09 *feature_pl3_ssp; >> >> > int i, num_regs, valid_p; >> >> > >> >> > if (! tdesc_has_registers (tdesc)) @@ -8429,6 +8430,9 @@ >> >> > i386_validate_tdesc_p (i386_gdbarch_tdep *tdep, >> >> > /* Try PKEYS */ >> >> > feature_pkeys =3D tdesc_find_feature (tdesc, >> >> > "org.gnu.gdb.i386.pkeys"); >> >> > >> >> > + /* Try Shadow Stack. */ >> >> > + feature_pl3_ssp =3D tdesc_find_feature (tdesc, >> >> > + "org.gnu.gdb.i386.pl3_ssp"); >> >> > + >> >> > valid_p =3D 1; >> >> > >> >> > /* The XCR0 bits. */ >> >> > @@ -8544,6 +8548,15 @@ i386_validate_tdesc_p (i386_gdbarch_tdep >> >> *tdep, >> >> > =09=09=09=09=09 tdep->pkeys_register_names[i]); >> >> > } >> >> > >> >> > + if (feature_pl3_ssp !=3D nullptr) >> >> > + { >> >> > + if (tdep->ssp_regnum < 0) >> >> > +=09tdep->ssp_regnum =3D I386_PL3_SSP_REGNUM; >> >> > + >> >> > + valid_p &=3D tdesc_numbered_register (feature_pl3_ssp, tdesc= _data, >> >> > +=09=09=09=09=09 tdep->ssp_regnum, "pl3_ssp"); >> >> > + } >> >> > + >> >> > return valid_p; >> >> > } >> >> > >> >> > @@ -8835,6 +8848,9 @@ i386_gdbarch_init (struct gdbarch_info info, >> >> struct gdbarch_list *arches) >> >> > /* No segment base registers. */ >> >> > tdep->fsbase_regnum =3D -1; >> >> > >> >> > + /* No shadow stack pointer register. */ tdep->ssp_regnum =3D -= 1; >> >> > + >> >> > tdesc_arch_data_up tdesc_data =3D tdesc_data_alloc (); >> >> > >> >> > set_gdbarch_relocate_instruction (gdbarch, >> >> > i386_relocate_instruction); @@ -8955,13 +8971,15 @@ const struct >> >> > target_desc * i386_target_description (uint64_t xstate_bv_mask, >> >> > bool >> >> > segments) { >> >> > static target_desc *i386_tdescs \ >> >> > - [2/*SSE*/][2/*AVX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] = =3D >> {}; >> >> > + [2/*SSE*/][2/*AVX*/][2/*AVX512*/][2/*PKRU*/][2/*CET_U*/] \ >> >> > + [2/*segments*/] =3D {}; >> >> > target_desc **tdesc; >> >> > >> >> > tdesc =3D &i386_tdescs[(xstate_bv_mask & X86_XSTATE_SSE) ? 1 : 0= ] >> >> > [(xstate_bv_mask & X86_XSTATE_AVX) ? 1 : 0] >> >> > [(xstate_bv_mask & X86_XSTATE_AVX512) ? 1 : 0] >> >> > [(xstate_bv_mask & X86_XSTATE_PKRU) ? 1 : 0] >> >> > + [(xstate_bv_mask & X86_XSTATE_CET_U) ? 1 : 0] >> >> > [segments ? 1 : 0]; >> >> > >> >> > if (*tdesc =3D=3D NULL) >> >> > diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h index >> >> > 239bc8674e8..60d6f3eb732 100644 >> >> > --- a/gdb/i386-tdep.h >> >> > +++ b/gdb/i386-tdep.h >> >> > @@ -191,6 +191,9 @@ struct i386_gdbarch_tdep : gdbarch_tdep_base >> >> > /* PKEYS register names. */ >> >> > const char * const *pkeys_register_names =3D nullptr; >> >> > >> >> > + /* Shadow stack pointer register. */ int ssp_regnum =3D 0; >> >> >> >> I know all the other *_regnum fields start as 0, but I really don't >> >> understand why. Surely starting as -1 would make more sense? This >> >> isn't a hard requirement, but maybe you see some reason why these >> >> fields should start at 0 that I'm missing? >> > >> > I agree. As stated before setting the regnums to -1 here by default >> > would make more sense to me. So I think I should set ssp_regnum here t= o >> -1. >> > I'd do that, unless I see some issues with that when testing of course= . >> > >> > The other registers set to 0 here could be fixed separately. >> > >> > Does that sound reasonable? >>=20 >> Yes absolutely, I certainly don't want you to change unrelated code as p= art >> of this work, but we might as well improve new code as its added. >> Your proposal sounds perfect to me. > > The patch was pretty straight-forward. =F0=9F=98=8A > Thanks for providing the feedback here, I think this makes the code much = better. > >> > >> >> > + >> >> > /* Register number for %fsbase. Set this to -1 to indicate the >> >> > absence of segment base registers. */ >> >> > int fsbase_regnum =3D 0; >> >> > @@ -293,6 +296,7 @@ enum i386_regnum >> >> > I386_ZMM0H_REGNUM,=09=09/* %zmm0h */ >> >> > I386_ZMM7H_REGNUM =3D I386_ZMM0H_REGNUM + 7, >> >> > I386_PKRU_REGNUM, >> >> > + I386_PL3_SSP_REGNUM, >> >> > I386_FSBASE_REGNUM, >> >> > I386_GSBASE_REGNUM >> >> > }; >> >> >> >> >> >> > diff --git a/gdb/testsuite/gdb.arch/amd64-ssp.exp >> >> > b/gdb/testsuite/gdb.arch/amd64-ssp.exp >> >> > new file mode 100644 >> >> > index 00000000000..6ddc875b9a3 >> >> > --- /dev/null >> >> > +++ b/gdb/testsuite/gdb.arch/amd64-ssp.exp >> >> > @@ -0,0 +1,50 @@ >> >> > +# Copyright 2018-2024 Free Software Foundation, Inc. >> >> > + >> >> > +# This program is free software; you can redistribute it and/or >> >> > +modify # it under the terms of the GNU General Public License as >> >> > +published by # the Free Software Foundation; either version 3 of >> >> > +the License, or # (at your option) any later version. >> >> > +# >> >> > +# This program is distributed in the hope that it will be useful, >> >> > +# but WITHOUT ANY WARRANTY; without even the implied warranty >> of # >> >> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # >> >> GNU >> >> > +General Public License for more details. >> >> > +# >> >> > +# You should have received a copy of the GNU General Public >> >> > +License # along with this program. If not, see >> . >> >> > + >> >> > +# Test accessing the shadow stack pointer register. >> >> > + >> >> > +require allow_ssp_tests >> >> > + >> >> > +standard_testfile amd64-shadow-stack.c >> >> >> >> Test source files should be named to match the .exp file. But in >> >> this case 'amd64-shadow-stack' seems better than 'amd64-ssp', so I'd >> >> renamed the .exp file to amd64-shadow-stack.exp, then this line can b= e >> just: >> >> >> >> standard_testfile >> > >> > Yes, I agree and will fix. >> > >> >> > + >> >> > +save_vars { ::env(GLIBC_TUNABLES) } { >> >> > + >> >> > + append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK" >> >> > + >> >> > + if { [prepare_for_testing "failed to prepare" ${testfile} ${sr= cfile} \ >> >> > +=09 additional_flags=3D"-fcf-protection=3Dreturn"] } { >> >> > +=09return -1 >> >> > + } >> >> > + >> >> > + if {![runto_main]} { >> >> > +=09return -1 >> >> > + } >> >> >> >> The 'return -1' can become just 'return'. The return value doesn't >> >> mean anything. >> > >> > True will fix (also in following patches, that have the same for the t= est >> files). >> > >> >> > + >> >> > + # Read PL3_SSP register. >> >> > + set ssp_main [get_hexadecimal_valueof "\$pl3_ssp" "read >> >> > + pl3_ssp value"] >> >> > + >> >> > + # Write PL3_SSP register. >> >> > + gdb_test "print /x \$pl3_ssp =3D 0x12345678" "=3D 0x12345678" = "set >> >> > + pl3_ssp >> >> value" >> >> > + gdb_test "print /x \$pl3_ssp" "=3D 0x12345678" "read pl3_ssp >> >> > + value after >> >> setting" >> >> > + >> >> > + # Restore original value. >> >> > + gdb_test "print /x \$pl3_ssp =3D $ssp_main" "=3D $ssp_main" >> >> > + "restore >> >> original pl3_ssp" >> >> > + >> >> > + # Potential CET violations often only occur after resuming >> >> > + normal >> >> execution. >> >> > + # Therefore, it is important to test normal program continuati= on >> after >> >> > + # configuring the shadow stack pointer. >> >> > + gdb_continue_to_end >> >> >> >> I assume that if we continue with the bogus value in place the >> >> inferior would either give an error or terminate. Is it worth trying >> >> this and checking that the inferior behaves as expected? >> > >> > If we don't reset the shadow stack pointer to it's original value we w= ill see >> a SEGV. >> > Dependent on the address of the wrong shadow stack pointer it's either >> > a SEGV with si code that points to a control flow protection fault or = a >> different si code. >> > >> > So if I stay in a valid address range for configuring pl3_ssp but >> > don't restore the original value I'll see a control flow protection ex= ception: >> > >> > [...] >> > breakpoint 1, 0x0000555555555148 in main ()^M >> > (gdb) print /x $pl3_ssp^M >> > $1 =3D 0x7ffff7bfffe8^M >> > (gdb) PASS: gdb.arch/amd64-ssp.exp: get hexadecimal valueof "$pl3_ssp" >> > print /x $pl3_ssp =3D 0x7ffff7bfffe0^M >> > $2 =3D 0x7ffff7bfffe0^M >> > (gdb) PASS: gdb.arch/amd64-ssp.exp: set pl3_ssp value print /x >> > $pl3_ssp^M >> > $3 =3D 0x7ffff7bfffe0^M >> > (gdb) PASS: gdb.arch/amd64-ssp.exp: read pl3_ssp value after setting >> > continue^M Continuing.^M ^M Program received signal SIGSEGV, >> > Segmentation fault.^M >> > 0x0000555555555158 in main ()^M >> > (gdb) FAIL: gdb.arch/amd64-ssp.exp: continue until exit >> > >> > Siginfo shows si_code =3D 10, which indicates a control protection fau= lt. >> > >> > p $_siginfo^M >> > $4 =3D {si_signo =3D 11, si_errno =3D 0, si_code =3D 10, [...] >> > >> > If I set the value of pl3_ssp as in the current test (0x12345678) I'll >> > see a different SEGV actually >> > >> > p $_siginfo >> > $4 =3D {si_signo =3D 11, si_errno =3D 0, si_code =3D 1, [...] >> > >> >> >> >> What if, say, the $pl3_ssp value only ever made it as far as the >> >> register cache, and was never actually written back to the inferior? >> >> I don't think the above test would actually spot this bug, right? >> > >> > Hm, if I understand you correctly here and you mean the scenario as >> > shown above the above test would spot this bug I think (as we saw a fa= il). >> > >> > Does my example above show what you described or do you mean a >> > different scenario? >>=20 >> Yes, something like the above would check that the register is actually = being >> written back to the hardware, and is written to the expected location. >>=20 >> The current test, as written in the patch, writes a bad value to the sha= dow >> stack, then restores the correct value. What if the bad value never act= ually >> got written back to the hardware at all, and was just being held in the >> register cache? >>=20 >> Having a test that writes a bad value, then does 'continue', and expects= to >> see something like 'Program received signal ...' would be a reasonable >> indication that the write to the shadow stack actually made it to the h/= w. >>=20 >> Thanks, >> Andrew > > > Yes, I agree, I'll add: > > ~~~ > with_test_prefix "invalid ssp" { > =09write_invalid_ssp > > =09# Continue until SIGSEV to test that the value is written back to HW. > =09gdb_test "continue" \ > =09 [multi_line \ > =09=09"Continuing\\." \ > =09=09"" \ > =09=09"Program received signal SIGSEGV, Segmentation fault\\." \ > =09=09"$hex in main \\(\\)"] \ > =09 "continue to SIGSEGV" > } > > clean_restart ${binfile} > if { ![runto_main] } { > =09return -1 > } > > with_test_prefix "restore original ssp" { > =09# Read PL3_SSP register. > =09set ssp_main [get_hexadecimal_valueof "\$pl3_ssp" "read pl3_ssp value"= ] > > =09write_invalid_ssp > > =09# Restore original value. > =09gdb_test "print /x \$pl3_ssp =3D $ssp_main" "=3D $ssp_main" "restore o= riginal value" > > =09# Now we should not see a SIGSEV, since the original value is restored= . > =09gdb_continue_to_end > } Sorry for the late reply. This test looks great, exactly what I imagined. Thanks, Andrew