@@ -0,0 +1,136 @@+.. SPDX-License-Identifier: GPL-2.0++=========================================+Control-flow Enforcement Technology (CET)+=========================================++[1] Overview+============++Control-flow Enforcement Technology (CET) is an Intel processor feature+that provides protection against return/jump-oriented programming (ROP)+attacks. It can be set up to protect both applications and the kernel.+Only user-mode protection is implemented in the 64-bit kernel, including+support for running legacy 32-bit applications.++CET introduces Shadow Stack and Indirect Branch Tracking. Shadow stack is+a secondary stack allocated from memory and cannot be directly modified by+applications. When executing a CALL instruction, the processor pushes the+return address to both the normal stack and the shadow stack. Upon+function return, the processor pops the shadow stack copy and compares it+to the normal stack copy. If the two differ, the processor raises a+control-protection fault. Indirect branch tracking verifies indirect+CALL/JMP targets are intended as marked by the compiler with 'ENDBR'+opcodes.++There are two Kconfig options:++ X86_SHADOW_STACK, and X86_IBT.++To build a CET-enabled kernel, Binutils v2.31 and GCC v8.1 or LLVM v10.0.1+or later are required. To build a CET-enabled application, GLIBC v2.28 or+later is also required.++There are two command-line options for disabling CET features::++ no_user_shstk - disables user shadow stack, and+ no_user_ibt - disables user indirect branch tracking.++At run time, /proc/cpuinfo shows CET features if the processor supports+CET.++[2] Application Enabling+========================++An application's CET capability is marked in its ELF header and can be+verified from readelf/llvm-readelf output:++ readelf -n <application> | grep -a SHSTK+ properties: x86 feature: IBT, SHSTK++If an application supports CET and is statically linked, it will run with+CET protection. If the application needs any shared libraries, the loader+checks all dependencies and enables CET when all requirements are met.++[3] Backward Compatibility+==========================++GLIBC provides a few CET tunables via the GLIBC_TUNABLES environment+variable:++GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK,-IBT+ Turn off SHSTK/IBT.++GLIBC_TUNABLES=glibc.tune.x86_shstk=<on, permissive>+ This controls how dlopen() handles SHSTK legacy libraries::++ on - continue with SHSTK enabled;+ permissive - continue with SHSTK off.++Details can be found in the GLIBC manual pages.++[4] CET arch_prctl()'s+======================++Several arch_prctl()'s have been added for CET:++arch_prctl(ARCH_X86_CET_STATUS, u64 *addr)+ Return CET feature status.++ The parameter 'addr' is a pointer to a user buffer.+ On returning to the caller, the kernel fills the following+ information::++ *addr = shadow stack/indirect branch tracking status+ *(addr + 1) = shadow stack base address+ *(addr + 2) = shadow stack size++arch_prctl(ARCH_X86_CET_DISABLE, unsigned int features)+ Disable shadow stack and/or indirect branch tracking as specified in+ 'features'. Return -EPERM if CET is locked.++arch_prctl(ARCH_X86_CET_LOCK)+ Lock in all CET features. They cannot be turned off afterwards.++Note:+ There is no CET-enabling arch_prctl function. By design, CET is enabled+ automatically if the binary and the system can support it.++[5] The implementation of the Shadow Stack+==========================================++Shadow Stack size+-----------------++A task's shadow stack is allocated from memory to a fixed size of+MIN(RLIMIT_STACK, 4 GB). In other words, the shadow stack is allocated to+the maximum size of the normal stack, but capped to 4 GB. However,+a compat-mode application's address space is smaller, each of its thread's+shadow stack size is MIN(1/4 RLIMIT_STACK, 4 GB).++Signal+------++The main program and its signal handlers use the same shadow stack.+Because the shadow stack stores only return addresses, a large shadow+stack covers the condition that both the program stack and the signal+alternate stack run out.++The kernel creates a restore token for the shadow stack restoring address+and verifies that token when restoring from the signal handler.++Fork+----++The shadow stack's vma has VM_SHADOW_STACK flag set; its PTEs are required+to be read-only and dirty. When a shadow stack PTE is not RO and dirty, a+shadow access triggers a page fault with the shadow stack access bit set+in the page fault error code.++When a task forks a child, its shadow stack PTEs are copied and both the+parent's and the child's shadow stack PTEs are cleared of the dirty bit.+Upon the next shadow stack access, the resulting shadow stack page fault+is handled by page copy/re-use.++When a pthread child is created, the kernel allocates a new shadow stack+for the new thread.
Shadow Stack provides protection against function return address
corruption. It is active when the processor supports it, the kernel has
CONFIG_X86_SHADOW_STACK enabled, and the application is built for the
feature. This is only implemented for the 64-bit kernel. When it is
enabled, legacy non-Shadow Stack applications continue to work, but without
protection.
Signed-off-by: Yu-cheng Yu <redacted>
Cc: Kees Cook <redacted>
---
v25:
- Remove X86_CET and use X86_SHADOW_STACK directly.
v24:
- Update for the splitting X86_CET to X86_SHADOW_STACK and X86_IBT.
arch/x86/Kconfig | 22 ++++++++++++++++++++++
arch/x86/Kconfig.assembler | 5 +++++
2 files changed, 27 insertions(+)
Control-flow Enforcement Technology (CET) introduces these MSRs:
MSR_IA32_U_CET (user-mode CET settings),
MSR_IA32_PL3_SSP (user-mode shadow stack pointer),
MSR_IA32_PL0_SSP (kernel-mode shadow stack pointer),
MSR_IA32_PL1_SSP (Privilege Level 1 shadow stack pointer),
MSR_IA32_PL2_SSP (Privilege Level 2 shadow stack pointer),
MSR_IA32_S_CET (kernel-mode CET settings),
MSR_IA32_INT_SSP_TAB (exception shadow stack table).
The two user-mode MSRs belong to XFEATURE_CET_USER. The first three of
kernel-mode MSRs belong to XFEATURE_CET_KERNEL. Both XSAVES states are
supervisor states. This means that there is no direct, unprivileged access
to these states, making it harder for an attacker to subvert CET.
For sigreturn and future ptrace() support, shadow stack address and MSR
reserved bits are checked before written to the supervisor states.
Signed-off-by: Yu-cheng Yu <redacted>
Cc: Kees Cook <redacted>
---
v25:
- Update xsave_cpuid_features[]. Now CET XSAVES features depend on
X86_FEATURE_SHSTK (vs. the software-defined X86_FEATURE_CET).
arch/x86/include/asm/fpu/types.h | 23 +++++++++++++++++++++--
arch/x86/include/asm/fpu/xstate.h | 6 ++++--
arch/x86/include/asm/msr-index.h | 19 +++++++++++++++++++
arch/x86/kernel/fpu/xstate.c | 10 +++++++++-
4 files changed, 53 insertions(+), 5 deletions(-)
@@ -35,7 +35,8 @@XFEATURE_MASK_BNDCSR)/* All currently supported supervisor features */-#define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID)+#define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \+XFEATURE_MASK_CET_USER)/**Asupervisorstatecomponentmaynotalwayscontainvaluableinformation,
@@ -62,7 +63,8 @@*Unsupportedsupervisorfeatures.Whenasupervisorfeatureinthismaskis*supportedinthefuture,moveittothesupportedsupervisorfeaturemask.*/-#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT)+#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT | \+XFEATURE_MASK_CET_KERNEL)/* All supervisor states including supported and unsupported states. */#define XFEATURE_MASK_SUPERVISOR_ALL (XFEATURE_MASK_SUPERVISOR_SUPPORTED | \
A control-protection fault is triggered when a control-flow transfer
attempt violates Shadow Stack or Indirect Branch Tracking constraints.
For example, the return address for a RET instruction differs from the copy
on the shadow stack; or an indirect JMP instruction, without the NOTRACK
prefix, arrives at a non-ENDBR opcode.
The control-protection fault handler works in a similar way as the general
protection fault handler. It provides the si_code SEGV_CPERR to the signal
handler.
Signed-off-by: Yu-cheng Yu <redacted>
Cc: Kees Cook <redacted>
Cc: Michael Kerrisk <redacted>
---
v25:
- Change CONFIG_X86_CET to CONFIG_X86_SHADOW_STACK.
- Change X86_FEATURE_CET to X86_FEATURE_SHSTK.
arch/x86/include/asm/idtentry.h | 4 ++
arch/x86/kernel/idt.c | 4 ++
arch/x86/kernel/signal_compat.c | 2 +-
arch/x86/kernel/traps.c | 63 ++++++++++++++++++++++++++++++
include/uapi/asm-generic/siginfo.h | 3 +-
5 files changed, 74 insertions(+), 2 deletions(-)
@@ -571,6 +571,10 @@ DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_SS, exc_stack_segment);DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_GP,exc_general_protection);DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_AC,exc_alignment_check);+#ifdef CONFIG_X86_SHADOW_STACK+DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_CP,exc_control_protection);+#endif+/* Raw exception entries which need extra work */DECLARE_IDTENTRY_RAW(X86_TRAP_UD,exc_invalid_op);DECLARE_IDTENTRY_RAW(X86_TRAP_BP,exc_int3);
@@ -606,6 +607,68 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)cond_local_irq_disable(regs);}+#ifdef CONFIG_X86_SHADOW_STACK+staticconstchar*constcontrol_protection_err[]={+"unknown",+"near-ret",+"far-ret/iret",+"endbranch",+"rstorssp",+"setssbsy",+"unknown",+};++staticDEFINE_RATELIMIT_STATE(cpf_rate,DEFAULT_RATELIMIT_INTERVAL,+DEFAULT_RATELIMIT_BURST);++/*+*Whenacontrolprotectionexceptionoccurs,sendasignaltotheresponsible+*application.Currently,controlprotectionisonlyenabledforusermode.+*Thisexceptionshouldnotcomefromkernelmode.+*/+DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)+{+structtask_struct*tsk;++if(!user_mode(regs)){+pr_emerg("PANIC: unexpected kernel control protection fault\n");+die("kernel control protection fault",regs,error_code);+panic("Machine halted.");+}++cond_local_irq_enable(regs);++if(!boot_cpu_has(X86_FEATURE_SHSTK))+WARN_ONCE(1,"Control protection fault with CET support disabled\n");++tsk=current;+tsk->thread.error_code=error_code;+tsk->thread.trap_nr=X86_TRAP_CP;++/*+*Ratelimittopreventlogspamming.+*/+if(show_unhandled_signals&&unhandled_signal(tsk,SIGSEGV)&&+__ratelimit(&cpf_rate)){+unsignedlongssp;+intcpf_type;++cpf_type=array_index_nospec(error_code,ARRAY_SIZE(control_protection_err));++rdmsrl(MSR_IA32_PL3_SSP,ssp);+pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)",+tsk->comm,task_pid_nr(tsk),+regs->ip,regs->sp,ssp,error_code,+control_protection_err[cpf_type]);+print_vma_addr(KERN_CONT" in ",regs->ip);+pr_cont("\n");+}++force_sig_fault(SIGSEGV,SEGV_CPERR,(void__user*)0);+cond_local_irq_disable(regs);+}+#endif+staticbooldo_int3(structpt_regs*regs){intres;
The x86 family of processors do not directly create read-only and Dirty
PTEs. These PTEs are created by software. One such case is that kernel
read-only pages are historically setup as Dirty.
New processors that support Shadow Stack regard read-only and Dirty PTEs as
shadow stack pages. This results in ambiguity between shadow stack and
kernel read-only pages. To resolve this, removed Dirty from kernel read-
only pages.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kees Cook <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Peter Zijlstra <peterz@infradead.org>
---
arch/x86/include/asm/pgtable_types.h | 6 +++---
arch/x86/mm/pat/set_memory.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -1932,7 +1932,7 @@ int set_memory_nx(unsigned long addr, int numpages)intset_memory_ro(unsignedlongaddr,intnumpages){-returnchange_page_attr_clear(&addr,numpages,__pgprot(_PAGE_RW),0);+returnchange_page_attr_clear(&addr,numpages,__pgprot(_PAGE_RW|_PAGE_DIRTY),0);}intset_memory_rw(unsignedlongaddr,intnumpages)
To prepare the introduction of _PAGE_COW, move pmd_write() and
pud_write() up in the file, so that they can be used by other
helpers below.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
arch/x86/include/asm/pgtable.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
There is essentially no room left in the x86 hardware PTEs on some OSes
(not Linux). That left the hardware architects looking for a way to
represent a new memory type (shadow stack) within the existing bits.
They chose to repurpose a lightly-used state: Write=0, Dirty=1.
The reason it's lightly used is that Dirty=1 is normally set by hardware
and cannot normally be set by hardware on a Write=0 PTE. Software must
normally be involved to create one of these PTEs, so software can simply
opt to not create them.
In places where Linux normally creates Write=0, Dirty=1, it can use the
software-defined _PAGE_COW in place of the hardware _PAGE_DIRTY. In other
words, whenever Linux needs to create Write=0, Dirty=1, it instead creates
Write=0, Cow=1, except for shadow stack, which is Write=0, Dirty=1. This
clearly separates shadow stack from other data, and results in the
following:
(a) A modified, copy-on-write (COW) page: (Write=0, Cow=1)
(b) A R/O page that has been COW'ed: (Write=0, Cow=1)
The user page is in a R/O VMA, and get_user_pages() needs a writable
copy. The page fault handler creates a copy of the page and sets
the new copy's PTE as Write=0 and Cow=1.
(c) A shadow stack PTE: (Write=0, Dirty=1)
(d) A shared shadow stack PTE: (Write=0, Cow=1)
When a shadow stack page is being shared among processes (this happens
at fork()), its PTE is made Dirty=0, so the next shadow stack access
causes a fault, and the page is duplicated and Dirty=1 is set again.
This is the COW equivalent for shadow stack pages, even though it's
copy-on-access rather than copy-on-write.
(e) A page where the processor observed a Write=1 PTE, started a write, set
Dirty=1, but then observed a Write=0 PTE. That's possible today, but
will not happen on processors that support shadow stack.
Define _PAGE_COW and update pte_*() helpers and apply the same changes to
pmd and pud.
After this, there are six free bits left in the 64-bit PTE, and no more
free bits in the 32-bit PTE (except for PAE) and Shadow Stack is not
implemented for the 32-bit kernel.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
v24:
- Replace CONFIG_X86_CET with CONFIG_X86_SHADOW_STACK, to reflect the Kconfig
changes.
arch/x86/include/asm/pgtable.h | 195 ++++++++++++++++++++++++---
arch/x86/include/asm/pgtable_types.h | 42 +++++-
2 files changed, 216 insertions(+), 21 deletions(-)
@@ -23,7 +23,8 @@#define _PAGE_BIT_SOFTW2 10 /* " */#define _PAGE_BIT_SOFTW3 11 /* " */#define _PAGE_BIT_PAT_LARGE 12 /* On 2MB or 1GB pages */-#define _PAGE_BIT_SOFTW4 58 /* available for programmer */+#define _PAGE_BIT_SOFTW4 57 /* available for programmer */+#define _PAGE_BIT_SOFTW5 58 /* available for programmer */#define _PAGE_BIT_PKEY_BIT0 59 /* Protection Keys, bit 1/4 */#define _PAGE_BIT_PKEY_BIT1 60 /* Protection Keys, bit 2/4 */#define _PAGE_BIT_PKEY_BIT2 61 /* Protection Keys, bit 3/4 */
@@ -36,6 +37,15 @@#define _PAGE_BIT_SOFT_DIRTY _PAGE_BIT_SOFTW3 /* software dirty tracking */#define _PAGE_BIT_DEVMAP _PAGE_BIT_SOFTW4+/*+*Indicatesacopy-on-writepage.+*/+#ifdef CONFIG_X86_SHADOW_STACK+#define _PAGE_BIT_COW _PAGE_BIT_SOFTW5 /* copy-on-write */+#else+#define _PAGE_BIT_COW 0+#endif+/* If _PAGE_BIT_PRESENT is clear, we use these: *//* - if the user mapped it with PROT_NONE; pte_present gives true */#define _PAGE_BIT_PROTNONE _PAGE_BIT_GLOBAL
After the introduction of _PAGE_COW, a modified page's PTE can have either
_PAGE_DIRTY or _PAGE_COW. Change _PAGE_DIRTY to _PAGE_DIRTY_BITS.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: David Airlie <redacted>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <redacted>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Zhenyu Wang <redacted>
Cc: Zhi Wang <redacted>
---
drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The read-only and Dirty PTE has been used to indicate copy-on-write pages.
However, newer x86 processors also regard a read-only and Dirty PTE as a
shadow stack page. In order to separate the two, the software-defined
_PAGE_COW is created to replace _PAGE_DIRTY for the copy-on-write case, and
pte_*() are updated.
Pte_modify() changes a PTE to 'newprot', but it doesn't use the pte_*().
Introduce fixup_dirty_pte(), which sets a dirty PTE, based on _PAGE_RW,
to either _PAGE_DIRTY or _PAGE_COW.
Apply the same changes to pmd_modify().
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
arch/x86/include/asm/pgtable.h | 37 ++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
When Shadow Stack is introduced, [R/O + _PAGE_DIRTY] PTE is reserved for
shadow stack. Copy-on-write PTEs have [R/O + _PAGE_COW].
When a PTE goes from [R/W + _PAGE_DIRTY] to [R/O + _PAGE_COW], it could
become a transient shadow stack PTE in two cases:
The first case is that some processors can start a write but end up seeing
a read-only PTE by the time they get to the Dirty bit, creating a transient
shadow stack PTE. However, this will not occur on processors supporting
Shadow Stack, and a TLB flush is not necessary.
The second case is that when _PAGE_DIRTY is replaced with _PAGE_COW non-
atomically, a transient shadow stack PTE can be created as a result.
Thus, prevent that with cmpxchg.
Dave Hansen, Jann Horn, Andy Lutomirski, and Peter Zijlstra provided many
insights to the issue. Jann Horn provided the cmpxchg solution.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
arch/x86/include/asm/pgtable.h | 36 ++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
A shadow stack PTE must be read-only and have _PAGE_DIRTY set. However,
read-only and Dirty PTEs also exist for copy-on-write (COW) pages. These
two cases are handled differently for page faults. Introduce
VM_SHADOW_STACK to track shadow stack VMAs.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: Kees Cook <redacted>
---
v24:
- Change VM_SHSTK to VM_SHADOW_STACK.
- Change CONFIG_X86_CET to CONFIG_X86_SHADOW_STACK to reflect Kconfig changes.
Documentation/filesystems/proc.rst | 1 +
arch/x86/mm/mmap.c | 2 ++
fs/proc/task_mmu.c | 3 +++
include/linux/mm.h | 8 ++++++++
4 files changed, 14 insertions(+)
@@ -549,6 +549,7 @@ encoded manner. The codes are the following: mg mergable advise flag bt arm64 BTI guarded page mt arm64 MTE allocation tags are enabled+ ss shadow stack page == ======================================= Note that there is no guarantee that every flag and associated mnemonic will
Shadow stack accesses are those that are performed by the CPU where it
expects to encounter a shadow stack mapping. These accesses are performed
implicitly by CALL/RET at the site of the shadow stack pointer. These
accesses are made explicitly by shadow stack management instructions like
WRUSSQ.
Shadow stacks accesses to shadow-stack mapping can see faults in normal,
valid operation just like regular accesses to regular mappings. Shadow
stacks need some of the same features like delayed allocation, swap and
copy-on-write.
Shadow stack accesses can also result in errors, such as when a shadow
stack overflows, or if a shadow stack access occurs to a non-shadow-stack
mapping.
In handling a shadow stack page fault, verify it occurs within a shadow
stack mapping. It is always an error otherwise. For valid shadow stack
accesses, set FAULT_FLAG_WRITE to effect copy-on-write. Because clearing
_PAGE_DIRTY (vs. _PAGE_RW) is used to trigger the fault, shadow stack read
fault and shadow stack write fault are not differentiated and both are
handled as a write access.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
v24:
- Change VM_SHSTK to VM_SHADOW_STACK.
arch/x86/include/asm/trap_pf.h | 2 ++
arch/x86/mm/fault.c | 19 +++++++++++++++++++
2 files changed, 21 insertions(+)
When serving a page fault, maybe_mkwrite() makes a PTE writable if it is in
a writable vma. A shadow stack vma is writable, but its PTEs need
_PAGE_DIRTY to be set to become writable. For this reason, maybe_mkwrite()
has been updated.
There are a few places that call pte_mkwrite() directly, but have the
same result as from maybe_mkwrite(). These sites need to be updated for
shadow stack as well. Thus, change them to maybe_mkwrite():
- do_anonymous_page() and migrate_vma_insert_page() check VM_WRITE directly
and call pte_mkwrite(), which is the same as maybe_mkwrite(). Change
them to maybe_mkwrite().
- In do_numa_page(), if the numa entry was writable, then pte_mkwrite()
is called directly. Fix it by doing maybe_mkwrite(). Make the same
changes to do_huge_pmd_numa_page().
- In change_pte_range(), pte_mkwrite() is called directly. Replace it with
maybe_mkwrite().
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: Kees Cook <redacted>
---
v25:
- Apply same changes to do_huge_pmd_numa_page() as to do_numa_page().
mm/huge_memory.c | 2 +-
mm/memory.c | 5 ++---
mm/migrate.c | 3 +--
mm/mprotect.c | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
When serving a page fault, maybe_mkwrite() makes a PTE writable if its vma
has VM_WRITE.
A shadow stack vma has VM_SHADOW_STACK. Its PTEs have _PAGE_DIRTY, but not
_PAGE_WRITE. In fork(), _PAGE_DIRTY is cleared to cause copy-on-write,
and in the page fault handler, _PAGE_DIRTY is restored and the shadow stack
page is writable again.
Introduce an x86 version of maybe_mkwrite(), which sets proper PTE bits
according to VM flags.
Apply the same changes to maybe_pmd_mkwrite().
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: Kees Cook <redacted>
---
v24:
- Instead of doing arch_maybe_mkwrite(), overwrite maybe*_mkwrite() with x86
versions.
- Change VM_SHSTK to VM_SHADOW_STACK.
arch/x86/include/asm/pgtable.h | 6 ++++++
arch/x86/mm/pgtable.c | 20 ++++++++++++++++++++
include/linux/mm.h | 2 ++
mm/huge_memory.c | 2 ++
4 files changed, 30 insertions(+)
INCSSP(Q/D) increments shadow stack pointer and 'pops and discards' the
first and the last elements in the range, effectively touches those memory
areas.
The maximum moving distance by INCSSPQ is 255 * 8 = 2040 bytes and
255 * 4 = 1020 bytes by INCSSPD. Both ranges are far from PAGE_SIZE.
Thus, putting a gap page on both ends of a shadow stack prevents INCSSP,
CALL, and RET from going beyond.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: Kees Cook <redacted>
---
v25:
- Move SHADOW_STACK_GUARD_GAP to arch/x86/mm/mmap.c.
v24:
- Instead changing vm_*_gap(), create x86-specific versions.
arch/x86/include/asm/page_types.h | 7 +++++
arch/x86/mm/mmap.c | 46 +++++++++++++++++++++++++++++++
include/linux/mm.h | 4 +++
3 files changed, 57 insertions(+)
In change_pte_range(), when a PTE is changed for prot_numa, _PAGE_RW is
preserved to avoid the additional write fault after the NUMA hinting fault.
However, pte_write() now includes both normal writable and shadow stack
(RW=0, Dirty=1) PTEs, but the latter does not have _PAGE_RW and has no need
to preserve it.
Exclude shadow stack from preserve_write test, and apply the same change to
change_huge_pmd().
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
---
v25:
- Move is_shadow_stack_mapping() to a separate line.
v24:
- Change arch_shadow_stack_mapping() to is_shadow_stack_mapping().
mm/huge_memory.c | 7 +++++++
mm/mprotect.c | 7 +++++++
2 files changed, 14 insertions(+)
Can_follow_write_pte() ensures a read-only page is COWed by checking the
FOLL_COW flag, and uses pte_dirty() to validate the flag is still valid.
Like a writable data page, a shadow stack page is writable, and becomes
read-only during copy-on-write, but it is always dirty. Thus, in the
can_follow_write_pte() check, it belongs to the writable page case and
should be excluded from the read-only page pte_dirty() check. Apply
the same changes to can_follow_write_pmd().
While at it, also split the long line into smaller ones.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kirill A. Shutemov <redacted>
Cc: Kees Cook <redacted>
---
v26:
- Instead of passing vm_flags, pass down vma pointer to can_follow_write_*().
v25:
- Split long line into smaller ones.
v24:
- Change arch_shadow_stack_mapping() to is_shadow_stack_mapping().
mm/gup.c | 16 ++++++++++++----
mm/huge_memory.c | 16 ++++++++++++----
2 files changed, 24 insertions(+), 8 deletions(-)
@@ -1071,6 +1071,7 @@ unsigned long do_mmap(struct file *file,unsignedlonglen,unsignedlongprot,unsignedlongflags,+vm_flags_tvm_flags,unsignedlongpgoff,unsignedlong*populate,structlist_head*uf)
@@ -1078,7 +1079,6 @@ unsigned long do_mmap(struct file *file,structvm_area_struct*vma;structvm_region*region;structrb_node*rb;-vm_flags_tvm_flags;unsignedlongcapabilities,result;intret;
@@ -1097,7 +1097,7 @@ unsigned long do_mmap(struct file *file,/* we've determined that we can make the mapping, now translate what we*nowknowintoVMAflags*/-vm_flags=determine_vm_flags(file,prot,flags,capabilities);+vm_flags|=determine_vm_flags(file,prot,flags,capabilities);/* we're going to need to record the mapping */region=kmem_cache_zalloc(vm_region_jar,GFP_KERNEL);
@@ -535,6 +536,10 @@ struct thread_struct {unsignedintsig_on_uaccess_err:1;+#ifdef CONFIG_X86_SHADOW_STACK+structcet_statuscet;+#endif+/* Floating point and extended processor state */structfpufpu;/*
A couple of versions ago I said:
" struct shstk_desc shstk;
or so"
but no movement here. That thing is still called cet_status even though
there's nothing status-related with it.
So what's up?
+static unsigned long alloc_shstk(unsigned long size)
+{
+ struct mm_struct *mm = current->mm;
+ unsigned long addr, populate;
+ int flags = MAP_ANONYMOUS | MAP_PRIVATE;
The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
Please fix it up everywhere.
+
+ while (1) {
+ int r;
+
+ r = vm_munmap(cet->shstk_base, cet->shstk_size);
int r = vm_munmap...
+
+ /*
+ * vm_munmap() returns -EINTR when mmap_lock is held by
+ * something else, and that lock should not be held for a
+ * long time. Retry it for the case.
+ */
+ if (r == -EINTR) {
+ cond_resched();
+ continue;
+ }
+ break;
+ }
vm_munmap() can return other negative error values, where are you
handling those?
Same question as before: what guarantees that current doesn't change
from under you here?
One of the worst thing to do is to ignore review comments. I'd strongly
suggest you pay more attention and avoid that in the future.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
A couple of versions ago I said:
" struct shstk_desc shstk;
or so"
but no movement here. That thing is still called cet_status even though
there's nothing status-related with it.
So what's up?
Sorry about that. After that email thread, we went ahead to separate
shadow stack and ibt into different files. I thought about the struct,
the file names cet.h, etc. The struct still needs to include ibt
status, and if it is shstk_desc, the name is not entirely true. One
possible approach is, we don't make it a struct here, and put every item
directly in thread_struct. However, the benefit of putting all in a
struct is understandable (you might argue the opposite :-)). Please
make the call, and I will do the change.
quoted
+static unsigned long alloc_shstk(unsigned long size)
+{
+ struct mm_struct *mm = current->mm;
+ unsigned long addr, populate;
+ int flags = MAP_ANONYMOUS | MAP_PRIVATE;
The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
Please fix it up everywhere.
Yes, the comments are in patch #23: Handle thread shadow stack. I
wanted to add that in the patch that takes the path.
quoted
+
+ while (1) {
+ int r;
+
+ r = vm_munmap(cet->shstk_base, cet->shstk_size);
int r = vm_munmap...
quoted
+
+ /*
+ * vm_munmap() returns -EINTR when mmap_lock is held by
+ * something else, and that lock should not be held for a
+ * long time. Retry it for the case.
+ */
+ if (r == -EINTR) {
+ cond_resched();
+ continue;
+ }
+ break;
+ }
vm_munmap() can return other negative error values, where are you
handling those?
On Wed, Apr 28, 2021 at 11:39:00AM -0700, Yu, Yu-cheng wrote:
Sorry about that. After that email thread, we went ahead to separate shadow
stack and ibt into different files. I thought about the struct, the file
names cet.h, etc. The struct still needs to include ibt status, and if it
is shstk_desc, the name is not entirely true. One possible approach is, we
don't make it a struct here, and put every item directly in thread_struct.
However, the benefit of putting all in a struct is understandable (you might
argue the opposite :-)). Please make the call, and I will do the change.
/me looks forward into the patchset...
So this looks like the final version of it:
@@ -15,6 +15,7 @@ struct cet_status { unsigned long shstk_base; unsigned long shstk_size; unsigned int locked:1;+ unsigned int ibt_enabled:1; };
If so, that thing should be simply:
struct cet {
unsigned long shstk_base;
unsigned long shstk_size;
unsigned int shstk_lock : 1,
ibt : 1;
}
Is that ibt flag per thread or why is it here? I guess I'll find out.
/me greps...
ah yes, it is.
Yes, the comments are in patch #23: Handle thread shadow stack. I wanted to
add that in the patch that takes the path.
That comes next, I'll look there.
quoted
vm_munmap() can return other negative error values, where are you
handling those?
For other error values, the loop stops.
And then what happens?
quoted
quoted
+ cet->shstk_base = 0;
+ cet->shstk_size = 0;
You clear those here without even checking whether unmap failed somehow.
And then stuff leaks but we don't care, right?
Someone else's problem, I'm sure.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Apr 28, 2021 at 11:39:00AM -0700, Yu, Yu-cheng wrote:
quoted
Sorry about that. After that email thread, we went ahead to separate shadow
stack and ibt into different files. I thought about the struct, the file
names cet.h, etc. The struct still needs to include ibt status, and if it
is shstk_desc, the name is not entirely true. One possible approach is, we
don't make it a struct here, and put every item directly in thread_struct.
However, the benefit of putting all in a struct is understandable (you might
argue the opposite :-)). Please make the call, and I will do the change.
/me looks forward into the patchset...
So this looks like the final version of it:
@@ -15,6 +15,7 @@ struct cet_status { unsigned long shstk_base; unsigned long shstk_size; unsigned int locked:1;+ unsigned int ibt_enabled:1; };
If so, that thing should be simply:
struct cet {
unsigned long shstk_base;
unsigned long shstk_size;
unsigned int shstk_lock : 1,
ibt : 1;
}
Is that ibt flag per thread or why is it here? I guess I'll find out.
/me greps...
ah yes, it is.
The lock applies to both shadow stack and ibt. So maybe just "locked"?
quoted
Yes, the comments are in patch #23: Handle thread shadow stack. I wanted to
add that in the patch that takes the path.
That comes next, I'll look there.
quoted
quoted
vm_munmap() can return other negative error values, where are you
handling those?
For other error values, the loop stops.
And then what happens?
quoted
quoted
quoted
+ cet->shstk_base = 0;
+ cet->shstk_size = 0;
You clear those here without even checking whether unmap failed somehow.
And then stuff leaks but we don't care, right?
Someone else's problem, I'm sure.
vm_munmap() returns error as the following:
(1) -EINVAL: address/size/alignment is wrong.
For shadow stack, the kernel keeps track of it, this cannot/should not
happen. Should it happen, it is a bug. The kernel can probably do WARN().
(2) -ENOMEM: when doing __split_vma()/__vma_adjust(), kmem_cache_alloc()
fails.
Not much we can do. Perhaps WARN()?
(3) -EINTR: mmap_write_lock_killable(mm) fails.
This should only happen to a pthread. When a thread is existing, its
siblings are holding mm->mmap_lock. This is handled here.
Right now, in the kernel, only the munmap() syscall returns
__vm_munmap() error code, otherwise the error is not checked. Within
the kernel and if -EINTR is not expected, this makes sense as explained
above.
Thanks for questioning. This piece needs to be correct.
Yu-cheng
On Thu, Apr 29, 2021 at 09:17:06AM -0700, Yu, Yu-cheng wrote:
The lock applies to both shadow stack and ibt. So maybe just "locked"?
Sure.
vm_munmap() returns error as the following:
(1) -EINVAL: address/size/alignment is wrong.
For shadow stack, the kernel keeps track of it, this cannot/should not
happen.
You mean nothing might corrupt
cet->shstk_base
cet->shstk_size
?
I can't count the ways I've heard "should not happen" before and then it
happening anyway.
So probably not but we better catch stuff like that instead of leaking.
Should it happen, it is a bug.
Ack.
The kernel can probably do WARN().
Most definitely WARN. You need to catch funsies like that. But WARN_ONCE
should be enough for now.
(2) -ENOMEM: when doing __split_vma()/__vma_adjust(), kmem_cache_alloc()
fails.
Not much we can do. Perhaps WARN()?
You got it.
Bottom line is: if you can check for this and it is cheap, then
definitely. Code changes, gets rewritten, reorganized, the old
assertions change significance, and so on...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
A shadow stack restore token marks a restore point of the shadow stack, and
the address in a token must point directly above the token, which is within
the same shadow stack. This is distinctively different from other pointers
on the shadow stack, since those pointers point to executable code area.
The restore token can be used as an extra protection for signal handling.
To deliver a signal, create a shadow stack restore token and put the token
and the signal restorer address on the shadow stack. In sigreturn, verify
the token and restore from it the shadow stack pointer.
Introduce token setup and verify routines. Also introduce WRUSS, which is
a kernel-mode instruction but writes directly to user shadow stack. It is
used to construct user signal stack as described above.
Signed-off-by: Yu-cheng Yu <redacted>
Cc: Kees Cook <redacted>
---
v25:
- Update inline assembly syntax, use %[].
- Change token address from (unsigned long) to (u64/u32 __user *).
- Change -EPERM to -EFAULT.
arch/x86/include/asm/cet.h | 9 ++
arch/x86/include/asm/special_insns.h | 32 +++++++
arch/x86/kernel/shstk.c | 126 +++++++++++++++++++++++++++
3 files changed, 167 insertions(+)
Why do you have to look at the second, busy bit, too in order to
determine the mode?
Also, you don't need most of those defines - see below.
+/*
+ * Create a restore token on the shadow stack. A token is always 8-byte
+ * and aligned to 8.
+ */
+static int _create_rstor_token(bool ia32, unsigned long ssp,
+ unsigned long *token_addr)
+{
+ unsigned long addr;
+
+ *token_addr = 0;
What for? Callers should check this function's retval and then interpret
the validity of token_addr and it should not unconditionally write into
it.
+
+ if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4))
Flip this logic:
if ((ia32 && !IS_ALIGNED(ssp, 4)) || !IS_ALIGNED(ssp, 8))
Yah, so this is weird. Why does the restore token need to be at -8
instead on the shadow stack address itself?
Looking at
Figure 18-2. RSTORSSP to Switch to New Shadow Stack
Figure 18-3. SAVEPREVSSP to Save a Restore Point
in the SDM, it looks like unnecessarily more complex than it should be.
But maybe there's some magic I'm missing.
+
+ /* Is the token for 64-bit? */
+ if (!ia32)
+ ssp |= TOKEN_MODE_64;
In both cases, you should write *new_ssp only when write_user_shstk_*
functions have succeeded.
+ }
+ }
+
+ return err;
+}
+
+/*
+ * Verify token_addr point to a valid token, and then set *new_ssp
points
+ * according to the token.
+ */
+int shstk_check_rstor_token(bool ia32, unsigned long token_addr, unsigned long *new_ssp)
+{
+ unsigned long token;
+
+ *new_ssp = 0;
Same as above.
+
+ if (!IS_ALIGNED(token_addr, 8))
+ return -EINVAL;
+
+ if (get_user(token, (unsigned long __user *)token_addr))
+ return -EFAULT;
+
+ /* Is 64-bit mode flag correct? */
+ if (!ia32 && !IS_TOKEN_64(token))
+ return -EINVAL;
+ else if (ia32 && !IS_TOKEN_32(token))
+ return -EINVAL;
That test can be done using the XOR function - i.e., you want to return
an error value when the two things are different.
In order to make this more readable, you call ia32 "proc32" to be clear
what that variable denotes - a 32-bit process. Then, you do
bool shstk32 = !(token & BIT(0));
if (proc32 ^ shstk32)
return -EINVAL;
Voila.
Why do you have to look at the second, busy bit, too in order to
determine the mode?
If the busy bit is set, it is only for SAVEPREVSSP, and invalid as a
normal restore token.
Also, you don't need most of those defines - see below.
quoted
+/*
+ * Create a restore token on the shadow stack. A token is always 8-byte
+ * and aligned to 8.
+ */
+static int _create_rstor_token(bool ia32, unsigned long ssp,
+ unsigned long *token_addr)
+{
+ unsigned long addr;
+
+ *token_addr = 0;
What for? Callers should check this function's retval and then interpret
the validity of token_addr and it should not unconditionally write into
it.
Ok.
quoted
+
+ if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4))
Flip this logic:
if ((ia32 && !IS_ALIGNED(ssp, 4)) || !IS_ALIGNED(ssp, 8))
Yah, so this is weird. Why does the restore token need to be at -8
instead on the shadow stack address itself?
With the lower two bits masked out, the restore token must point
directly above itself.
Looking at
Figure 18-2. RSTORSSP to Switch to New Shadow Stack
Figure 18-3. SAVEPREVSSP to Save a Restore Point
in the SDM, it looks like unnecessarily more complex than it should be.
But maybe there's some magic I'm missing.
quoted
+
+ /* Is the token for 64-bit? */
+ if (!ia32)
+ ssp |= TOKEN_MODE_64;
|= BIT(0);
Ok, then, we don't use #define's. I will put in comments about what it
is doing, and fix the rest.
Thanks,
Yu-cheng
On Tue, May 18, 2021 at 02:14:14AM +0200, Eugene Syromiatnikov wrote:
quoted
Speaking of which, I wonder what would happen if a 64-bit process makes
a 32-bit system call (using int 0x80, for example), and gets a signal.
I guess that's the next patch. And I see amluto has some concerns...
/me goes read.
In the next revision, there will be no "signal context extension"
struct. However, the flow for 64, ia32 and x32 will be similar. I will
send that out after some testing.
Thanks,
Yu-cheng
@@ -235,9 +235,14 @@ static inline void clwb(volatile void *__p)}#ifdef CONFIG_X86_SHADOW_STACK-#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32)staticinlineintwrite_user_shstk_32(u32__user*addr,u32val){+if(WARN_ONCE(!IS_ENABLED(CONFIG_IA32_EMULATION)&&+!IS_ENABLED(CONFIG_X86_X32),+"%s used but not supported.\n",__func__)){+return-EFAULT;+}+asm_volatile_goto("1: wrussd %[val], (%[addr])\n"_ASM_EXTABLE(1b,%l[fail])::[addr]"r"(addr),[val]"r"(val)
@@ -246,13 +251,6 @@ static inline int write_user_shstk_32(u32 __user *addr, u32 val)fail:return-EFAULT;}-#else-staticinlineintwrite_user_shstk_32(u32__user*addr,u32val)-{-WARN_ONCE(1,"%s used but not supported.\n",__func__);-return-EFAULT;-}-#endifstaticinlineintwrite_user_shstk_64(u64__user*addr,u64val){
These are static functions. I thought that would make the static scope
clear. I can remove "_".
No, "_" or "__" prefixed functions are generally supposed to denote
internal interfaces which should not be used by other kernel facilities.
In that case you have the external api <function_name> and the lower
level helpers _<function_name>, __<function_name>, etc. They can be
static but not necessarily.
This is not the case here so you can simply drop the "_" prefixes.
If the busy bit is set, it is only for SAVEPREVSSP, and invalid as a
normal restore token.
Sure but the busy bit is independent from the mode.
With the lower two bits masked out, the restore token must point
directly above itself.
That I know - I'm just questioning the design. It should be
addr = ALIGN_DOWN(ssp, 8);
Plain and simple.
Not this silly pushing and popping of stuff. But it is too late now
anyway and it's not like hw people talk to software people who get to
implement their shit.
Ok, then, we don't use #define's. I will put in comments about what it
is doing, and fix the rest.
On Fri, May 21, 2021 at 09:17:24AM -0700, Yu, Yu-cheng wrote:
If !IS_ALIGNED(ssp, 4), then certainly !IS_ALIGNED(ssp, 8).
... but the reverse is true: when it is aligned by 8, it is already
aligned by 4. Whoops, that's tricky. Pls put a comment over it so that
we don't forget.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
For clone() with CLONE_VM specified, the child and the parent must have
separate shadow stacks. Thus, the kernel allocates, and frees on thread
exit a new shadow stack for the child.
Use stack_size passed from clone3() syscall for thread shadow stack size,
but cap it to min(RLIMIT_STACK, 4 GB). A compat-mode thread shadow stack
size is further reduced to 1/4. This allows more threads to run in a 32-
bit address space.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/cet.h | 5 +++
arch/x86/include/asm/mmu_context.h | 3 ++
arch/x86/kernel/process.c | 15 ++++++--
arch/x86/kernel/shstk.c | 57 +++++++++++++++++++++++++++++-
4 files changed, 76 insertions(+), 4 deletions(-)
@@ -122,8 +124,9 @@ static int set_new_tls(struct task_struct *p, unsigned long tls)returndo_set_thread_area_64(p,ARCH_SET_FS,tls);}-intcopy_thread(unsignedlongclone_flags,unsignedlongsp,unsignedlongarg,-structtask_struct*p,unsignedlongtls)+intcopy_thread(unsignedlongclone_flags,unsignedlongsp,+unsignedlongstack_size,structtask_struct*p,+unsignedlongtls){structinactive_task_frame*frame;structfork_frame*fork_frame;
@@ -163,7 +166,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,/* Kernel thread ? */if(unlikely(p->flags&(PF_KTHREAD|PF_IO_WORKER))){memset(childregs,0,sizeof(structpt_regs));-kthread_frame_init(frame,sp,arg);+kthread_frame_init(frame,sp,stack_size);return0;}
@@ -181,6 +184,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,if(clone_flags&CLONE_SETTLS)ret=set_new_tls(p,tls);+#ifdef CONFIG_X86_64+/* Allocate a new shadow stack for pthread */+if(!ret)+ret=shstk_setup_thread(p,clone_flags,stack_size);+#endif+if(!ret&&unlikely(test_tsk_thread_flag(current,TIF_IO_BITMAP)))io_bitmap_share(p);
@@ -70,6 +70,55 @@ int shstk_setup(void)return0;}+intshstk_setup_thread(structtask_struct*tsk,unsignedlongclone_flags,+unsignedlongstack_size)+{+unsignedlongaddr,size;+structcet_user_state*state;+structcet_status*cet=&tsk->thread.cet;++if(!cet->shstk_size)+return0;++if((clone_flags&(CLONE_VFORK|CLONE_VM))!=CLONE_VM)+return0;++state=get_xsave_addr(&tsk->thread.fpu.state.xsave,+XFEATURE_CET_USER);++if(!state)+return-EINVAL;++if(stack_size==0)+return-EINVAL;++/* Cap shadow stack size to 4 GB */+size=min_t(unsignedlonglong,rlimit(RLIMIT_STACK),SZ_4G);+size=min(size,stack_size);++/*+*Compat-modepthreadssharealimitedaddressspace.+*Ifeachfunctioncalltakesanaverageoffourslots+*stackspace,allocate1/4ofstacksizeforshadowstack.+*/+if(in_compat_syscall())+size/=4;+size=round_up(size,PAGE_SIZE);+addr=alloc_shstk(size);++if(IS_ERR_VALUE(addr)){+cet->shstk_base=0;+cet->shstk_size=0;+returnPTR_ERR((void*)addr);+}++fpu__prepare_write(&tsk->thread.fpu);+state->user_ssp=(u64)(addr+size);+cet->shstk_base=addr;+cet->shstk_size=size;+return0;+}+voidshstk_free(structtask_struct*tsk){structcet_status*cet=&tsk->thread.cet;
On Tue, Apr 27, 2021 at 01:43:08PM -0700, Yu-cheng Yu wrote:
quoted hunk
@@ -181,6 +184,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, if (clone_flags & CLONE_SETTLS) ret = set_new_tls(p, tls);+#ifdef CONFIG_X86_64
IS_ENABLED
+ /* Allocate a new shadow stack for pthread */
+ if (!ret)
+ ret = shstk_setup_thread(p, clone_flags, stack_size);
+#endif
+
And why is this addition here...
if (!ret && unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
io_bitmap_share(p);
+int shstk_setup_thread(struct task_struct *tsk, unsigned long clone_flags,
Judging by what this function does, its name wants to be
shstk_alloc_thread_stack()
or so?
+ unsigned long stack_size)
+{
+ unsigned long addr, size;
+ struct cet_user_state *state;
+ struct cet_status *cet = &tsk->thread.cet;
The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
+
+ if (!cet->shstk_size)
+ return 0;
+
This check needs a comment.
+ if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM)
+ return 0;
+
+ state = get_xsave_addr(&tsk->thread.fpu.state.xsave,
+ XFEATURE_CET_USER);
Let that line stick out.
+
+ if (!state)
+ return -EINVAL;
+
+ if (stack_size == 0)
if (!stack_size)
+ return -EINVAL;
and that test needs to be done first in the function.
+
+ /* Cap shadow stack size to 4 GB */
Why?
+ size = min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G);
+ size = min(size, stack_size);
+
+ /*
+ * Compat-mode pthreads share a limited address space.
+ * If each function call takes an average of four slots
+ * stack space, allocate 1/4 of stack size for shadow stack.
+ */
+ if (in_compat_syscall())
+ size /= 4;
cet_user_state has u64, cet_status has unsigned longs. Make them all u64.
And since cet_status is per thread, but I had suggested struct
shstk_desc, I think now that that should be called
struct thread_shstk
or so to denote *exactly* what it is.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
+int shstk_setup_thread(struct task_struct *tsk, unsigned long clone_flags,
Judging by what this function does, its name wants to be
shstk_alloc_thread_stack()
or so?
quoted
+ unsigned long stack_size)
+{
+ unsigned long addr, size;
+ struct cet_user_state *state;
+ struct cet_status *cet = &tsk->thread.cet;
The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
quoted
+
+ if (!cet->shstk_size)
+ return 0;
+
This check needs a comment.
quoted
+ if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM)
+ return 0;
+
+ state = get_xsave_addr(&tsk->thread.fpu.state.xsave,
+ XFEATURE_CET_USER);
Let that line stick out.
quoted
+
+ if (!state)
+ return -EINVAL;
+
+ if (stack_size == 0)
if (!stack_size)
quoted
+ return -EINVAL;
and that test needs to be done first in the function.
quoted
+
+ /* Cap shadow stack size to 4 GB */
Why?
This is not necessary. I will make it just stack_size, which is passed
in from copy_thread().
quoted
+ size = min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G);
+ size = min(size, stack_size);
+
+ /*
+ * Compat-mode pthreads share a limited address space.
+ * If each function call takes an average of four slots
+ * stack space, allocate 1/4 of stack size for shadow stack.
+ */
+ if (in_compat_syscall())
+ size /= 4;
cet_user_state has u64, cet_status has unsigned longs. Make them all u64.
And since cet_status is per thread, but I had suggested struct
shstk_desc, I think now that that should be called
struct thread_shstk
or so to denote *exactly* what it is.
So this struct will be:
struct thread_shstk {
u64 shstk_base;
u64 shstk_size;
u64 locked:1;
u64 ibt:1;
};
Ok?
Thanks,
Yu-cheng
On Mon, May 10, 2021 at 03:57:56PM -0700, Yu, Yu-cheng wrote:
So this struct will be:
struct thread_shstk {
u64 shstk_base;
u64 shstk_size;
u64 locked:1;
u64 ibt:1;
};
Ok?
Pretty much.
You can even remove the "shstk_" from the members and when you call the
pointer "shstk", accessing the members will read
shstk->base
shstk->size
...
and all is organic and readable :)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: David Laight <hidden> Date: 2021-05-12 08:12:39
From: Borislav Petkov
Sent: 11 May 2021 18:10
On Mon, May 10, 2021 at 03:57:56PM -0700, Yu, Yu-cheng wrote:
quoted
So this struct will be:
struct thread_shstk {
u64 shstk_base;
u64 shstk_size;
u64 locked:1;
u64 ibt:1;
No point in bit fields?
quoted
};
Ok?
Pretty much.
You can even remove the "shstk_" from the members and when you call the
pointer "shstk", accessing the members will read
shstk->base
shstk->size
...
and all is organic and readable :)
And entirely not greppable.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Apr 27, 2021 at 01:43:08PM -0700, Yu-cheng Yu wrote:
quoted
@@ -181,6 +184,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, if (clone_flags & CLONE_SETTLS) ret = set_new_tls(p, tls);+#ifdef CONFIG_X86_64
IS_ENABLED
quoted
+ /* Allocate a new shadow stack for pthread */
+ if (!ret)
+ ret = shstk_setup_thread(p, clone_flags, stack_size);
+#endif
+
And why is this addition here...
quoted
if (!ret && unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
io_bitmap_share(p);
... instead of here?
<---
io_bitmap_share() does refcount_inc(¤t->thread.io_bitmap->refcnt),
and the function won't fail. However, shadow stack allocation can fail.
So, maybe leave io_bitmap_share() at the end?
Thanks,
Yu-cheng
On Tue, May 11, 2021 at 11:35:03AM -0700, Yu, Yu-cheng wrote:
io_bitmap_share() does refcount_inc(¤t->thread.io_bitmap->refcnt), and
the function won't fail. However, shadow stack allocation can fail. So,
maybe leave io_bitmap_share() at the end?
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 <redacted>
Cc: Kees Cook <redacted>
---
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(-)
@@ -196,6 +196,15 @@ struct _xstate {/* New processor state extensions go here: */};+/*+*Locatedattheendofsigcontext->fpstate,alignedto8.+*total_sizekeepstrackoffurtherextensionofthestruct.+*/+structsc_ext{+unsignedlongtotal_size;+unsignedlongssp;+};+/**The32-bitsignalframe:*/
From: Andy Lutomirski <luto@kernel.org> Date: 2021-04-28 23:04:18
On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu [off-list ref] 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 Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu [off-list ref] wrote:
quoted
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 [off-list ref] wrote:
quoted
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.
From: Andy Lutomirski <luto@kernel.org> Date: 2021-04-29 14:44:23
On Thu, Apr 29, 2021 at 12:28 AM Cyrill Gorcunov [off-list ref] wrote:
On Wed, Apr 28, 2021 at 04:03:55PM -0700, Andy Lutomirski wrote:
quoted
On Tue, Apr 27, 2021 at 1:44 PM Yu-cheng Yu [off-list ref] wrote:
quoted
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