From: "H.J. Lu" <hjl.tools@gmail.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: GNU C Library <libc-alpha@sourceware.org>,
"Senkevich, Andrew" <andrew.senkevich@intel.com>,
"Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com>
Subject: Re: [PATCH 2/2] jmpbuf: Add paddings for target specific usage
Date: Wed, 29 Nov 2017 21:16:00 -0000 [thread overview]
Message-ID: <CAMe9rOp+KxdpLJDYLHFbrh_kF-CEJ=jCmB6UcJs3fghd=REmBQ@mail.gmail.com> (raw)
In-Reply-To: <CAMe9rOr3bKzEE-0ab7Yue_rS0snMZ3_8b=BWfJzjkjvgAp39XQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4223 bytes --]
On Sat, Nov 25, 2017 at 8:34 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 21, 2017 at 2:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Nov 15, 2017 at 5:20 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>> On 11/14/2017 02:10 PM, H.J. Lu wrote:
>>>>>
>>>>> +static int
>>>>> +do_test (void)
>>>>> +{
>>>>> + sigjmp_buf sj;
>>>>> + struct support_next_to_fault sigset_t_buf
>>>>> + = support_next_to_fault_allocate (SIZEOF_SIGSET_T);
>>>>> + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer;
>>>>> + sigset_t m;
>>>>> +
>>>>> + sigemptyset (&m);
>>>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T);
>>>>> + sigprocmask (SIG_SETMASK, m_p, NULL);
>>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>>>> + if (sigsetjmp (sj, 0) == 0)
>>>>> + {
>>>>> + sigaddset (&m, SIGUSR1);
>>>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T);
>>>>> + sigprocmask (SIG_SETMASK, m_p, NULL);
>>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>>>> + siglongjmp (sj, 1);
>>>>> + return EXIT_FAILURE;
>>>>> + }
>>>>> + sigprocmask (SIG_SETMASK, NULL, m_p);
>>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>>>> + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
>>>>> +}
>>>>
>>>>
>>>> Sorry, I don't understand anymore what this test is supposed to test and
>>>> how.
>>>
>>> This tests the reduced __jmp_buf_sigset_t used by __saved_mask is
>>> bigger than sigset expected by kernel.
>>>
>>>> To be honest, I don't like how you inject the internal definition of
>>>> jmp_buf. Is this the way we do it for the nptl types?
>>>
>>> I only need to make some room in
>>>
>>> /* Calling environment, plus possibly a saved signal mask. */
>>> struct __jmp_buf_tag
>>> {
>>> /* NOTE: The machine-dependent definitions of `__sigsetjmp'
>>> assume that a `jmp_buf' begins with a `__jmp_buf' and that
>>> `__mask_was_saved' follows it. Do not move these members
>>> or add others before it. */
>>> __jmp_buf __jmpbuf; /* Calling environment. */
>>> int __mask_was_saved; /* Saved the signal mask? */
>>> __sigset_t __saved_mask; /* Saved signal mask. */
>>> };
>>>
>>> for target specific purpose. I changed it to
>>>
>>> struct __jmp_buf_tag
>>> {
>>> /* NOTE: The machine-dependent definitions of `__sigsetjmp'
>>> assume that a `jmp_buf' begins with a `__jmp_buf' and that
>>> `__mask_was_saved' follows it. Do not move these members
>>> or add others before it. */
>>> __jmp_buf __jmpbuf; /* Calling environment. */
>>> int __mask_was_saved; /* Saved the signal mask? */
>>> union
>>> {
>>> __sigset_t __saved_mask_compat;
>>> struct
>>> {
>>> __jmp_buf_sigset_t __saved_mask;
>>> /* Paddings for architecture specific usage. */
>>> unsigned long int __padding[12];
>>> } __saved;
>>> } __saved_mask;
>>> };
>>>
>>> #define __saved_mask __saved_mask.__saved.__saved_mask
>>>
>>> I did only to __sigset_t in __jmp_buf_tag and this test verifies that the size
>>> of __jmp_buf_sigset_t works with sigprocmask.
>>>
>>> Are you suggesting we make some room in __sigset_t directly? This will
>>> require very extensive changes. If not, what do you exactly suggest?
>>>
>>>> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant
>>>> (_NSIGS?) somewhere.
>>>
>>> There is a _Static_assert in include/setjmp.h. But it isn't triggered. This
>>> updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h. Now
>>> I got
>>>
>>> ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static
>>> assertion failed: "size of ___saved_mask < size of
>>> __sigprocmask_sigset_t"
>>> _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t),
>>>
>>> if __jmp_buf_sigset_t is too small.
>>>
>>> --
>>> H.J.
>>
>> Any comments, suggestions or objections? The patch is at
>>
>> https://sourceware.org/ml/libc-alpha/2017-11/msg00510.html
>>
>
> This is the updated patch. It limited the changes to Linux/x86.
> Any comments?
>
This is the patch I am checking in tomorrow.
Andrew, you can include <jmp_buf-ssp.h> to get
SHADOW_STACK_POINTER_OFFSET in jmp_buf.
--
H.J.
[-- Attachment #2: 0001-x86-Make-a-space-in-jmpbuf-for-shadow-stack-pointer.patch --]
[-- Type: text/x-patch, Size: 11386 bytes --]
From 902cbf786768eb04dde1c7333ff40ecae15f8a54 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 10 Jun 2017 17:23:06 -0700
Subject: [PATCH] x86: Make a space in jmpbuf for shadow stack pointer
To support Shadow Stack (SHSTK) in Intel Control-flow Enforcement
Technology (CET) in setjmp/longjmp, we need to save shadow stack
pointer in jmp_buf. The __saved_mask field in jmp_buf has type
of __sigset_t. On Linux, __sigset_t is defined as
#define _SIGSET_NWORDS (1024 / (8 * sizeof (unsigned long int)))
typedef struct
{
unsigned long int __val[_SIGSET_NWORDS];
} __sigset_t;
which is much bigger than expected by the __sigprocmask system call,
which has
typedef struct {
unsigned long sig[_NSIG_WORDS];
} sigset_t;
For Linux/x86, we can shrink __sigset_t used by __saved_mask in jmp_buf
to add paddings for shadow stack pointer. As long as the new __sigset_t
is not smaller than sigset_t expected by the __sigprocmask system call,
it should work correctly.
This patch adds an internal header file, <setjmpP.h>, to define
__jmp_buf_sigset_t for __saved_mask in jmp_buf for Linux/x86 with a
space to store shadow stack pointer. It verifies __jmp_buf_sigset_t has
the suitable size for the __sigprocmask system call. A run-time test,
tst-saved_mask-1.c, is added to verify that size of __jmp_buf_sigset_t
is sufficient. If its size is too small, the test fails with
rt_sigprocmask(SIG_SETMASK, strace: umoven: short read (4 < 8) @0x7fa8aa28effc
0x7fa8aa28effc, NULL, 8) = -1 EFAULT (Bad address)
rt_sigprocmask(SIG_SETMASK, strace: umoven: short read (4 < 8) @0x7fa8aa28effc
0x7fa8aa28effc, NULL, 8) = -1 EFAULT (Bad address)
rt_sigprocmask(SIG_SETMASK, NULL, 0x7fa8aa28effc, 8) = -1 EFAULT (Bad address)
exit_group(1) = ?
Tested with build-many-glibcs.py.
* debug/longjmp_chk.c: Include <setjmpP.h> instead of
<setjmp.h>.
* setjmp/longjmp.c: Include <setjmpP.h> instead of <setjmp.h>.
(__libc_siglongjmp): Cast &env[0].__saved_mask to "sigset_t *".
* setjmp/sigjmp.c: Include <setjmpP.h> instead of <setjmp.h>.
(__sigjmp_save): Cast &env[0].__saved_mask to "sigset_t *".
* sysdeps/generic/setjmpP.h: New file.
* sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym: Likewise.
* sysdeps/unix/sysv/linux/x86/setjmpP.h: Likewise.
* sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c: Likewise.
* sysdeps/unix/sysv/linux/x86/Makefile (gen-as-const-headers):
Add jmp_buf-ssp.sym.
(tests): Add tst-saved_mask-1.
---
debug/longjmp_chk.c | 2 +-
setjmp/longjmp.c | 5 +-
setjmp/sigjmp.c | 4 +-
sysdeps/generic/setjmpP.h | 24 +++++++++
sysdeps/unix/sysv/linux/x86/Makefile | 5 ++
sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym | 5 ++
sysdeps/unix/sysv/linux/x86/setjmpP.h | 67 ++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c | 55 +++++++++++++++++++++
8 files changed, 162 insertions(+), 5 deletions(-)
create mode 100644 sysdeps/generic/setjmpP.h
create mode 100644 sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym
create mode 100644 sysdeps/unix/sysv/linux/x86/setjmpP.h
create mode 100644 sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c
diff --git a/debug/longjmp_chk.c b/debug/longjmp_chk.c
index 1cea6c05ef..61be0da0cd 100644
--- a/debug/longjmp_chk.c
+++ b/debug/longjmp_chk.c
@@ -15,7 +15,7 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <setjmp.h>
+#include <setjmpP.h>
#define __longjmp ____longjmp_chk
#define __libc_siglongjmp __longjmp_chk
diff --git a/setjmp/longjmp.c b/setjmp/longjmp.c
index 2453c2c124..e144a874d7 100644
--- a/setjmp/longjmp.c
+++ b/setjmp/longjmp.c
@@ -16,7 +16,7 @@
<http://www.gnu.org/licenses/>. */
#include <stddef.h>
-#include <setjmp.h>
+#include <setjmpP.h>
#include <signal.h>
@@ -31,7 +31,8 @@ __libc_siglongjmp (sigjmp_buf env, int val)
if (env[0].__mask_was_saved)
/* Restore the saved signal mask. */
- (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask,
+ (void) __sigprocmask (SIG_SETMASK,
+ (sigset_t *) &env[0].__saved_mask,
(sigset_t *) NULL);
/* Call the machine-dependent function to restore machine state. */
diff --git a/setjmp/sigjmp.c b/setjmp/sigjmp.c
index 30839ae819..32b727fae8 100644
--- a/setjmp/sigjmp.c
+++ b/setjmp/sigjmp.c
@@ -16,7 +16,7 @@
<http://www.gnu.org/licenses/>. */
#include <stddef.h>
-#include <setjmp.h>
+#include <setjmpP.h>
#include <signal.h>
/* This function is called by the `sigsetjmp' macro
@@ -28,7 +28,7 @@ __sigjmp_save (sigjmp_buf env, int savemask)
{
env[0].__mask_was_saved = (savemask &&
__sigprocmask (SIG_BLOCK, (sigset_t *) NULL,
- &env[0].__saved_mask) == 0);
+ (sigset_t *) &env[0].__saved_mask) == 0);
return 0;
}
diff --git a/sysdeps/generic/setjmpP.h b/sysdeps/generic/setjmpP.h
new file mode 100644
index 0000000000..a878ce6b35
--- /dev/null
+++ b/sysdeps/generic/setjmpP.h
@@ -0,0 +1,24 @@
+/* Internal header file for <setjmp.h>. Generic version.
+ Copyright (C) 2017 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 _SETJMPP_H
+#define _SETJMPP_H 1
+
+#include <setjmp.h>
+
+#endif /* setjmpP.h */
diff --git a/sysdeps/unix/sysv/linux/x86/Makefile b/sysdeps/unix/sysv/linux/x86/Makefile
index c069570683..c55a43e58d 100644
--- a/sysdeps/unix/sysv/linux/x86/Makefile
+++ b/sysdeps/unix/sysv/linux/x86/Makefile
@@ -19,3 +19,8 @@ endif
ifeq ($(subdir),elf)
sysdep_routines += dl-vdso
endif
+
+ifeq ($(subdir),setjmp)
+gen-as-const-headers += jmp_buf-ssp.sym
+tests += tst-saved_mask-1
+endif
diff --git a/sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym b/sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym
new file mode 100644
index 0000000000..12829196db
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/jmp_buf-ssp.sym
@@ -0,0 +1,5 @@
+#include <setjmpP.h>
+#undef __saved_mask
+
+--
+SHADOW_STACK_POINTER_OFFSET offsetof(struct __jmp_buf_tag, __saved_mask.__saved.__shadow_stack_pointer)
diff --git a/sysdeps/unix/sysv/linux/x86/setjmpP.h b/sysdeps/unix/sysv/linux/x86/setjmpP.h
new file mode 100644
index 0000000000..69a46d0ce2
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/setjmpP.h
@@ -0,0 +1,67 @@
+/* Internal header file for <setjmp.h>. Linux/x86 version.
+ Copyright (C) 2017 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 _SETJMPP_H
+#define _SETJMPP_H 1
+
+#include <bits/types/__sigset_t.h>
+
+/* The biggest signal number + 1. As of kernel 4.14, x86 _NSIG is 64.
+ Define it to 513 to leave some rooms for future use. */
+#define _JUMP_BUF_SIGSET_NSIG 513
+/* Number of longs to hold all signals. */
+#define _JUMP_BUF_SIGSET_NWORDS \
+ ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
+
+typedef struct
+ {
+ unsigned long int __val[_JUMP_BUF_SIGSET_NWORDS];
+ } __jmp_buf_sigset_t;
+
+typedef union
+ {
+ __sigset_t __saved_mask_compat;
+ struct
+ {
+ __jmp_buf_sigset_t __saved_mask;
+ /* Used for shadow stack pointer. */
+ unsigned long int __shadow_stack_pointer;
+ } __saved;
+ } __jmpbuf_arch_t;
+
+#undef __sigset_t
+#define __sigset_t __jmpbuf_arch_t
+#include <setjmp.h>
+#undef __saved_mask
+#define __saved_mask __saved_mask.__saved.__saved_mask
+
+#include <signal.h>
+
+#define _SIGPROCMASK_NSIG_WORDS (_NSIG / (8 * sizeof (unsigned long int)))
+
+typedef struct
+ {
+ unsigned long int __val[_SIGPROCMASK_NSIG_WORDS];
+ } __sigprocmask_sigset_t;
+
+extern jmp_buf ___buf;
+extern __typeof (___buf[0].__saved_mask) ___saved_mask;
+_Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t),
+ "size of ___saved_mask < size of __sigprocmask_sigset_t");
+
+#endif /* setjmpP.h */
diff --git a/sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c b/sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c
new file mode 100644
index 0000000000..f383408b63
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/tst-saved_mask-1.c
@@ -0,0 +1,55 @@
+/* Test that sigprocmask does not read from the unused part of jmpbuf.
+ Copyright (C) 2017 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 <stdlib.h>
+#include <signal.h>
+#include <string.h>
+#include <errno.h>
+#include <setjmpP.h>
+#include <support/next_to_fault.h>
+
+#define SIZEOF_SIGSET_T sizeof (__jmp_buf_sigset_t)
+
+static int
+do_test (void)
+{
+ sigjmp_buf sj;
+ struct support_next_to_fault sigset_t_buf
+ = support_next_to_fault_allocate (SIZEOF_SIGSET_T);
+ sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer;
+ sigset_t m;
+
+ sigemptyset (&m);
+ memcpy (m_p, &m, SIZEOF_SIGSET_T);
+ sigprocmask (SIG_SETMASK, m_p, NULL);
+ memcpy (&m, m_p, SIZEOF_SIGSET_T);
+ if (sigsetjmp (sj, 0) == 0)
+ {
+ sigaddset (&m, SIGUSR1);
+ memcpy (m_p, &m, SIZEOF_SIGSET_T);
+ sigprocmask (SIG_SETMASK, m_p, NULL);
+ memcpy (&m, m_p, SIZEOF_SIGSET_T);
+ siglongjmp (sj, 1);
+ return EXIT_FAILURE;
+ }
+ sigprocmask (SIG_SETMASK, NULL, m_p);
+ memcpy (&m, m_p, SIZEOF_SIGSET_T);
+ return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+#include <support/test-driver.c>
--
2.14.3
prev parent reply other threads:[~2017-11-29 21:16 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-07 22:39 H.J. Lu
2017-11-08 6:06 ` Florian Weimer
2017-11-08 18:27 ` H.J. Lu
2017-11-13 13:09 ` Florian Weimer
2017-11-13 14:05 ` H.J. Lu
2017-11-13 16:34 ` Florian Weimer
2017-11-13 16:44 ` H.J. Lu
2017-11-13 19:40 ` H.J. Lu
2017-11-13 23:22 ` H.J. Lu
2017-11-14 12:26 ` Florian Weimer
2017-11-14 13:11 ` H.J. Lu
2017-11-15 11:10 ` Florian Weimer
2017-11-15 13:20 ` H.J. Lu
2017-11-21 22:22 ` H.J. Lu
2017-11-25 16:34 ` H.J. Lu
2017-11-29 21:16 ` H.J. Lu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMe9rOp+KxdpLJDYLHFbrh_kF-CEJ=jCmB6UcJs3fghd=REmBQ@mail.gmail.com' \
--to=hjl.tools@gmail.com \
--cc=andrew.senkevich@intel.com \
--cc=fweimer@redhat.com \
--cc=igor.v.tsimbalist@intel.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).