[libunwind] [libunwind][AArch64] Protect PC within libunwind's context. (PR #113368)

Daniel Kiss via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 25 13:00:57 PDT 2024


https://github.com/DanielKristofKiss updated https://github.com/llvm/llvm-project/pull/113368

>From 3dd2f4da57eb164e67fd54b66c09cc8b771ee24f Mon Sep 17 00:00:00 2001
From: Daniel Kiss <daniel.kiss at arm.com>
Date: Wed, 16 Oct 2024 14:48:25 -0700
Subject: [PATCH 1/5] [libunwind][AArch64] Protect PC within libunwind's
 context.

Libunwind manages the regiser context including the program counter
which is used effectively as return address.
To increase the robustness of libunwind let's protect the stored address
with PAC. Since there is no unwind info for this let's use the A key and
the base address of the context/registers as modifier.
__libunwind_Registers_arm64_jumpto can go anywhere where the given buffer
's PC points to. After this patch it needs a signed PC therefore the context
 is more harder to cract outside of libunwind.

The register value is internal to libunwind and the change is not visible
on the the APIs.
w
---
 libunwind/src/Registers.hpp            | 91 ++++++++++++++++++++++++--
 libunwind/src/UnwindRegistersRestore.S |  8 ++-
 libunwind/src/UnwindRegistersSave.S    |  8 ++-
 3 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 861e6b5f6f2c58..91bd95b1306169 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -1823,9 +1823,61 @@ extern "C" void *__libunwind_cet_get_jump_target() {
 #endif
 
 class _LIBUNWIND_HIDDEN Registers_arm64 {
+protected:
+  /// The program counter is used effectively as a return address
+  /// when the context is restored therefore protect it with PAC.
+  /// The base address of the context is used with the A key for
+  /// authentication and signing. Return address authentication is
+  /// still managed according to the unwind info.
+  inline uint64_t getAuthSalt() const {
+    return reinterpret_cast<uint64_t>(this);
+  }
+#if defined(_LIBUNWIND_IS_NATIVE_ONLY)
+  // Authenticate the given pointer and return with the raw value
+  // if the authentication is succeeded.
+  inline uint64_t auth(uint64_t ptr, uint64_t salt) const {
+    register uint64_t x17 __asm("x17") = ptr;
+    register uint64_t x16 __asm("x16") = salt;
+    asm("hint  0xc" // autia1716
+        : "+r"(x17)
+        : "r"(x16)
+        :);
+
+    uint64_t checkValue = ptr;
+    // Support for machines without FPAC.
+    // Strip the upper bits with `XPACLRI` and compare with the
+    // authenticated value.
+    asm("mov   x30, %[checkValue]     \r\n"
+        "hint  0x7                    \r\n"
+        "mov   %[checkValue], x30     \r\n"
+        : [checkValue] "+r"(checkValue)
+        :
+        : "x30");
+    if (x17 != checkValue)
+      _LIBUNWIND_ABORT("IP PAC authentication failure");
+    return x17;
+  }
+
+  // Sign the PC with the A-KEY and the current salt.
+  inline void updatePC(uint64_t value) {
+    register uint64_t x17 __asm("x17") = value;
+    register uint64_t x16 __asm("x16") = getAuthSalt();
+    asm("hint 0x8" : "+r"(x17) : "r"(x16)); // pacia1716
+    _registers.__pc = x17;
+  }
+#else //! defined(_LIBUNWIND_IS_NATIVE_ONLY)
+  // Remote unwinding is not supported by this protection.
+  inline uint64_t auth(uint64_t ptr, uint64_t salt) const { return ptr; }
+  inline void updatePC(uint64_t value) { _registers.__pc = value; }
+#endif
+
 public:
   Registers_arm64();
   Registers_arm64(const void *registers);
+  Registers_arm64(const Registers_arm64 &other);
+  Registers_arm64(const Registers_arm64 &&other) = delete;
+  Registers_arm64 &operator=(const Registers_arm64 &other);
+  Registers_arm64 &operator=(Registers_arm64 &&other) = delete;
 
   bool        validRegister(int num) const;
   uint64_t    getRegister(int num) const;
@@ -1845,8 +1897,14 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
 
   uint64_t  getSP() const         { return _registers.__sp; }
   void      setSP(uint64_t value) { _registers.__sp = value; }
-  uint64_t  getIP() const         { return _registers.__pc; }
-  void      setIP(uint64_t value) { _registers.__pc = value; }
+  uint64_t getIP() const { return auth(_registers.__pc, getAuthSalt()); }
+  void setIP(uint64_t value) {
+    // First authenticate the current value of the IP to ensure the context
+    // is still valid. This also ensure the setIP can't be used for signing
+    // arbitrary values.
+    auth(_registers.__pc, getAuthSalt());
+    updatePC(value);
+  }
   uint64_t  getFP() const         { return _registers.__fp; }
   void      setFP(uint64_t value) { _registers.__fp = value; }
 
@@ -1862,8 +1920,8 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
 
   GPRs    _registers;
   double  _vectorHalfRegisters[32];
-  // Currently only the lower double in 128-bit vectore registers
-  // is perserved during unwinding.  We could define new register
+  // Currently only the lower double in 128-bit vector registers
+  // is preserved during unwinding.  We could define new register
   // numbers (> 96) which mean whole vector registers, then this
   // struct would need to change to contain whole vector registers.
 };
@@ -1874,6 +1932,8 @@ inline Registers_arm64::Registers_arm64(const void *registers) {
   memcpy(&_registers, registers, sizeof(_registers));
   static_assert(sizeof(GPRs) == 0x110,
                 "expected VFP registers to be at offset 272");
+  // getcontext signs the PC with the base address of the context.
+  updatePC(auth(_registers.__pc, reinterpret_cast<uint64_t>(registers)));
   memcpy(_vectorHalfRegisters,
          static_cast<const uint8_t *>(registers) + sizeof(GPRs),
          sizeof(_vectorHalfRegisters));
@@ -1882,6 +1942,25 @@ inline Registers_arm64::Registers_arm64(const void *registers) {
 inline Registers_arm64::Registers_arm64() {
   memset(&_registers, 0, sizeof(_registers));
   memset(&_vectorHalfRegisters, 0, sizeof(_vectorHalfRegisters));
+  // We don't know the PC but let's sign to indicate we have a valid
+  // register set.
+  updatePC(0);
+}
+
+inline Registers_arm64::Registers_arm64(const Registers_arm64 &other) {
+  memcpy(&_registers, &other._registers, sizeof(_registers));
+  memcpy(&_vectorHalfRegisters, &other._vectorHalfRegisters,
+         sizeof(_vectorHalfRegisters));
+  updatePC(other.getIP());
+}
+
+inline Registers_arm64 &
+Registers_arm64::operator=(const Registers_arm64 &other) {
+  memcpy(&_registers, &other._registers, sizeof(_registers));
+  memcpy(&_vectorHalfRegisters, &other._vectorHalfRegisters,
+         sizeof(_vectorHalfRegisters));
+  updatePC(other.getIP());
+  return *this;
 }
 
 inline bool Registers_arm64::validRegister(int regNum) const {
@@ -1902,7 +1981,7 @@ inline bool Registers_arm64::validRegister(int regNum) const {
 
 inline uint64_t Registers_arm64::getRegister(int regNum) const {
   if (regNum == UNW_REG_IP || regNum == UNW_AARCH64_PC)
-    return _registers.__pc;
+    return getIP();
   if (regNum == UNW_REG_SP || regNum == UNW_AARCH64_SP)
     return _registers.__sp;
   if (regNum == UNW_AARCH64_RA_SIGN_STATE)
@@ -1918,7 +1997,7 @@ inline uint64_t Registers_arm64::getRegister(int regNum) const {
 
 inline void Registers_arm64::setRegister(int regNum, uint64_t value) {
   if (regNum == UNW_REG_IP || regNum == UNW_AARCH64_PC)
-    _registers.__pc = value;
+    setIP(value);
   else if (regNum == UNW_REG_SP || regNum == UNW_AARCH64_SP)
     _registers.__sp = value;
   else if (regNum == UNW_AARCH64_RA_SIGN_STATE)
diff --git a/libunwind/src/UnwindRegistersRestore.S b/libunwind/src/UnwindRegistersRestore.S
index 180a66582f41b5..2b11aadeb2d779 100644
--- a/libunwind/src/UnwindRegistersRestore.S
+++ b/libunwind/src/UnwindRegistersRestore.S
@@ -676,7 +676,13 @@ DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
   ldp    d28,d29, [x0, #0x1F0]
   ldr    d30,     [x0, #0x200]
   ldr    d31,     [x0, #0x208]
-
+  // Authenticate return address with the address of the context.
+  mov    x16,  x0
+  mov    x17, x30
+  hint 0xc // autia1716
+  mov    x30, x17
+  mov    x16, xzr
+  mov    x17, xzr
   // Finally, restore sp. This must be done after the last read from the
   // context struct, because it is allocated on the stack, and an exception
   // could clobber the de-allocated portion of the stack after sp has been
diff --git a/libunwind/src/UnwindRegistersSave.S b/libunwind/src/UnwindRegistersSave.S
index fab234fcd6f318..c346806dc53609 100644
--- a/libunwind/src/UnwindRegistersSave.S
+++ b/libunwind/src/UnwindRegistersSave.S
@@ -744,7 +744,13 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
   str    x30,     [x0, #0x0F0]
   mov    x1,sp
   str    x1,      [x0, #0x0F8]
-  str    x30,     [x0, #0x100]    // store return address as pc
+  // Sign the return address as pc with the address of the context
+  mov    x17, x30
+  mov    x16, x0
+  hint 0x8 // pacia1716
+  str    x17,     [x0, #0x100]    // store return address as pc
+  mov    x17, xzr
+  mov    x16, xzr
   // skip cpsr
   stp    d0, d1,  [x0, #0x110]
   stp    d2, d3,  [x0, #0x120]

>From 1423c03b1cc31749027b84f9ec063e0b5ba02c3a Mon Sep 17 00:00:00 2001
From: Daniel Kiss <daniel.kiss at arm.com>
Date: Thu, 24 Oct 2024 20:35:44 +0200
Subject: [PATCH 2/5] add volatile to asm() and some line break.

---
 libunwind/src/Registers.hpp | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 91bd95b1306169..154d967b07caec 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -1838,18 +1838,18 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
   inline uint64_t auth(uint64_t ptr, uint64_t salt) const {
     register uint64_t x17 __asm("x17") = ptr;
     register uint64_t x16 __asm("x16") = salt;
-    asm("hint  0xc" // autia1716
-        : "+r"(x17)
-        : "r"(x16)
-        :);
+    asm volatile ("hint  0xc" // autia1716
+                  : "+r"(x17)
+                  : "r"(x16)
+                  :);
 
     uint64_t checkValue = ptr;
     // Support for machines without FPAC.
     // Strip the upper bits with `XPACLRI` and compare with the
     // authenticated value.
-    asm("mov   x30, %[checkValue]     \r\n"
-        "hint  0x7                    \r\n"
-        "mov   %[checkValue], x30     \r\n"
+    asm volatile ("mov   x30, %[checkValue]     \r\n" \
+                  "hint  0x7                    \r\n" \
+                  "mov   %[checkValue], x30     \r\n" \
         : [checkValue] "+r"(checkValue)
         :
         : "x30");
@@ -1862,7 +1862,7 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
   inline void updatePC(uint64_t value) {
     register uint64_t x17 __asm("x17") = value;
     register uint64_t x16 __asm("x16") = getAuthSalt();
-    asm("hint 0x8" : "+r"(x17) : "r"(x16)); // pacia1716
+    asm volatile("hint 0x8" : "+r"(x17) : "r"(x16)); // pacia1716
     _registers.__pc = x17;
   }
 #else //! defined(_LIBUNWIND_IS_NATIVE_ONLY)

>From e22d884354b1b3d67c24598bc580567f2406e544 Mon Sep 17 00:00:00 2001
From: Daniel Kiss <daniel.kiss at arm.com>
Date: Fri, 25 Oct 2024 18:44:48 +0200
Subject: [PATCH 3/5] test suspected compiler issue around returning with
 `register` variable

---
 libunwind/src/Registers.hpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 154d967b07caec..530ab0f9ddec5f 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -1836,13 +1836,14 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
   // Authenticate the given pointer and return with the raw value
   // if the authentication is succeeded.
   inline uint64_t auth(uint64_t ptr, uint64_t salt) const {
+    uint64_t ret;
     register uint64_t x17 __asm("x17") = ptr;
     register uint64_t x16 __asm("x16") = salt;
     asm volatile ("hint  0xc" // autia1716
                   : "+r"(x17)
                   : "r"(x16)
                   :);
-
+    ret = x17;
     uint64_t checkValue = ptr;
     // Support for machines without FPAC.
     // Strip the upper bits with `XPACLRI` and compare with the
@@ -1855,7 +1856,7 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
         : "x30");
     if (x17 != checkValue)
       _LIBUNWIND_ABORT("IP PAC authentication failure");
-    return x17;
+    return ret;
   }
 
   // Sign the PC with the A-KEY and the current salt.

>From 20c1aa96d3eda4b83b3cf0bc2f4bfe834ef67f36 Mon Sep 17 00:00:00 2001
From: Daniel Kiss <daniel.kiss at arm.com>
Date: Fri, 25 Oct 2024 20:42:06 +0200
Subject: [PATCH 4/5] add debug log

---
 libunwind/src/Registers.hpp | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 530ab0f9ddec5f..14053ace492966 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -1839,23 +1839,25 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
     uint64_t ret;
     register uint64_t x17 __asm("x17") = ptr;
     register uint64_t x16 __asm("x16") = salt;
-    asm volatile ("hint  0xc" // autia1716
-                  : "+r"(x17)
-                  : "r"(x16)
-                  :);
+    asm volatile("hint  0xc" // autia1716
+                 : "+r"(x17)
+                 : "r"(x16)
+                 :);
     ret = x17;
     uint64_t checkValue = ptr;
     // Support for machines without FPAC.
     // Strip the upper bits with `XPACLRI` and compare with the
     // authenticated value.
-    asm volatile ("mov   x30, %[checkValue]     \r\n" \
-                  "hint  0x7                    \r\n" \
-                  "mov   %[checkValue], x30     \r\n" \
+    asm volatile("mov   x30, %[checkValue]     \r\n" \
+                 "hint  0x7                    \r\n" \
+                 "mov   %[checkValue], x30     \r\n" \
         : [checkValue] "+r"(checkValue)
         :
         : "x30");
-    if (x17 != checkValue)
+    if (x17 != checkValue) {
+      _LIBUNWIND_LOG("x17 %llx, strip %llx, ptr %llc", x17, checkValue, ptr);
       _LIBUNWIND_ABORT("IP PAC authentication failure");
+    }
     return ret;
   }
 

>From 9ff7d832d0e11674450cea561966e442d0e02926 Mon Sep 17 00:00:00 2001
From: Daniel Kiss <daniel.kiss at arm.com>
Date: Fri, 25 Oct 2024 22:00:28 +0200
Subject: [PATCH 5/5] typo

---
 libunwind/src/Registers.hpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 14053ace492966..d58c84ba75b90e 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -1855,7 +1855,7 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
         :
         : "x30");
     if (x17 != checkValue) {
-      _LIBUNWIND_LOG("x17 %llx, strip %llx, ptr %llc", x17, checkValue, ptr);
+      _LIBUNWIND_LOG("x17 %llx, strip %llx, ptr %llx", x17, checkValue, ptr);
       _LIBUNWIND_ABORT("IP PAC authentication failure");
     }
     return ret;



More information about the cfe-commits mailing list