[PATCH 0/2] nptl: Update struct pthread_unwind_buf

[PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

struct pthread_unwind_buf is updated to save and restore shadow stack
register with backward binary compatibility.

H.J. Lu (2):
  Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)"
  nptl: Update struct pthread_unwind_buf [BZ #22743]

 bits/types/__cancel_jmp_buf_tag.h                  | 28 ++++++++
 csu/libc-start.c                                   |  6 +-
 nptl/Makefile                                      |  3 +-
 nptl/cleanup.c                                     |  9 ++-
 nptl/cleanup_defer.c                               | 16 +++--
 nptl/descr.h                                       | 78 +++++++++++++++++-----
 nptl/pthread_create.c                              |  9 ++-
 nptl/unwind.c                                      |  6 +-
 sysdeps/i386/nptl/tcb-offsets.sym                  |  1 +
 sysdeps/i386/nptl/tls.h                            |  4 ++
 sysdeps/nptl/pthread.h                             |  7 +-
 sysdeps/unix/sysv/linux/hppa/pthread.h             |  7 +-
 .../linux/x86/bits/types/__cancel_jmp_buf_tag.h    | 31 +++++++++
 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h        | 36 ++++++++++
 sysdeps/unix/sysv/linux/x86/pthreaddef.h           | 36 ++++++++++
 sysdeps/x86_64/nptl/tcb-offsets.sym                |  1 +
 sysdeps/x86_64/nptl/tls.h                          |  5 +-
 17 files changed, 238 insertions(+), 45 deletions(-)
 create mode 100644 bits/types/__cancel_jmp_buf_tag.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/pthreaddef.h

-- 
2.14.3

[PATCH 1/2] Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)"

[H.J. Lu]

This reverts commit 2ec0e7eade0ea1258acd5c6f5e5e9bfaeb5041a8.

This is needed to save and restore shadow stack register in setjmp and
longjmp.

	[BZ #22563]
	* sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New.
	* sysdeps/i386/nptl/tls.h (tcbhead_t): Add feature_1.
	* sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New.
	* sysdeps/x86_64/nptl/tls.h (tcbhead_t): Rename __glibc_unused1
	to feature_1.

	[BZ #22563]
	* bits/types/__cancel_jmp_buf_tag.h: New file.
	* sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
	* sysdeps/unix/sysv/linux/x86/pthreaddef.h: Likewise.
	* sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h: Likewise.
	* nptl/Makefile (headers): Add
	bits/types/__cancel_jmp_buf_tag.h.
	* nptl/descr.h [NEED_SAVED_MASK_IN_CANCEL_JMP_BUF]
	(pthread_unwind_buf): Add saved_mask to cancel_jmp_buf.
	* sysdeps/nptl/pthread.h: Include
	<bits/types/__cancel_jmp_buf_tag.h>.
	(__pthread_unwind_buf_t): Use struct __cancel_jmp_buf_tag with
	__cancel_jmp_buf.
	* sysdeps/unix/sysv/linux/hppa/pthread.h: Likewise.
---
 bits/types/__cancel_jmp_buf_tag.h                  | 28 +++++++++++++++++
 nptl/Makefile                                      |  3 +-
 nptl/descr.h                                       |  3 ++
 sysdeps/i386/nptl/tcb-offsets.sym                  |  1 +
 sysdeps/i386/nptl/tls.h                            |  4 +++
 sysdeps/nptl/pthread.h                             |  7 ++---
 sysdeps/unix/sysv/linux/hppa/pthread.h             |  7 ++---
 .../linux/x86/bits/types/__cancel_jmp_buf_tag.h    | 31 +++++++++++++++++++
 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h        | 36 ++++++++++++++++++++++
 sysdeps/unix/sysv/linux/x86/pthreaddef.h           | 22 +++++++++++++
 sysdeps/x86_64/nptl/tcb-offsets.sym                |  1 +
 sysdeps/x86_64/nptl/tls.h                          |  5 ++-
 12 files changed, 136 insertions(+), 12 deletions(-)
 create mode 100644 bits/types/__cancel_jmp_buf_tag.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
 create mode 100644 sysdeps/unix/sysv/linux/x86/pthreaddef.h

diff --git a/bits/types/__cancel_jmp_buf_tag.h b/bits/types/__cancel_jmp_buf_tag.h
new file mode 100644
index 0000000000..62f5c61f83
--- /dev/null
+++ b/bits/types/__cancel_jmp_buf_tag.h
@@ -0,0 +1,28 @@
+/* Define struct __cancel_jmp_buf_tag.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef ____cancel_jmp_buf_tag_defined
+#define ____cancel_jmp_buf_tag_defined 1
+
+struct __cancel_jmp_buf_tag
+  {
+    __jmp_buf __cancel_jmp_buf;
+    int __mask_was_saved;
+  };
+
+#endif
diff --git a/nptl/Makefile b/nptl/Makefile
index 6fc2c8bb6a..7940b3d26b 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -22,7 +22,8 @@ subdir	:= nptl
 
 include ../Makeconfig
 
-headers := pthread.h semaphore.h bits/semaphore.h
+headers := pthread.h semaphore.h bits/semaphore.h \
+	   bits/types/__cancel_jmp_buf_tag.h
 
 extra-libs := libpthread
 extra-libs-others := $(extra-libs)
diff --git a/nptl/descr.h b/nptl/descr.h
index 64ba29e1cb..1cc6b09d1e 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -65,6 +65,9 @@ struct pthread_unwind_buf
   {
     __jmp_buf jmp_buf;
     int mask_was_saved;
+#ifdef NEED_SAVED_MASK_IN_CANCEL_JMP_BUF
+    __sigset_t saved_mask;
+#endif
   } cancel_jmp_buf[1];
 
   union
diff --git a/sysdeps/i386/nptl/tcb-offsets.sym b/sysdeps/i386/nptl/tcb-offsets.sym
index 695a810386..250f1a6e13 100644
--- a/sysdeps/i386/nptl/tcb-offsets.sym
+++ b/sysdeps/i386/nptl/tcb-offsets.sym
@@ -15,3 +15,4 @@ POINTER_GUARD		offsetof (tcbhead_t, pointer_guard)
 #ifndef __ASSUME_PRIVATE_FUTEX
 PRIVATE_FUTEX		offsetof (tcbhead_t, private_futex)
 #endif
+FEATURE_1_OFFSET	offsetof (tcbhead_t, feature_1)
diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h
index fcda135b7c..30643d452a 100644
--- a/sysdeps/i386/nptl/tls.h
+++ b/sysdeps/i386/nptl/tls.h
@@ -50,6 +50,10 @@ typedef struct
   void *__private_tm[4];
   /* GCC split stack support.  */
   void *__private_ss;
+  /* Bit 0: IBT.
+     Bit 1: SHSTK.
+   */
+  unsigned int feature_1;
 } tcbhead_t;
 
 # define TLS_MULTIPLE_THREADS_IN_TCB 1
diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
index df049abf74..c8ba5a75c5 100644
--- a/sysdeps/nptl/pthread.h
+++ b/sysdeps/nptl/pthread.h
@@ -27,6 +27,7 @@
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/__cancel_jmp_buf_tag.h>
 
 
 /* Detach state.  */
@@ -523,11 +524,7 @@ extern void pthread_testcancel (void);
 
 typedef struct
 {
-  struct
-  {
-    __jmp_buf __cancel_jmp_buf;
-    int __mask_was_saved;
-  } __cancel_jmp_buf[1];
+  struct __cancel_jmp_buf_tag __cancel_jmp_buf[1];
   void *__pad[4];
 } __pthread_unwind_buf_t __attribute__ ((__aligned__));
 
diff --git a/sysdeps/unix/sysv/linux/hppa/pthread.h b/sysdeps/unix/sysv/linux/hppa/pthread.h
index 11a024db59..3df5e7c2ac 100644
--- a/sysdeps/unix/sysv/linux/hppa/pthread.h
+++ b/sysdeps/unix/sysv/linux/hppa/pthread.h
@@ -27,6 +27,7 @@
 #include <bits/setjmp.h>
 #include <bits/wordsize.h>
 #include <bits/types/struct_timespec.h>
+#include <bits/types/__cancel_jmp_buf_tag.h>
 
 
 /* Detach state.  */
@@ -499,11 +500,7 @@ extern void pthread_testcancel (void);
 
 typedef struct
 {
-  struct
-  {
-    __jmp_buf __cancel_jmp_buf;
-    int __mask_was_saved;
-  } __cancel_jmp_buf[1];
+  struct __cancel_jmp_buf_tag __cancel_jmp_buf[1];
   void *__pad[4];
 } __pthread_unwind_buf_t __attribute__ ((__aligned__));
 
diff --git a/sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h b/sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
new file mode 100644
index 0000000000..70efbb190c
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
@@ -0,0 +1,31 @@
+/* Define struct __cancel_jmp_buf_tag.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef ____cancel_jmp_buf_tag_defined
+#define ____cancel_jmp_buf_tag_defined 1
+
+#include <bits/types/__sigset_t.h>
+
+struct __cancel_jmp_buf_tag
+  {
+    __jmp_buf __cancel_jmp_buf;
+    int __mask_was_saved;
+    __sigset_t __saved_mask;
+  };
+
+#endif
diff --git a/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h b/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
new file mode 100644
index 0000000000..247a62e9a0
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
@@ -0,0 +1,36 @@
+/* Internal pthread header.  Linux/x86 version.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include_next <nptl/pthreadP.h>
+
+#ifndef _PTHREADP_H_X86
+#define _PTHREADP_H_X86 1
+
+extern struct pthread_unwind_buf ____pthread_unwind_buf_private;
+
+_Static_assert (sizeof (____pthread_unwind_buf_private.cancel_jmp_buf)
+		>= sizeof (struct __jmp_buf_tag),
+		"size of cancel_jmp_buf < sizeof __jmp_buf_tag");
+
+extern __pthread_unwind_buf_t ____pthread_unwind_buf;
+
+_Static_assert (sizeof (____pthread_unwind_buf.__cancel_jmp_buf)
+		>= sizeof (struct __jmp_buf_tag),
+		"size of __cancel_jmp_buf < sizeof __jmp_buf_tag");
+
+#endif
diff --git a/sysdeps/unix/sysv/linux/x86/pthreaddef.h b/sysdeps/unix/sysv/linux/x86/pthreaddef.h
new file mode 100644
index 0000000000..a405a65666
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/pthreaddef.h
@@ -0,0 +1,22 @@
+/* Pthread macros.  Linux/x86 version.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include_next <pthreaddef.h>
+
+/* Need saved_mask in cancel_jmp_buf.  */
+#define NEED_SAVED_MASK_IN_CANCEL_JMP_BUF 1
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index 8a25c482cb..03b6dba5c3 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -15,6 +15,7 @@ VGETCPU_CACHE_OFFSET	offsetof (tcbhead_t, vgetcpu_cache)
 #ifndef __ASSUME_PRIVATE_FUTEX
 PRIVATE_FUTEX		offsetof (tcbhead_t, private_futex)
 #endif
+FEATURE_1_OFFSET	offsetof (tcbhead_t, feature_1)
 
 -- Not strictly offsets, but these values are also used in the TCB.
 TCB_CANCELSTATE_BITMASK	 CANCELSTATE_BITMASK
diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index bdd02376f9..7f0b292f42 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -56,7 +56,10 @@ typedef struct
 # else
   int __glibc_reserved1;
 # endif
-  int __glibc_unused1;
+  /* Bit 0: IBT.
+     Bit 1: SHSTK.
+   */
+  unsigned int feature_1;
   /* Reservation of some values for the TM ABI.  */
   void *__private_tm[4];
   /* GCC split stack support.  */
-- 
2.14.3

[PATCH 2/2] nptl: Update struct pthread_unwind_buf [BZ #22743]

[H.J. Lu]

In glibc 2.28, the size of cancel_jmp_buf in struct pthread_unwind_buf
has been increased to match the size of __jmp_buf_tag on Linux/x86 in
order to save and restore shadow stack register.  pthread_unwind_buf is
used in <pthread.h>, whose address is passed from applications to
libpthread.  To access the private data in struct pthread_unwind_buf,
which is placed after cancel_jmp_buf, in libpthread, we must know which
struct pthread_unwind_buf, before glibc 28 and after glibc 2.28, is used
in caller.  If the size of caller's struct pthread_unwind_buf is smaller
than what libpthread expects, libpthread will override caller's stack
since struct pthread_unwind_buf is placed on caller's stack.

We enable shadow stack at run-time only if program and all used shared
objects, including dlopened ones, are shadow stack enabled, which means
that they must be compiled with GCC 8 or above and glibc 2.28 or above.
Since we need to save and restore shadow stack register only if shadow
stack is enabled, we can safely assume that caller is compiled with
smaller struct pthread_unwind_buf on stack if shadow stack isn't enabled
at run-time.  For callers with larger struct pthread_unwind_buf, but
shadow stack isn't enabled, we just have some unused space on caller's
stack.

struct pthread_unwind_buf is changed to union of

1. struct cancel_jmp_buf[1], which contains the common fields of struct
full and struct compat_pthread_unwind_buf.
2. struct full_pthread_unwind_buf, which is the full layout of the
cleanup buffer.
3. struct compat_pthread_unwind_buf, which is the compatible layout of
the cleanup buffer.

A macro, UNWIND_BUF_PRIV, is added to get the pointer to the priv field.
By default, it uses the priv field of struct compat_pthread_unwind_buf.
If a target defines NEED_SAVED_MASK_IN_CANCEL_JMP_BUF, it must provide
its own version of UNEIND_BUF_PRIV to get the pointer to the priv field.
On Linux/x86, it uses the priv field of struct compat_pthread_unwind_buf
if shadow stack is disabled and struct full_pthread_unwind_buf if shadow
stack is enabled. The overhead of in __pthread_register_cancel on x86-64
is:

Without UNWIND_BUF_PRIV:

	movq	%fs:768,%rax
	movq	%rax, 72(%rdi)
	movq	%fs:760,%rax
	movq	%rax, 80(%rdi)
	movq	%rdi,%fs:768
	ret

With UNWIND_BUF_PRIV:

	movl	%fs:76,%ecx
	leaq	72(%rdi), %rdx
	leaq	200(%rdi), %rax
	andl	$2, %ecx
	cmove	%rdx, %rax
	movq	%fs:768,%rdx
	movq	%rdx, (%rax)
	movq	%fs:760,%rdx
	movq	%rdx, 8(%rax)
	movq	%rdi,%fs:768
	ret

Note: There is an unused pointer space in pthread_unwind_buf_data.  But
it isn't suitable for saving and restoring shadow stack register since
x32 is a 64-bit process with 32-bit software pointer and kernel may
place x32 shadow stack above 4GB.  We need to save and restore 64-bit
shadow stack register for x32.

	[BZ #22743]
	* csu/libc-start.c (LIBC_START_MAIN): Use the full version of
	the cleanup buffer.
	* nptl/cleanup.c (__pthread_register_cancel): Use UNWIND_BUF_PRIV
	to access the priv field in the cleanup buffer.
	(__pthread_unregister_cancel): Likewise.
	* nptl/cleanup_defer.c (__pthread_register_cancel_defer):
	Likewise.
	(__pthread_unregister_cancel_restore): Likewise.
	* nptl/unwind.c (unwind_stop): Likewise.
	(__pthread_unwind_next): Likewise.
	* nptl/descr.h (pthread_unwind_buf_data): New.
	(full_pthread_unwind_buf): Likewise.
	(compat_pthread_unwind_buf): Likewise.
	(pthread_unwind_buf): Updated to use full_pthread_unwind_buf
	and compat_pthread_unwind_buf.
	(UNWIND_BUF_PRIV): New.  Macro to get pointer to the priv field
	in the cleanup buffer.
	* nptl/pthread_create.c (START_THREAD_DEFN): Use the full
	version of the cleanup buffer.
	(__pthread_create_2_1): Use THREAD_COPY_ADDITONAL_INFO to copy
	additonal info if defined.
	* sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h: Use the full
	version of the cleanup buffer to check cancel_jmp_buf size.
	* sysdeps/unix/sysv/linux/x86/pthreaddef.h
	(THREAD_COPY_ADDITONAL_INFO): New.
	(UNWIND_BUF_PRIV): Likewise.
---
 csu/libc-start.c                            |  6 ++-
 nptl/cleanup.c                              |  9 ++--
 nptl/cleanup_defer.c                        | 16 +++---
 nptl/descr.h                                | 75 ++++++++++++++++++++++-------
 nptl/pthread_create.c                       |  9 +++-
 nptl/unwind.c                               |  6 ++-
 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h |  2 +-
 sysdeps/unix/sysv/linux/x86/pthreaddef.h    | 14 ++++++
 8 files changed, 103 insertions(+), 34 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 605222fa3f..c6bbc97ef0 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -298,8 +298,10 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
       struct pthread *self = THREAD_SELF;
 
       /* Store old info.  */
-      unwind_buf.priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
-      unwind_buf.priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+      unwind_buf.full.priv.data.prev
+	= THREAD_GETMEM (self, cleanup_jmp_buf);
+      unwind_buf.full.priv.data.cleanup
+	= THREAD_GETMEM (self, cleanup);
 
       /* Store the new cleanup handler info.  */
       THREAD_SETMEM (self, cleanup_jmp_buf, &unwind_buf);
diff --git a/nptl/cleanup.c b/nptl/cleanup.c
index d21b86e88b..6403a42d46 100644
--- a/nptl/cleanup.c
+++ b/nptl/cleanup.c
@@ -28,8 +28,9 @@ __pthread_register_cancel (__pthread_unwind_buf_t *buf)
   struct pthread *self = THREAD_SELF;
 
   /* Store old info.  */
-  ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
-  ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+  union pthread_unwind_buf_data *priv = UNWIND_BUF_PRIV (self, ibuf);
+  priv->data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
+  priv->data.cleanup = THREAD_GETMEM (self, cleanup);
 
   /* Store the new cleanup handler info.  */
   THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
@@ -42,7 +43,9 @@ __cleanup_fct_attribute
 __pthread_unregister_cancel (__pthread_unwind_buf_t *buf)
 {
   struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
+  struct pthread *self = THREAD_SELF;
 
-  THREAD_SETMEM (THREAD_SELF, cleanup_jmp_buf, ibuf->priv.data.prev);
+  THREAD_SETMEM (self, cleanup_jmp_buf,
+		 UNWIND_BUF_PRIV (self, ibuf)->data.prev);
 }
 hidden_def (__pthread_unregister_cancel)
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 5701ce4213..fddf7434db 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -28,8 +28,9 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
   struct pthread *self = THREAD_SELF;
 
   /* Store old info.  */
-  ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
-  ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+  union pthread_unwind_buf_data *priv = UNWIND_BUF_PRIV (self, ibuf);
+  priv->data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
+  priv->data.cleanup = THREAD_GETMEM (self, cleanup);
 
   int cancelhandling = THREAD_GETMEM (self, cancelhandling);
 
@@ -49,9 +50,9 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
 	cancelhandling = curval;
       }
 
-  ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
-				? PTHREAD_CANCEL_ASYNCHRONOUS
-				: PTHREAD_CANCEL_DEFERRED);
+  priv->data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
+			   ? PTHREAD_CANCEL_ASYNCHRONOUS
+			   : PTHREAD_CANCEL_DEFERRED);
 
   /* Store the new cleanup handler info.  */
   THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
