| Message ID | 20210427204315.24153-26-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > When shadow stack is enabled, a task's shadow stack states must be saved > along with the signal context and later restored in sigreturn. However, > currently there is no systematic facility for extending a signal context. > There is some space left in the ucontext, but changing ucontext is likely > to create compatibility issues and there is not enough space for further > extensions. > > Introduce a signal context extension struct 'sc_ext', which is used to save > shadow stack restore token address. The extension is located above the fpu > states, plus alignment. The struct can be extended (such as the ibt's > wait_endbr status to be introduced later), and sc_ext.total_size field > keeps track of total size. I still don't like this. Here's how the signal layout works, for better or for worse: The kernel has: struct rt_sigframe { char __user *pretcode; struct ucontext uc; struct siginfo info; /* fp state follows here */ }; This is roughly the actual signal frame. But userspace does not have this struct declared, and user code does not know the sizes of the fields. So it's accessed in a nonsensical way. The signal handler function is passed a pointer to the whole sigframe implicitly in RSP, a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. User code can *find* the fp state by following a pointer from mcontext, which is, in turn, found via uc: struct ucontext { unsigned long uc_flags; struct ucontext *uc_link; stack_t uc_stack; struct sigcontext uc_mcontext; <-- fp pointer is in here sigset_t uc_sigmask; /* mask last for extensibility */ }; The kernel, in sigreturn, works a bit differently. The sigreturn variants know the base address of the frame but don't have the benefit of receiving pointers to the fields. So instead the kernel takes advantage of the fact that it knows the offset to uc and parses uc accordingly. And the kernel follows the pointer in mcontext to find the fp state. The latter bit is quite important later. The kernel does not parse info at all. The fp state is its own mess. When XSAVE happened, Intel kindly (?) gave us a software defined area between the "legacy" x87 region and the modern supposedly extensible part. Linux sticks the following structure in that hole: struct _fpx_sw_bytes { /* * If set to FP_XSTATE_MAGIC1 then this is an xstate context. * 0 if a legacy frame. */ __u32 magic1; /* * Total size of the fpstate area: * * - if magic1 == 0 then it's sizeof(struct _fpstate) * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) * plus extensions (if any) */ __u32 extended_size; /* * Feature bit mask (including FP/SSE/extended state) that is present * in the memory layout: */ __u64 xfeatures; /* * Actual XSAVE state size, based on the xfeatures saved in the layout. * 'extended_size' is greater than 'xstate_size': */ __u32 xstate_size; /* For future use: */ __u32 padding[7]; }; That's where we are right now upstream. The kernel has a parser for the FPU state that is bugs piled upon bugs and is going to have to be rewritten sometime soon. On top of all this, we have two upcoming features, both of which require different kinds of extensions: 1. AVX-512. (Yeah, you thought this story was over a few years ago, but no. And AMX makes it worse.) To make a long story short, we promised user code many years ago that a signal frame fit in 2048 bytes with some room to spare. With AVX-512 this is false. With AMX it's so wrong it's not even funny. The only way out of the mess anyone has come up with involves making the length of the FPU state vary depending on which features are INIT, i.e. making it more compact than "compact" mode is. This has a side effect: it's no longer possible to modify the state in place, because enabling a feature with no space allocated will make the structure bigger, and the stack won't have room. Fortunately, one can relocate the entire FPU state, update the pointer in mcontext, and the kernel will happily follow the pointer. So new code on a new kernel using a super-compact state could expand the state by allocating new memory (on the heap? very awkwardly on the stack?) and changing the pointer. For all we know, some code already fiddles with the pointer. This is great, except that your patch sticks more data at the end of the FPU block that no one is expecting, and your sigreturn code follows that pointer, and will read off into lala land. 2. CET. CET wants us to find a few more bytes somewhere, and those bytes logically belong in ucontext, and here we are. This is *almost*, but not quite, easy: struct ucontext is already variable length! Unfortunately, the whole variable length portion is used up by uc_sigmask. So I propose that we introduce a brand new bona fide extension mechanism. It works like this: First, we add a struct ucontext_extension at the end. It looks like: struct ucontext_extension { u64 length; /* sizeof(struct ucontext_extension) */ u64 flags; /* we will want this some day */ [CET stuff here] [future stuff here] }; And we locate it by scrounging a word somewhere in ucontext to give the offset from the beginning of struct ucontext to ucontext_extension. We indicate the presence of this feature using a new uc_flags bit. I can think of a couple of vaguely reasonable places: a) the reserved word in sigcontext. This is fine for x86 but not so great if other architectures want to do this. b) uc_link. Fine everywhere but powerpc. Oops. c) use the high bits of uc_flags. After all, once we add extensions, we don't need new flags, so we can steal 16 high bits of uc_flags for this. I think I'm in favor of (c). We do: (uc_flags & 0xffff0000) == 0: extension not present Otherwise the extension region is at ucontext + (uc_flags >> 16). And sigreturn finds the extension the same way, because CRIU can already migrate a signal frame from one kernel to another, your patch breaks this, and having sigreturn hardcode the offset would also break it. What do you think?
On 4/28/2021 4:03 PM, Andy Lutomirski wrote: > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> >> When shadow stack is enabled, a task's shadow stack states must be saved >> along with the signal context and later restored in sigreturn. However, >> currently there is no systematic facility for extending a signal context. >> There is some space left in the ucontext, but changing ucontext is likely >> to create compatibility issues and there is not enough space for further >> extensions. >> >> Introduce a signal context extension struct 'sc_ext', which is used to save >> shadow stack restore token address. The extension is located above the fpu >> states, plus alignment. The struct can be extended (such as the ibt's >> wait_endbr status to be introduced later), and sc_ext.total_size field >> keeps track of total size. > > I still don't like this. > > Here's how the signal layout works, for better or for worse: > > The kernel has: > > struct rt_sigframe { > char __user *pretcode; > struct ucontext uc; > struct siginfo info; > /* fp state follows here */ > }; > > This is roughly the actual signal frame. But userspace does not have > this struct declared, and user code does not know the sizes of the > fields. So it's accessed in a nonsensical way. The signal handler > function is passed a pointer to the whole sigframe implicitly in RSP, > a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. > User code can *find* the fp state by following a pointer from > mcontext, which is, in turn, found via uc: > > struct ucontext { > unsigned long uc_flags; > struct ucontext *uc_link; > stack_t uc_stack; > struct sigcontext uc_mcontext; <-- fp pointer is in here > sigset_t uc_sigmask; /* mask last for extensibility */ > }; > > The kernel, in sigreturn, works a bit differently. The sigreturn > variants know the base address of the frame but don't have the benefit > of receiving pointers to the fields. So instead the kernel takes > advantage of the fact that it knows the offset to uc and parses uc > accordingly. And the kernel follows the pointer in mcontext to find > the fp state. The latter bit is quite important later. The kernel > does not parse info at all. > > The fp state is its own mess. When XSAVE happened, Intel kindly (?) > gave us a software defined area between the "legacy" x87 region and > the modern supposedly extensible part. Linux sticks the following > structure in that hole: > > struct _fpx_sw_bytes { > /* > * If set to FP_XSTATE_MAGIC1 then this is an xstate context. > * 0 if a legacy frame. > */ > __u32 magic1; > > /* > * Total size of the fpstate area: > * > * - if magic1 == 0 then it's sizeof(struct _fpstate) > * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) > * plus extensions (if any) > */ > __u32 extended_size; > > /* > * Feature bit mask (including FP/SSE/extended state) that is present > * in the memory layout: > */ > __u64 xfeatures; > > /* > * Actual XSAVE state size, based on the xfeatures saved in the layout. > * 'extended_size' is greater than 'xstate_size': > */ > __u32 xstate_size; > > /* For future use: */ > __u32 padding[7]; > }; > > > That's where we are right now upstream. The kernel has a parser for > the FPU state that is bugs piled upon bugs and is going to have to be > rewritten sometime soon. On top of all this, we have two upcoming > features, both of which require different kinds of extensions: > > 1. AVX-512. (Yeah, you thought this story was over a few years ago, > but no. And AMX makes it worse.) To make a long story short, we > promised user code many years ago that a signal frame fit in 2048 > bytes with some room to spare. With AVX-512 this is false. With AMX > it's so wrong it's not even funny. The only way out of the mess > anyone has come up with involves making the length of the FPU state > vary depending on which features are INIT, i.e. making it more compact > than "compact" mode is. This has a side effect: it's no longer > possible to modify the state in place, because enabling a feature with > no space allocated will make the structure bigger, and the stack won't > have room. Fortunately, one can relocate the entire FPU state, update > the pointer in mcontext, and the kernel will happily follow the > pointer. So new code on a new kernel using a super-compact state > could expand the state by allocating new memory (on the heap? very > awkwardly on the stack?) and changing the pointer. For all we know, > some code already fiddles with the pointer. This is great, except > that your patch sticks more data at the end of the FPU block that no > one is expecting, and your sigreturn code follows that pointer, and > will read off into lala land. > > 2. CET. CET wants us to find a few more bytes somewhere, and those > bytes logically belong in ucontext, and here we are. > > This is *almost*, but not quite, easy: struct ucontext is already > variable length! Unfortunately, the whole variable length portion is > used up by uc_sigmask. So I propose that we introduce a brand new > bona fide extension mechanism. It works like this: > > First, we add a struct ucontext_extension at the end. It looks like: > > struct ucontext_extension { > u64 length; /* sizeof(struct ucontext_extension) */ > u64 flags; /* we will want this some day */ > [CET stuff here] > [future stuff here] > }; > > And we locate it by scrounging a word somewhere in ucontext to give > the offset from the beginning of struct ucontext to > ucontext_extension. We indicate the presence of this feature using a > new uc_flags bit. I can think of a couple of vaguely reasonable > places: > > a) the reserved word in sigcontext. This is fine for x86 but not so > great if other architectures want to do this. > > b) uc_link. Fine everywhere but powerpc. Oops. > > c) use the high bits of uc_flags. After all, once we add extensions, > we don't need new flags, so we can steal 16 high bits of uc_flags for > this. > > I think I'm in favor of (c). We do: > > (uc_flags & 0xffff0000) == 0: extension not present > > Otherwise the extension region is at ucontext + (uc_flags >> 16). > > And sigreturn finds the extension the same way, because CRIU can > already migrate a signal frame from one kernel to another, your patch > breaks this, and having sigreturn hardcode the offset would also break > it. > > What do you think? > There are a lot of things in here. I think I could create some patches for ucontext_extension and send out for discussion. Thanks for explaining this! Yu-cheng
On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote: > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > When shadow stack is enabled, a task's shadow stack states must be saved > > along with the signal context and later restored in sigreturn. However, > > currently there is no systematic facility for extending a signal context. > > There is some space left in the ucontext, but changing ucontext is likely > > to create compatibility issues and there is not enough space for further > > extensions. > > > > Introduce a signal context extension struct 'sc_ext', which is used to save > > shadow stack restore token address. The extension is located above the fpu > > states, plus alignment. The struct can be extended (such as the ibt's > > wait_endbr status to be introduced later), and sc_ext.total_size field > > keeps track of total size. > > I still don't like this. > > Here's how the signal layout works, for better or for worse: > > The kernel has: > > struct rt_sigframe { > char __user *pretcode; > struct ucontext uc; > struct siginfo info; > /* fp state follows here */ > }; > > This is roughly the actual signal frame. But userspace does not have > this struct declared, and user code does not know the sizes of the > fields. So it's accessed in a nonsensical way. The signal handler Well, not really. While indeed this is not declared as a part of API the structure is widely used for rt_sigreturn syscall (and we're using it inside criu thus any change here will simply break the restore procedure). Sorry out of time right now, I'll read your mail more carefully once time permit.
On Thu, Apr 29, 2021 at 12:28 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote: > > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > > When shadow stack is enabled, a task's shadow stack states must be saved > > > along with the signal context and later restored in sigreturn. However, > > > currently there is no systematic facility for extending a signal context. > > > There is some space left in the ucontext, but changing ucontext is likely > > > to create compatibility issues and there is not enough space for further > > > extensions. > > > > > > Introduce a signal context extension struct 'sc_ext', which is used to save > > > shadow stack restore token address. The extension is located above the fpu > > > states, plus alignment. The struct can be extended (such as the ibt's > > > wait_endbr status to be introduced later), and sc_ext.total_size field > > > keeps track of total size. > > > > I still don't like this. > > > > Here's how the signal layout works, for better or for worse: > > > > The kernel has: > > > > struct rt_sigframe { > > char __user *pretcode; > > struct ucontext uc; > > struct siginfo info; > > /* fp state follows here */ > > }; > > > > This is roughly the actual signal frame. But userspace does not have > > this struct declared, and user code does not know the sizes of the > > fields. So it's accessed in a nonsensical way. The signal handler > > Well, not really. While indeed this is not declared as a part of API > the structure is widely used for rt_sigreturn syscall (and we're using > it inside criu thus any change here will simply break the restore > procedure). Sorry out of time right now, I'll read your mail more > carefully once time permit. I skimmed the CRIU code. You appear to declare struct rt_sigframe, and you use the offset from the start of rt_sigframe to uc. You also use the offset to the /* fp state follows here */ part, but that's unnecessary -- you could just as easily have put the fp state at any other address -- the kernel will happily follow the pointer you supply regardless of where it points. So the only issues I can see are if you write the fp state on top of something else or if you inadvertently fill in the proposed extension part of uc_flags. Right now you seem to be ignoring uc_flags, which I presume means that you are filling it in as zero. Even if the offset of the fp state in the kernel rt_sigframe changes, the kernel should still successfully parse the signal frame you generate. I suppose there is another potential issue: would CRIU have issues if the *save* runs on a kernel that uses this proposed extension mechanism? Are you doing something with the saved state that would get confused? --Andy
On Thu, Apr 29, 2021 at 07:44:03AM -0700, Andy Lutomirski wrote: > On Thu, Apr 29, 2021 at 12:28 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > > > On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote: > > > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > > > > When shadow stack is enabled, a task's shadow stack states must be saved > > > > along with the signal context and later restored in sigreturn. However, > > > > currently there is no systematic facility for extending a signal context. > > > > There is some space left in the ucontext, but changing ucontext is likely > > > > to create compatibility issues and there is not enough space for further > > > > extensions. > > > > > > > > Introduce a signal context extension struct 'sc_ext', which is used to save > > > > shadow stack restore token address. The extension is located above the fpu > > > > states, plus alignment. The struct can be extended (such as the ibt's > > > > wait_endbr status to be introduced later), and sc_ext.total_size field > > > > keeps track of total size. > > > > > > I still don't like this. > > > > > > Here's how the signal layout works, for better or for worse: > > > > > > The kernel has: > > > > > > struct rt_sigframe { > > > char __user *pretcode; > > > struct ucontext uc; > > > struct siginfo info; > > > /* fp state follows here */ > > > }; > > > > > > This is roughly the actual signal frame. But userspace does not have > > > this struct declared, and user code does not know the sizes of the > > > fields. So it's accessed in a nonsensical way. The signal handler > > > > Well, not really. While indeed this is not declared as a part of API > > the structure is widely used for rt_sigreturn syscall (and we're using > > it inside criu thus any change here will simply break the restore > > procedure). Sorry out of time right now, I'll read your mail more > > carefully once time permit. > > I skimmed the CRIU code. You appear to declare struct rt_sigframe, > and you use the offset from the start of rt_sigframe to uc. You also > use the offset to the /* fp state follows here */ part, but that's > unnecessary -- you could just as easily have put the fp state at any > other address -- the kernel will happily follow the pointer you supply > regardless of where it points. So the only issues I can see are if Yes, since rt_sigreturn is a very late stage of restore we've to preallocate memory early and surely doing it in one slab is a way more convenient, thus that's why fp context follows the structure declaration (well, it was at least I didn't touch this code for years already). Anyway, I meant that assuming "user code does not know the sizes of the fields" is not exactly true and strictly speaking current layout became a part of nondeclared api (simply because kernel use this structure run-time only and even if something get changed in kernel declaration for this structure the kernel itself will be fine because it doesn't save it on disk between kernel updates) but for tools like criu such change might require update. IOW, I mean that changing rt_sigreturn structure itself is not safe and we can't reorder the members and theirs size. Or I misunderstood you, and you meant only the fact that fp may be arbitrary, pointing for any place in userspace? If so then indeed, fp can be pointing to context placed anywere since kernel will fetch it from the address during rt_sigreturn processing. > you write the fp state on top of something else or if you > inadvertently fill in the proposed extension part of uc_flags. Right > now you seem to be ignoring uc_flags, which I presume means that you > are filling it in as zero. Even if the offset of the fp state in the yes > kernel rt_sigframe changes, the kernel should still successfully parse > the signal frame you generate. > > I suppose there is another potential issue: would CRIU have issues if > the *save* runs on a kernel that uses this proposed extension > mechanism? Are you doing something with the saved state that would > get confused? You know, actually I guess criu may not work with new extensions, we have a predefined max xsave frame size and any new extensions to cpu states will require additional support. I must confess I didn't follow much on CET internals yet so I must be simply wrong. CC'ing Anrew just in case. Cyrill
* Andy Lutomirski: > The kernel has: > > struct rt_sigframe { > char __user *pretcode; > struct ucontext uc; > struct siginfo info; > /* fp state follows here */ > }; > > This is roughly the actual signal frame. But userspace does not have > this struct declared, and user code does not know the sizes of the > fields. So it's accessed in a nonsensical way. The signal handler > function is passed a pointer to the whole sigframe implicitly in RSP, > a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. > User code can *find* the fp state by following a pointer from > mcontext, which is, in turn, found via uc: > > struct ucontext { > unsigned long uc_flags; > struct ucontext *uc_link; > stack_t uc_stack; > struct sigcontext uc_mcontext; <-- fp pointer is in here > sigset_t uc_sigmask; /* mask last for extensibility */ > }; I must say that I haven't reviewed this in detail, but for historic reasons, glibc userspace has a differently-sized sigset_t. So the kernel ucontext (used in signals) and user ucontext (used for swapcontext et al.) are already fully decoupled? Thanks, Florian
On 4/28/2021 4:03 PM, Andy Lutomirski wrote: > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> >> When shadow stack is enabled, a task's shadow stack states must be saved >> along with the signal context and later restored in sigreturn. However, >> currently there is no systematic facility for extending a signal context. >> There is some space left in the ucontext, but changing ucontext is likely >> to create compatibility issues and there is not enough space for further >> extensions. >> >> Introduce a signal context extension struct 'sc_ext', which is used to save >> shadow stack restore token address. The extension is located above the fpu >> states, plus alignment. The struct can be extended (such as the ibt's >> wait_endbr status to be introduced later), and sc_ext.total_size field >> keeps track of total size. > > I still don't like this. > > Here's how the signal layout works, for better or for worse: > > The kernel has: > > struct rt_sigframe { > char __user *pretcode; > struct ucontext uc; > struct siginfo info; > /* fp state follows here */ > }; > > This is roughly the actual signal frame. But userspace does not have > this struct declared, and user code does not know the sizes of the > fields. So it's accessed in a nonsensical way. The signal handler > function is passed a pointer to the whole sigframe implicitly in RSP, > a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. > User code can *find* the fp state by following a pointer from > mcontext, which is, in turn, found via uc: > > struct ucontext { > unsigned long uc_flags; > struct ucontext *uc_link; > stack_t uc_stack; > struct sigcontext uc_mcontext; <-- fp pointer is in here > sigset_t uc_sigmask; /* mask last for extensibility */ > }; > > The kernel, in sigreturn, works a bit differently. The sigreturn > variants know the base address of the frame but don't have the benefit > of receiving pointers to the fields. So instead the kernel takes > advantage of the fact that it knows the offset to uc and parses uc > accordingly. And the kernel follows the pointer in mcontext to find > the fp state. The latter bit is quite important later. The kernel > does not parse info at all. > > The fp state is its own mess. When XSAVE happened, Intel kindly (?) > gave us a software defined area between the "legacy" x87 region and > the modern supposedly extensible part. Linux sticks the following > structure in that hole: > > struct _fpx_sw_bytes { > /* > * If set to FP_XSTATE_MAGIC1 then this is an xstate context. > * 0 if a legacy frame. > */ > __u32 magic1; > > /* > * Total size of the fpstate area: > * > * - if magic1 == 0 then it's sizeof(struct _fpstate) > * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) > * plus extensions (if any) > */ > __u32 extended_size; > > /* > * Feature bit mask (including FP/SSE/extended state) that is present > * in the memory layout: > */ > __u64 xfeatures; > > /* > * Actual XSAVE state size, based on the xfeatures saved in the layout. > * 'extended_size' is greater than 'xstate_size': > */ > __u32 xstate_size; > > /* For future use: */ > __u32 padding[7]; > }; > > > That's where we are right now upstream. The kernel has a parser for > the FPU state that is bugs piled upon bugs and is going to have to be > rewritten sometime soon. On top of all this, we have two upcoming > features, both of which require different kinds of extensions: > > 1. AVX-512. (Yeah, you thought this story was over a few years ago, > but no. And AMX makes it worse.) To make a long story short, we > promised user code many years ago that a signal frame fit in 2048 > bytes with some room to spare. With AVX-512 this is false. With AMX > it's so wrong it's not even funny. The only way out of the mess > anyone has come up with involves making the length of the FPU state > vary depending on which features are INIT, i.e. making it more compact > than "compact" mode is. This has a side effect: it's no longer > possible to modify the state in place, because enabling a feature with > no space allocated will make the structure bigger, and the stack won't > have room. Fortunately, one can relocate the entire FPU state, update > the pointer in mcontext, and the kernel will happily follow the > pointer. So new code on a new kernel using a super-compact state > could expand the state by allocating new memory (on the heap? very > awkwardly on the stack?) and changing the pointer. For all we know, > some code already fiddles with the pointer. This is great, except > that your patch sticks more data at the end of the FPU block that no > one is expecting, and your sigreturn code follows that pointer, and > will read off into lala land. > Then, what about we don't do that at all. Is it possible from now on we don't stick more data at the end, and take the relocating-fpu approach? > 2. CET. CET wants us to find a few more bytes somewhere, and those > bytes logically belong in ucontext, and here we are. > Fortunately, we can spare CET the need of ucontext extension. When the kernel handles sigreturn, the user-mode shadow stack pointer is right at the restore token. There is no need to put that in ucontext. However, the WAIT_ENDBR status needs to be saved/restored for signals. Since IBT is now dependent on shadow stack, we can use a spare bit of the shadow stack restore token for that. I have tested the change, and will send out another version of the whole series. > This is *almost*, but not quite, easy: struct ucontext is already > variable length! Unfortunately, the whole variable length portion is > used up by uc_sigmask. So I propose that we introduce a brand new > bona fide extension mechanism. It works like this: > > First, we add a struct ucontext_extension at the end. It looks like: > > struct ucontext_extension { > u64 length; /* sizeof(struct ucontext_extension) */ > u64 flags; /* we will want this some day */ > [CET stuff here] > [future stuff here] > }; > > And we locate it by scrounging a word somewhere in ucontext to give > the offset from the beginning of struct ucontext to > ucontext_extension. We indicate the presence of this feature using a > new uc_flags bit. I can think of a couple of vaguely reasonable > places: > > a) the reserved word in sigcontext. This is fine for x86 but not so > great if other architectures want to do this. > > b) uc_link. Fine everywhere but powerpc. Oops. > > c) use the high bits of uc_flags. After all, once we add extensions, > we don't need new flags, so we can steal 16 high bits of uc_flags for > this. > > I think I'm in favor of (c). We do: > > (uc_flags & 0xffff0000) == 0: extension not present > > Otherwise the extension region is at ucontext + (uc_flags >> 16). > > And sigreturn finds the extension the same way, because CRIU can > already migrate a signal frame from one kernel to another, your patch > breaks this, and having sigreturn hardcode the offset would also break > it. > > What do you think? >
On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 4/28/2021 4:03 PM, Andy Lutomirski wrote: > > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > >> > >> When shadow stack is enabled, a task's shadow stack states must be saved > >> along with the signal context and later restored in sigreturn. However, > >> currently there is no systematic facility for extending a signal context. > >> There is some space left in the ucontext, but changing ucontext is likely > >> to create compatibility issues and there is not enough space for further > >> extensions. > >> > >> Introduce a signal context extension struct 'sc_ext', which is used to save > >> shadow stack restore token address. The extension is located above the fpu > >> states, plus alignment. The struct can be extended (such as the ibt's > >> wait_endbr status to be introduced later), and sc_ext.total_size field > >> keeps track of total size. > > > > I still don't like this. > > > > Here's how the signal layout works, for better or for worse: > > > > The kernel has: > > > > struct rt_sigframe { > > char __user *pretcode; > > struct ucontext uc; > > struct siginfo info; > > /* fp state follows here */ > > }; > > > > This is roughly the actual signal frame. But userspace does not have > > this struct declared, and user code does not know the sizes of the > > fields. So it's accessed in a nonsensical way. The signal handler > > function is passed a pointer to the whole sigframe implicitly in RSP, > > a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. > > User code can *find* the fp state by following a pointer from > > mcontext, which is, in turn, found via uc: > > > > struct ucontext { > > unsigned long uc_flags; > > struct ucontext *uc_link; > > stack_t uc_stack; > > struct sigcontext uc_mcontext; <-- fp pointer is in here > > sigset_t uc_sigmask; /* mask last for extensibility */ > > }; > > > > The kernel, in sigreturn, works a bit differently. The sigreturn > > variants know the base address of the frame but don't have the benefit > > of receiving pointers to the fields. So instead the kernel takes > > advantage of the fact that it knows the offset to uc and parses uc > > accordingly. And the kernel follows the pointer in mcontext to find > > the fp state. The latter bit is quite important later. The kernel > > does not parse info at all. > > > > The fp state is its own mess. When XSAVE happened, Intel kindly (?) > > gave us a software defined area between the "legacy" x87 region and > > the modern supposedly extensible part. Linux sticks the following > > structure in that hole: > > > > struct _fpx_sw_bytes { > > /* > > * If set to FP_XSTATE_MAGIC1 then this is an xstate context. > > * 0 if a legacy frame. > > */ > > __u32 magic1; > > > > /* > > * Total size of the fpstate area: > > * > > * - if magic1 == 0 then it's sizeof(struct _fpstate) > > * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) > > * plus extensions (if any) > > */ > > __u32 extended_size; > > > > /* > > * Feature bit mask (including FP/SSE/extended state) that is present > > * in the memory layout: > > */ > > __u64 xfeatures; > > > > /* > > * Actual XSAVE state size, based on the xfeatures saved in the layout. > > * 'extended_size' is greater than 'xstate_size': > > */ > > __u32 xstate_size; > > > > /* For future use: */ > > __u32 padding[7]; > > }; > > > > > > That's where we are right now upstream. The kernel has a parser for > > the FPU state that is bugs piled upon bugs and is going to have to be > > rewritten sometime soon. On top of all this, we have two upcoming > > features, both of which require different kinds of extensions: > > > > 1. AVX-512. (Yeah, you thought this story was over a few years ago, > > but no. And AMX makes it worse.) To make a long story short, we > > promised user code many years ago that a signal frame fit in 2048 > > bytes with some room to spare. With AVX-512 this is false. With AMX > > it's so wrong it's not even funny. The only way out of the mess > > anyone has come up with involves making the length of the FPU state > > vary depending on which features are INIT, i.e. making it more compact > > than "compact" mode is. This has a side effect: it's no longer > > possible to modify the state in place, because enabling a feature with > > no space allocated will make the structure bigger, and the stack won't > > have room. Fortunately, one can relocate the entire FPU state, update > > the pointer in mcontext, and the kernel will happily follow the > > pointer. So new code on a new kernel using a super-compact state > > could expand the state by allocating new memory (on the heap? very > > awkwardly on the stack?) and changing the pointer. For all we know, > > some code already fiddles with the pointer. This is great, except > > that your patch sticks more data at the end of the FPU block that no > > one is expecting, and your sigreturn code follows that pointer, and > > will read off into lala land. > > > > Then, what about we don't do that at all. Is it possible from now on we > don't stick more data at the end, and take the relocating-fpu approach? > > > 2. CET. CET wants us to find a few more bytes somewhere, and those > > bytes logically belong in ucontext, and here we are. > > > > Fortunately, we can spare CET the need of ucontext extension. When the > kernel handles sigreturn, the user-mode shadow stack pointer is right at > the restore token. There is no need to put that in ucontext. That seems entirely reasonable. This might also avoid needing to teach CRIU about CET at all. > > However, the WAIT_ENDBR status needs to be saved/restored for signals. > Since IBT is now dependent on shadow stack, we can use a spare bit of > the shadow stack restore token for that. That seems like unnecessary ABI coupling. We have plenty of bits in uc_flags, and we have an entire reserved word in sigcontext. How about just sticking this bit in one of those places? --Andy
On 4/30/2021 10:47 AM, Andy Lutomirski wrote: > On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >> >> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>> >>>> When shadow stack is enabled, a task's shadow stack states must be saved >>>> along with the signal context and later restored in sigreturn. However, >>>> currently there is no systematic facility for extending a signal context. >>>> There is some space left in the ucontext, but changing ucontext is likely >>>> to create compatibility issues and there is not enough space for further >>>> extensions. >>>> >>>> Introduce a signal context extension struct 'sc_ext', which is used to save >>>> shadow stack restore token address. The extension is located above the fpu >>>> states, plus alignment. The struct can be extended (such as the ibt's >>>> wait_endbr status to be introduced later), and sc_ext.total_size field >>>> keeps track of total size. >>> >>> I still don't like this. >>> >>> Here's how the signal layout works, for better or for worse: >>> >>> The kernel has: >>> >>> struct rt_sigframe { >>> char __user *pretcode; >>> struct ucontext uc; >>> struct siginfo info; >>> /* fp state follows here */ >>> }; >>> >>> This is roughly the actual signal frame. But userspace does not have >>> this struct declared, and user code does not know the sizes of the >>> fields. So it's accessed in a nonsensical way. The signal handler >>> function is passed a pointer to the whole sigframe implicitly in RSP, >>> a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. >>> User code can *find* the fp state by following a pointer from >>> mcontext, which is, in turn, found via uc: >>> >>> struct ucontext { >>> unsigned long uc_flags; >>> struct ucontext *uc_link; >>> stack_t uc_stack; >>> struct sigcontext uc_mcontext; <-- fp pointer is in here >>> sigset_t uc_sigmask; /* mask last for extensibility */ >>> }; >>> >>> The kernel, in sigreturn, works a bit differently. The sigreturn >>> variants know the base address of the frame but don't have the benefit >>> of receiving pointers to the fields. So instead the kernel takes >>> advantage of the fact that it knows the offset to uc and parses uc >>> accordingly. And the kernel follows the pointer in mcontext to find >>> the fp state. The latter bit is quite important later. The kernel >>> does not parse info at all. >>> >>> The fp state is its own mess. When XSAVE happened, Intel kindly (?) >>> gave us a software defined area between the "legacy" x87 region and >>> the modern supposedly extensible part. Linux sticks the following >>> structure in that hole: >>> >>> struct _fpx_sw_bytes { >>> /* >>> * If set to FP_XSTATE_MAGIC1 then this is an xstate context. >>> * 0 if a legacy frame. >>> */ >>> __u32 magic1; >>> >>> /* >>> * Total size of the fpstate area: >>> * >>> * - if magic1 == 0 then it's sizeof(struct _fpstate) >>> * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) >>> * plus extensions (if any) >>> */ >>> __u32 extended_size; >>> >>> /* >>> * Feature bit mask (including FP/SSE/extended state) that is present >>> * in the memory layout: >>> */ >>> __u64 xfeatures; >>> >>> /* >>> * Actual XSAVE state size, based on the xfeatures saved in the layout. >>> * 'extended_size' is greater than 'xstate_size': >>> */ >>> __u32 xstate_size; >>> >>> /* For future use: */ >>> __u32 padding[7]; >>> }; >>> >>> >>> That's where we are right now upstream. The kernel has a parser for >>> the FPU state that is bugs piled upon bugs and is going to have to be >>> rewritten sometime soon. On top of all this, we have two upcoming >>> features, both of which require different kinds of extensions: >>> >>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>> but no. And AMX makes it worse.) To make a long story short, we >>> promised user code many years ago that a signal frame fit in 2048 >>> bytes with some room to spare. With AVX-512 this is false. With AMX >>> it's so wrong it's not even funny. The only way out of the mess >>> anyone has come up with involves making the length of the FPU state >>> vary depending on which features are INIT, i.e. making it more compact >>> than "compact" mode is. This has a side effect: it's no longer >>> possible to modify the state in place, because enabling a feature with >>> no space allocated will make the structure bigger, and the stack won't >>> have room. Fortunately, one can relocate the entire FPU state, update >>> the pointer in mcontext, and the kernel will happily follow the >>> pointer. So new code on a new kernel using a super-compact state >>> could expand the state by allocating new memory (on the heap? very >>> awkwardly on the stack?) and changing the pointer. For all we know, >>> some code already fiddles with the pointer. This is great, except >>> that your patch sticks more data at the end of the FPU block that no >>> one is expecting, and your sigreturn code follows that pointer, and >>> will read off into lala land. >>> >> >> Then, what about we don't do that at all. Is it possible from now on we >> don't stick more data at the end, and take the relocating-fpu approach? >> >>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>> bytes logically belong in ucontext, and here we are. >>> >> >> Fortunately, we can spare CET the need of ucontext extension. When the >> kernel handles sigreturn, the user-mode shadow stack pointer is right at >> the restore token. There is no need to put that in ucontext. > > That seems entirely reasonable. This might also avoid needing to > teach CRIU about CET at all. > >> >> However, the WAIT_ENDBR status needs to be saved/restored for signals. >> Since IBT is now dependent on shadow stack, we can use a spare bit of >> the shadow stack restore token for that. > > That seems like unnecessary ABI coupling. We have plenty of bits in > uc_flags, and we have an entire reserved word in sigcontext. How > about just sticking this bit in one of those places? Yes, I will make it UC_WAIT_ENDBR. Thanks, Yu-cheng
On Fri, Apr 30, 2021 at 10:47 AM Andy Lutomirski <luto@kernel.org> wrote: > > On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > > > On 4/28/2021 4:03 PM, Andy Lutomirski wrote: > > > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > >> > > >> When shadow stack is enabled, a task's shadow stack states must be saved > > >> along with the signal context and later restored in sigreturn. However, > > >> currently there is no systematic facility for extending a signal context. > > >> There is some space left in the ucontext, but changing ucontext is likely > > >> to create compatibility issues and there is not enough space for further > > >> extensions. > > >> > > >> Introduce a signal context extension struct 'sc_ext', which is used to save > > >> shadow stack restore token address. The extension is located above the fpu > > >> states, plus alignment. The struct can be extended (such as the ibt's > > >> wait_endbr status to be introduced later), and sc_ext.total_size field > > >> keeps track of total size. > > > > > > I still don't like this. > > > > > > Here's how the signal layout works, for better or for worse: > > > > > > The kernel has: > > > > > > struct rt_sigframe { > > > char __user *pretcode; > > > struct ucontext uc; > > > struct siginfo info; > > > /* fp state follows here */ > > > }; > > > > > > This is roughly the actual signal frame. But userspace does not have > > > this struct declared, and user code does not know the sizes of the > > > fields. So it's accessed in a nonsensical way. The signal handler > > > function is passed a pointer to the whole sigframe implicitly in RSP, > > > a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. > > > User code can *find* the fp state by following a pointer from > > > mcontext, which is, in turn, found via uc: > > > > > > struct ucontext { > > > unsigned long uc_flags; > > > struct ucontext *uc_link; > > > stack_t uc_stack; > > > struct sigcontext uc_mcontext; <-- fp pointer is in here > > > sigset_t uc_sigmask; /* mask last for extensibility */ > > > }; > > > > > > The kernel, in sigreturn, works a bit differently. The sigreturn > > > variants know the base address of the frame but don't have the benefit > > > of receiving pointers to the fields. So instead the kernel takes > > > advantage of the fact that it knows the offset to uc and parses uc > > > accordingly. And the kernel follows the pointer in mcontext to find > > > the fp state. The latter bit is quite important later. The kernel > > > does not parse info at all. > > > > > > The fp state is its own mess. When XSAVE happened, Intel kindly (?) > > > gave us a software defined area between the "legacy" x87 region and > > > the modern supposedly extensible part. Linux sticks the following > > > structure in that hole: > > > > > > struct _fpx_sw_bytes { > > > /* > > > * If set to FP_XSTATE_MAGIC1 then this is an xstate context. > > > * 0 if a legacy frame. > > > */ > > > __u32 magic1; > > > > > > /* > > > * Total size of the fpstate area: > > > * > > > * - if magic1 == 0 then it's sizeof(struct _fpstate) > > > * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct _xstate) > > > * plus extensions (if any) > > > */ > > > __u32 extended_size; > > > > > > /* > > > * Feature bit mask (including FP/SSE/extended state) that is present > > > * in the memory layout: > > > */ > > > __u64 xfeatures; > > > > > > /* > > > * Actual XSAVE state size, based on the xfeatures saved in the layout. > > > * 'extended_size' is greater than 'xstate_size': > > > */ > > > __u32 xstate_size; > > > > > > /* For future use: */ > > > __u32 padding[7]; > > > }; > > > > > > > > > That's where we are right now upstream. The kernel has a parser for > > > the FPU state that is bugs piled upon bugs and is going to have to be > > > rewritten sometime soon. On top of all this, we have two upcoming > > > features, both of which require different kinds of extensions: > > > > > > 1. AVX-512. (Yeah, you thought this story was over a few years ago, > > > but no. And AMX makes it worse.) To make a long story short, we > > > promised user code many years ago that a signal frame fit in 2048 > > > bytes with some room to spare. With AVX-512 this is false. With AMX > > > it's so wrong it's not even funny. The only way out of the mess > > > anyone has come up with involves making the length of the FPU state > > > vary depending on which features are INIT, i.e. making it more compact > > > than "compact" mode is. This has a side effect: it's no longer > > > possible to modify the state in place, because enabling a feature with > > > no space allocated will make the structure bigger, and the stack won't > > > have room. Fortunately, one can relocate the entire FPU state, update > > > the pointer in mcontext, and the kernel will happily follow the > > > pointer. So new code on a new kernel using a super-compact state > > > could expand the state by allocating new memory (on the heap? very > > > awkwardly on the stack?) and changing the pointer. For all we know, > > > some code already fiddles with the pointer. This is great, except > > > that your patch sticks more data at the end of the FPU block that no > > > one is expecting, and your sigreturn code follows that pointer, and > > > will read off into lala land. > > > > > > > Then, what about we don't do that at all. Is it possible from now on we > > don't stick more data at the end, and take the relocating-fpu approach? > > > > > 2. CET. CET wants us to find a few more bytes somewhere, and those > > > bytes logically belong in ucontext, and here we are. > > > > > > > Fortunately, we can spare CET the need of ucontext extension. When the > > kernel handles sigreturn, the user-mode shadow stack pointer is right at > > the restore token. There is no need to put that in ucontext. > > That seems entirely reasonable. This might also avoid needing to > teach CRIU about CET at all. Wait, what's the actual shadow stack token format? And is the token on the new stack or the old stack when sigaltstack is in use? For that matter, is there any support for an alternate shadow stack for signals? --Andy
<vedvyas.shanbhogue@intel.com>,Dave Martin <Dave.Martin@arm.com>,Weijiang Yang <weijiang.yang@intel.com>,Pengfei Xu <pengfei.xu@intel.com>,Haitao Huang <haitao.huang@intel.com> From: "H. Peter Anvin" <hpa@zytor.com> Message-ID: <2360408E-9967-469C-96AF-E8AC40044021@zytor.com> There are a few words at the end of the FXSAVE area for software use for this reason: so we can extend the signal frame – originally to enable saving the XSAVE state but that doesn't use up the whole software available area. On May 2, 2021 4:23:49 PM PDT, Andy Lutomirski <luto@kernel.org> wrote: >On Fri, Apr 30, 2021 at 10:47 AM Andy Lutomirski <luto@kernel.org> >wrote: >> >> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> >wrote: >> > >> > On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >> > > On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu ><yu-cheng.yu@intel.com> wrote: >> > >> >> > >> When shadow stack is enabled, a task's shadow stack states must >be saved >> > >> along with the signal context and later restored in sigreturn. >However, >> > >> currently there is no systematic facility for extending a signal >context. >> > >> There is some space left in the ucontext, but changing ucontext >is likely >> > >> to create compatibility issues and there is not enough space for >further >> > >> extensions. >> > >> >> > >> Introduce a signal context extension struct 'sc_ext', which is >used to save >> > >> shadow stack restore token address. The extension is located >above the fpu >> > >> states, plus alignment. The struct can be extended (such as the >ibt's >> > >> wait_endbr status to be introduced later), and sc_ext.total_size >field >> > >> keeps track of total size. >> > > >> > > I still don't like this. >> > > >> > > Here's how the signal layout works, for better or for worse: >> > > >> > > The kernel has: >> > > >> > > struct rt_sigframe { >> > > char __user *pretcode; >> > > struct ucontext uc; >> > > struct siginfo info; >> > > /* fp state follows here */ >> > > }; >> > > >> > > This is roughly the actual signal frame. But userspace does not >have >> > > this struct declared, and user code does not know the sizes of >the >> > > fields. So it's accessed in a nonsensical way. The signal >handler >> > > function is passed a pointer to the whole sigframe implicitly in >RSP, >> > > a pointer to &frame->info in RSI, anda pointer to &frame->uc in >RDX. >> > > User code can *find* the fp state by following a pointer from >> > > mcontext, which is, in turn, found via uc: >> > > >> > > struct ucontext { >> > > unsigned long uc_flags; >> > > struct ucontext *uc_link; >> > > stack_t uc_stack; >> > > struct sigcontext uc_mcontext; <-- fp pointer is in here >> > > sigset_t uc_sigmask; /* mask last for extensibility >*/ >> > > }; >> > > >> > > The kernel, in sigreturn, works a bit differently. The sigreturn >> > > variants know the base address of the frame but don't have the >benefit >> > > of receiving pointers to the fields. So instead the kernel takes >> > > advantage of the fact that it knows the offset to uc and parses >uc >> > > accordingly. And the kernel follows the pointer in mcontext to >find >> > > the fp state. The latter bit is quite important later. The >kernel >> > > does not parse info at all. >> > > >> > > The fp state is its own mess. When XSAVE happened, Intel kindly >(?) >> > > gave us a software defined area between the "legacy" x87 region >and >> > > the modern supposedly extensible part. Linux sticks the >following >> > > structure in that hole: >> > > >> > > struct _fpx_sw_bytes { >> > > /* >> > > * If set to FP_XSTATE_MAGIC1 then this is an xstate >context. >> > > * 0 if a legacy frame. >> > > */ >> > > __u32 magic1; >> > > >> > > /* >> > > * Total size of the fpstate area: >> > > * >> > > * - if magic1 == 0 then it's sizeof(struct _fpstate) >> > > * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct >_xstate) >> > > * plus extensions (if any) >> > > */ >> > > __u32 extended_size; >> > > >> > > /* >> > > * Feature bit mask (including FP/SSE/extended state) that >is present >> > > * in the memory layout: >> > > */ >> > > __u64 xfeatures; >> > > >> > > /* >> > > * Actual XSAVE state size, based on the xfeatures saved in >the layout. >> > > * 'extended_size' is greater than 'xstate_size': >> > > */ >> > > __u32 xstate_size; >> > > >> > > /* For future use: */ >> > > __u32 padding[7]; >> > > }; >> > > >> > > >> > > That's where we are right now upstream. The kernel has a parser >for >> > > the FPU state that is bugs piled upon bugs and is going to have >to be >> > > rewritten sometime soon. On top of all this, we have two >upcoming >> > > features, both of which require different kinds of extensions: >> > > >> > > 1. AVX-512. (Yeah, you thought this story was over a few years >ago, >> > > but no. And AMX makes it worse.) To make a long story short, we >> > > promised user code many years ago that a signal frame fit in 2048 >> > > bytes with some room to spare. With AVX-512 this is false. With >AMX >> > > it's so wrong it's not even funny. The only way out of the mess >> > > anyone has come up with involves making the length of the FPU >state >> > > vary depending on which features are INIT, i.e. making it more >compact >> > > than "compact" mode is. This has a side effect: it's no longer >> > > possible to modify the state in place, because enabling a feature >with >> > > no space allocated will make the structure bigger, and the stack >won't >> > > have room. Fortunately, one can relocate the entire FPU state, >update >> > > the pointer in mcontext, and the kernel will happily follow the >> > > pointer. So new code on a new kernel using a super-compact state >> > > could expand the state by allocating new memory (on the heap? >very >> > > awkwardly on the stack?) and changing the pointer. For all we >know, >> > > some code already fiddles with the pointer. This is great, >except >> > > that your patch sticks more data at the end of the FPU block that >no >> > > one is expecting, and your sigreturn code follows that pointer, >and >> > > will read off into lala land. >> > > >> > >> > Then, what about we don't do that at all. Is it possible from now >on we >> > don't stick more data at the end, and take the relocating-fpu >approach? >> > >> > > 2. CET. CET wants us to find a few more bytes somewhere, and >those >> > > bytes logically belong in ucontext, and here we are. >> > > >> > >> > Fortunately, we can spare CET the need of ucontext extension. When >the >> > kernel handles sigreturn, the user-mode shadow stack pointer is >right at >> > the restore token. There is no need to put that in ucontext. >> >> That seems entirely reasonable. This might also avoid needing to >> teach CRIU about CET at all. > >Wait, what's the actual shadow stack token format? And is the token >on the new stack or the old stack when sigaltstack is in use? For >that matter, is there any support for an alternate shadow stack for >signals? > >--Andy
On 5/2/2021 4:23 PM, Andy Lutomirski wrote: > On Fri, Apr 30, 2021 at 10:47 AM Andy Lutomirski <luto@kernel.org> wrote: >> >> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >>> >>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>>> >>>>> When shadow stack is enabled, a task's shadow stack states must be saved >>>>> along with the signal context and later restored in sigreturn. However, >>>>> currently there is no systematic facility for extending a signal context. >>>>> There is some space left in the ucontext, but changing ucontext is likely >>>>> to create compatibility issues and there is not enough space for further >>>>> extensions. >>>>> >>>>> Introduce a signal context extension struct 'sc_ext', which is used to save >>>>> shadow stack restore token address. The extension is located above the fpu >>>>> states, plus alignment. The struct can be extended (such as the ibt's >>>>> wait_endbr status to be introduced later), and sc_ext.total_size field >>>>> keeps track of total size. >>>> >>>> I still don't like this. >>>> >>>> Here's how the signal layout works, for better or for worse: >>>> [...] >>>> >>>> That's where we are right now upstream. The kernel has a parser for >>>> the FPU state that is bugs piled upon bugs and is going to have to be >>>> rewritten sometime soon. On top of all this, we have two upcoming >>>> features, both of which require different kinds of extensions: >>>> >>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>>> but no. And AMX makes it worse.) To make a long story short, we >>>> promised user code many years ago that a signal frame fit in 2048 >>>> bytes with some room to spare. With AVX-512 this is false. With AMX >>>> it's so wrong it's not even funny. The only way out of the mess >>>> anyone has come up with involves making the length of the FPU state >>>> vary depending on which features are INIT, i.e. making it more compact >>>> than "compact" mode is. This has a side effect: it's no longer >>>> possible to modify the state in place, because enabling a feature with >>>> no space allocated will make the structure bigger, and the stack won't >>>> have room. Fortunately, one can relocate the entire FPU state, update >>>> the pointer in mcontext, and the kernel will happily follow the >>>> pointer. So new code on a new kernel using a super-compact state >>>> could expand the state by allocating new memory (on the heap? very >>>> awkwardly on the stack?) and changing the pointer. For all we know, >>>> some code already fiddles with the pointer. This is great, except >>>> that your patch sticks more data at the end of the FPU block that no >>>> one is expecting, and your sigreturn code follows that pointer, and >>>> will read off into lala land. >>>> >>> >>> Then, what about we don't do that at all. Is it possible from now on we >>> don't stick more data at the end, and take the relocating-fpu approach? >>> >>>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>>> bytes logically belong in ucontext, and here we are. >>>> >>> >>> Fortunately, we can spare CET the need of ucontext extension. When the >>> kernel handles sigreturn, the user-mode shadow stack pointer is right at >>> the restore token. There is no need to put that in ucontext. >> >> That seems entirely reasonable. This might also avoid needing to >> teach CRIU about CET at all. > > Wait, what's the actual shadow stack token format? And is the token > on the new stack or the old stack when sigaltstack is in use? For > that matter, is there any support for an alternate shadow stack for > signals? > The restore token is a pointer pointing directly above itself and bit[0] indicates 64-bit mode. Because the shadow stack stores only return addresses, there is no alternate shadow stack. However, the application can allocate and switch to a new shadow stack. Yu-cheng
> On May 3, 2021, at 8:14 AM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 5/2/2021 4:23 PM, Andy Lutomirski wrote: >>> On Fri, Apr 30, 2021 at 10:47 AM Andy Lutomirski <luto@kernel.org> wrote: >>> >>> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >>>> >>>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>>>> >>>>>> When shadow stack is enabled, a task's shadow stack states must be saved >>>>>> along with the signal context and later restored in sigreturn. However, >>>>>> currently there is no systematic facility for extending a signal context. >>>>>> There is some space left in the ucontext, but changing ucontext is likely >>>>>> to create compatibility issues and there is not enough space for further >>>>>> extensions. >>>>>> >>>>>> Introduce a signal context extension struct 'sc_ext', which is used to save >>>>>> shadow stack restore token address. The extension is located above the fpu >>>>>> states, plus alignment. The struct can be extended (such as the ibt's >>>>>> wait_endbr status to be introduced later), and sc_ext.total_size field >>>>>> keeps track of total size. >>>>> >>>>> I still don't like this. >>>>> >>>>> Here's how the signal layout works, for better or for worse: >>>>> > > [...] > >>>>> >>>>> That's where we are right now upstream. The kernel has a parser for >>>>> the FPU state that is bugs piled upon bugs and is going to have to be >>>>> rewritten sometime soon. On top of all this, we have two upcoming >>>>> features, both of which require different kinds of extensions: >>>>> >>>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>>>> but no. And AMX makes it worse.) To make a long story short, we >>>>> promised user code many years ago that a signal frame fit in 2048 >>>>> bytes with some room to spare. With AVX-512 this is false. With AMX >>>>> it's so wrong it's not even funny. The only way out of the mess >>>>> anyone has come up with involves making the length of the FPU state >>>>> vary depending on which features are INIT, i.e. making it more compact >>>>> than "compact" mode is. This has a side effect: it's no longer >>>>> possible to modify the state in place, because enabling a feature with >>>>> no space allocated will make the structure bigger, and the stack won't >>>>> have room. Fortunately, one can relocate the entire FPU state, update >>>>> the pointer in mcontext, and the kernel will happily follow the >>>>> pointer. So new code on a new kernel using a super-compact state >>>>> could expand the state by allocating new memory (on the heap? very >>>>> awkwardly on the stack?) and changing the pointer. For all we know, >>>>> some code already fiddles with the pointer. This is great, except >>>>> that your patch sticks more data at the end of the FPU block that no >>>>> one is expecting, and your sigreturn code follows that pointer, and >>>>> will read off into lala land. >>>>> >>>> >>>> Then, what about we don't do that at all. Is it possible from now on we >>>> don't stick more data at the end, and take the relocating-fpu approach? >>>> >>>>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>>>> bytes logically belong in ucontext, and here we are. >>>>> >>>> >>>> Fortunately, we can spare CET the need of ucontext extension. When the >>>> kernel handles sigreturn, the user-mode shadow stack pointer is right at >>>> the restore token. There is no need to put that in ucontext. >>> >>> That seems entirely reasonable. This might also avoid needing to >>> teach CRIU about CET at all. >> Wait, what's the actual shadow stack token format? And is the token >> on the new stack or the old stack when sigaltstack is in use? For >> that matter, is there any support for an alternate shadow stack for >> signals? > > The restore token is a pointer pointing directly above itself and bit[0] indicates 64-bit mode. > > Because the shadow stack stores only return addresses, there is no alternate shadow stack. However, the application can allocate and switch to a new shadow stack. I think we should make the ABI support an alternate shadow stack even if we don’t implement it initially. After all, some day someone might want to register a handler for shadow stack overflow. > > Yu-cheng
On 5/3/2021 8:29 AM, Andy Lutomirski wrote: > >> On May 3, 2021, at 8:14 AM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >> >> On 5/2/2021 4:23 PM, Andy Lutomirski wrote: >>>> On Fri, Apr 30, 2021 at 10:47 AM Andy Lutomirski <luto@kernel.org> wrote: >>>> >>>> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >>>>> >>>>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>>>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>>>>> >>>>>>> When shadow stack is enabled, a task's shadow stack states must be saved >>>>>>> along with the signal context and later restored in sigreturn. However, >>>>>>> currently there is no systematic facility for extending a signal context. >>>>>>> There is some space left in the ucontext, but changing ucontext is likely >>>>>>> to create compatibility issues and there is not enough space for further >>>>>>> extensions. >>>>>>> >>>>>>> Introduce a signal context extension struct 'sc_ext', which is used to save >>>>>>> shadow stack restore token address. The extension is located above the fpu >>>>>>> states, plus alignment. The struct can be extended (such as the ibt's >>>>>>> wait_endbr status to be introduced later), and sc_ext.total_size field >>>>>>> keeps track of total size. >>>>>> >>>>>> I still don't like this. >>>>>> >>>>>> Here's how the signal layout works, for better or for worse: >>>>>> >> >> [...] >> >>>>>> >>>>>> That's where we are right now upstream. The kernel has a parser for >>>>>> the FPU state that is bugs piled upon bugs and is going to have to be >>>>>> rewritten sometime soon. On top of all this, we have two upcoming >>>>>> features, both of which require different kinds of extensions: >>>>>> >>>>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>>>>> but no. And AMX makes it worse.) To make a long story short, we >>>>>> promised user code many years ago that a signal frame fit in 2048 >>>>>> bytes with some room to spare. With AVX-512 this is false. With AMX >>>>>> it's so wrong it's not even funny. The only way out of the mess >>>>>> anyone has come up with involves making the length of the FPU state >>>>>> vary depending on which features are INIT, i.e. making it more compact >>>>>> than "compact" mode is. This has a side effect: it's no longer >>>>>> possible to modify the state in place, because enabling a feature with >>>>>> no space allocated will make the structure bigger, and the stack won't >>>>>> have room. Fortunately, one can relocate the entire FPU state, update >>>>>> the pointer in mcontext, and the kernel will happily follow the >>>>>> pointer. So new code on a new kernel using a super-compact state >>>>>> could expand the state by allocating new memory (on the heap? very >>>>>> awkwardly on the stack?) and changing the pointer. For all we know, >>>>>> some code already fiddles with the pointer. This is great, except >>>>>> that your patch sticks more data at the end of the FPU block that no >>>>>> one is expecting, and your sigreturn code follows that pointer, and >>>>>> will read off into lala land. >>>>>> >>>>> >>>>> Then, what about we don't do that at all. Is it possible from now on we >>>>> don't stick more data at the end, and take the relocating-fpu approach? >>>>> >>>>>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>>>>> bytes logically belong in ucontext, and here we are. >>>>>> >>>>> >>>>> Fortunately, we can spare CET the need of ucontext extension. When the >>>>> kernel handles sigreturn, the user-mode shadow stack pointer is right at >>>>> the restore token. There is no need to put that in ucontext. >>>> >>>> That seems entirely reasonable. This might also avoid needing to >>>> teach CRIU about CET at all. >>> Wait, what's the actual shadow stack token format? And is the token >>> on the new stack or the old stack when sigaltstack is in use? For >>> that matter, is there any support for an alternate shadow stack for >>> signals? >> >> The restore token is a pointer pointing directly above itself and bit[0] indicates 64-bit mode. >> >> Because the shadow stack stores only return addresses, there is no alternate shadow stack. However, the application can allocate and switch to a new shadow stack. > > I think we should make the ABI support an alternate shadow stack even if we don’t implement it initially. After all, some day someone might want to register a handler for shadow stack overflow. > Agree. We can probably add something in parallel of sigaltstack(), and let the user choose separately alternate normal/shadow stacks. Thanks, Yu-cheng
On 4/30/2021 11:32 AM, Yu, Yu-cheng wrote: > On 4/30/2021 10:47 AM, Andy Lutomirski wrote: >> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> >> wrote: >>> >>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> >>>> wrote: >>>>> >>>>> When shadow stack is enabled, a task's shadow stack states must be >>>>> saved >>>>> along with the signal context and later restored in sigreturn. >>>>> However, >>>>> currently there is no systematic facility for extending a signal >>>>> context. >>>>> There is some space left in the ucontext, but changing ucontext is >>>>> likely >>>>> to create compatibility issues and there is not enough space for >>>>> further >>>>> extensions. >>>>> >>>>> Introduce a signal context extension struct 'sc_ext', which is used >>>>> to save >>>>> shadow stack restore token address. The extension is located above >>>>> the fpu >>>>> states, plus alignment. The struct can be extended (such as the ibt's >>>>> wait_endbr status to be introduced later), and sc_ext.total_size field >>>>> keeps track of total size. >>>> >>>> I still don't like this. >>>> >>>> Here's how the signal layout works, for better or for worse: >>>> >>>> The kernel has: >>>> >>>> struct rt_sigframe { >>>> char __user *pretcode; >>>> struct ucontext uc; >>>> struct siginfo info; >>>> /* fp state follows here */ >>>> }; >>>> >>>> This is roughly the actual signal frame. But userspace does not have >>>> this struct declared, and user code does not know the sizes of the >>>> fields. So it's accessed in a nonsensical way. The signal handler >>>> function is passed a pointer to the whole sigframe implicitly in RSP, >>>> a pointer to &frame->info in RSI, anda pointer to &frame->uc in RDX. >>>> User code can *find* the fp state by following a pointer from >>>> mcontext, which is, in turn, found via uc: >>>> >>>> struct ucontext { >>>> unsigned long uc_flags; >>>> struct ucontext *uc_link; >>>> stack_t uc_stack; >>>> struct sigcontext uc_mcontext; <-- fp pointer is in here >>>> sigset_t uc_sigmask; /* mask last for extensibility */ >>>> }; >>>> >>>> The kernel, in sigreturn, works a bit differently. The sigreturn >>>> variants know the base address of the frame but don't have the benefit >>>> of receiving pointers to the fields. So instead the kernel takes >>>> advantage of the fact that it knows the offset to uc and parses uc >>>> accordingly. And the kernel follows the pointer in mcontext to find >>>> the fp state. The latter bit is quite important later. The kernel >>>> does not parse info at all. >>>> >>>> The fp state is its own mess. When XSAVE happened, Intel kindly (?) >>>> gave us a software defined area between the "legacy" x87 region and >>>> the modern supposedly extensible part. Linux sticks the following >>>> structure in that hole: >>>> >>>> struct _fpx_sw_bytes { >>>> /* >>>> * If set to FP_XSTATE_MAGIC1 then this is an xstate context. >>>> * 0 if a legacy frame. >>>> */ >>>> __u32 magic1; >>>> >>>> /* >>>> * Total size of the fpstate area: >>>> * >>>> * - if magic1 == 0 then it's sizeof(struct _fpstate) >>>> * - if magic1 == FP_XSTATE_MAGIC1 then it's sizeof(struct >>>> _xstate) >>>> * plus extensions (if any) >>>> */ >>>> __u32 extended_size; >>>> >>>> /* >>>> * Feature bit mask (including FP/SSE/extended state) that is >>>> present >>>> * in the memory layout: >>>> */ >>>> __u64 xfeatures; >>>> >>>> /* >>>> * Actual XSAVE state size, based on the xfeatures saved in >>>> the layout. >>>> * 'extended_size' is greater than 'xstate_size': >>>> */ >>>> __u32 xstate_size; >>>> >>>> /* For future use: */ >>>> __u32 padding[7]; >>>> }; >>>> >>>> >>>> That's where we are right now upstream. The kernel has a parser for >>>> the FPU state that is bugs piled upon bugs and is going to have to be >>>> rewritten sometime soon. On top of all this, we have two upcoming >>>> features, both of which require different kinds of extensions: >>>> >>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>>> but no. And AMX makes it worse.) To make a long story short, we >>>> promised user code many years ago that a signal frame fit in 2048 >>>> bytes with some room to spare. With AVX-512 this is false. With AMX >>>> it's so wrong it's not even funny. The only way out of the mess >>>> anyone has come up with involves making the length of the FPU state >>>> vary depending on which features are INIT, i.e. making it more compact >>>> than "compact" mode is. This has a side effect: it's no longer >>>> possible to modify the state in place, because enabling a feature with >>>> no space allocated will make the structure bigger, and the stack won't >>>> have room. Fortunately, one can relocate the entire FPU state, update >>>> the pointer in mcontext, and the kernel will happily follow the >>>> pointer. So new code on a new kernel using a super-compact state >>>> could expand the state by allocating new memory (on the heap? very >>>> awkwardly on the stack?) and changing the pointer. For all we know, >>>> some code already fiddles with the pointer. This is great, except >>>> that your patch sticks more data at the end of the FPU block that no >>>> one is expecting, and your sigreturn code follows that pointer, and >>>> will read off into lala land. >>>> >>> >>> Then, what about we don't do that at all. Is it possible from now on we >>> don't stick more data at the end, and take the relocating-fpu approach? >>> >>>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>>> bytes logically belong in ucontext, and here we are. >>>> >>> >>> Fortunately, we can spare CET the need of ucontext extension. When the >>> kernel handles sigreturn, the user-mode shadow stack pointer is right at >>> the restore token. There is no need to put that in ucontext. >> >> That seems entirely reasonable. This might also avoid needing to >> teach CRIU about CET at all. >> >>> >>> However, the WAIT_ENDBR status needs to be saved/restored for signals. >>> Since IBT is now dependent on shadow stack, we can use a spare bit of >>> the shadow stack restore token for that. >> >> That seems like unnecessary ABI coupling. We have plenty of bits in >> uc_flags, and we have an entire reserved word in sigcontext. How >> about just sticking this bit in one of those places? > > Yes, I will make it UC_WAIT_ENDBR. Personally, I think an explicit flag is cleaner than using a reserved word somewhere. However, there is a small issue: ia32 has no uc_flags. This series can support legacy apps up to now. But, instead of creating too many special cases, perhaps we should drop CET support of ia32? Thoughts? Thanks, Yu-cheng
On 5/4/2021 1:49 PM, Yu, Yu-cheng wrote: > On 4/30/2021 11:32 AM, Yu, Yu-cheng wrote: >> On 4/30/2021 10:47 AM, Andy Lutomirski wrote: >>> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> >>> wrote: >>>> >>>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: >>>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> >>>>> wrote: >>>>>> >>>>>> When shadow stack is enabled, a task's shadow stack states must be >>>>>> saved >>>>>> along with the signal context and later restored in sigreturn. >>>>>> However, >>>>>> currently there is no systematic facility for extending a signal >>>>>> context. >>>>>> There is some space left in the ucontext, but changing ucontext is >>>>>> likely >>>>>> to create compatibility issues and there is not enough space for >>>>>> further >>>>>> extensions. >>>>>> >>>>>> Introduce a signal context extension struct 'sc_ext', which is >>>>>> used to save >>>>>> shadow stack restore token address. The extension is located >>>>>> above the fpu >>>>>> states, plus alignment. The struct can be extended (such as the >>>>>> ibt's >>>>>> wait_endbr status to be introduced later), and sc_ext.total_size >>>>>> field >>>>>> keeps track of total size. >>>>> >>>>> I still don't like this. >>>>> [...] >>>>> >>>>> That's where we are right now upstream. The kernel has a parser for >>>>> the FPU state that is bugs piled upon bugs and is going to have to be >>>>> rewritten sometime soon. On top of all this, we have two upcoming >>>>> features, both of which require different kinds of extensions: >>>>> >>>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, >>>>> but no. And AMX makes it worse.) To make a long story short, we >>>>> promised user code many years ago that a signal frame fit in 2048 >>>>> bytes with some room to spare. With AVX-512 this is false. With AMX >>>>> it's so wrong it's not even funny. The only way out of the mess >>>>> anyone has come up with involves making the length of the FPU state >>>>> vary depending on which features are INIT, i.e. making it more compact >>>>> than "compact" mode is. This has a side effect: it's no longer >>>>> possible to modify the state in place, because enabling a feature with >>>>> no space allocated will make the structure bigger, and the stack won't >>>>> have room. Fortunately, one can relocate the entire FPU state, update >>>>> the pointer in mcontext, and the kernel will happily follow the >>>>> pointer. So new code on a new kernel using a super-compact state >>>>> could expand the state by allocating new memory (on the heap? very >>>>> awkwardly on the stack?) and changing the pointer. For all we know, >>>>> some code already fiddles with the pointer. This is great, except >>>>> that your patch sticks more data at the end of the FPU block that no >>>>> one is expecting, and your sigreturn code follows that pointer, and >>>>> will read off into lala land. >>>>> >>>> >>>> Then, what about we don't do that at all. Is it possible from now >>>> on we >>>> don't stick more data at the end, and take the relocating-fpu approach? >>>> >>>>> 2. CET. CET wants us to find a few more bytes somewhere, and those >>>>> bytes logically belong in ucontext, and here we are. >>>>> >>>> >>>> Fortunately, we can spare CET the need of ucontext extension. When the >>>> kernel handles sigreturn, the user-mode shadow stack pointer is >>>> right at >>>> the restore token. There is no need to put that in ucontext. >>> >>> That seems entirely reasonable. This might also avoid needing to >>> teach CRIU about CET at all. >>> >>>> >>>> However, the WAIT_ENDBR status needs to be saved/restored for signals. >>>> Since IBT is now dependent on shadow stack, we can use a spare bit of >>>> the shadow stack restore token for that. >>> >>> That seems like unnecessary ABI coupling. We have plenty of bits in >>> uc_flags, and we have an entire reserved word in sigcontext. How >>> about just sticking this bit in one of those places? >> >> Yes, I will make it UC_WAIT_ENDBR. > > Personally, I think an explicit flag is cleaner than using a reserved > word somewhere. However, there is a small issue: ia32 has no uc_flags. > > This series can support legacy apps up to now. But, instead of creating > too many special cases, perhaps we should drop CET support of ia32? > > Thoughts? > Once we have UC_WAIT_ENDBR, IBT signal handling becomes quite simple. Like the following: diff --git a/arch/x86/include/uapi/asm/ucontext.h b/arch/x86/include/uapi/asm/ucontext.h index 5657b7a49f03..96375d609e11 100644 --- a/arch/x86/include/uapi/asm/ucontext.h +++ b/arch/x86/include/uapi/asm/ucontext.h @@ -49,6 +49,11 @@ */ #define UC_SIGCONTEXT_SS 0x2 #define UC_STRICT_RESTORE_SS 0x4 + +/* + * UC_WAIT_ENDBR indicates the task is in wait-ENDBR status. + */ +#define UC_WAIT_ENDBR 0x08 #endif #include <asm-generic/ucontext.h> diff --git a/arch/x86/kernel/ibt.c b/arch/x86/kernel/ibt.c index d2563dd4759f..da804314ddc4 100644 --- a/arch/x86/kernel/ibt.c +++ b/arch/x86/kernel/ibt.c @@ -66,3 +66,32 @@ void ibt_disable(void) ibt_set_clear_msr_bits(0, CET_ENDBR_EN); current->thread.cet.ibt = 0; } + +int ibt_get_clear_wait_endbr(void) +{ + u64 msr_val = 0; + + if (!current->thread.cet.ibt) + return 0; + + fpregs_lock(); + + if (test_thread_flag(TIF_NEED_FPU_LOAD)) + __fpregs_load_activate(); + + if (!rdmsrl_safe(MSR_IA32_U_CET, &msr_val)) + wrmsrl(MSR_IA32_U_CET, msr_val & ~CET_WAIT_ENDBR); + + fpregs_unlock(); + + return msr_val & CET_WAIT_ENDBR; +} + +int ibt_set_wait_endbr(void) +{ + if (!current->thread.cet.ibt) + return 0; + + + return ibt_set_clear_msr_bits(CET_WAIT_ENDBR, 0); +} diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 66b662e57e19..5afd15419006 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -46,6 +46,7 @@ #include <asm/syscall.h> #include <asm/sigframe.h> #include <asm/signal.h> +#include <asm/cet.h> #ifdef CONFIG_X86_64 /* @@ -134,6 +135,9 @@ static int restore_sigcontext(struct pt_regs *regs, */ if (unlikely(!(uc_flags & UC_STRICT_RESTORE_SS) && user_64bit_mode(regs))) force_valid_ss(regs); + + if (uc_flags & UC_WAIT_ENDBR) + ibt_set_wait_endbr(); #endif return fpu__restore_sig((void __user *)sc.fpstate, @@ -433,6 +437,9 @@ static unsigned long frame_uc_flags(struct pt_regs *regs) if (likely(user_64bit_mode(regs))) flags |= UC_STRICT_RESTORE_SS; + if (ibt_get_clear_wait_endbr()) + flags |= UC_WAIT_ENDBR; + return flags; } However, this cannot handle ia32 with no SA_SIGINFO. For that, can we create a synthetic token on the shadow stack? - The token points to itself with reserved bit[1] set, and cannot be used for RSTORSSP. - The token only exists for ia32 with no SA_SIGINFO *AND* when the task is in wait-endbr. The signal shadow stack will look like this: --> ssp before signal synthetic IBT token (for ia32 no SA_SIGINFO) shadow stack restore token sigreturn address The synthetic token is not valid in other situations. How is that? Thanks, Yu-cheng
On Thu, May 6, 2021 at 3:05 PM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 5/4/2021 1:49 PM, Yu, Yu-cheng wrote: > > On 4/30/2021 11:32 AM, Yu, Yu-cheng wrote: > >> On 4/30/2021 10:47 AM, Andy Lutomirski wrote: > >>> On Fri, Apr 30, 2021 at 10:00 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> > >>> wrote: > >>>> > >>>> On 4/28/2021 4:03 PM, Andy Lutomirski wrote: > >>>>> On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> > >>>>> wrote: > >>>>>> > >>>>>> When shadow stack is enabled, a task's shadow stack states must be > >>>>>> saved > >>>>>> along with the signal context and later restored in sigreturn. > >>>>>> However, > >>>>>> currently there is no systematic facility for extending a signal > >>>>>> context. > >>>>>> There is some space left in the ucontext, but changing ucontext is > >>>>>> likely > >>>>>> to create compatibility issues and there is not enough space for > >>>>>> further > >>>>>> extensions. > >>>>>> > >>>>>> Introduce a signal context extension struct 'sc_ext', which is > >>>>>> used to save > >>>>>> shadow stack restore token address. The extension is located > >>>>>> above the fpu > >>>>>> states, plus alignment. The struct can be extended (such as the > >>>>>> ibt's > >>>>>> wait_endbr status to be introduced later), and sc_ext.total_size > >>>>>> field > >>>>>> keeps track of total size. > >>>>> > >>>>> I still don't like this. > >>>>> > > [...] > > >>>>> > >>>>> That's where we are right now upstream. The kernel has a parser for > >>>>> the FPU state that is bugs piled upon bugs and is going to have to be > >>>>> rewritten sometime soon. On top of all this, we have two upcoming > >>>>> features, both of which require different kinds of extensions: > >>>>> > >>>>> 1. AVX-512. (Yeah, you thought this story was over a few years ago, > >>>>> but no. And AMX makes it worse.) To make a long story short, we > >>>>> promised user code many years ago that a signal frame fit in 2048 > >>>>> bytes with some room to spare. With AVX-512 this is false. With AMX > >>>>> it's so wrong it's not even funny. The only way out of the mess > >>>>> anyone has come up with involves making the length of the FPU state > >>>>> vary depending on which features are INIT, i.e. making it more compact > >>>>> than "compact" mode is. This has a side effect: it's no longer > >>>>> possible to modify the state in place, because enabling a feature with > >>>>> no space allocated will make the structure bigger, and the stack won't > >>>>> have room. Fortunately, one can relocate the entire FPU state, update > >>>>> the pointer in mcontext, and the kernel will happily follow the > >>>>> pointer. So new code on a new kernel using a super-compact state > >>>>> could expand the state by allocating new memory (on the heap? very > >>>>> awkwardly on the stack?) and changing the pointer. For all we know, > >>>>> some code already fiddles with the pointer. This is great, except > >>>>> that your patch sticks more data at the end of the FPU block that no > >>>>> one is expecting, and your sigreturn code follows that pointer, and > >>>>> will read off into lala land. > >>>>> > >>>> > >>>> Then, what about we don't do that at all. Is it possible from now > >>>> on we > >>>> don't stick more data at the end, and take the relocating-fpu approach? > >>>> > >>>>> 2. CET. CET wants us to find a few more bytes somewhere, and those > >>>>> bytes logically belong in ucontext, and here we are. > >>>>> > >>>> > >>>> Fortunately, we can spare CET the need of ucontext extension. When the > >>>> kernel handles sigreturn, the user-mode shadow stack pointer is > >>>> right at > >>>> the restore token. There is no need to put that in ucontext. > >>> > >>> That seems entirely reasonable. This might also avoid needing to > >>> teach CRIU about CET at all. > >>> > >>>> > >>>> However, the WAIT_ENDBR status needs to be saved/restored for signals. > >>>> Since IBT is now dependent on shadow stack, we can use a spare bit of > >>>> the shadow stack restore token for that. > >>> > >>> That seems like unnecessary ABI coupling. We have plenty of bits in > >>> uc_flags, and we have an entire reserved word in sigcontext. How > >>> about just sticking this bit in one of those places? > >> > >> Yes, I will make it UC_WAIT_ENDBR. > > > > Personally, I think an explicit flag is cleaner than using a reserved > > word somewhere. However, there is a small issue: ia32 has no uc_flags. > > > > This series can support legacy apps up to now. But, instead of creating > > too many special cases, perhaps we should drop CET support of ia32? > > > > Thoughts? I'm really not thrilled about coupling IBT and SHSTK like this. Here are a couple of possible solutions: - Don't support IBT in 32-bit mode, or maybe just don't support IBT with legacy 32-bit signals. The actual mechanics of this could be awkward. Maybe we would reject the sigaction() call or the IBT-enabling request if they conflict? - Find some space in the signal frame for these flags. Looking around a bit, sigframe_ia32 has fpstate_unused, but I can imagine things like CRIU getting very confused if it stops being unused. sigframe_ia32 uses sigcontext_32, which has a bunch of reserved space in __gsh, __fsh, etc. rt_sigframe_ia32 has uc_flags, so this isn't a real problem. I don't have a brilliant solution here.
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index 5e3d9b7fd5fb..423abcd181f2 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -202,7 +202,8 @@ do { \ */ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size, - void __user **fpstate) + void __user **fpstate, + void __user *restorer) { unsigned long sp, fx_aligned, math_size; @@ -220,6 +221,10 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs, sp = fpu__alloc_mathframe(sp, 1, &fx_aligned, &math_size); *fpstate = (struct _fpstate_32 __user *) sp; + + if (save_extra_state_to_sigframe(1, *fpstate, restorer)) + return (void __user *)-1L; + if (copy_fpstate_to_sigframe(*fpstate, (void __user *)fx_aligned, math_size) < 0) return (void __user *) -1L; @@ -249,8 +254,6 @@ int ia32_setup_frame(int sig, struct ksignal *ksig, 0x80cd, /* int $0x80 */ }; - frame = get_sigframe(ksig, regs, sizeof(*frame), &fp); - if (ksig->ka.sa.sa_flags & SA_RESTORER) { restorer = ksig->ka.sa.sa_restorer; } else { @@ -262,6 +265,8 @@ int ia32_setup_frame(int sig, struct ksignal *ksig, restorer = &frame->retcode; } + frame = get_sigframe(ksig, regs, sizeof(*frame), &fp, restorer); + if (!user_access_begin(frame, sizeof(*frame))) return -EFAULT; @@ -317,7 +322,13 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig, 0, }; - frame = get_sigframe(ksig, regs, sizeof(*frame), &fp); + if (ksig->ka.sa.sa_flags & SA_RESTORER) + restorer = ksig->ka.sa.sa_restorer; + else + restorer = current->mm->context.vdso + + vdso_image_32.sym___kernel_rt_sigreturn; + + frame = get_sigframe(ksig, regs, sizeof(*frame), &fp, restorer); if (!user_access_begin(frame, sizeof(*frame))) return -EFAULT; @@ -334,11 +345,6 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig, unsafe_put_user(0, &frame->uc.uc_link, Efault); unsafe_compat_save_altstack(&frame->uc.uc_stack, regs->sp, Efault); - if (ksig->ka.sa.sa_flags & SA_RESTORER) - restorer = ksig->ka.sa.sa_restorer; - else - restorer = current->mm->context.vdso + - vdso_image_32.sym___kernel_rt_sigreturn; unsafe_put_user(ptr_to_compat(restorer), &frame->pretcode, Efault); /* diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h index ef6155213b7e..5e66919bd2fe 100644 --- a/arch/x86/include/asm/cet.h +++ b/arch/x86/include/asm/cet.h @@ -6,6 +6,8 @@ #include <linux/types.h> struct task_struct; +struct sc_ext; + /* * Per-thread CET status */ diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index 8d33ad80704f..2a3c6a160dd6 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -443,6 +443,8 @@ static inline void copy_kernel_to_fpregs(union fpregs_state *fpstate) __copy_kernel_to_fpregs(fpstate, -1); } +extern int save_extra_state_to_sigframe(int ia32, void __user *fp, + void __user *restorer); extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size); /* diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h index 844d60eb1882..10d7fa192d48 100644 --- a/arch/x86/include/uapi/asm/sigcontext.h +++ b/arch/x86/include/uapi/asm/sigcontext.h @@ -196,6 +196,15 @@ struct _xstate { /* New processor state extensions go here: */ }; +/* + * Located at the end of sigcontext->fpstate, aligned to 8. + * total_size keeps track of further extension of the struct. + */ +struct sc_ext { + unsigned long total_size; + unsigned long ssp; +}; + /* * The 32-bit signal frame: */ diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index a4ec65317a7f..0488407bec81 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -52,6 +52,107 @@ static inline int check_for_xstate(struct fxregs_state __user *buf, return 0; } +int save_extra_state_to_sigframe(int ia32, void __user *fp, void __user *restorer) +{ + int err = 0; + +#ifdef CONFIG_X86_SHADOW_STACK + struct cet_status *cet = ¤t->thread.cet; + unsigned long token_addr = 0, new_ssp = 0; + struct sc_ext ext = {}; + + if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) + return 0; + + if (cet->shstk_size) { + err = shstk_setup_rstor_token(ia32, (unsigned long)restorer, + &token_addr, &new_ssp); + if (err) + return err; + + ext.ssp = token_addr; + + fpregs_lock(); + if (test_thread_flag(TIF_NEED_FPU_LOAD)) + __fpregs_load_activate(); + if (new_ssp) + err = wrmsrl_safe(MSR_IA32_PL3_SSP, new_ssp); + fpregs_unlock(); + } + + if (!err && ext.ssp) { + void __user *p = fp; + + ext.total_size = sizeof(ext); + + p = fp; + if (ia32) + p += sizeof(struct fregs_state); + + p += fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE; + p = (void __user *)ALIGN((unsigned long)p, 8); + + if (copy_to_user(p, &ext, sizeof(ext))) + return -EFAULT; + } +#endif + return err; +} + +static int get_extra_state_from_sigframe(int ia32, void __user *fp, struct sc_ext *ext) +{ + int err = 0; + +#ifdef CONFIG_X86_SHADOW_STACK + struct cet_status *cet = ¤t->thread.cet; + void __user *p; + + if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) + return 0; + + if (!cet->shstk_size) + return 0; + + memset(ext, 0, sizeof(*ext)); + + p = fp; + if (ia32) + p += sizeof(struct fregs_state); + + p += fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE; + p = (void __user *)ALIGN((unsigned long)p, 8); + + if (copy_from_user(ext, p, sizeof(*ext))) + return -EFAULT; + + if (ext->total_size != sizeof(*ext)) + return -EFAULT; + + if (cet->shstk_size) + err = shstk_check_rstor_token(ia32, ext->ssp, &ext->ssp); +#endif + return err; +} + +/* + * Called from __fpu__restore_sig() and protected by fpregs_lock(). + */ +static int restore_extra_state_to_xregs(struct sc_ext *sc_ext) +{ + int err = 0; + +#ifdef CONFIG_X86_SHADOW_STACK + struct cet_status *cet = ¤t->thread.cet; + + if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) + return 0; + + if (cet->shstk_size) + err = wrmsrl_safe(MSR_IA32_PL3_SSP, sc_ext->ssp); +#endif + return err; +} + /* * Signal frame handlers. */ @@ -295,6 +396,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) struct task_struct *tsk = current; struct fpu *fpu = &tsk->thread.fpu; struct user_i387_ia32_struct env; + struct sc_ext sc_ext; u64 user_xfeatures = 0; int fx_only = 0; int ret = 0; @@ -335,6 +437,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) if ((unsigned long)buf_fx % 64) fx_only = 1; + ret = get_extra_state_from_sigframe(ia32_fxstate, buf, &sc_ext); + if (ret) + return ret; + if (!ia32_fxstate) { /* * Attempt to restore the FPU registers directly from user @@ -365,9 +471,17 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) xfeatures_mask_supervisor()) copy_kernel_to_xregs(&fpu->state.xsave, xfeatures_mask_supervisor()); - fpregs_mark_activate(); + + ret = restore_extra_state_to_xregs(&sc_ext); + if (!ret) { + fpregs_mark_activate(); + } else { + fpregs_deactivate(fpu); + fpu__clear_user_states(fpu); + } + fpregs_unlock(); - return 0; + return ret; } fpregs_unlock(); } else { @@ -430,6 +544,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) ret = copy_kernel_to_xregs_err(&fpu->state.xsave, user_xfeatures | xfeatures_mask_supervisor()); + if (!ret) + ret = restore_extra_state_to_xregs(&sc_ext); } else if (use_fxsr()) { ret = __copy_from_user(&fpu->state.fxsave, buf_fx, state_size); if (ret) { @@ -491,12 +607,29 @@ int fpu__restore_sig(void __user *buf, int ia32_frame) return __fpu__restore_sig(buf, buf_fx, size); } +static unsigned long fpu__alloc_sigcontext_ext(unsigned long sp) +{ +#ifdef CONFIG_X86_SHADOW_STACK + struct cet_status *cet = ¤t->thread.cet; + + /* + * sigcontext_ext is at: fpu + fpu_user_xstate_size + + * FP_XSTATE_MAGIC2_SIZE, then aligned to 8. + */ + if (cet->shstk_size) + sp -= (sizeof(struct sc_ext) + 8); +#endif + return sp; +} + unsigned long fpu__alloc_mathframe(unsigned long sp, int ia32_frame, unsigned long *buf_fx, unsigned long *size) { unsigned long frame_size = xstate_sigframe_size(); + sp = fpu__alloc_sigcontext_ext(sp); + *buf_fx = sp = round_down(sp - frame_size, 64); if (ia32_frame && use_fxsr()) { frame_size += sizeof(struct fregs_state); diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index f306e85a08a6..56d2412174c8 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -239,6 +239,9 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size, unsigned long buf_fx = 0; int onsigstack = on_sig_stack(sp); int ret; +#ifdef CONFIG_X86_64 + void __user *restorer = NULL; +#endif /* redzone */ if (IS_ENABLED(CONFIG_X86_64)) @@ -270,6 +273,12 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size, if (onsigstack && !likely(on_sig_stack(sp))) return (void __user *)-1L; +#ifdef CONFIG_X86_64 + if (ka->sa.sa_flags & SA_RESTORER) + restorer = ka->sa.sa_restorer; + ret = save_extra_state_to_sigframe(0, *fpstate, restorer); +#endif + /* save i387 and extended state */ ret = copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size); if (ret < 0)
When shadow stack is enabled, a task's shadow stack states must be saved along with the signal context and later restored in sigreturn. However, currently there is no systematic facility for extending a signal context. There is some space left in the ucontext, but changing ucontext is likely to create compatibility issues and there is not enough space for further extensions. Introduce a signal context extension struct 'sc_ext', which is used to save shadow stack restore token address. The extension is located above the fpu states, plus alignment. The struct can be extended (such as the ibt's wait_endbr status to be introduced later), and sc_ext.total_size field keeps track of total size. Introduce routines for the allocation, save, and restore for sc_ext: - fpu__alloc_sigcontext_ext(), - save_extra_state_to_sigframe(), - get_extra_state_from_sigframe(), - restore_extra_state_to_xregs(). Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Cc: Kees Cook <keescook@chromium.org> --- v25: - Update commit log/comments for the sc_ext struct. - Use restorer address already calculated. - Change CONFIG_X86_CET to CONFIG_X86_SHADOW_STACK. - Change X86_FEATURE_CET to X86_FEATURE_SHSTK. - Eliminate writing to MSR_IA32_U_CET for shadow stack. - Change wrmsrl() to wrmsrl_safe() and handle error. v24: - Split out shadow stack token routines to a separate patch. - Put signal frame save/restore routines to fpu/signal.c and re-name accordingly. arch/x86/ia32/ia32_signal.c | 24 +++-- arch/x86/include/asm/cet.h | 2 + arch/x86/include/asm/fpu/internal.h | 2 + arch/x86/include/uapi/asm/sigcontext.h | 9 ++ arch/x86/kernel/fpu/signal.c | 137 ++++++++++++++++++++++++- arch/x86/kernel/signal.c | 9 ++ 6 files changed, 172 insertions(+), 11 deletions(-)