Re: [PATCH 03/10] x86/cet: Signal handling for shadow stack

From: Florian Weimer
Date: Thu Jun 07 2018 - 14:59:14 EST


On 06/07/2018 08:30 PM, Andy Lutomirski wrote:
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> wrote:

Set and restore shadow stack pointer for signals.

How does this interact with siglongjmp()?

We plan to use some unused signal mask bits in the jump buffer (we have a lot of those in glibc for some reason) to store the shadow stack pointer.

This patch makes me extremely nervous due to the possibility of ABI
issues and CRIU breakage.

diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
index 844d60eb1882..6c8997a0156a 100644
--- a/arch/x86/include/uapi/asm/sigcontext.h
+++ b/arch/x86/include/uapi/asm/sigcontext.h
@@ -230,6 +230,7 @@ struct sigcontext_32 {
__u32 fpstate; /* Zero when no FPU/extended context */
__u32 oldmask;
__u32 cr2;
+ __u32 ssp;
};

/*
@@ -262,6 +263,7 @@ struct sigcontext_64 {
__u64 trapno;
__u64 oldmask;
__u64 cr2;
+ __u64 ssp;

/*
* fpstate is really (struct _fpstate *) or (struct _xstate *)
@@ -320,6 +322,7 @@ struct sigcontext {
struct _fpstate __user *fpstate;
__u32 oldmask;
__u32 cr2;
+ __u32 ssp;

Is it actually okay to modify these structures like this? They're
part of the user ABI, and I don't know whether any user code relies on
the size being constant.

Probably not. Historically, these things have been tacked at the end of the floating point state, see struct _xstate:

/* New processor state extensions go here: */

However, I'm not sure if this is really ideal because I doubt that everyone who needs the shadow stack pointer also wants to sacrifice space for the AVX-512 save area (which is already a backwards compatibility hazard). Other architectures have variable offsets and some TLV-style setup here.

Thanks,
Florian