@@ -64,11 +65,12 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
 {
   struct pthread *self = THREAD_SELF;
   struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
+  union pthread_unwind_buf_data *priv = UNWIND_BUF_PRIV (self, ibuf);
 
-  THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
+  THREAD_SETMEM (self, cleanup_jmp_buf, priv->data.prev);
 
   int cancelhandling;
-  if (ibuf->priv.data.canceltype != PTHREAD_CANCEL_DEFERRED
+  if (priv->data.canceltype != PTHREAD_CANCEL_DEFERRED
       && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
 	  & CANCELTYPE_BITMASK) == 0)
     {
diff --git a/nptl/descr.h b/nptl/descr.h
index 1cc6b09d1e..662696dca6 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -55,11 +55,30 @@
    / PTHREAD_KEY_2NDLEVEL_SIZE)
 
 
+/* Private data in the cleanup buffer.  */
+union pthread_unwind_buf_data
+{
+  /* This is the placeholder of the public version.  */
+  void *pad[4];
 
+  struct
+  {
+    /* Pointer to the previous cleanup buffer.  */
+    struct pthread_unwind_buf *prev;
 
-/* Internal version of the buffer to store cancellation handler
+    /* Backward compatibility: state of the old-style cleanup
+       handler at the time of the previous new-style cleanup handler
+       installment.  */
+    struct _pthread_cleanup_buffer *cleanup;
+
+    /* Cancellation type before the push call.  */
+    int canceltype;
+  } data;
+};
+
+/* Internal full version of the buffer to store cancellation handler
    information.  */
-struct pthread_unwind_buf
+struct full_pthread_unwind_buf
 {
   struct
   {
@@ -70,27 +89,49 @@ struct pthread_unwind_buf
 #endif
   } cancel_jmp_buf[1];
 
-  union
+  union pthread_unwind_buf_data priv;
+};
+
+/* Internal compatible version of the buffer to store cancellation
+   handler information.  */
+struct compat_pthread_unwind_buf
+{
+  struct
   {
-    /* This is the placeholder of the public version.  */
-    void *pad[4];
+    __jmp_buf jmp_buf;
+    int mask_was_saved;
+  } cancel_jmp_buf[1];
+
+  union pthread_unwind_buf_data priv;
+};
 
+/* Internal version of the buffer to store cancellation handler
+   information.  */
+struct pthread_unwind_buf
+{
+  union
+  {
+    /* The common fields of full and compatible versions.  */
     struct
     {
-      /* Pointer to the previous cleanup buffer.  */
-      struct pthread_unwind_buf *prev;
-
-      /* Backward compatibility: state of the old-style cleanup
-	 handler at the time of the previous new-style cleanup handler
-	 installment.  */
-      struct _pthread_cleanup_buffer *cleanup;
-
-      /* Cancellation type before the push call.  */
-      int canceltype;
-    } data;
-  } priv;
+      __jmp_buf jmp_buf;
+      int mask_was_saved;
+    } cancel_jmp_buf[1];
+    struct full_pthread_unwind_buf full;
+    struct compat_pthread_unwind_buf compat;
+  };
 };
 
+/* Get pointer to the priv field from THREAD_SELF, "self", and pointer
+   to the cleanup buffer, "p".  By default, the compatible version is
+   used.  If a target defines NEED_SAVED_MASK_IN_CANCEL_JMP_BUF, it
+   must provide its own version of UNEIND_BUF_PRIV.  */
+#ifndef UNWIND_BUF_PRIV
+# ifdef NEED_SAVED_MASK_IN_CANCEL_JMP_BUF
+#  error "UNWIND_BUF_PRIV is undefined!"
+# endif
+# define UNWIND_BUF_PRIV(self,p) (&((p)->compat.priv))
+#endif
 
 /* Opcodes and data types for communication with the signal handler to
    change user/group IDs.  */
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index caaf07c134..082615e080 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -428,8 +428,8 @@ START_THREAD_DEFN
   struct pthread_unwind_buf unwind_buf;
 
   /* No previous handlers.  */
-  unwind_buf.priv.data.prev = NULL;
-  unwind_buf.priv.data.cleanup = NULL;
+  unwind_buf.full.priv.data.prev = NULL;
+  unwind_buf.full.priv.data.cleanup = NULL;
 
   int not_first_call;
   not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
@@ -701,6 +701,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
   THREAD_COPY_POINTER_GUARD (pd);
 #endif
 
+  /* Copy additonal info.  */
+#ifdef THREAD_COPY_ADDITONAL_INFO
+  THREAD_COPY_ADDITONAL_INFO (pd);
+#endif
+
   /* Verify the sysinfo bits were copied in allocate_stack if needed.  */
 #ifdef NEED_DL_SYSINFO
   CHECK_THREAD_SYSINFO (pd);
diff --git a/nptl/unwind.c b/nptl/unwind.c
index b37a063c53..f58be0ee5f 100644
--- a/nptl/unwind.c
+++ b/nptl/unwind.c
@@ -66,7 +66,8 @@ unwind_stop (int version, _Unwind_Action actions,
       /* Handle the compatibility stuff.  Execute all handlers
 	 registered with the old method which would be unwound by this
 	 step.  */
-      struct _pthread_cleanup_buffer *oldp = buf->priv.data.cleanup;
+      struct _pthread_cleanup_buffer *oldp
+	= UNWIND_BUF_PRIV (self, buf)->data.cleanup;
       void *cfa = (void *) (_Unwind_Ptr) _Unwind_GetCFA (context);
 
       if (curp != oldp && (do_longjump || FRAME_LEFT (cfa, curp, adj)))
@@ -133,6 +134,7 @@ __pthread_unwind_next (__pthread_unwind_buf_t *buf)
 {
   struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
 
-  __pthread_unwind ((__pthread_unwind_buf_t *) ibuf->priv.data.prev);
+  __pthread_unwind ((__pthread_unwind_buf_t *)
+		    UNWIND_BUF_PRIV (THREAD_SELF, ibuf)->data.prev);
 }
 hidden_def (__pthread_unwind_next)
diff --git a/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h b/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
index 247a62e9a0..ff9ea4cb6d 100644
--- a/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
+++ b/sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
@@ -23,7 +23,7 @@
 
 extern struct pthread_unwind_buf ____pthread_unwind_buf_private;
 
-_Static_assert (sizeof (____pthread_unwind_buf_private.cancel_jmp_buf)
+_Static_assert (sizeof (____pthread_unwind_buf_private.full.cancel_jmp_buf)
 		>= sizeof (struct __jmp_buf_tag),
 		"size of cancel_jmp_buf < sizeof __jmp_buf_tag");
 
diff --git a/sysdeps/unix/sysv/linux/x86/pthreaddef.h b/sysdeps/unix/sysv/linux/x86/pthreaddef.h
index a405a65666..52198aef73 100644
--- a/sysdeps/unix/sysv/linux/x86/pthreaddef.h
+++ b/sysdeps/unix/sysv/linux/x86/pthreaddef.h
@@ -20,3 +20,17 @@
 
 /* Need saved_mask in cancel_jmp_buf.  */
 #define NEED_SAVED_MASK_IN_CANCEL_JMP_BUF 1
+
+/* Wee need to copy feature_1 in pthread_create.  */
+#define THREAD_COPY_ADDITONAL_INFO(descr)				\
+  ((descr)->header.feature_1						\
+   = THREAD_GETMEM (THREAD_SELF, header.feature_1))
+
+/* Use the compatible struct __cancel_jmp_buf_tag if shadow stack is
+   disabled.  */
+#undef UNWIND_BUF_PRIV
+#define UNWIND_BUF_PRIV(self,p) \
+  (__extension__ ({							\
+     unsigned int feature_1 = THREAD_GETMEM (self, header.feature_1);	\
+     (((feature_1 & (1 << 1)) == 0)					\
+      ? &((p)->compat.priv) : &((p)->full.priv));}))
-- 
2.14.3

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/01/2018 12:57 PM, H.J. Lu wrote:
> struct pthread_unwind_buf is updated to save and restore shadow stack
> register with backward binary compatibility.
> 
> H.J. Lu (2):
>   Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)"
>   nptl: Update struct pthread_unwind_buf [BZ #22743]
> 

High Level:

To enable Intel CET we need to make sure that all places that change
execution context abruptly also adjust the shadow stack such that the
subsequent return matches what is presently on the stack. The current
setjmp/longjmp are just such functions that change execution context
abruptly, and to use longjmp to return to a setjmp location we must
also restore the shadow stack on the return jump.

The cancellation push/pop sequences also use a jump buffer, and at a
high-level your solution is to expand those buffers with enough space
(an ABI change) to store the shadow stack pointer. The ABI transition
is managed using the CET note markup, and safety is ensured in that way
as a all-or-nothing ABI change.

Note that it depends heavily CET markup rolling out smoothly with glibc
2.28. If any distro enables CET markup without glibc 2.28 then it might
be possible to have objects that use the old-ABI jump buffers in a CET
enabled application and crash. Similarly if users build and use their
own runtime.

Why not version __pthread_register_cancel instead? The versioned symbol
would seem to me a more robust indicator of the larger jump buffer than
the requirement on coordinating markup + glibc 2.28 in all distros and
for all users?

I would like to see an argument made for CET markup against versioning
__pthread_register_cleanup.

Design:

Commit f33632ccd1dec3217583fcfdd965afb62954203c finds space in the
existing sigset_t by reducing the number of possible signals down to
513, the kernel is only using 64, and the remaining space is used to
store the 64-bit shadow stack pointer.

However, this isn't the only place that jump buffers like those used
in setjmp/longjmp are used. Internally in pthread_cleanup_push and
pthread_clenaup_pop the cancellation is handled by a special
structure that is almost identical to the jump buffers used by
setjmp/longjmp. However, because they are not identical, and were
designed to avoid needing to save/restore the process signal mask
during cancellation (as an optimization), these buffers also do
not have enough space to store the shadow stack pointer.

Therefore there needs to be an expansion in the size of the jump
buffer used for cancellation to accomodate the shadow stack pointer.

Your patches present the most logical next step here, which is to
make the cancellation jump buffer the same size as the normal jump
buffer used for setjmp/longjmp.

Changing the cancellation jump buffer is an ABI change though since
the jump buffer is allocated in the application stack via the the
pthread_cleanup_push macro. Some way must be found to determine which
size of jump buffer the caller is using to know if the newer larger
version can be used.

It is not sufficient to version pthread_create to make this distinction
because an application may have been compiled with a new glibc, get
the new version of pthread_create, and then call a DSO which used the
old pthread_cleanup_push, and so expecte the old-ABI jump buffer.

It is not sufficient to use priv.pad[3] because on ABIs with 32-bit
pointers like x32 there is insufficient space to store the 64-bit
shadow stack pointer.

It *is* sufficient to version __pthread_register_cleanup in theory
because on a caller-by-caller basis this would indicate if the caller
had been compiled with the old or new-ABI jump buffer and read
accordingly.

Your patches take another approach, which is to use the binutils CET
markup to indicate an all-or-nothing ABI transition to the new larger
sized jump buffer. If CET is enabled, then the application would have
been compiled with a CET enabled gcc, CET enabled glibc, and CET
enabled binutils, and the result is a CET enabled application which
is then assumed to have the newer larger sized cancellation jump buffer.
Your solution avoids adding another symbol version and uses the existing
CET markup as the deciding factor.

Why might we reject versioning __pthread_register_cleanup? It would seem
that since glibc internally controls the pthread_unwind_buf it will never
be passed by pointer to another TU to be used in ways we don't control
(the problem s390x faced when trying to increase the size of jmpbuf
for setjmp/longjmp) e.g. loaded by a newer version of __pthread_register_cleanup
that expects a different sized object. The internals and their use
as expected from pthread_cleanup_push/pop are all in the same translation
unit.

Again, I would like to see an argument made for CET markup against versioning
__pthread_register_cleanup.

Details:

The name NEED_SAVED_MASK_IN_CANCEL_JMP_BUF while true is slightly
misleading IMO. While it is true that you need the saved mask there, the
actual logical goal is to make the structure match in layout with the
non-cancel jmp_buf. I would rename this to NEED_SETJMP_LAYOUT or something
like that to indicate the intent is to make the structure match the layout
used by setjmp jump buffers.

Both UNWIND_BUF_PRIV and THREAD_COPY_ADDITONAL_INFO are exmaples of typo-prone
macro APIs. We should work hard to define defaults and have unconditional
users of these macros such that -Wundef can warn us if we make mistakes
e.g. https://sourceware.org/glibc/wiki/Wundef

Notes:
- This current patch set passes the ABI tests I ran last time so it is good
  to note that your patches do not appear to introduce any other ABI regressions.

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* Carlos O'Donell:

> I would like to see an argument made for CET markup against versioning
> __pthread_register_cleanup.

I don't think __pthread_register_cleanup.  It's __sigsetjmp.  We would
have to version __sigsetjmp.  Changing the name would be ideal, but
this will be difficult because of its returns-twice nature.

I also don't have a problem with requiring -fexceptions for
cancellation handlers with CET support.  For additional safety, we
could change sigsetjmp to write the shadow stack pointer before the
kernel signal mask, so that it will still be in bounds for legacy
cancellation handler allocation.  __pthread_register_cleanup will
overwrite it, but I don't think we ever need to restore it, so that
shouldn't be a problem.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Thu, Feb 8, 2018 at 1:25 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/01/2018 12:57 PM, H.J. Lu wrote:
>> struct pthread_unwind_buf is updated to save and restore shadow stack
>> register with backward binary compatibility.
>>
>> H.J. Lu (2):
>>   Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)"
>>   nptl: Update struct pthread_unwind_buf [BZ #22743]
>>
>
> High Level:
>
> To enable Intel CET we need to make sure that all places that change
> execution context abruptly also adjust the shadow stack such that the
> subsequent return matches what is presently on the stack. The current
> setjmp/longjmp are just such functions that change execution context
> abruptly, and to use longjmp to return to a setjmp location we must
> also restore the shadow stack on the return jump.
>
> The cancellation push/pop sequences also use a jump buffer, and at a
> high-level your solution is to expand those buffers with enough space
> (an ABI change) to store the shadow stack pointer. The ABI transition
> is managed using the CET note markup, and safety is ensured in that way
> as a all-or-nothing ABI change.
>
> Note that it depends heavily CET markup rolling out smoothly with glibc
> 2.28. If any distro enables CET markup without glibc 2.28 then it might
> be possible to have objects that use the old-ABI jump buffers in a CET
> enabled application and crash. Similarly if users build and use their
> own runtime.
>
> Why not version __pthread_register_cancel instead? The versioned symbol
> would seem to me a more robust indicator of the larger jump buffer than
> the requirement on coordinating markup + glibc 2.28 in all distros and
> for all users?
>
> I would like to see an argument made for CET markup against versioning
> __pthread_register_cleanup.
>
> Design:
>
> Commit f33632ccd1dec3217583fcfdd965afb62954203c finds space in the
> existing sigset_t by reducing the number of possible signals down to
> 513, the kernel is only using 64, and the remaining space is used to
> store the 64-bit shadow stack pointer.
>
> However, this isn't the only place that jump buffers like those used
> in setjmp/longjmp are used. Internally in pthread_cleanup_push and
> pthread_clenaup_pop the cancellation is handled by a special
> structure that is almost identical to the jump buffers used by
> setjmp/longjmp. However, because they are not identical, and were
> designed to avoid needing to save/restore the process signal mask
> during cancellation (as an optimization), these buffers also do
> not have enough space to store the shadow stack pointer.
>
> Therefore there needs to be an expansion in the size of the jump
> buffer used for cancellation to accomodate the shadow stack pointer.
>
> Your patches present the most logical next step here, which is to
> make the cancellation jump buffer the same size as the normal jump
> buffer used for setjmp/longjmp.
>
> Changing the cancellation jump buffer is an ABI change though since
> the jump buffer is allocated in the application stack via the the
> pthread_cleanup_push macro. Some way must be found to determine which
> size of jump buffer the caller is using to know if the newer larger
> version can be used.
>
> It is not sufficient to version pthread_create to make this distinction
> because an application may have been compiled with a new glibc, get
> the new version of pthread_create, and then call a DSO which used the
> old pthread_cleanup_push, and so expecte the old-ABI jump buffer.
>
> It is not sufficient to use priv.pad[3] because on ABIs with 32-bit
> pointers like x32 there is insufficient space to store the 64-bit
> shadow stack pointer.
>
> It *is* sufficient to version __pthread_register_cleanup in theory
> because on a caller-by-caller basis this would indicate if the caller
> had been compiled with the old or new-ABI jump buffer and read
> accordingly.
>
> Your patches take another approach, which is to use the binutils CET
> markup to indicate an all-or-nothing ABI transition to the new larger
> sized jump buffer. If CET is enabled, then the application would have
> been compiled with a CET enabled gcc, CET enabled glibc, and CET
> enabled binutils, and the result is a CET enabled application which
> is then assumed to have the newer larger sized cancellation jump buffer.
> Your solution avoids adding another symbol version and uses the existing
> CET markup as the deciding factor.
>
> Why might we reject versioning __pthread_register_cleanup? It would seem
> that since glibc internally controls the pthread_unwind_buf it will never
> be passed by pointer to another TU to be used in ways we don't control
> (the problem s390x faced when trying to increase the size of jmpbuf
> for setjmp/longjmp) e.g. loaded by a newer version of __pthread_register_cleanup
> that expects a different sized object. The internals and their use
> as expected from pthread_cleanup_push/pop are all in the same translation
> unit.
>
> Again, I would like to see an argument made for CET markup against versioning
> __pthread_register_cleanup.

Symbol versioning works well when only opaque data pointers are used.
A pointer of struct pthread_unwind_buf is passed to libpthread from user
programs.  Within libpthread, we need to know the exact layout of struct
pthread_unwind_buf.  It is next to impossible to use symbol versioning here.

> Details:
>
> The name NEED_SAVED_MASK_IN_CANCEL_JMP_BUF while true is slightly
> misleading IMO. While it is true that you need the saved mask there, the
> actual logical goal is to make the structure match in layout with the
> non-cancel jmp_buf. I would rename this to NEED_SETJMP_LAYOUT or something

Will do.

> like that to indicate the intent is to make the structure match the layout
> used by setjmp jump buffers.
>
> Both UNWIND_BUF_PRIV and THREAD_COPY_ADDITONAL_INFO are exmaples of typo-prone
> macro APIs. We should work hard to define defaults and have unconditional
> users of these macros such that -Wundef can warn us if we make mistakes
> e.g. https://sourceware.org/glibc/wiki/Wundef

Will do.

> Notes:
> - This current patch set passes the ABI tests I ran last time so it is good
>   to note that your patches do not appear to introduce any other ABI regressions.
>

Thanks.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/08/2018 03:55 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> I would like to see an argument made for CET markup against versioning
>> __pthread_register_cleanup.
> 
> I don't think __pthread_register_cleanup.  It's __sigsetjmp.  We would
> have to version __sigsetjmp.  Changing the name would be ideal, but
> this will be difficult because of its returns-twice nature.

I'm looking for a solution that yields a clearly diagnosable failure
when users mix a compiler which emits CET markup and a glibc which has
the smaller sized cancellation jump buffer.

If we version __sigsetjmp, we need to decide the semantics of the old
and new version.

A naive old version would not save the shadow stack pointer to the
reclaimed space after the newer smaller internal __sigset_t, and a newer
version would save the shadow stack pointer.

This leads to sigsegv when a user mixes as above by accident, and they
will have no warning about the problem. The CET markup will be present
in all objects they have, but when run with a newer glibc it will crash
by corrupting the stack when it writes the shadow stack pointer.

My suggestion with __pthread_register_cancel et.al. (and yes we would 
have to version all the interfaces) was to basically data version the
structure, but my original idea revolved around the idea of shifting
__pad[3] up to the start of the structure and use it for versioning.

However, your idea to version __sigsetjmp has reminded me that
__saved_mask need only be non-zero to work properly and we could
store a version cookie there, which could allow us to do the following:

* Version __sigsetjmp, and store a version cookie in __saved_mask
  indicating the version of the structure.
* Have the old __sigsetjmp disable CET since it clearly indicates
  that we have the wrong sized structure regardless of CET flags.
* Adjust the unwinder to look at __saved_mask version cooke to decide
  which sized structure it is dealing with.

Is this feasible? It would give high quality diagnostics and potentially
allow us to extend the cancellation jump buffer with more data in the
future.

> I also don't have a problem with requiring -fexceptions for
> cancellation handlers with CET support.  For additional safety, we
> could change sigsetjmp to write the shadow stack pointer before the
> kernel signal mask, so that it will still be in bounds for legacy
> cancellation handler allocation.  __pthread_register_cleanup will
> overwrite it, but I don't think we ever need to restore it, so that
> shouldn't be a problem.
 
In the truncated sigjmp_buf used by pthread cancellation we have only 
void * which is not large enough on x32, where you need a 64-bit
shadow stack pointer. It would work on every other machine though.
HJ has mentioned this problem already IIRC.

Why would __pthread_register_cancel overwrite it?

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/08/2018 05:27 AM, H.J. Lu wrote:
> On Thu, Feb 8, 2018 at 1:25 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> Again, I would like to see an argument made for CET markup against versioning
>> __pthread_register_cleanup.
> 
> Symbol versioning works well when only opaque data pointers are used.
> A pointer of struct pthread_unwind_buf is passed to libpthread from user
> programs.  Within libpthread, we need to know the exact layout of struct
> pthread_unwind_buf.  It is next to impossible to use symbol versioning here.

My worry is about incorrectly marked up objects which use the wrong unwind
buffer size resulting in difficult to diagnose crashes.

The exact issue would be users using a gcc and binutils which marks up binaries
with CET notes, but glibc headers which are not CET enabled.

This will happen when we ship Developer Toolset with a gcc and binutils that
has CET support, but the system glibc is still < 2.28. Then users will take
these binaries and try to run them on systems with glibc >= 2.28 because we
allow this backwards compatibility, and this will segfault because we are
turning on CET, but mixing different sized unwind buffers.

Please tell me if you think my worries are unfounded, or of sufficiently
low risk that they will not be a problem, or that we can somehow say that
what was done was unsupported e.g. saying that Developer Toolset used with
Intel CET flags to build anything on any system with glibc < 2.28 generates
unsupported binaries.

My suggestion in the other email is that we briefly investigate what it would
take version the data in the unwind buffer such that we *can* handle different
sized objects properly, and enable us to potentially grow the unwind data
we have in the future.

To reiterate from my other email:
* Version __sigsetjmp, and store a version cookie in __saved_mask
  indicating the version of the structure.
* Have the old __sigsetjmp disable CET since it clearly indicates
  that we have the wrong sized structure regardless of CET flags.
* Adjust the unwinder to look at __saved_mask version cooke to decide
  which sized structure it is dealing with.

The third bullet point is effectively what you are *already* doing
with the CET flags, but using the flags in a coarse way to decide
which of 2 sizes of unwind buffers we are using. This has the problems that
I outline above which is that it is error prone. While the symbol version
mechanism will always tell us exactly which sized object was used when
the TU was compiled.

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* Carlos O'Donell:

> In the truncated sigjmp_buf used by pthread cancellation we have only 
> void * which is not large enough on x32, where you need a 64-bit
> shadow stack pointer. It would work on every other machine though.
> HJ has mentioned this problem already IIRC.
>
> Why would __pthread_register_cancel overwrite it?

__pthread_unwind_buf_t is defined as:

typedef struct
{
  struct
  {
    __jmp_buf __cancel_jmp_buf;
    int __mask_was_saved;
  } __cancel_jmp_buf[1];
  void *__pad[4];
} __pthread_unwind_buf_t __attribute__ ((__aligned__));

pthread_cleanup_push does this:

# define pthread_cleanup_push(routine, arg) \
  do {									      \
    __pthread_unwind_buf_t __cancel_buf;				      \
    void (*__cancel_routine) (void *) = (routine);			      \
    void *__cancel_arg = (arg);						      \
    int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *)     \
					__cancel_buf.__cancel_jmp_buf, 0);    \
    if (__glibc_unlikely (__not_first_call))				      \
      {									      \
	__cancel_routine (__cancel_arg);				      \
	__pthread_unwind_next (&__cancel_buf);				      \
	/* NOTREACHED */						      \
      }									      \
									      \
    __pthread_register_cancel (&__cancel_buf);				      \
    do {

__pthread_register_cancel overwrites __cancel_buf.__pad.  If
__sigsetjmp were to write to that memory area, it would not matter, as
long as we skip restoring the shadow stack pointer during unwinding
(which we do not need to do because we never return along the regular
execution path recorded on the shadow stack).

In short, the only thing need to ensure is that the over-write from
__sigsetjmp stays within __cancel_buf.  Then we are good, without
changing the stack layout for cancellation.

My proposal is still rather hackish, but so is the existing code (the
truncated jump buffer), and HJ's approach of storing the shadow stack
pointer in the signal save area of the non-truncated jump buffer.  But
I think we can make it work.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Fri, Feb 9, 2018 at 2:48 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Carlos O'Donell:
>
>> In the truncated sigjmp_buf used by pthread cancellation we have only
>> void * which is not large enough on x32, where you need a 64-bit
>> shadow stack pointer. It would work on every other machine though.
>> HJ has mentioned this problem already IIRC.
>>
>> Why would __pthread_register_cancel overwrite it?
>
> __pthread_unwind_buf_t is defined as:
>
> typedef struct
> {
>   struct
>   {
>     __jmp_buf __cancel_jmp_buf;
>     int __mask_was_saved;
>   } __cancel_jmp_buf[1];
>   void *__pad[4];
> } __pthread_unwind_buf_t __attribute__ ((__aligned__));
>
> pthread_cleanup_push does this:
>
> # define pthread_cleanup_push(routine, arg) \
>   do {                                                                        \
>     __pthread_unwind_buf_t __cancel_buf;                                      \
>     void (*__cancel_routine) (void *) = (routine);                            \
>     void *__cancel_arg = (arg);                                               \
>     int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *)     \
>                                         __cancel_buf.__cancel_jmp_buf, 0);    \
>     if (__glibc_unlikely (__not_first_call))                                  \
>       {                                                                       \
>         __cancel_routine (__cancel_arg);                                      \
>         __pthread_unwind_next (&__cancel_buf);                                \
>         /* NOTREACHED */                                                      \
>       }                                                                       \
>                                                                               \
>     __pthread_register_cancel (&__cancel_buf);                                \
>     do {
>
> __pthread_register_cancel overwrites __cancel_buf.__pad.  If
> __sigsetjmp were to write to that memory area, it would not matter, as
> long as we skip restoring the shadow stack pointer during unwinding
> (which we do not need to do because we never return along the regular
> execution path recorded on the shadow stack).

That is true.

> In short, the only thing need to ensure is that the over-write from
> __sigsetjmp stays within __cancel_buf.  Then we are good, without
> changing the stack layout for cancellation.

That is correct.

> My proposal is still rather hackish, but so is the existing code (the

A pointer to a buffer in user program is passed to libpthread.  There is a
jmp buf in the buffer followed by other fields.  Since the size of jmp buf is
increased in glibc 2.28, we need to know the offset of other fields. Otherwise
libpthread may write beyond the buffer in user program.  I don't see how
symbol versioning can help us here since the INTERNAL libpthread functions
don't know the layout of __pthread_unwind_buf_t of USER programs.

> truncated jump buffer), and HJ's approach of storing the shadow stack
> pointer in the signal save area of the non-truncated jump buffer.  But
> I think we can make it work.



-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

>> My proposal is still rather hackish, but so is the existing code (the
>
> A pointer to a buffer in user program is passed to libpthread.
> There is a jmp buf in the buffer followed by other fields.  Since
> the size of jmp buf is increased in glibc 2.28, we need to know the
> offset of other fields. Otherwise libpthread may write beyond the
> buffer in user program.  I don't see how symbol versioning can help
> us here since the INTERNAL libpthread functions don't know the
> layout of __pthread_unwind_buf_t of USER programs.

I suggest *not* to increase the size of the jump buffer.

CET markup will not be correct for static libraries compiled against
2.27 or earlier with a CET-enabled toolchain, so this is the only
completely safe approach.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Fri, Feb 9, 2018 at 4:11 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>>> My proposal is still rather hackish, but so is the existing code (the
>>
>> A pointer to a buffer in user program is passed to libpthread.
>> There is a jmp buf in the buffer followed by other fields.  Since
>> the size of jmp buf is increased in glibc 2.28, we need to know the
>> offset of other fields. Otherwise libpthread may write beyond the
>> buffer in user program.  I don't see how symbol versioning can help
>> us here since the INTERNAL libpthread functions don't know the
>> layout of __pthread_unwind_buf_t of USER programs.
>
> I suggest *not* to increase the size of the jump buffer.

Where do we save shadow stack pointer?

> CET markup will not be correct for static libraries compiled against
> 2.27 or earlier with a CET-enabled toolchain, so this is the only
> completely safe approach.

I don't believe so.  As long as there is a single input .o file which
isn't marked
with CET, the output static binary won't be marked with CET.  If what
you said is true, please file a glibc bug report.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Fri, Feb 9, 2018 at 4:34 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Feb 9, 2018 at 4:11 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>>> My proposal is still rather hackish, but so is the existing code (the
>>>
>>> A pointer to a buffer in user program is passed to libpthread.
>>> There is a jmp buf in the buffer followed by other fields.  Since
>>> the size of jmp buf is increased in glibc 2.28, we need to know the
>>> offset of other fields. Otherwise libpthread may write beyond the
>>> buffer in user program.  I don't see how symbol versioning can help
>>> us here since the INTERNAL libpthread functions don't know the
>>> layout of __pthread_unwind_buf_t of USER programs.
>>
>> I suggest *not* to increase the size of the jump buffer.
>
> Where do we save shadow stack pointer?
>
>> CET markup will not be correct for static libraries compiled against
>> 2.27 or earlier with a CET-enabled toolchain, so this is the only
>> completely safe approach.
>
> I don't believe so.  As long as there is a single input .o file which
> isn't marked
> with CET, the output static binary won't be marked with CET.  If what
> you said is true, please file a glibc bug report.
>
>

I built glibc master with gcc-8.0.1 -mcet -fcf-protection.  Some object
files do get CET marker as expected.  But static executable isn't:

[hjl@gnu-skx-1 build-x86_64-linux]$ readelf -n elf/sln.o

Displaying notes found in: .note.gnu.property
  Owner                 Data size Description
  GNU                  0x00000010 NT_GNU_PROPERTY_TYPE_0
      Properties: x86 feature: IBT, SHSTK
[hjl@gnu-skx-1 build-x86_64-linux]$ readelf -n elf/sln

Displaying notes found in: .note.ABI-tag
  Owner                 Data size Description
  GNU                  0x00000010 NT_GNU_ABI_TAG (ABI version tag)
    OS: Linux, ABI: 3.2.0

Displaying notes found in: .note.gnu.build-id
  Owner                 Data size Description
  GNU                  0x00000014 NT_GNU_BUILD_ID (unique build ID bitstring)
    Build ID: ab9587f0ef16086b740923f72bc1a024b6185b06
[hjl@gnu-skx-1 build-x86_64-linux]$

My CET ecosystem design prevents what you worried from happening.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

On 02/09/2018 03:13 PM, H.J. Lu wrote:
> I built glibc master with gcc-8.0.1 -mcet -fcf-protection.  Some object
> files do get CET marker as expected.  But static executable isn't:

I said static libraries.  If I compile this code (based on the example 
from the manual page) on Fedora rawhide:

#include <pthread.h>
#include <stdio.h>

static int done = 0;
static int cleanup_pop_arg = 0;
static int cnt = 0;

static void
cleanup_handler(void *arg)
{
   printf("Called clean-up handler\n");
   cnt = 0;
}

void *
thread_start(void *arg)
{
   time_t start, curr;

   printf("New thread started\n");

   pthread_cleanup_push(cleanup_handler, NULL);

   curr = start = time(NULL);

   while (!done) {
     pthread_testcancel();           /* A cancellation point */
     if (curr < time(NULL)) {
       curr = time(NULL);
       printf("cnt = %d\n", cnt);  /* A cancellation point */
       cnt++;
     }
   }

   pthread_cleanup_pop(cleanup_pop_arg);
   return NULL;
}

the small jump buffer is used:

0000000000000030 <thread_start>:
   30:   f3 0f 1e fa             endbr64
   34:   53                      push   %rbx
   35:   bf 00 00 00 00          mov    $0x0,%edi
                         36: R_X86_64_32 .rodata.str1.1+0x18
   3a:   48 83 ec 70             sub    $0x70,%rsp
   3e:   e8 00 00 00 00          callq  43 <thread_start+0x13>
                         3f: R_X86_64_PC32       puts-0x4
   43:   31 f6                   xor    %esi,%esi
   45:   48 89 e7                mov    %rsp,%rdi
   48:   e8 00 00 00 00          callq  4d <thread_start+0x1d>
                         49: R_X86_64_PC32       __sigsetjmp-0x4
   4d:   f3 0f 1e fa             endbr64
   51:   85 c0                   test   %eax,%eax
   53:   75 51                   jne    a6 <thread_start+0x76>
   55:   48 89 e7                mov    %rsp,%rdi
   58:   e8 00 00 00 00          callq  5d <thread_start+0x2d>
                         59: R_X86_64_PC32  __pthread_register_cancel-0x4

And it looks to me that readelf says the object file is compatible with CET:

Displaying notes found in: .note.gnu.property
   Owner                 Data size       Description
   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
       Properties: x86 feature: IBT, SHSTK

Thanks,
Florian

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Fri, Feb 9, 2018 at 6:33 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 02/09/2018 03:13 PM, H.J. Lu wrote:
>>
>> I built glibc master with gcc-8.0.1 -mcet -fcf-protection.  Some object
>> files do get CET marker as expected.  But static executable isn't:
>
>
> I said static libraries.  If I compile this code (based on the example from
> the manual page) on Fedora rawhide:
>

Glibc never provides binary compatibility with static libraries.  My suggestions
are

1. Recompile static libraries after CET is enabled in glibc.  Or
2. Don't compile static libraries with CET.

BTW, we don't have space to save shadow stack register with existing
cancel buf.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/09/2018 04:34 AM, H.J. Lu wrote:
> On Fri, Feb 9, 2018 at 4:11 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>>> My proposal is still rather hackish, but so is the existing code (the
>>>
>>> A pointer to a buffer in user program is passed to libpthread.
>>> There is a jmp buf in the buffer followed by other fields.  Since
>>> the size of jmp buf is increased in glibc 2.28, we need to know the
>>> offset of other fields. Otherwise libpthread may write beyond the
>>> buffer in user program.  I don't see how symbol versioning can help
>>> us here since the INTERNAL libpthread functions don't know the
>>> layout of __pthread_unwind_buf_t of USER programs.
>>
>> I suggest *not* to increase the size of the jump buffer.
> 
> Where do we save shadow stack pointer?

typedef struct
{
  struct
  {
    __jmp_buf __cancel_jmp_buf;
    int __mask_was_saved;
  } __cancel_jmp_buf[1];


  void *__pad[4];
  ^^^^^^^^^^^^^^^ Save the shadow stack pointer here.


} __pthread_unwind_buf_t __attribute__ ((__aligned__));

Save the shadow stack pointer to __pad[4] by making the
internal sigset_t smaller and moving it down.

The key aspect of Florian's recommendation is a realization
that a pthread_cleanup_pop can only restore you to the *same*
function e.g. the earlier pthread_cleanup_push, and therefore
does not need to change the shadow stack pointer.

All subsequent unwinding will proceed from one jump buffer
to the next, and through the unwinder, until it reaches
pthread_create. When will we ever return via a normal return
instruction and need to verify via the shadow stack?

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/09/2018 02:48 AM, Florian Weimer wrote:
> In short, the only thing need to ensure is that the over-write from
> __sigsetjmp stays within __cancel_buf.  Then we are good, without
> changing the stack layout for cancellation.
> 
> My proposal is still rather hackish, but so is the existing code (the
> truncated jump buffer), and HJ's approach of storing the shadow stack
> pointer in the signal save area of the non-truncated jump buffer.  But
> I think we can make it work.
 
I disagree completely. I think this is an an elegant solution to an ugly
problem. We need to see if HJ has any other requirements which may conflict
with this suggestion.

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Fri, Feb 23, 2018 at 9:46 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/09/2018 04:34 AM, H.J. Lu wrote:
>> On Fri, Feb 9, 2018 at 4:11 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>>> My proposal is still rather hackish, but so is the existing code (the
>>>>
>>>> A pointer to a buffer in user program is passed to libpthread.
>>>> There is a jmp buf in the buffer followed by other fields.  Since
>>>> the size of jmp buf is increased in glibc 2.28, we need to know the
>>>> offset of other fields. Otherwise libpthread may write beyond the
>>>> buffer in user program.  I don't see how symbol versioning can help
>>>> us here since the INTERNAL libpthread functions don't know the
>>>> layout of __pthread_unwind_buf_t of USER programs.
>>>
>>> I suggest *not* to increase the size of the jump buffer.
>>
>> Where do we save shadow stack pointer?
>
> typedef struct
> {
>   struct
>   {
>     __jmp_buf __cancel_jmp_buf;
>     int __mask_was_saved;
>   } __cancel_jmp_buf[1];
>
>
>   void *__pad[4];
>   ^^^^^^^^^^^^^^^ Save the shadow stack pointer here.
>
>
> } __pthread_unwind_buf_t __attribute__ ((__aligned__));
>
> Save the shadow stack pointer to __pad[4] by making the
> internal sigset_t smaller and moving it down.
>
> The key aspect of Florian's recommendation is a realization
> that a pthread_cleanup_pop can only restore you to the *same*
> function e.g. the earlier pthread_cleanup_push, and therefore
> does not need to change the shadow stack pointer.

PLEASE take a closer look:

Yes, there are

void *__pad[4];

But the name is misleading.   It isn't real padding.  This is
an opaque array:

/* Private data in the cleanup buffer.  */
union pthread_unwind_buf_data
{
  /* This is the placeholder of the public version.  */
  void *pad[4];

  struct
  {
    /* Pointer to the previous cleanup buffer.  */
    struct pthread_unwind_buf *prev;

    /* Backward compatibility: state of the old-style cleanup
       handler at the time of the previous new-style cleanup handler
       installment.  */
    struct _pthread_cleanup_buffer *cleanup;

    /* Cancellation type before the push call.  */
    int canceltype;
  } data;
};

Only the last element in __pad[4] is unused.  There is

---
Note: There is an unused pointer space in pthread_unwind_buf_data.  But
it isn't suitable for saving and restoring shadow stack register since
x32 is a 64-bit process with 32-bit software pointer and kernel may
place x32 shadow stack above 4GB.  We need to save and restore 64-bit
shadow stack register for x32.
---

in my commit log to explain why it isn't suitable for shadow stack.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> PLEASE take a closer look:
>
> Yes, there are
>
> void *__pad[4];
>
> But the name is misleading.   It isn't real padding.  This is
> an opaque array:
>
> /* Private data in the cleanup buffer.  */
> union pthread_unwind_buf_data
> {
>   /* This is the placeholder of the public version.  */
>   void *pad[4];
>
>   struct
>   {
>     /* Pointer to the previous cleanup buffer.  */
>     struct pthread_unwind_buf *prev;
>
>     /* Backward compatibility: state of the old-style cleanup
>        handler at the time of the previous new-style cleanup handler
>        installment.  */
>     struct _pthread_cleanup_buffer *cleanup;
>
>     /* Cancellation type before the push call.  */
>     int canceltype;
>   } data;
> };
>
> Only the last element in __pad[4] is unused.  There is

The entire __pad array is unused until the handler is registered,
which happens *after* the call to __sigsetjmp, in the
__pthread_register_cancel function.  This means that __sigsetjmp may
clobber it.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sat, Feb 24, 2018 at 7:46 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> PLEASE take a closer look:
>>
>> Yes, there are
>>
>> void *__pad[4];
>>
>> But the name is misleading.   It isn't real padding.  This is
>> an opaque array:
>>
>> /* Private data in the cleanup buffer.  */
>> union pthread_unwind_buf_data
>> {
>>   /* This is the placeholder of the public version.  */
>>   void *pad[4];
>>
>>   struct
>>   {
>>     /* Pointer to the previous cleanup buffer.  */
>>     struct pthread_unwind_buf *prev;
>>
>>     /* Backward compatibility: state of the old-style cleanup
>>        handler at the time of the previous new-style cleanup handler
>>        installment.  */
>>     struct _pthread_cleanup_buffer *cleanup;
>>
>>     /* Cancellation type before the push call.  */
>>     int canceltype;
>>   } data;
>> };
>>
>> Only the last element in __pad[4] is unused.  There is
>
> The entire __pad array is unused until the handler is registered,
> which happens *after* the call to __sigsetjmp, in the
> __pthread_register_cancel function.  This means that __sigsetjmp may
> clobber it.

Please check out hjl/setjmp/pad branch and check it on x86-64.

1. It uses pad array in struct pthread_unwind_buf to save and restore shadow
stack register if size of struct pthread_unwind_buf is no less than
offset of shadow stack pointer + shadow stack pointer size.

2. It stores (int64_t) -1 as shadow stack register in x86-64 setjmp and read
it back in x86-64 longjmp to verify that it is unchanged.

I got

FAIL: nptl/tst-basic3
FAIL: nptl/tst-cancel-self
FAIL: nptl/tst-cancel-self-cancelstate
FAIL: nptl/tst-cancel-self-canceltype
FAIL: nptl/tst-cancel-self-testcancel
FAIL: nptl/tst-cancel1
FAIL: nptl/tst-cancel10
FAIL: nptl/tst-cancel11
FAIL: nptl/tst-cancel12
FAIL: nptl/tst-cancel13
FAIL: nptl/tst-cancel14
FAIL: nptl/tst-cancel15
FAIL: nptl/tst-cancel16
FAIL: nptl/tst-cancel17
FAIL: nptl/tst-cancel18
FAIL: nptl/tst-cancel20
FAIL: nptl/tst-cancel21
FAIL: nptl/tst-cancel21-static
FAIL: nptl/tst-cancel24
FAIL: nptl/tst-cancel24-static
FAIL: nptl/tst-cancel25
FAIL: nptl/tst-cancel4
FAIL: nptl/tst-cancel4_1
FAIL: nptl/tst-cancel4_2
FAIL: nptl/tst-cancel5
FAIL: nptl/tst-cancel7
FAIL: nptl/tst-cancel9
FAIL: nptl/tst-cancelx13
FAIL: nptl/tst-cancelx15
FAIL: nptl/tst-cancelx21
FAIL: nptl/tst-cancelx7
FAIL: nptl/tst-cleanup0
FAIL: nptl/tst-cleanup0-cmp
FAIL: nptl/tst-cleanup1
FAIL: nptl/tst-cleanup3
FAIL: nptl/tst-cleanup4
FAIL: nptl/tst-cleanupx0
FAIL: nptl/tst-cleanupx4
FAIL: nptl/tst-cond-except
FAIL: nptl/tst-cond22
FAIL: nptl/tst-cond25
FAIL: nptl/tst-cond7
FAIL: nptl/tst-cond8
FAIL: nptl/tst-cond8-static
FAIL: nptl/tst-execstack
FAIL: nptl/tst-exit2
FAIL: nptl/tst-exit3
FAIL: nptl/tst-join1
FAIL: nptl/tst-join5
FAIL: nptl/tst-mutex8
FAIL: nptl/tst-mutex8-static
FAIL: nptl/tst-mutexpi8
FAIL: nptl/tst-mutexpi8-static
FAIL: nptl/tst-once3
FAIL: nptl/tst-once4
FAIL: nptl/tst-oncex3
FAIL: nptl/tst-oncex4
FAIL: nptl/tst-sem11
FAIL: nptl/tst-sem11-static
FAIL: nptl/tst-sem12
FAIL: nptl/tst-sem12-static
FAIL: nptl/tst-tsd5
FAIL: nss/tst-cancel-getpwuid_r
FAIL: rt/tst-mqueue8
FAIL: nptl/tst-setuid2

For nptl/tst-tsd5, it went like this:

1. __libc_start_main calls __sigsetjmp:

 /* Memory for the cancellation buffer.  */
  struct pthread_unwind_buf unwind_buf;

  int not_first_call;
  not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
  if (__glibc_likely (! not_first_call))

__sigsetjmp stores -1 as shadow stack pointer.

2.  After calling  __sigsetjmp, __libc_start_main does

      /* Store old info.  */
      unwind_buf.priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
      unwind_buf.priv.data.cleanup = THREAD_GETMEM (self, cleanup);

which overrides shadow stack pointer.

What have I done wrong?


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> What have I done wrong?

My understanding (shared by Carlos as far as I know) is that you do
not need to restore the shadow stack pointer when unwinding for
cancellation or thread exit.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 1:26 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> What have I done wrong?
>
> My understanding (shared by Carlos as far as I know) is that you do
> not need to restore the shadow stack pointer when unwinding for
> cancellation or thread exit.

It may be true for thread exit.  Are you saying that a function will never
return after cancellation point?

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> On Sun, Feb 25, 2018 at 1:26 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> What have I done wrong?
>>
>> My understanding (shared by Carlos as far as I know) is that you do
>> not need to restore the shadow stack pointer when unwinding for
>> cancellation or thread exit.
>
> It may be true for thread exit.  Are you saying that a function will never
> return after cancellation point?

That's certainly the intent.  Cancellation is supposed to be
unstoppable.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 3:59 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> On Sun, Feb 25, 2018 at 1:26 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>> What have I done wrong?
>>>
>>> My understanding (shared by Carlos as far as I know) is that you do
>>> not need to restore the shadow stack pointer when unwinding for
>>> cancellation or thread exit.
>>
>> It may be true for thread exit.  Are you saying that a function will never
>> return after cancellation point?
>
> That's certainly the intent.  Cancellation is supposed to be
> unstoppable.

The question is what happens after cancellation.  Can a function return
after cancellation?

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 4:53 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Feb 25, 2018 at 3:59 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> On Sun, Feb 25, 2018 at 1:26 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>> * H. J. Lu:
>>>>
>>>>> What have I done wrong?
>>>>
>>>> My understanding (shared by Carlos as far as I know) is that you do
>>>> not need to restore the shadow stack pointer when unwinding for
>>>> cancellation or thread exit.
>>>
>>> It may be true for thread exit.  Are you saying that a function will never
>>> return after cancellation point?
>>
>> That's certainly the intent.  Cancellation is supposed to be
>> unstoppable.
>
> The question is what happens after cancellation.  Can a function return
> after cancellation?
>

Let me rephrase.  Can a function, which contains cancellation points,
return to its caller after cancellation happened?

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> Let me rephrase.  Can a function, which contains cancellation points,
> return to its caller after cancellation happened?

I don't think it can.  It's undefined to use longjmp to jump out of a
cancellation handler.  On the C++ side, we make sure that the
unwinding always proceeds, even across catch (...) blocks which
swallow the exception.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 4:58 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> Let me rephrase.  Can a function, which contains cancellation points,
>> return to its caller after cancellation happened?
>
> I don't think it can.  It's undefined to use longjmp to jump out of a
> cancellation handler.  On the C++ side, we make sure that the
> unwinding always proceeds, even across catch (...) blocks which
> swallow the exception.

libpthread cancellation implementation passes cancel_jmp_buf to
libgcc unwinder, which saves and restores shadow stack register
for C++ exception.  How can libgcc unwinder tell when to save and
restore shadow stack register?

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> libpthread cancellation implementation passes cancel_jmp_buf to
> libgcc unwinder,

Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
that's just an opaque closure argument for the callback.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> libpthread cancellation implementation passes cancel_jmp_buf to
>> libgcc unwinder,
>
> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
> that's just an opaque closure argument for the callback.

Yes.  Libgcc unwinder needs to deal with it.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 5:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>> libgcc unwinder,
>>
>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>> that's just an opaque closure argument for the callback.
>
> Yes.  Libgcc unwinder needs to deal with it.
>

Because of  libgcc unwinder,  provide another set of setjmp/longjmp
without saving and restoring shadow stack register for thread cancellation
won't solve our problem.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>> libgcc unwinder,
>>
>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>> that's just an opaque closure argument for the callback.
>
> Yes.  Libgcc unwinder needs to deal with it.

Please point me to the code.  Thanks.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>> libgcc unwinder,
>>>
>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>> that's just an opaque closure argument for the callback.
>>
>> Yes.  Libgcc unwinder needs to deal with it.
>
> Please point me to the code.  Thanks.

sysdeps/nptl/unwind-forcedunwind.c has

_Unwind_Reason_Code
_Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
      void *stop_argument)
{
  if (__glibc_unlikely (libgcc_s_handle == NULL))
    pthread_cancel_init ();
  else
    atomic_read_barrier ();

  _Unwind_Reason_Code (*forcedunwind)
    (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
    = libgcc_s_forcedunwind;
  PTR_DEMANGLE (forcedunwind);
  return forcedunwind (exc, stop, stop_argument);
}

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Florian Weimer]

* H. J. Lu:

> On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>> * H. J. Lu:
>>>>
>>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>>> libgcc unwinder,
>>>>
>>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>>> that's just an opaque closure argument for the callback.
>>>
>>> Yes.  Libgcc unwinder needs to deal with it.
>>
>> Please point me to the code.  Thanks.
>
> sysdeps/nptl/unwind-forcedunwind.c has
>
> _Unwind_Reason_Code
> _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>       void *stop_argument)
> {
>   if (__glibc_unlikely (libgcc_s_handle == NULL))
>     pthread_cancel_init ();
>   else
>     atomic_read_barrier ();
>
>   _Unwind_Reason_Code (*forcedunwind)
>     (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
>     = libgcc_s_forcedunwind;
>   PTR_DEMANGLE (forcedunwind);
>   return forcedunwind (exc, stop, stop_argument);
> }

Thanks.  I think stop_argument ends up in the private_2 member inside
unwind.inc, which is only passed back to the callback (the stop
function pointer) in _Unwind_ForcedUnwind_Phase2, and not interpreted
by libgcc itself.  So this shouldn't be a problem.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Sun, Feb 25, 2018 at 6:13 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * H. J. Lu:
>
>> On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>> * H. J. Lu:
>>>>>
>>>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>>>> libgcc unwinder,
>>>>>
>>>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>>>> that's just an opaque closure argument for the callback.
>>>>
>>>> Yes.  Libgcc unwinder needs to deal with it.
>>>
>>> Please point me to the code.  Thanks.
>>
>> sysdeps/nptl/unwind-forcedunwind.c has
>>
>> _Unwind_Reason_Code
>> _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>>       void *stop_argument)
>> {
>>   if (__glibc_unlikely (libgcc_s_handle == NULL))
>>     pthread_cancel_init ();
>>   else
>>     atomic_read_barrier ();
>>
>>   _Unwind_Reason_Code (*forcedunwind)
>>     (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
>>     = libgcc_s_forcedunwind;
>>   PTR_DEMANGLE (forcedunwind);
>>   return forcedunwind (exc, stop, stop_argument);
>> }
>
> Thanks.  I think stop_argument ends up in the private_2 member inside
> unwind.inc, which is only passed back to the callback (the stop
> function pointer) in _Unwind_ForcedUnwind_Phase2, and not interpreted
> by libgcc itself.  So this shouldn't be a problem.

Please take a look at hjl/setjmp/cancel branch:

https://github.com/hjl-tools/glibc/tree/hjl/setjmp/cancel

Functions, like LIBC_START_MAIN, START_THREAD_DEFN as well as these
with thread cancellation, call setjmp, but never return to their
callers after longjmp returns.  This patch adds <bits/setjmp-cancel.h>
and <setjmp-cancelP.h> to provide a version of setjmp family functions,
__setjmp_cancel and __sigsetjmp_cancel, which are used to implement
thread cancellation.  The default __setjmp_cancel and __sigsetjmp_cancel
are defined as setjmp and __sigsetjmp, respectively, which are the same
as before.

On x86, a different version is added to avoid saving and restoring
shadow stack register.  __libc_longjmp, which is a private interface
for cancellation implementation in libpthread, is changed to call
__longjmp_cancel instead of __longjmp.

This leaves cancel_jmp_buf unchanged.  But is it worth it?

1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
save and restore shadow stack register.
2. I still need yet to add the new version of __sigsetjmp for older binaries.
3, Older .o files compiled against glibc 2.27 are still incompatible with
glibc 2.28.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 02/25/2018 07:55 PM, H.J. Lu wrote:
> On Sun, Feb 25, 2018 at 6:13 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * H. J. Lu:
>>
>>> On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>> * H. J. Lu:
>>>>
>>>>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>>> * H. J. Lu:
>>>>>>
>>>>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>>>>> libgcc unwinder,
>>>>>>
>>>>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>>>>> that's just an opaque closure argument for the callback.
>>>>>
>>>>> Yes.  Libgcc unwinder needs to deal with it.
>>>>
>>>> Please point me to the code.  Thanks.
>>>
>>> sysdeps/nptl/unwind-forcedunwind.c has
>>>
>>> _Unwind_Reason_Code
>>> _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>>>       void *stop_argument)
>>> {
>>>   if (__glibc_unlikely (libgcc_s_handle == NULL))
>>>     pthread_cancel_init ();
>>>   else
>>>     atomic_read_barrier ();
>>>
>>>   _Unwind_Reason_Code (*forcedunwind)
>>>     (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
>>>     = libgcc_s_forcedunwind;
>>>   PTR_DEMANGLE (forcedunwind);
>>>   return forcedunwind (exc, stop, stop_argument);
>>> }
>>
>> Thanks.  I think stop_argument ends up in the private_2 member inside
>> unwind.inc, which is only passed back to the callback (the stop
>> function pointer) in _Unwind_ForcedUnwind_Phase2, and not interpreted
>> by libgcc itself.  So this shouldn't be a problem.
> 
> Please take a look at hjl/setjmp/cancel branch:
> 
> https://github.com/hjl-tools/glibc/tree/hjl/setjmp/cancel

OK.

> Functions, like LIBC_START_MAIN, START_THREAD_DEFN as well as these
> with thread cancellation, call setjmp, but never return to their
> callers after longjmp returns.  This patch adds <bits/setjmp-cancel.h>
> and <setjmp-cancelP.h> to provide a version of setjmp family functions,
> __setjmp_cancel and __sigsetjmp_cancel, which are used to implement
> thread cancellation.  The default __setjmp_cancel and __sigsetjmp_cancel
> are defined as setjmp and __sigsetjmp, respectively, which are the same
> as before.

> On x86, a different version is added to avoid saving and restoring
> shadow stack register.  __libc_longjmp, which is a private interface
> for cancellation implementation in libpthread, is changed to call
> __longjmp_cancel instead of __longjmp.

The consequence of this solution is that any function calls after the
first longjmp will continue to extend the shadow stack because there is
no restoration? I think this is OK, you effectively need to have enough
shadow stack to run through all of the unwind functions as if they were
nested frames on the shadow stack.

Do we see that being a problem?
 
> This leaves cancel_jmp_buf unchanged.  But is it worth it?

Absolutely it is worth it. This new design looks much simpler to support.

The technical trade is as follows:

* Three new versioned symbols for __sigsetjmp, __sigsetjmp_cancel,
  and __setjmp_cancel.

  * You can avoid versioning by placing the shadow stack pointer such
    that it is written into the pad[4] of the cancel_jmp_buf, and thus
    it would always have space to be written. However, it is cleaner
    to have cancel versions of these functions that do less.

vs.

* An ABI change that requires coordination across glibc, gcc, and
  binutils to ensure there is a "flag day" where the ABI can be correctly
  selected at runtime.

The latter is not easy to execute for downstream, and the versioned
symbols provide a good barrier in terms of compatibility and errors
reported by the dynamic loader.
 
> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
> save and restore shadow stack register.

OK.

> 2. I still need yet to add the new version of __sigsetjmp for older binaries.

OK.

> 3, Older .o files compiled against glibc 2.27 are still incompatible with
> glibc 2.28.
 
This is normal, and has never been assured.

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Wed, Feb 28, 2018 at 3:23 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/25/2018 07:55 PM, H.J. Lu wrote:
>> On Sun, Feb 25, 2018 at 6:13 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>> On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>> * H. J. Lu:
>>>>>
>>>>>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>>>> * H. J. Lu:
>>>>>>>
>>>>>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>>>>>> libgcc unwinder,
>>>>>>>
>>>>>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>>>>>> that's just an opaque closure argument for the callback.
>>>>>>
>>>>>> Yes.  Libgcc unwinder needs to deal with it.
>>>>>
>>>>> Please point me to the code.  Thanks.
>>>>
>>>> sysdeps/nptl/unwind-forcedunwind.c has
>>>>
>>>> _Unwind_Reason_Code
>>>> _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>>>>       void *stop_argument)
>>>> {
>>>>   if (__glibc_unlikely (libgcc_s_handle == NULL))
>>>>     pthread_cancel_init ();
>>>>   else
>>>>     atomic_read_barrier ();
>>>>
>>>>   _Unwind_Reason_Code (*forcedunwind)
>>>>     (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
>>>>     = libgcc_s_forcedunwind;
>>>>   PTR_DEMANGLE (forcedunwind);
>>>>   return forcedunwind (exc, stop, stop_argument);
>>>> }
>>>
>>> Thanks.  I think stop_argument ends up in the private_2 member inside
>>> unwind.inc, which is only passed back to the callback (the stop
>>> function pointer) in _Unwind_ForcedUnwind_Phase2, and not interpreted
>>> by libgcc itself.  So this shouldn't be a problem.
>>
>> Please take a look at hjl/setjmp/cancel branch:
>>
>> https://github.com/hjl-tools/glibc/tree/hjl/setjmp/cancel
>
> OK.
>
>> Functions, like LIBC_START_MAIN, START_THREAD_DEFN as well as these
>> with thread cancellation, call setjmp, but never return to their
>> callers after longjmp returns.  This patch adds <bits/setjmp-cancel.h>
>> and <setjmp-cancelP.h> to provide a version of setjmp family functions,
>> __setjmp_cancel and __sigsetjmp_cancel, which are used to implement
>> thread cancellation.  The default __setjmp_cancel and __sigsetjmp_cancel
>> are defined as setjmp and __sigsetjmp, respectively, which are the same
>> as before.
>
>> On x86, a different version is added to avoid saving and restoring
>> shadow stack register.  __libc_longjmp, which is a private interface
>> for cancellation implementation in libpthread, is changed to call
>> __longjmp_cancel instead of __longjmp.
>
> The consequence of this solution is that any function calls after the
> first longjmp will continue to extend the shadow stack because there is
> no restoration? I think this is OK, you effectively need to have enough
> shadow stack to run through all of the unwind functions as if they were
> nested frames on the shadow stack.
>
> Do we see that being a problem?
>
>> This leaves cancel_jmp_buf unchanged.  But is it worth it?
>
> Absolutely it is worth it. This new design looks much simpler to support.
>
> The technical trade is as follows:
>
> * Three new versioned symbols for __sigsetjmp, __sigsetjmp_cancel,
>   and __setjmp_cancel.
>
>   * You can avoid versioning by placing the shadow stack pointer such
>     that it is written into the pad[4] of the cancel_jmp_buf, and thus
>     it would always have space to be written. However, it is cleaner
>     to have cancel versions of these functions that do less.
>
> vs.
>
> * An ABI change that requires coordination across glibc, gcc, and
>   binutils to ensure there is a "flag day" where the ABI can be correctly
>   selected at runtime.
>
> The latter is not easy to execute for downstream, and the versioned
> symbols provide a good barrier in terms of compatibility and errors
> reported by the dynamic loader.
>
>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>> save and restore shadow stack register.

I have been testing this.  I ran into one issue.  GCC knows that setjmp will
return via longjmp and inserts ENDBR after it.  But it doesn't know
  __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
or add NOTRACK prefix to the corresponding longjmps.

> OK.
>
>> 2. I still need yet to add the new version of __sigsetjmp for older binaries.
>
> OK.
>
>> 3, Older .o files compiled against glibc 2.27 are still incompatible with
>> glibc 2.28.
>
> This is normal, and has never been assured.
>
> --
> Cheers,
> Carlos.



-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 03/07/2018 03:56 AM, H.J. Lu wrote:
>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>>> save and restore shadow stack register.
> 
> I have been testing this.  I ran into one issue.  GCC knows that setjmp will
> return via longjmp and inserts ENDBR after it.  But it doesn't know
>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
> or add NOTRACK prefix to the corresponding longjmps.

I would rather GCC did not know about these implementation details.

I have no objection to the NOTRACK prefix in the corresponding longjmps.

What would be a downside to this choice?

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>>>> save and restore shadow stack register.
>>
>> I have been testing this.  I ran into one issue.  GCC knows that setjmp will
>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
>> or add NOTRACK prefix to the corresponding longjmps.
>
> I would rather GCC did not know about these implementation details.
>
> I have no objection to the NOTRACK prefix in the corresponding longjmps.
>
> What would be a downside to this choice?
>

NOTRACK prefix is typically generated by compiler for switch table.  Compiler
knows each indirect jump target is valid and pointer load for indirect jump is
generated by compiler in read-only section.  This is pretty safe since there is
very little chance for malicious codes to temper the pointer value.  However,
in case of longjmp, the indirect jump target is in jmpbuf.   There is
a possilibty
for malicious codes to change the indirect jump target such that longjmp wil
jump to the wrong place.  Use NOTRACK prefix here defeats the purpose of
indirect branch tracking in CET.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>>>>> save and restore shadow stack register.
>>>
>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp will
>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
>>> or add NOTRACK prefix to the corresponding longjmps.
>>
>> I would rather GCC did not know about these implementation details.
>>
>> I have no objection to the NOTRACK prefix in the corresponding longjmps.
>>
>> What would be a downside to this choice?
>>
>
> NOTRACK prefix is typically generated by compiler for switch table.  Compiler
> knows each indirect jump target is valid and pointer load for indirect jump is
> generated by compiler in read-only section.  This is pretty safe since there is
> very little chance for malicious codes to temper the pointer value.  However,
> in case of longjmp, the indirect jump target is in jmpbuf.   There is
> a possilibty
> for malicious codes to change the indirect jump target such that longjmp wil
> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose of
> indirect branch tracking in CET.
>

Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
return twice, similar to setjmp.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>>>>>> save and restore shadow stack register.
>>>>
>>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp will
>>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
>>>> or add NOTRACK prefix to the corresponding longjmps.
>>>
>>> I would rather GCC did not know about these implementation details.
>>>
>>> I have no objection to the NOTRACK prefix in the corresponding longjmps.
>>>
>>> What would be a downside to this choice?
>>>
>>
>> NOTRACK prefix is typically generated by compiler for switch table.  Compiler
>> knows each indirect jump target is valid and pointer load for indirect jump is
>> generated by compiler in read-only section.  This is pretty safe since there is
>> very little chance for malicious codes to temper the pointer value.  However,
>> in case of longjmp, the indirect jump target is in jmpbuf.   There is
>> a possilibty
>> for malicious codes to change the indirect jump target such that longjmp wil
>> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose of
>> indirect branch tracking in CET.
>>
>
> Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
> return twice, similar to setjmp.
>

Here is the GCC patch:


diff --git a/gcc/calls.c b/gcc/calls.c
index 19c95b8455b..d1f436dfa91 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags)
     name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl);

   if (fndecl && name_decl
-      && IDENTIFIER_LENGTH (name_decl) <= 11
+      && IDENTIFIER_LENGTH (name_decl) <= 18
       /* Exclude functions not at the file scope, or not `extern',
    since they are not the magic functions we would otherwise
    think they are.
@@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags)
   }

       /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */
-      if (! strcmp (tname, "setjmp")
-    || ! strcmp (tname, "sigsetjmp")
+      if (! strncmp (tname, "setjmp", 6)
+    || ! strncmp (tname, "sigsetjmp", 9)
     || ! strcmp (name, "savectx")
     || ! strcmp (name, "vfork")
     || ! strcmp (name, "getcontext"))


-- 
H.J.

RE: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Tsimbalist, Igor V]

> -----Original Message-----
> From: H.J. Lu [mailto:hjl.tools@gmail.com]
> Sent: Wednesday, March 7, 2018 11:07 PM
> To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com>
> Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc-
> alpha@sourceware.org>
> Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf
> 
> On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com>
> wrote:
> >>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
> >>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which
> won't
> >>>>>> save and restore shadow stack register.
> >>>>
> >>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp
> will
> >>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
> >>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to
> GCC
> >>>> or add NOTRACK prefix to the corresponding longjmps.
> >>>
> >>> I would rather GCC did not know about these implementation details.
> >>>
> >>> I have no objection to the NOTRACK prefix in the corresponding
> longjmps.
> >>>
> >>> What would be a downside to this choice?
> >>>
> >>
> >> NOTRACK prefix is typically generated by compiler for switch table.
> Compiler
> >> knows each indirect jump target is valid and pointer load for indirect
> jump is
> >> generated by compiler in read-only section.  This is pretty safe since there
> is
> >> very little chance for malicious codes to temper the pointer value.
> However,
> >> in case of longjmp, the indirect jump target is in jmpbuf.   There is
> >> a possilibty
> >> for malicious codes to change the indirect jump target such that longjmp
> wil
> >> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose
> of
> >> indirect branch tracking in CET.
> >>
> >
> > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
> > return twice, similar to setjmp.
> >
> 
> Here is the GCC patch:
> 
> 
> diff --git a/gcc/calls.c b/gcc/calls.c
> index 19c95b8455b..d1f436dfa91 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags)
>      name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl);
> 
>    if (fndecl && name_decl
> -      && IDENTIFIER_LENGTH (name_decl) <= 11
> +      && IDENTIFIER_LENGTH (name_decl) <= 18
>        /* Exclude functions not at the file scope, or not `extern',
>     since they are not the magic functions we would otherwise
>     think they are.
> @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags)
>    }
> 
>        /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */
> -      if (! strcmp (tname, "setjmp")
> -    || ! strcmp (tname, "sigsetjmp")
> +      if (! strncmp (tname, "setjmp", 6)
> +    || ! strncmp (tname, "sigsetjmp", 9)
>      || ! strcmp (name, "savectx")
>      || ! strcmp (name, "vfork")
>      || ! strcmp (name, "getcontext"))

Should it be compared with the whole function name (__setjmp_cancel and
__sigsetjmp_cancel) as something like setjmp_my_func will be also detected?

Also there was an error in detection of __getcontext, which is 12 bytes, but it
will be fixed by this patch.

Igor

> --
> H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Thu, Mar 8, 2018 at 4:24 AM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
>> -----Original Message-----
>> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> Sent: Wednesday, March 7, 2018 11:07 PM
>> To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V
>> <igor.v.tsimbalist@intel.com>
>> Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc-
>> alpha@sourceware.org>
>> Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf
>>
>> On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com>
>> wrote:
>> >>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>> >>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which
>> won't
>> >>>>>> save and restore shadow stack register.
>> >>>>
>> >>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp
>> will
>> >>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>> >>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to
>> GCC
>> >>>> or add NOTRACK prefix to the corresponding longjmps.
>> >>>
>> >>> I would rather GCC did not know about these implementation details.
>> >>>
>> >>> I have no objection to the NOTRACK prefix in the corresponding
>> longjmps.
>> >>>
>> >>> What would be a downside to this choice?
>> >>>
>> >>
>> >> NOTRACK prefix is typically generated by compiler for switch table.
>> Compiler
>> >> knows each indirect jump target is valid and pointer load for indirect
>> jump is
>> >> generated by compiler in read-only section.  This is pretty safe since there
>> is
>> >> very little chance for malicious codes to temper the pointer value.
>> However,
>> >> in case of longjmp, the indirect jump target is in jmpbuf.   There is
>> >> a possilibty
>> >> for malicious codes to change the indirect jump target such that longjmp
>> wil
>> >> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose
>> of
>> >> indirect branch tracking in CET.
>> >>
>> >
>> > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
>> > return twice, similar to setjmp.
>> >
>>
>> Here is the GCC patch:
>>
>>
>> diff --git a/gcc/calls.c b/gcc/calls.c
>> index 19c95b8455b..d1f436dfa91 100644
>> --- a/gcc/calls.c
>> +++ b/gcc/calls.c
>> @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags)
>>      name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl);
>>
>>    if (fndecl && name_decl
>> -      && IDENTIFIER_LENGTH (name_decl) <= 11
>> +      && IDENTIFIER_LENGTH (name_decl) <= 18
>>        /* Exclude functions not at the file scope, or not `extern',
>>     since they are not the magic functions we would otherwise
>>     think they are.
>> @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags)
>>    }
>>
>>        /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */
>> -      if (! strcmp (tname, "setjmp")
>> -    || ! strcmp (tname, "sigsetjmp")
>> +      if (! strncmp (tname, "setjmp", 6)
>> +    || ! strncmp (tname, "sigsetjmp", 9)
>>      || ! strcmp (name, "savectx")
>>      || ! strcmp (name, "vfork")
>>      || ! strcmp (name, "getcontext"))
>
> Should it be compared with the whole function name (__setjmp_cancel and
> __sigsetjmp_cancel) as something like setjmp_my_func will be also detected?

True.  This is the patch I have tested:

https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a

> Also there was an error in detection of __getcontext, which is 12 bytes, but it
> will be fixed by this patch.


-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 03/08/2018 04:48 AM, H.J. Lu wrote:
> True.  This is the patch I have tested:
> 
> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a

I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
documentation since binutils uses the 0x3E prefix for it and that matches
the Intel CET docs. Please correct me if I'm wrong.

In which case I agree, using NOTRACK is going to prevent a useful use
of CET against writes to the cancellation jump buffer.

This patch looks good to me, but is not a *correctness* issue, it is
simply that we want to extend coverage to the private cancellation
setjmp/longjmp buffers.

Is there any way we can do this with source markup instead of via
a fragile list in the compiler?

Presumably users may want to markup their own code like this also
if they have custom implementations of functions that behave like
setjmp/longjmp?

-- 
Cheers,
Carlos.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[H.J. Lu]

On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 03/08/2018 04:48 AM, H.J. Lu wrote:
>> True.  This is the patch I have tested:
>>
>> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a
>
> I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
> documentation since binutils uses the 0x3E prefix for it and that matches
> the Intel CET docs. Please correct me if I'm wrong.

That is correct.

> In which case I agree, using NOTRACK is going to prevent a useful use
> of CET against writes to the cancellation jump buffer.

True.

> This patch looks good to me, but is not a *correctness* issue, it is
> simply that we want to extend coverage to the private cancellation
> setjmp/longjmp buffers.
>
> Is there any way we can do this with source markup instead of via
> a fragile list in the compiler?
>
> Presumably users may want to markup their own code like this also
> if they have custom implementations of functions that behave like
> setjmp/longjmp?
>

Yes, we can use __attribute__((__returns_twice__)).  I updated
hjl/setjmp/cancel branch to do that.  No GCC changes are needed.

-- 
H.J.

Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

[Carlos O'Donell]

On 03/08/2018 10:23 PM, H.J. Lu wrote:
> On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 03/08/2018 04:48 AM, H.J. Lu wrote:
>>> True.  This is the patch I have tested:
>>>
>>> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a
>>
>> I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
>> documentation since binutils uses the 0x3E prefix for it and that matches
>> the Intel CET docs. Please correct me if I'm wrong.
> 
> That is correct.
> 
>> In which case I agree, using NOTRACK is going to prevent a useful use
>> of CET against writes to the cancellation jump buffer.
> 
> True.
> 
>> This patch looks good to me, but is not a *correctness* issue, it is
>> simply that we want to extend coverage to the private cancellation
>> setjmp/longjmp buffers.
>>
>> Is there any way we can do this with source markup instead of via
>> a fragile list in the compiler?
>>
>> Presumably users may want to markup their own code like this also
>> if they have custom implementations of functions that behave like
>> setjmp/longjmp?
>>
> 
> Yes, we can use __attribute__((__returns_twice__)).  I updated
> hjl/setjmp/cancel branch to do that.  No GCC changes are needed.

Is the next step for me to do another round of review on this branch?

Cheers,
Carlos.