A control protection exception is triggered when a control flow transfer
attempt violated shadow stack or indirect branch tracking constraints.
For example, the return address for a RET instruction differs from the
safe copy on the shadow stack; or a JMP instruction arrives at a non-
ENDBR instruction.
The control protection exception handler works in a similar way as the
general protection fault handler.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/entry/entry_64.S | 2 +-
arch/x86/include/asm/traps.h | 3 ++
arch/x86/kernel/idt.c | 4 +++
arch/x86/kernel/traps.c | 58 ++++++++++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 1 deletion(-)
@@ -578,6 +578,64 @@ do_general_protection(struct pt_regs *regs, long error_code)}NOKPROBE_SYMBOL(do_general_protection);+staticconstchar*control_protection_err[]=+{+"unknown",+"near-ret",+"far-ret/iret",+"endbranch",+"rstorssp",+"setssbsy",+};++/*+*Whenacontrolprotectionexceptionoccurs,sendasignal+*totheresponsibleapplication.Currently,control+*protectionisonlyenabledfortheusermode.This+*exceptionshouldnotcomefromthekernelmode.+*/+dotraplinkagevoid+do_control_protection(structpt_regs*regs,longerror_code)+{+structtask_struct*tsk;++RCU_LOCKDEP_WARN(!rcu_is_watching(),"entry code didn't wake RCU");+if(notify_die(DIE_TRAP,"control protection fault",regs,+error_code,X86_TRAP_CP,SIGSEGV)==NOTIFY_STOP)+return;+cond_local_irq_enable(regs);++if(!user_mode(regs))+die("kernel control protection fault",regs,error_code);++if(!static_cpu_has(X86_FEATURE_SHSTK)&&+!static_cpu_has(X86_FEATURE_IBT))+WARN_ONCE(1,"CET is disabled but got control "+"protection fault\n");++tsk=current;+tsk->thread.error_code=error_code;+tsk->thread.trap_nr=X86_TRAP_CP;++if(show_unhandled_signals&&unhandled_signal(tsk,SIGSEGV)&&+printk_ratelimit()){+unsignedintmax_err;++max_err=ARRAY_SIZE(control_protection_err)-1;+if((error_code<0)||(error_code>max_err))+error_code=0;+pr_info("%s[%d] control protection ip:%lx sp:%lx error:%lx(%s)",+tsk->comm,task_pid_nr(tsk),+regs->ip,regs->sp,error_code,+control_protection_err[error_code]);+print_vma_addr(KERN_CONT" in ",regs->ip);+pr_cont("\n");+}++force_sig_info(SIGSEGV,SEND_SIG_PRIV,tsk);+}+NOKPROBE_SYMBOL(do_control_protection);+dotraplinkagevoidnotracedo_int3(structpt_regs*regs,longerror_code){#ifdef CONFIG_DYNAMIC_FTRACE
The shadow stack for clone/fork is handled as the following:
(1) If ((clone_flags & (CLONE_VFORK | CLONE_VM)) == CLONE_VM),
the kernel allocates (and frees on thread exit) a new SHSTK
for the child.
It is possible for the kernel to complete the clone syscall
and set the child's SHSTK pointer to NULL and let the child
thread allocate a SHSTK for itself. There are two issues
in this approach: It is not compatible with existing code
that does inline syscall and it cannot handle signals before
the child can successfully allocate a SHSTK.
(2) For (clone_flags & CLONE_VFORK), the child uses the existing
SHSTK.
(3) For all other cases, the SHSTK is copied/reused whenever the
parent or the child does a call/ret.
This patch handles cases (1) & (2). Case (3) is handled in
the SHSTK page fault patches.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/cet.h | 2 ++
arch/x86/include/asm/mmu_context.h | 3 +++
arch/x86/kernel/cet.c | 34 ++++++++++++++++++++++++++++++
arch/x86/kernel/process.c | 1 +
arch/x86/kernel/process_64.c | 7 ++++++
5 files changed, 47 insertions(+)
@@ -325,6 +325,13 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,if(sp)childregs->sp=sp;+/* Allocate a new shadow stack for pthread */+if((clone_flags&(CLONE_VFORK|CLONE_VM))==CLONE_VM){+err=cet_setup_thread_shstk(p);+if(err)+gotoout;+}+err=-ENOMEM;if(unlikely(test_tsk_thread_flag(me,TIF_IO_BITMAP))){p->thread.io_bitmap_ptr=kmemdup(me->thread.io_bitmap_ptr,
WRUSS is a new kernel-mode instruction but writes directly
to user shadow stack memory. This is used to construct
a return address on the shadow stack for the signal
handler.
This instruction can fault if the user shadow stack is
invalid shadow stack memory. In that case, the kernel does
fixup.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/special_insns.h | 32 ++++++++++++++++++++++++++++
arch/x86/mm/fault.c | 9 ++++++++
2 files changed, 41 insertions(+)
When a task does fork(), its shadow stack must be duplicated for
the child. However, the child may not actually use all pages of
of the copied shadow stack. This patch implements a flow that
is similar to copy-on-write of an anonymous page, but for shadow
stack memory. A shadow stack PTE needs to be RO and dirty. We
use this dirty bit requirement to effect the copying of shadow
stack pages.
In copy_one_pte(), we clear the dirty bit from the shadow stack
PTE. On the next shadow stack access to the PTE, a page fault
occurs. At that time, we then copy/re-use the page and fix the
PTE.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/mm/pgtable.c | 15 +++++++++++++++
include/asm-generic/pgtable.h | 8 ++++++++
mm/memory.c | 7 ++++++-
3 files changed, 29 insertions(+), 1 deletion(-)
XSAVES saves both system and user states. The Linux kernel
currently does not save/restore any system states. This patch
creates the framework for supporting system states.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/fpu/internal.h | 3 +-
arch/x86/include/asm/fpu/xstate.h | 9 ++-
arch/x86/kernel/fpu/core.c | 7 +-
arch/x86/kernel/fpu/init.c | 10 ---
arch/x86/kernel/fpu/xstate.c | 112 +++++++++++++++++-----------
5 files changed, 80 insertions(+), 61 deletions(-)
@@ -19,10 +19,10 @@#define XSAVE_YMM_SIZE 256#define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)-/* System features */-#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)--/* All currently supported features */+/*+*SUPPORTED_XFEATURES_MASKindicatesallfeatures+*implementedinandsupportedbythekernel.+*/#define SUPPORTED_XFEATURES_MASK (XFEATURE_MASK_FP | \XFEATURE_MASK_SSE|\XFEATURE_MASK_YMM|\
@@ -219,30 +222,31 @@ void fpstate_sanitize_xstate(struct fpu *fpu)*/voidfpu__init_cpu_xstate(void){-if(!boot_cpu_has(X86_FEATURE_XSAVE)||!xfeatures_mask_user)+if(!boot_cpu_has(X86_FEATURE_XSAVE)||!xfeatures_mask_all)return;++cr4_set_bits(X86_CR4_OSXSAVE);+/*-*MakeitclearthatXSAVESsystemstatesarenotyet-*implementedshouldanyoneexpectittoworkbychanging-*bitsinXFEATURE_MASK_*macrosandXCR0.+*XCR_XFEATURE_ENABLED_MASKsetsthefeaturesthataremanaged+*byXSAVE{C,OPT}andXRSTOR.OnlyXSAVEuserstatescanbe+*sethere.*/-WARN_ONCE((xfeatures_mask_user&XFEATURE_MASK_SYSTEM),-"x86/fpu: XSAVES system states are not yet implemented.\n");+xsetbv(XCR_XFEATURE_ENABLED_MASK,+xfeatures_mask_user);-xfeatures_mask_user&=~XFEATURE_MASK_SYSTEM;--cr4_set_bits(X86_CR4_OSXSAVE);-xsetbv(XCR_XFEATURE_ENABLED_MASK,xfeatures_mask_user);+/*+*MSR_IA32_XSSsetswhichXSAVESsystemstatestobemanagedby+*XSAVES.OnlyXSAVESsystemstatescanbesethere.+*/+if(boot_cpu_has(X86_FEATURE_XSAVES))+wrmsrl(MSR_IA32_XSS,+xfeatures_mask_all&~xfeatures_mask_user);}-/*-*Notethatinthefuturewewilllikelyneedapairof-*functionshere:oneforuserxstatesandtheotherfor-*systemxstates.Fornow,theyarethesame.-*/staticintxfeature_enabled(enumxfeaturexfeature){-return!!(xfeatures_mask_user&BIT_ULL(xfeature));+return!!(xfeatures_mask_all&BIT_ULL(xfeature));}/*
@@ -348,7 +352,7 @@ static int xfeature_is_aligned(int xfeature_nr)*/staticvoid__initsetup_xstate_comp(void){-unsignedintxstate_comp_sizes[sizeof(xfeatures_mask_user)*8];+unsignedintxstate_comp_sizes[sizeof(xfeatures_mask_all)*8];inti;/*
@@ -441,11 +445,10 @@ static int xfeature_uncompacted_offset(int xfeature_nr)u32eax,ebx,ecx,edx;/*-*OnlyXSAVESsupportssystemstatesanditusescompacted-*format.Checkingasystemstate'suncompactedoffsetis-*anerror.+*Checkingasystemorunsupportedstate'suncompactedoffset+*isanerror.*/-if(XFEATURE_MASK_SYSTEM&(1<<xfeature_nr)){+if(~xfeatures_mask_user&BIT_ULL(xfeature_nr)){WARN_ONCE(1,"No fixed offset for xstate %d\n",xfeature_nr);return-1;}
@@ -482,7 +485,7 @@ int using_compacted_format(void)intvalidate_xstate_header(conststructxstate_header*hdr){/* No unknown or system features may be set */-if(hdr->xfeatures&(~xfeatures_mask_user|XFEATURE_MASK_SYSTEM))+if(hdr->xfeatures&~xfeatures_mask_user)return-EINVAL;/* Userspace must use the uncompacted format */
@@ -760,10 +777,11 @@ void __init fpu__init_system_xstate(void)*/for(i=0;i<ARRAY_SIZE(xsave_cpuid_features);i++){if(!boot_cpu_has(xsave_cpuid_features[i]))-xfeatures_mask_user&=~BIT_ULL(i);+xfeatures_mask_all&=~BIT_ULL(i);}-xfeatures_mask_user&=fpu__get_supported_xfeatures_mask();+xfeatures_mask_all&=SUPPORTED_XFEATURES_MASK;+xfeatures_mask_user=xfeatures_mask_all&cpu_user_xfeatures_mask;/* Enable xstate instructions to be able to continue with initialization: */fpu__init_cpu_xstate();
If a page fault is triggered by a shadow stack access (e.g.
call/ret) or shadow stack management instructions (e.g.
wrussq), then bit[6] of the page fault error code is set.
In access_error(), we check if a shadow stack page fault
is within a shadow stack memory area.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/traps.h | 2 ++
arch/x86/mm/fault.c | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
When Shadow Stack is enabled, the [R/O + PAGE_DIRTY_HW] setting is
reserved only for the Shadow Stack. For non-Shadow Stack R/O PTEs,
we use [R/O + PAGE_DIRTY_SW].
When a PTE goes from [R/W + PAGE_DIRTY_HW] to [R/O + PAGE_DIRTY_SW],
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 therefore we don't need a TLB
flush here.
The second case is that when the software, without atomic, tests &
replaces PAGE_DIRTY_HW with PAGE_DIRTY_SW, a transient Shadow Stack
PTE can exist. This is prevented 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>
---
arch/x86/include/asm/pgtable.h | 58 ++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
There are a few places that need do_mmap() with mm->mmap_sem held.
Create an in-line function for that.
Signed-off-by: Yu-cheng Yu <redacted>
---
include/linux/mm.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
@@ -2318,6 +2318,24 @@ static inline void mm_populate(unsigned long addr, unsigned long len)staticinlinevoidmm_populate(unsignedlongaddr,unsignedlonglen){}#endif+staticinlineunsignedlongdo_mmap_locked(unsignedlongaddr,+unsignedlonglen,unsignedlongprot,unsignedlongflags,+vm_flags_tvm_flags)+{+structmm_struct*mm=current->mm;+unsignedlongpopulate;++down_write(&mm->mmap_sem);+addr=do_mmap(NULL,addr,len,prot,flags,vm_flags,0,+&populate,NULL);+up_write(&mm->mmap_sem);++if(populate)+mm_populate(addr,populate);++returnaddr;+}+/* These take the mm semaphore themselves */externint__must_checkvm_brk(unsignedlong,unsignedlong);externint__must_checkvm_brk_flags(unsignedlong,unsignedlong,unsignedlong);
can_follow_write_pte/pmd look for the (RO & DIRTY) PTE/PMD to
verify an exclusive RO page still exists after a broken COW.
A shadow stack PTE is RO & PAGE_DIRTY_SW when it is shared,
otherwise RO & PAGE_DIRTY_HW.
Introduce pte_exclusive() and pmd_exclusive() to also verify a
shadow stack PTE is exclusive.
Also rename can_follow_write_pte/pmd() to can_follow_write() to
make their meaning clear; i.e. "Can we write to the page?", not
"Is the PTE writable?"
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/mm/pgtable.c | 19 +++++++++++++++++++
include/asm-generic/pgtable.h | 4 ++++
mm/gup.c | 8 +++++---
mm/huge_memory.c | 8 +++++---
4 files changed, 33 insertions(+), 6 deletions(-)
When setting up a signal, the kernel creates a shadow stack
restore token at the current SHSTK address and then stores the
token's address in the signal frame, right after the FPU state.
Before restoring a signal, the kernel verifies and then uses the
restore token to set the SHSTK pointer.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/ia32/ia32_signal.c | 13 +++
arch/x86/include/asm/cet.h | 5 ++
arch/x86/include/asm/sighandling.h | 5 ++
arch/x86/include/uapi/asm/sigcontext.h | 17 ++++
arch/x86/kernel/cet.c | 115 +++++++++++++++++++++++++
arch/x86/kernel/signal.c | 96 +++++++++++++++++++++
6 files changed, 251 insertions(+)
@@ -196,6 +196,23 @@ struct _xstate {/* New processor state extensions go here: */};+#ifdef __x86_64__+/*+*Sigcontextextension(structsc_ext)islocatedafter+*sigcontext->fpstate.Becausecurrentlyonlytheshadow+*stackpointerissavedthereandtheshadowstackdepends+*onXSAVES,wecanfindsc_extfromsigcontext->fpstate.+*+*The64-bitfpstatehasasizeoffpu_user_xstate_size,plus+*FP_XSTATE_MAGIC2_SIZEwhenXSAVE*isused.Thestructsc_ext+*islocatedattheendofsigcontext->fpstate,alignedto8.+*/+structsc_ext{+unsignedlongtotal_size;+unsignedlongssp;+};+#endif+/**The32-bitsignalframe:*/
@@ -46,6 +47,69 @@ static unsigned long get_shstk_addr(void)returnptr;}+/*+*Verifytherestoretokenattheaddressof'ssp'is+*validandthensetshadowstackpointeraccordingtothe+*token.+*/+staticintverify_rstor_token(boolia32,unsignedlongssp,+unsignedlong*new_ssp)+{+unsignedlongtoken;++*new_ssp=0;++if(!IS_ALIGNED(ssp,8))+return-EINVAL;++if(get_user(token,(unsignedlong__user*)ssp))+return-EFAULT;++/* Is 64-bit mode flag correct? */+if(ia32&&(token&3)!=0)+return-EINVAL;+elseif((token&3)!=1)+return-EINVAL;++token&=~(1UL);++if((!ia32&&!IS_ALIGNED(token,8))||!IS_ALIGNED(token,4))+return-EINVAL;++if((ALIGN_DOWN(token,8)-8)!=ssp)+return-EINVAL;++*new_ssp=token;+return0;+}++/*+*Createarestoretokenontheshadowstack.+*Atokenisalways8-byteandalignedto8.+*/+staticintcreate_rstor_token(boolia32,unsignedlongssp,+unsignedlong*new_ssp)+{+unsignedlongaddr;++*new_ssp=0;++if((!ia32&&!IS_ALIGNED(ssp,8))||!IS_ALIGNED(ssp,4))+return-EINVAL;++addr=ALIGN_DOWN(ssp,8)-8;++/* Is the token for 64-bit? */+if(!ia32)+ssp|=1;++if(write_user_shstk_64(addr,ssp))+return-EFAULT;++*new_ssp=addr;+return0;+}+intcet_setup_shstk(void){unsignedlongaddr,size;
Update _PAGE_DIRTY to _PAGE_DIRTY_BITS in split_2MB_gtt_entry().
In order to support Control Flow Enforcement (CET), _PAGE_DIRTY
is now _PAGE_DIRTY_HW or _PAGE_DIRTY_SW.
Signed-off-by: Yu-cheng Yu <redacted>
---
drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
We are going to create _PAGE_DIRTY_SW for non-hardware, memory
management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
_PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
bits more clear. There are no functional changes in this
patch.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/pgtable.h | 6 +++---
arch/x86/include/asm/pgtable_types.h | 17 +++++++++--------
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kvm/vmx.c | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
@@ -5848,7 +5848,7 @@ static int init_rmode_identity_map(struct kvm *kvm)/* Set up identity-mapping pagetable for EPT in real mode */for(i=0;i<PT32_ENT_PER_PAGE;i++){tmp=(i<<22)+(_PAGE_PRESENT|_PAGE_RW|_PAGE_USER|-_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_PSE);+_PAGE_ACCESSED|_PAGE_DIRTY_HW|_PAGE_PSE);r=kvm_write_guest_page(kvm,identity_map_pfn,&tmp,i*sizeof(tmp),sizeof(tmp));if(r<0)
arch_prctl(ARCH_CET_STATUS, unsigned long *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 = SHSTK/IBT status
*(addr + 1) = SHSTK base address
*(addr + 2) = SHSTK size
arch_prctl(ARCH_CET_DISABLE, unsigned long features)
Disable CET features specified in 'features'. Return
-EPERM if CET is locked.
arch_prctl(ARCH_CET_LOCK)
Lock in CET feature.
arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)
Allocate a new SHSTK.
The parameter 'addr' is a pointer to a user buffer and indicates
the desired SHSTK size to allocate. On returning to the caller
the buffer contains the address of the new SHSTK.
Signed-off-by: H.J. Lu <redacted>
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/cet.h | 5 ++
arch/x86/include/uapi/asm/prctl.h | 5 ++
arch/x86/kernel/Makefile | 2 +-
arch/x86/kernel/cet.c | 27 +++++++++++
arch/x86/kernel/cet_prctl.c | 79 +++++++++++++++++++++++++++++++
arch/x86/kernel/process.c | 5 ++
6 files changed, 122 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/cet_prctl.c
@@ -110,6 +110,33 @@ static int create_rstor_token(bool ia32, unsigned long ssp,return0;}+intcet_alloc_shstk(unsignedlong*arg)+{+unsignedlonglen=*arg;+unsignedlongaddr;+unsignedlongtoken;+unsignedlongssp;++addr=do_mmap_locked(0,len,PROT_READ,+MAP_ANONYMOUS|MAP_PRIVATE,VM_SHSTK);+if(addr>=TASK_SIZE_MAX)+return-ENOMEM;++/* Restore token is 8 bytes and aligned to 8 bytes */+ssp=addr+len;+token=ssp;++if(!in_ia32_syscall())+token|=1;+ssp-=8;++if(write_user_shstk_64(ssp,token))+return-EINVAL;++*arg=addr;+return0;+}+intcet_setup_shstk(void){unsignedlongaddr,size;
@@ -792,6 +792,11 @@ long do_arch_prctl_common(struct task_struct *task, int option,returnget_cpuid_mode();caseARCH_SET_CPUID:returnset_cpuid_mode(task,cpuid_enabled);+caseARCH_CET_STATUS:+caseARCH_CET_DISABLE:+caseARCH_CET_LOCK:+caseARCH_CET_ALLOC_SHSTK:+returnprctl_cet(option,cpuid_enabled);}return-EINVAL;
Add shadow stack pages to memory accounting.
Also check if the system has enough memory before enabling CET.
Signed-off-by: Yu-cheng Yu <yu-cheng.yu.intel.com>
---
mm/mmap.c | 5 +++++
1 file changed, 5 insertions(+)
Function returns could unwind stacks beyond its allocated area.
We do not merge shadow stack areas.
This and VMA guards prevent shadow stack underflow.
Signed-off-by: Yu-cheng Yu <redacted>
---
mm/mmap.c | 6 ++++++
1 file changed, 6 insertions(+)
Add the following shadow stack management instructions.
INCSSP:
Increment shadow stack pointer by the steps specified.
RDSSP:
Read SSP register into a GPR.
SAVEPREVSSP:
Use "prev ssp" token at top of current shadow stack to
create a "restore token" on previous shadow stack.
RSTORSSP:
Restore from a "restore token" pointed by a GPR to SSP.
WRSS:
Write to kernel-mode shadow stack (kernel-mode instruction).
WRUSS:
Write to user-mode shadow stack (kernel-mode instruction).
SETSSBSY:
Verify the "supervisor token" pointed by IA32_PL0_SSP MSR,
if valid, set the token to busy, and set SSP to the value
of IA32_PL0_SSP MSR.
CLRSSBSY:
Verify the "supervisor token" pointed by a GPR, if valid,
clear the busy bit from the token.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/lib/x86-opcode-map.txt | 26 +++++++++++++------
tools/objtool/arch/x86/lib/x86-opcode-map.txt | 26 +++++++++++++------
2 files changed, 36 insertions(+), 16 deletions(-)
Introduce Kconfig option X86_INTEL_SHADOW_STACK_USER.
An application has shadow stack protection when all the following are
true:
(1) The kernel has X86_INTEL_SHADOW_STACK_USER enabled,
(2) The running processor supports the shadow stack,
(3) The application is built with shadow stack enabled tools & libs
and, and at runtime, all dependent shared libs can support shadow
stack.
If this kernel config option is enabled, but (2) or (3) above is not
true, the application runs without the shadow stack protection.
Existing legacy applications will continue to work without the shadow
stack protection.
The user-mode shadow stack protection is only implemented for the
64-bit kernel. Thirty-two bit applications are supported under the
compatibility mode.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/Kconfig | 24 ++++++++++++++++++++++++
arch/x86/Makefile | 7 +++++++
2 files changed, 31 insertions(+)
@@ -152,6 +152,13 @@ ifdef CONFIG_X86_X32endifexportCONFIG_X86_X32_ABI+# Check assembler shadow stack suppot+ifdef CONFIG_X86_INTEL_SHADOW_STACK_USER+ ifeq ($(call as-instr, saveprevssp, y),)+$(errorCONFIG_X86_INTEL_SHADOW_STACK_USERnotsupportedbytheassembler)+ endif+endif+## If the function graph tracer is used with mcount instead of fentry,# '-maccumulate-outgoing-args' is needed to prevent a GCC bug
A RO and dirty PTE exists in the following cases:
(a) A page is modified and then shared with a fork()'ed child;
(b) A R/O page that has been COW'ed;
(c) A SHSTK page.
The processor does not read the dirty bit for (a) and (b), but
checks the dirty bit for (c). To prevent the use of non-SHSTK
memory as SHSTK, we introduce a spare bit of the 64-bit PTE as
_PAGE_BIT_DIRTY_SW and use that for (a) and (b). This results
to the following possible PTE settings:
Modified PTE: (R/W + DIRTY_HW)
Modified and shared PTE: (R/O + DIRTY_SW)
R/O PTE COW'ed: (R/O + DIRTY_SW)
SHSTK PTE: (R/O + DIRTY_HW)
SHSTK PTE COW'ed: (R/O + DIRTY_HW)
SHSTK PTE shared: (R/O + DIRTY_SW)
Note that _PAGE_BIT_DRITY_SW is only used in R/O PTEs but
not R/W PTEs.
When this patch is applied, there are six free bits left in
the 64-bit PTE. There is no more free bit 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>
---
arch/x86/include/asm/pgtable.h | 129 ++++++++++++++++++++++-----
arch/x86/include/asm/pgtable_types.h | 14 ++-
2 files changed, 121 insertions(+), 22 deletions(-)
@@ -23,6 +23,7 @@#define _PAGE_BIT_SOFTW2 10 /* " */#define _PAGE_BIT_SOFTW3 11 /* " */#define _PAGE_BIT_PAT_LARGE 12 /* On 2MB or 1GB pages */+#define _PAGE_BIT_SOFTW5 57 /* available for programmer */#define _PAGE_BIT_SOFTW4 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 */
@@ -34,6 +35,7 @@#define _PAGE_BIT_CPA_TEST _PAGE_BIT_SOFTW1#define _PAGE_BIT_SOFT_DIRTY _PAGE_BIT_SOFTW3 /* software dirty tracking */#define _PAGE_BIT_DEVMAP _PAGE_BIT_SOFTW4+#define _PAGE_BIT_DIRTY_SW _PAGE_BIT_SOFTW5 /* was written to *//* If _PAGE_BIT_PRESENT is clear, we use these: *//* - if the user mapped it with PROT_NONE; pte_present gives true */
This patch implements THP shadow stack memory copying in the same
way as the previous patch for regular PTE.
In copy_huge_pmd(), we clear the dirty bit from the PMD. On the
next shadow stack access to the PMD, a page fault occurs. At
that time, the page is copied/re-used and the PMD is fixed.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/mm/pgtable.c | 8 ++++++++
include/asm-generic/pgtable.h | 2 ++
mm/huge_memory.c | 4 ++++
3 files changed, 14 insertions(+)
@@ -0,0 +1,259 @@+=========================================+Control Flow Enforcement Technology (CET)+=========================================++[1] Overview+============++Control Flow Enforcement Technology (CET) provides protection against+return/jump-oriented programming (ROP) attacks. It can be implemented+to protect both the kernel and applications. In the first phase,+only the user-mode protection is implemented on the 64-bit kernel.+However, 32-bit applications are supported under the compatibility+mode.++CET includes shadow stack (SHSTK) and indirect branch tracking (IBT).+The SHSTK is a secondary stack allocated from memory. The processor+automatically pushes/pops a secure copy to the SHSTK every return+address and, by comparing the secure copy to the program stack copy,+verifies function returns are as intended. The IBT verifies all+indirect CALL/JMP targets are intended and marked by the compiler with+'ENDBR' op codes.++There are two kernel configuration options:++ INTEL_X86_SHADOW_STACK_USER, and+ INTEL_X86_BRANCH_TRACKING_USER.++To build a CET-enabled kernel, Binutils v2.31 and GCC v8.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_cet_shstk - disables SHSTK, and+ no_cet_ibt - disables IBT.++At run time, /proc/cpuinfo shows the availability of SHSTK and IBT.++[2] CET assembly instructions+=============================++RDSSP %r+ Read the SHSTK pointer into %r.++INCSSP %r+ Unwind (increment) the SHSTK pointer (0 ~ 255) steps as indicated+ in the operand register. The GLIBC longjmp uses INCSSP to unwind+ the SHSTK until that matches the program stack. When it is+ necessary to unwind beyond 255 steps, longjmp divides and repeats+ the process.++RSTORSSP (%r)+ Switch to the SHSTK indicated in the 'restore token' pointed by+ the operand register and replace the 'restore token' with a new+ token to be saved (with SAVEPREVSSP) for the outgoing SHSTK.++::++ Before RSTORSSP++ Incoming SHSTK Current/Outgoing SHSTK++ |----------------------| |----------------------|+ addr=x | | ssp-> | |+ |----------------------| |----------------------|+ (%r)-> | rstor_token=(x|Lg) | addr=y-8 | |+ |----------------------| |----------------------|++ After RSTORSSP++ |----------------------| |----------------------|+| | | |+ |----------------------| |----------------------|+ ssp-> | rstor_token=(y|Bz|Lg)| addr=y-8 | |+ |----------------------| |----------------------|++ note:+1. Only valid addresses and restore tokens can be on the+ user-mode SHSTK.+2. A token is always of type u64 and must align to u64.+3. The incoming SHSTK pointer in a rstor_token must point to+ immediately above the token.+4. 'Lg' is bit[0] of a rstor_token indicating a 64-bit SHSTK.+5. 'Bz' is bit[1] of a rstor_token indicating the token is to+ be used only for the next SAVEPREVSSP and invalid for the+ RSTORSSP.++SAVEPREVSSP+ Store the SHSTK 'restore token' pointed by+ (current_SHSTK_pointer + 8).++::++ After SAVEPREVSSP++ |----------------------| |----------------------|+ ssp-> | | | |+ |----------------------| |----------------------|+| rstor_token=(y|Bz|Lg)| addr=y-8 | rstor_token(y|Lg) |+ |----------------------| |----------------------|++WRUSS %r0, (%r1)+ Write the value in %r0 to the SHSTK address pointed by (%r1).+ This is a kernel-mode only instruction.++ENDBR+ The compiler inserts an ENDBR at all valid branch targets. Any+ CALL/JMP to a target without an ENDBR triggers a control+ protection fault.++[3] Application Enabling+========================++An application's CET capability is marked in its ELF header and can+be verified from the following command output, in the+NT_GNU_PROPERTY_TYPE_0 field:++ readelf -n <application>++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 only when all+requirements are met.++[4] Legacy Libraries+====================++GLIBC provides a few tunables for backward compatibility.++GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK,-IBT+ Turn off SHSTK/IBT for the current shell.++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.++[5] CET system calls+====================++The following arch_prctl() system calls are added for CET:++arch_prctl(ARCH_CET_STATUS, unsigned long *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 = SHSTK/IBT status+ *(addr + 1) = SHSTK base address+ *(addr + 2) = SHSTK size++arch_prctl(ARCH_CET_DISABLE, unsigned long features)+ Disable SHSTK and/or IBT specified in 'features'. Return -EPERM+ if CET is locked.++arch_prctl(ARCH_CET_LOCK)+ Lock in CET feature.++arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)+ Allocate a new SHSTK and put a restore token at top.++ The parameter 'addr' is a pointer to a user buffer and indicates+ the desired SHSTK size to allocate. On returning to the caller,+ the kernel fills *addr with the base address of the new SHSTK.++arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)+ Allocate an IBT legacy code bitmap if the current task does not+ have one.++ The parameter 'addr' is a pointer to a user buffer.+ On returning to the caller, the kernel fills the following+ information:++ *addr = IBT bitmap base address+ *(addr + 1) = IBT bitmap size++[6] The implementation of the SHSTK+===================================++SHSTK size+----------++A task's SHSTK is allocated from memory to a fixed size of+RLIMIT_STACK.++Signal+------++The main program and its signal handlers use the same SHSTK. Because+the SHSTK stores only return addresses, we can use a large SHSTK to+cover the condition that both the program stack and the sigaltstack+run out.++The kernel creates a restore token at the SHSTK restoring address and+verifies that token when restoring from the signal handler.++Fork+----++The SHSTK's vma has VM_SHSTK flag set; its PTEs are required to be+read-only and dirty. When a SHSTK PTE is not present, RO, and dirty,+a SHSTK access triggers a page fault with an additional SHSTK bit set+in the page fault error code.++When a task forks a child, its SHSTK PTEs are copied and both the+parent's and the child's SHSTK PTEs are cleared of the dirty bit.+Upon the next SHSTK access, the resulting SHSTK page fault is handled+by page copy/re-use.++When a pthread child is created, the kernel allocates a new SHSTK for+the new thread.++Setjmp/Longjmp+--------------++Longjmp unwinds SHSTK until it matches the program stack.++Ucontext+--------++In GLIBC, getcontext/setcontext is implemented in similar way as+setjmp/longjmp.++When makecontext creates a new ucontext, a new SHSTK is allocated for+that context with ARCH_CET_ALLOC_SHSTK the syscall. The kernel+creates a restore token at the top of the new SHSTK and the user-mode+code switches to the new SHSTK with the RSTORSSP instruction.++[7] The management of read-only & dirty PTEs for SHSTK+======================================================++A RO and dirty PTE exists in the following cases:++(a) A page is modified and then shared with a fork()'ed child;+(b) A R/O page that has been COW'ed;+(c) A SHSTK page.++The processor only checks the dirty bit for (c). To prevent the use+of non-SHSTK memory as SHSTK, we use a spare bit of the 64-bit PTE as+DIRTY_SW for (a) and (b) above. This results to the following PTE+settings:++Modified PTE: (R/W + DIRTY_HW)+Modified and shared PTE: (R/O + DIRTY_SW)+R/O PTE, COW'ed: (R/O + DIRTY_SW)+SHSTK PTE: (R/O + DIRTY_HW)+SHSTK PTE, COW'ed: (R/O + DIRTY_HW)+SHSTK PTE, shared: (R/O + DIRTY_SW)++Note that DIRTY_SW is only used in R/O PTEs but not R/W PTEs.++[8] The implementation of IBT+=============================++The kernel provides IBT support in mmap() of the legacy code bit map.+However, the management of the bitmap is done in the GLIBC or the+application.
To support XSAVES system states, change some names to distinguish
user and system states.
Change:
supervisor to system
copy_init_fpstate_to_fpregs() to copy_init_user_fpstate_to_fpregs()
xfeatures_mask to xfeatures_mask_user
XCNTXT_MASK to SUPPORTED_XFEATURES_MASK (states supported)
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/fpu/internal.h | 5 +-
arch/x86/include/asm/fpu/xstate.h | 24 ++++----
arch/x86/kernel/fpu/core.c | 4 +-
arch/x86/kernel/fpu/init.c | 2 +-
arch/x86/kernel/fpu/signal.c | 6 +-
arch/x86/kernel/fpu/xstate.c | 88 +++++++++++++++--------------
6 files changed, 66 insertions(+), 63 deletions(-)
@@ -219,20 +219,20 @@ void fpstate_sanitize_xstate(struct fpu *fpu)*/voidfpu__init_cpu_xstate(void){-if(!boot_cpu_has(X86_FEATURE_XSAVE)||!xfeatures_mask)+if(!boot_cpu_has(X86_FEATURE_XSAVE)||!xfeatures_mask_user)return;/*-*MakeitclearthatXSAVESsupervisorstatesarenotyet+*MakeitclearthatXSAVESsystemstatesarenotyet*implementedshouldanyoneexpectittoworkbychanging*bitsinXFEATURE_MASK_*macrosandXCR0.*/-WARN_ONCE((xfeatures_mask&XFEATURE_MASK_SUPERVISOR),-"x86/fpu: XSAVES supervisor states are not yet implemented.\n");+WARN_ONCE((xfeatures_mask_user&XFEATURE_MASK_SYSTEM),+"x86/fpu: XSAVES system states are not yet implemented.\n");-xfeatures_mask&=~XFEATURE_MASK_SUPERVISOR;+xfeatures_mask_user&=~XFEATURE_MASK_SYSTEM;cr4_set_bits(X86_CR4_OSXSAVE);-xsetbv(XCR_XFEATURE_ENABLED_MASK,xfeatures_mask);+xsetbv(XCR_XFEATURE_ENABLED_MASK,xfeatures_mask_user);}/*
@@ -440,11 +441,11 @@ static int xfeature_uncompacted_offset(int xfeature_nr)u32eax,ebx,ecx,edx;/*-*OnlyXSAVESsupportssupervisorstatesanditusescompacted-*format.Checkingasupervisorstate'suncompactedoffsetis+*OnlyXSAVESsupportssystemstatesanditusescompacted+*format.Checkingasystemstate'suncompactedoffsetis*anerror.*/-if(XFEATURE_MASK_SUPERVISOR&(1<<xfeature_nr)){+if(XFEATURE_MASK_SYSTEM&(1<<xfeature_nr)){WARN_ONCE(1,"No fixed offset for xstate %d\n",xfeature_nr);return-1;}
@@ -465,7 +466,7 @@ static int xfeature_size(int xfeature_nr)/**'XSAVES'impliestwodifferentthings:-*1.savingofsupervisor/systemstate+*1.savingofsystemstate*2.usingthecompactedformat**Usethisfunctionwhendealingwiththecompactedformatso
@@ -480,8 +481,8 @@ int using_compacted_format(void)/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */intvalidate_xstate_header(conststructxstate_header*hdr){-/* No unknown or supervisor features may be set */-if(hdr->xfeatures&(~xfeatures_mask|XFEATURE_MASK_SUPERVISOR))+/* No unknown or system features may be set */+if(hdr->xfeatures&(~xfeatures_mask_user|XFEATURE_MASK_SYSTEM))return-EINVAL;/* Userspace must use the uncompacted format */
@@ -588,11 +589,11 @@ static void do_extra_xstate_size_checks(void)check_xstate_against_struct(i);/*-*Supervisorstatecomponentscanbemanagedonlyby+*Systemstatecomponentscanbemanagedonlyby*XSAVES,whichiscompacted-formatonly.*/if(!using_compacted_format())-XSTATE_WARN_ON(xfeature_is_supervisor(i));+XSTATE_WARN_ON(xfeature_is_system(i));/* Align from the end of the previous feature */if(xfeature_is_aligned(i))
@@ -706,7 +707,7 @@ static int init_xstate_size(void)*/staticvoidfpu__init_disable_system_xstate(void){-xfeatures_mask=0;+xfeatures_mask_user=0;cr4_clear_bits(X86_CR4_OSXSAVE);fpu__xstate_clear_all_cpu_caps();}
@@ -742,15 +743,15 @@ void __init fpu__init_system_xstate(void)}cpuid_count(XSTATE_CPUID,0,&eax,&ebx,&ecx,&edx);-xfeatures_mask=eax+((u64)edx<<32);+xfeatures_mask_user=eax+((u64)edx<<32);-if((xfeatures_mask&XFEATURE_MASK_FPSSE)!=XFEATURE_MASK_FPSSE){+if((xfeatures_mask_user&XFEATURE_MASK_FPSSE)!=XFEATURE_MASK_FPSSE){/**Thisindicatesthatsomethingreallyunexpectedhappened*withtheenumeration.DisableXSAVEandtrytocontinue*bootingwithoutit.ThisistooearlytoBUG().*/-pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n",xfeatures_mask);+pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n",xfeatures_mask_user);gotoout_disable;}
@@ -759,10 +760,10 @@ void __init fpu__init_system_xstate(void)*/for(i=0;i<ARRAY_SIZE(xsave_cpuid_features);i++){if(!boot_cpu_has(xsave_cpuid_features[i]))-xfeatures_mask&=~BIT(i);+xfeatures_mask_user&=~BIT_ULL(i);}-xfeatures_mask&=fpu__get_supported_xfeatures_mask();+xfeatures_mask_user&=fpu__get_supported_xfeatures_mask();/* Enable xstate instructions to be able to continue with initialization: */fpu__init_cpu_xstate();
From: Randy Dunlap <rdunlap@infradead.org> Date: 2018-09-21 16:55:37
On 9/21/18 8:03 AM, Yu-cheng Yu wrote:
Add shadow stack pages to memory accounting.
Also check if the system has enough memory before enabling CET.
Signed-off-by: Yu-cheng Yu <yu-cheng.yu.intel.com>
On Fri, 2018-09-21 at 09:55 -0700, Randy Dunlap wrote:
On 9/21/18 8:03 AM, Yu-cheng Yu wrote:
quoted
Add shadow stack pages to memory accounting.
Also check if the system has enough memory before enabling CET.
Signed-off-by: Yu-cheng Yu <yu-cheng.yu.intel.com>
So, this is an RFC, but there no mention of what you want comments *on*. :)
What do you want folks to review? What needs to get settled before this
is merged?
So, this is an RFC, but there no mention of what you want comments *on*. :)
What do you want folks to review? What needs to get settled before this
is merged?
Thanks, Dave!
These patches passed GLIBC built-in tests and more tests HJ and I put together
at https://github.com/hjl-tools/cet-smoke-test.
I made some changes since V3 as outlined in the cover letter.
In particular there are two new patches for the VMA guard and preventing shadow
stack merging. Does anyone have comments on those and the whole Shadow
Stack/IBT series in general?
Thanks,
Yu-cheng
By my counting, that doesn't qualify for a line-break, it hits 80.
If you were to do this line-break, coding style would have you liberally
sprinkle {} around.
else if (static_cpu_has(X86_FEATURE_FXSR))
copy_kernel_to_fxregs(&init_fpstate.fxsave);
else
By my counting, that doesn't qualify for a line-break, it hits 80.
If you were to do this line-break, coding style would have you liberally
sprinkle {} around.
If you haven't noticed, there's already a separate leaf:
/* Intel-defined CPU features, CPUID level 0x00000007:0 (EDX), word 18 */
in arch/x86/include/asm/cpufeatures.h
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
If you haven't noticed, there's already a separate leaf:
/* Intel-defined CPU features, CPUID level 0x00000007:0 (EDX), word 18 */
in arch/x86/include/asm/cpufeatures.h
On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
To support XSAVES system states, change some names to distinguish
user and system states.
I don't understand what the logic here is. SDM says:
XSAVES—Save Processor Extended States Supervisor
the stress being on "Supervisor" - why does it need to be renamed to
"system" now?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
quoted
To support XSAVES system states, change some names to distinguish
user and system states.
I don't understand what the logic here is. SDM says:
XSAVES—Save Processor Extended States Supervisor
the stress being on "Supervisor" - why does it need to be renamed to
"system" now?
Good point. However, "system" is more indicative; CET states are per-task and
not "Supervisor". Do we want to go back to "Supervisor" or add comments?
Yu-cheng
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2018-10-02 16:30:59
On 10/02/2018 09:21 AM, Yu-cheng Yu wrote:
On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
quoted
To support XSAVES system states, change some names to distinguish
user and system states.
I don't understand what the logic here is. SDM says:
XSAVES—Save Processor Extended States Supervisor
the stress being on "Supervisor" - why does it need to be renamed to
"system" now?
Good point. However, "system" is more indicative; CET states are per-task and
not "Supervisor". Do we want to go back to "Supervisor" or add comments?
This is one of those things where the SDM language does not match what
we use in the kernel. I think it's fine to call them "system" or
"kernel" states to make it consistent with our existing in-kernel
nomenclature.
I say add comments to clarify what the SDM calls it vs. what we do.
On Tue, Oct 02, 2018 at 09:30:52AM -0700, Dave Hansen wrote:
quoted
Good point. However, "system" is more indicative; CET states are per-task and
not "Supervisor". Do we want to go back to "Supervisor" or add comments?
This is one of those things where the SDM language does not match what
we use in the kernel. I think it's fine to call them "system" or
"kernel" states to make it consistent with our existing in-kernel
nomenclature.
I say add comments to clarify what the SDM calls it vs. what we do.
So AFAIU, the difference is that XSAVES is a CPL0 insn. Thus the
supervisor thing, I'd guess.
Now it looks like CET uses XSAVES (from skimming the patchset forward)
but then what our nomenclature is and how it all gets tied together,
needs to be explained somewhere prominent so that we're all on the same
page.
This patch's commit message is not even close. So I'd very much
appreciate a more verbose explanation, even if it repeats itself at
places.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
XSAVES saves both system and user states. The Linux kernel
currently does not save/restore any system states. This patch
creates the framework for supporting system states.
... and needs a lot more text explaining *why* it is doing that.
@@ -19,10 +19,10 @@#define XSAVE_YMM_SIZE 256#define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)-/* System features */-#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)
Previous patch renames it, this patch deletes it. Why do we need all
that unnecessary churn?
Also, this patch is trying to do a couple of things at once and
reviewing it is not trivial. Please split the changes logically.
@@ -219,30 +222,31 @@ void fpstate_sanitize_xstate(struct fpu *fpu) */ void fpu__init_cpu_xstate(void) {- if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_user)+ if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all) return;++ cr4_set_bits(X86_CR4_OSXSAVE);+ /*- * Make it clear that XSAVES system states are not yet- * implemented should anyone expect it to work by changing- * bits in XFEATURE_MASK_* macros and XCR0.+ * XCR_XFEATURE_ENABLED_MASK sets the features that are managed+ * by XSAVE{C, OPT} and XRSTOR. Only XSAVE user states can be+ * set here. */- WARN_ONCE((xfeatures_mask_user & XFEATURE_MASK_SYSTEM),- "x86/fpu: XSAVES system states are not yet implemented.\n");+ xsetbv(XCR_XFEATURE_ENABLED_MASK,+ xfeatures_mask_user);
No need to break the line here.
Also, you have a couple more places in your patches where you
unnecessarily break lines. Please don't do that, even if it exceeds 80
cols by a couple of chars.
- xfeatures_mask_user &= ~XFEATURE_MASK_SYSTEM;
-
- cr4_set_bits(X86_CR4_OSXSAVE);
- xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user);
+ /*
+ * MSR_IA32_XSS sets which XSAVES system states to be managed by
+ * XSAVES. Only XSAVES system states can be set here.
+ */
+ if (boot_cpu_has(X86_FEATURE_XSAVES))
+ wrmsrl(MSR_IA32_XSS,
+ xfeatures_mask_all & ~xfeatures_mask_user);
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Fri, Sep 21, 2018 at 08:03:43AM -0700, Yu-cheng Yu wrote:
WRUSS is a new kernel-mode instruction but writes directly
to user shadow stack memory. This is used to construct
a return address on the shadow stack for the signal
handler.
This instruction can fault if the user shadow stack is
invalid shadow stack memory. In that case, the kernel does
fixup.
+}
+#else
+static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
+{
+ WARN_ONCE(1, "write_user_shstk_32 used but not supported.\n");
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
From: Andy Lutomirski <luto@amacapital.net> Date: 2018-10-03 05:36:31
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref] wrote:
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
@@ -578,6 +578,64 @@ do_general_protection(struct pt_regs *regs, long error_code)}NOKPROBE_SYMBOL(do_general_protection);+staticconstchar*control_protection_err[]=+{+"unknown",+"near-ret",+"far-ret/iret",+"endbranch",+"rstorssp",+"setssbsy",+};++/*+*Whenacontrolprotectionexceptionoccurs,sendasignal+*totheresponsibleapplication.Currently,control+*protectionisonlyenabledfortheusermode.This+*exceptionshouldnotcomefromthekernelmode.+*/+dotraplinkagevoid+do_control_protection(structpt_regs*regs,longerror_code)+{+structtask_struct*tsk;++RCU_LOCKDEP_WARN(!rcu_is_watching(),"entry code didn't wake RCU");+if(notify_die(DIE_TRAP,"control protection fault",regs,+error_code,X86_TRAP_CP,SIGSEGV)==NOTIFY_STOP)+return;+cond_local_irq_enable(regs);++if(!user_mode(regs))+die("kernel control protection fault",regs,error_code);++if(!static_cpu_has(X86_FEATURE_SHSTK)&&+!static_cpu_has(X86_FEATURE_IBT))+WARN_ONCE(1,"CET is disabled but got control "+"protection fault\n");++tsk=current;+tsk->thread.error_code=error_code;+tsk->thread.trap_nr=X86_TRAP_CP;++if(show_unhandled_signals&&unhandled_signal(tsk,SIGSEGV)&&+printk_ratelimit()){+unsignedintmax_err;++max_err=ARRAY_SIZE(control_protection_err)-1;+if((error_code<0)||(error_code>max_err))+error_code=0;+pr_info("%s[%d] control protection ip:%lx sp:%lx error:%lx(%s)",+tsk->comm,task_pid_nr(tsk),+regs->ip,regs->sp,error_code,+control_protection_err[error_code]);+print_vma_addr(KERN_CONT" in ",regs->ip);+pr_cont("\n");+}++force_sig_info(SIGSEGV,SEND_SIG_PRIV,tsk);
That way, no information is provided to userspace (both application and
debugger), which is rather unfortunate. It would be nice if a new SEGV_*
code was added at least, and CET error (with error code constant provided
in UAPI) is passed via si_errno. (Having ip/sp/*ssp would be even
better, but I'm not exactly sure about ramifications of providing this
kind of information to user space).
On Fri, Sep 21, 2018 at 08:03:34AM -0700, Yu-cheng Yu wrote:
quoted hunk
Update _PAGE_DIRTY to _PAGE_DIRTY_BITS in split_2MB_gtt_entry().
In order to support Control Flow Enforcement (CET), _PAGE_DIRTY
is now _PAGE_DIRTY_HW or _PAGE_DIRTY_SW.
Signed-off-by: Yu-cheng Yu <redacted>
---
drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Matthew Wilcox <willy@infradead.org> Date: 2018-10-03 13:39:10
On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
We are going to create _PAGE_DIRTY_SW for non-hardware, memory
management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
_PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
bits more clear. There are no functional changes in this
patch.
I would like there to be some documentation in this patchset which
explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
Also, is it really necessary to rename PAGE_DIRTY? It feels like a
lot of churn.
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2018-10-03 14:08:37
On 10/03/2018 06:38 AM, Matthew Wilcox wrote:
On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
quoted
We are going to create _PAGE_DIRTY_SW for non-hardware, memory
management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
_PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
bits more clear. There are no functional changes in this
patch.
I would like there to be some documentation in this patchset which
explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
Also, is it really necessary to rename PAGE_DIRTY? It feels like a
lot of churn.
This is a lot of churn? Are we looking a the same patch? :)
arch/x86/include/asm/pgtable.h | 6 +++---
arch/x86/include/asm/pgtable_types.h | 17 +++++++++--------
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kvm/vmx.c | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
But, yeah, I think we need to. While it will take a little adjustment
in the brains of us old-timers and a bit of pain when switching from old
kernels to new, this makes it a lot more clear what is going on.
On Fri, Sep 21, 2018 at 08:03:44AM -0700, Yu-cheng Yu wrote:
When setting up a signal, the kernel creates a shadow stack
restore token at the current SHSTK address and then stores the
token's address in the signal frame, right after the FPU state.
Before restoring a signal, the kernel verifies and then uses the
restore token to set the SHSTK pointer.
@@ -46,6 +47,69 @@ static unsigned long get_shstk_addr(void) return ptr; }+/*+ * Verify the restore token at the address of 'ssp' is+ * valid and then set shadow stack pointer according to the+ * token.+ */+static int verify_rstor_token(bool ia32, unsigned long ssp,+ unsigned long *new_ssp)+{+ unsigned long token;++ *new_ssp = 0;++ if (!IS_ALIGNED(ssp, 8))+ return -EINVAL;++ if (get_user(token, (unsigned long __user *)ssp))+ return -EFAULT;+
+ /* Is 64-bit mode flag correct? */
+ if (ia32 && (token & 3) != 0)
+ return -EINVAL;
+ else if ((token & 3) != 1)
+ return -EINVAL;
+ token &= ~(1UL);
+
+ if ((!ia32 && !IS_ALIGNED(token, 8)) || !IS_ALIGNED(token, 4))
+ return -EINVAL;
+
+ if ((ALIGN_DOWN(token, 8) - 8) != ssp)
+ return -EINVAL;
+
+ *new_ssp = token;
+ return 0;
+}
+
+/*
+ * 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 *new_ssp)
+{
+ unsigned long addr;
+
+ *new_ssp = 0;
+
+ if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4))
+ return -EINVAL;
Maybe refactor this check into a separate function/macro?
+
+ addr = ALIGN_DOWN(ssp, 8) - 8;
+
+ /* Is the token for 64-bit? */
+ if (!ia32)
+ ssp |= 1;
Again, usage of a named constant might document it better.
+
+ if (write_user_shstk_64(addr, ssp))
This function is defined in "[RFC PATCH v4 19/27] x86/cet/shstk:
Introduce WRUSS instruction"
quoted hunk
+ return -EFAULT;
+
+ *new_ssp = addr;
+ return 0;
+}
+
int cet_setup_shstk(void)
{
unsigned long addr, size;
@@ -107,3 +171,54 @@ void cet_disable_free_shstk(struct task_struct *tsk) tsk->thread.cet.shstk_enabled = 0; }++int cet_restore_signal(unsigned long ssp)+{+ unsigned long new_ssp;+ int err;++ if (!current->thread.cet.shstk_enabled)+ return 0;++ err = verify_rstor_token(in_ia32_syscall(), ssp, &new_ssp);++ if (err)+ return err;++ return set_shstk_ptr(new_ssp);+}++/*+ * Setup the shadow stack for the signal handler: first,+ * create a restore token to keep track of the current ssp,+ * and then the return address of the signal handler.+ */+int cet_setup_signal(bool ia32, unsigned long rstor_addr,+ unsigned long *new_ssp)+{+ unsigned long ssp;+ int err;++ if (!current->thread.cet.shstk_enabled)+ return 0;++ ssp = get_shstk_addr();+ err = create_rstor_token(ia32, ssp, new_ssp);++ if (err)+ return err;++ if (ia32) {+ ssp = *new_ssp - sizeof(u32);+ err = write_user_shstk_32(ssp, (unsigned int)rstor_addr);+ } else {+ ssp = *new_ssp - sizeof(u64);+ err = write_user_shstk_64(ssp, rstor_addr);+ }++ if (err)+ return err;++ set_shstk_ptr(ssp);+ return 0;+}
That might be refactored in a separate function.
Also, it looks like that possible padding for 8-byte alignment
(copy_ext_{to,from}_user) is not accounted here.
It's probably makes sense to have this hunk as a part of "x86/cet/shstk:
Add Kconfig option for user-mode shadow stack", where VM_SHSTK was
initially introduced.
It's probably makes sense to have this hunk as a part of "x86/cet/shstk:
Add Kconfig option for user-mode shadow stack", where VM_SHSTK was
initially introduced.
Yes, move it to "mm/Introduce VM_SHSTK for shadow stack memory".
Yu-cheng
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref] wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Yu-cheng
On Wed, 2018-10-03 at 06:38 -0700, Matthew Wilcox wrote:
On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
quoted
We are going to create _PAGE_DIRTY_SW for non-hardware, memory
management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
_PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
bits more clear. There are no functional changes in this
patch.
I would like there to be some documentation in this patchset which
explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
I will add some comments for the difference between PAGE_SOFT_DIRTY and
PAGE_DIRTY_SW.
Yu-cheng
From: Andy Lutomirski <luto@kernel.org> Date: 2018-10-03 16:19:15
On Wed, Oct 3, 2018 at 9:06 AM Yu-cheng Yu [off-list ref] wrote:
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
quoted
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref] wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Various ELF loaders, perhaps? Do they use MAP_FIXED or do they just
use address hints?
That way, no information is provided to userspace (both application and
debugger), which is rather unfortunate. It would be nice if a new SEGV_*
code was added at least, and CET error (with error code constant provided
in UAPI) is passed via si_errno. (Having ip/sp/*ssp would be even
better, but I'm not exactly sure about ramifications of providing this
kind of information to user space).
On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
quoted
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref] wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Right, after some more code reading I figured out that it indeed
shouldn't affect MAP_FIXED, thank you for confirmation.
I'm not sure, however, whether such a change that provides no ability
to configure or affect it will go well with all the supported
architectures.
On Wed, 2018-10-03 at 18:32 +0200, Eugene Syromiatnikov wrote:
On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
quoted
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
quoted
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref]
wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Right, after some more code reading I figured out that it indeed
shouldn't affect MAP_FIXED, thank you for confirmation.
I'm not sure, however, whether such a change that provides no ability
to configure or affect it will go well with all the supported
architectures.
On Fri, Sep 21, 2018 at 5:09 PM Yu-cheng Yu [off-list ref] wrote:
When setting up a signal, the kernel creates a shadow stack
restore token at the current SHSTK address and then stores the
token's address in the signal frame, right after the FPU state.
Before restoring a signal, the kernel verifies and then uses the
restore token to set the SHSTK pointer.
[...]
+#ifdef CONFIG_X86_64
+static int copy_ext_from_user(struct sc_ext *ext, void __user *fpu)
+{
+ void __user *p;
+
+ if (!fpu)
+ return -EINVAL;
+
+ p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
+ p = (void __user *)ALIGN((unsigned long)p, 8);
+
+ if (!access_ok(VERIFY_READ, p, sizeof(*ext)))
+ return -EFAULT;
+
+ if (__copy_from_user(ext, p, sizeof(*ext)))
+ return -EFAULT;
Why do you first manually call access_ok(), then call
__copy_from_user() with the same size? Just use "if
(copy_from_user(ext, p, sizeof(*ext)))" (without underscores) and get
rid of the access_ok().
+ if (ext->total_size != sizeof(*ext))
+ return -EINVAL;
+ return 0;
+}
+
+static int copy_ext_to_user(void __user *fpu, struct sc_ext *ext)
+{
+ void __user *p;
+
+ if (!fpu)
+ return -EINVAL;
+
+ if (ext->total_size != sizeof(*ext))
+ return -EINVAL;
+
+ p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
+ p = (void __user *)ALIGN((unsigned long)p, 8);
+
+ if (!access_ok(VERIFY_WRITE, p, sizeof(*ext)))
+ return -EFAULT;
+
+ if (__copy_to_user(p, ext, sizeof(*ext)))
+ return -EFAULT;
On Wed, Oct 3, 2018 at 6:32 PM Eugene Syromiatnikov [off-list ref] wrote:
On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
quoted
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
quoted
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov [off-list ref] wrote:
quoted
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
quoted
Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Right, after some more code reading I figured out that it indeed
shouldn't affect MAP_FIXED, thank you for confirmation.
I'm not sure, however, whether such a change that provides no ability
to configure or affect it will go well with all the supported
architectures.
Is there a concrete reason why you think an architecture might not
like this? As far as I can tell, the virtual address space overhead
should be insignificant even for 32-bit systems.
On Fri, Sep 21, 2018 at 08:03:50AM -0700, Yu-cheng Yu wrote:
arch_prctl(ARCH_CET_STATUS, unsigned long *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 = SHSTK/IBT status
*(addr + 1) = SHSTK base address
*(addr + 2) = SHSTK size
The subtle detail here is that x32 binaries will get 64-bit value, which
is not entirely obvious. I think, it might be better to define
a structure type for it as a part of UAPI, for example:
struct user_cet_status {
__u32 struct_size;
__u32 features;
__kernel_ulong_t shstk_base;
__kernel_ulong_t shstk_size;
};
Adding "struct_size" field along with appropriate checks will also
allow for possible extensions, if they ever appear.
arch_prctl(ARCH_CET_DISABLE, unsigned long features)
Disable CET features specified in 'features'. Return
-EPERM if CET is locked.
While x86_64 and x32 will have 64-bit space for feature bits, IA-32 will
have only 32 bits.
arch_prctl(ARCH_CET_LOCK)
Lock in CET feature.
arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)
Allocate a new SHSTK.
The parameter 'addr' is a pointer to a user buffer and indicates
the desired SHSTK size to allocate. On returning to the caller
the buffer contains the address of the new SHSTK.
Again, on x32 that will be a pointer to a 64-bit value, which is not
entirely obvious from this description.
It's not clear whether inability to enable some CET feature in runtime
is unavailable by design or by omission; same for setting (an allocated)
shadow stack as task's shadow stack.
@@ -110,6 +110,33 @@ static int create_rstor_token(bool ia32, unsigned long ssp,return0;}+intcet_alloc_shstk(unsignedlong*arg)+{+unsignedlonglen=*arg;+unsignedlongaddr;+unsignedlongtoken;+unsignedlongssp;++addr=do_mmap_locked(0,len,PROT_READ,+MAP_ANONYMOUS|MAP_PRIVATE,VM_SHSTK);+if(addr>=TASK_SIZE_MAX)+return-ENOMEM;++/* Restore token is 8 bytes and aligned to 8 bytes */+ssp=addr+len;+token=ssp;++if(!in_ia32_syscall())+token|=1;
This pair of check and bit or'ing definitely asks for a macro or a
wrapper function.
@@ -792,6 +792,11 @@ long do_arch_prctl_common(struct task_struct *task, int option,returnget_cpuid_mode();caseARCH_SET_CPUID:returnset_cpuid_mode(task,cpuid_enabled);+caseARCH_CET_STATUS:+caseARCH_CET_DISABLE:+caseARCH_CET_LOCK:+caseARCH_CET_ALLOC_SHSTK:+returnprctl_cet(option,cpuid_enabled);
It's probably a good opportunity to change the strange name for an argument
of a dispatch call.
On Wed, Oct 03, 2018 at 06:52:40PM +0200, Jann Horn wrote:
On Wed, Oct 3, 2018 at 6:32 PM Eugene Syromiatnikov [off-list ref] wrote:
quoted
I'm not sure, however, whether such a change that provides no ability
to configure or affect it will go well with all the supported
architectures.
Is there a concrete reason why you think an architecture might not
like this? As far as I can tell, the virtual address space overhead
should be insignificant even for 32-bit systems.
Not really, and not architectures per se, but judging by some past
experiences with enabling ASLR, I would expect that all kinds of weird
applications may start to behave in all kinds of strange ways.
Not that I have anything more than this doubt, however; but this sort of
change without any ability to tune or revert it still looks unusual to me.
Hm, these defeinitions aren't much different comparing to NT_*
definitions in include/uapi/linux/elf.h, is it expected that those
properties have to be parsed individually for each architecture?
There's a lot of similar code with bpf stackmap .build-id code (commit
v4.17-rc1~148^2~156^2~3^2~1), it might be worthy generalising some ELF
traversal routines, since there's general need of parsing ELF property
segments.
@@ -19,10 +19,10 @@#define XSAVE_YMM_SIZE 256#define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)-/* System features */-#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)
Previous patch renames it, this patch deletes it. Why do we need all
that unnecessary churn?
Also, this patch is trying to do a couple of things at once and
reviewing it is not trivial. Please split the changes logically.
Yes, if we leave XFEATURE_MASK_SUPERVISOR unchanged in the previous patch, this
patch becomes much simpler. Perhaps we don't even need to split this one.
Hm, these defeinitions aren't much different comparing to NT_*
definitions in include/uapi/linux/elf.h, is it expected that those
properties have to be parsed individually for each architecture?
Yes, we have NT_GNU_PROPERTY_TYPE_0 defined in include/uapi/linux/elf.h.
GNU_PROPERTY_X86_FEATURE_1_xxxx is for X86 only.
[...]
There's a lot of similar code with bpf stackmap .build-id code (commit
v4.17-rc1~148^2~156^2~3^2~1), it might be worthy generalising some ELF
traversal routines, since there's general need of parsing ELF property
segments.
Only a small similarity exists. The routine find_note_type_0() does a lot more
validation. It appears stack_map_get_build_id() does not need that.
Yu-cheng
I think this could overflow: n_namesz can be u64 for elf64_note.
+ size = round_up(size + n->n_descsz, align);
Same here. You may want to use check_add_overflow(), etc, an u64 types.
+
+ if (buf + size < buf)
+ return NULL;
I don't understand this. You want to check size not exceeding the
allocation, which isn't passed into this function. Checking for a full
unsigned address wrap around is not sufficient to detect overflow.
Again, this "< buf" test doesn't look at all correct to me.
+ (pr->pr_type > GNU_PROPERTY_X86_FEATURE_1_AND) ||
+ (pr->pr_type > max_type))
+ return NULL;
+ else
+ return (buf + sizeof(*pr) + pr->pr_datasz);
+}
+
+/*
+ * Scan 'buf' for a pattern; return true if found.
+ * *pos is the distance from the beginning of buf to where
+ * the searched item or the next item is located.
+ */
+static int scan(u8 *buf, u32 buf_size, int item_size,
+ test_fn test, next_fn next, u32 *arg, u32 *pos)
I'm not a fan of the short "scan", "test" and "next" names, and I
really don't like an arg named "arg". Something slightly more
descriptive for all of these would be nice, please.
+{
+ int found = 0;
+ u8 *p, *max;
+
+ max = buf + buf_size;
+ if (max < buf)
+ return 0;
+
+ p = buf;
+
+ while ((p + item_size < max) && (p + item_size > buf)) {
These comparisons are safe due to the BUF_SIZE limit of buf_size and
the only used size of item_size, but if this becomes more generic, it
should be more defensive on the size calculations (e.g. make sure than
"item_size < max" and then here "p < max - item_size", etc).
I'd kind of rather this code walked the base type and check each for
the matching feature. What is the general specification for what
NT_GNU_PROPERTY_TYPE_0 contains?
+ if (test(p, arg)) {
+ found = 1;
+ break;
+ }
+
+ p = next(p, arg);
+ }
+
+ *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
+ return found;
+}
+
+/*
+ * Search a NT_GNU_PROPERTY_TYPE_0 for GNU_PROPERTY_X86_FEATURE_1_AND.
+ */
+static int find_feature_x86(struct file *file, unsigned long desc_size,
+ loff_t file_offset, u8 *buf, u32 *feature)
+{
+ u32 buf_pos;
+ unsigned long read_size;
+ unsigned long done;
+ int found = 0;
+ int ret = 0;
+ u32 last_pr = 0;
+
+ *feature = 0;
+ buf_pos = 0;
+
+ for (done = 0; done < desc_size; done += buf_pos) {
+ read_size = desc_size - done;
+ if (read_size > BUF_SIZE)
+ read_size = BUF_SIZE;
+
+ ret = kernel_read(file, buf, read_size, &file_offset);
+
+ if (ret != read_size)
+ return (ret < 0) ? ret : -EIO;
+
+ ret = 0;
+ found = scan(buf, read_size, sizeof(struct property_x86),
+ test_property_x86, next_property,
+ &last_pr, &buf_pos);
+
+ if ((!buf_pos) || found)
+ break;
+
+ file_offset += buf_pos - read_size;
+ }
+
+ if (found) {
+ struct property_x86 *pr =
+ (struct property_x86 *)(buf + buf_pos);
+
+ if (pr->pr_datasz == 4) {
+ u32 *max = (u32 *)(buf + read_size);
+ u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
+
+ if (data + 1 <= max) {
+ *feature = *data;
+ } else {
+ file_offset += buf_pos - read_size;
+ file_offset += sizeof(*pr);
+ ret = kernel_read(file, feature, 4,
+ &file_offset);
+ }
+ }
+ }
+
+ return ret;
+}
+
+/*
+ * Search a PT_NOTE segment for the first NT_GNU_PROPERTY_TYPE_0.
+ */
+static int find_note_type_0(struct file *file, unsigned long note_size,
+ loff_t file_offset, u32 align, u32 *feature)
+{
+ u8 *buf;
+ u32 buf_pos;
+ unsigned long read_size;
+ unsigned long done;
+ int found = 0;
+ int ret = 0;
+
+ buf = kmalloc(BUF_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
Why kmalloc over stack variable? (Or, does BUF_SIZE here really need
to be 1024?)
+
+ ret = find_feature_x86(file, n->n_descsz,
+ file_offset + start,
+ buf, feature);
+ file_offset += total;
+ buf_pos += total;
+ } else if (!buf_pos) {
+ *feature = 0;
+ break;
+ }
+ }
+
+ kfree(buf);
+ return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static int check_notes_32(struct file *file, struct elf32_phdr *phdr,
+ int phnum, u32 *feature)
+{
+ int i;
+ int err = 0;
+
+ for (i = 0; i < phnum; i++, phdr++) {
+ if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
+ continue;
+
+ err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
+ phdr->p_align, feature);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+#endif
+
+#ifdef CONFIG_X86_64
+static int check_notes_64(struct file *file, struct elf64_phdr *phdr,
+ int phnum, u32 *feature)
+{
+ int i;
+ int err = 0;
+
+ for (i = 0; i < phnum; i++, phdr++) {
+ if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
+ continue;
Instead of a separate parser here, wouldn't it be a bit nicer to
attach this to the existing binfmt_elf program header parsing loop:
elf_ppnt = elf_phdata;
for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++)
switch (elf_ppnt->p_type) {
case PT_GNU_STACK:
...
case PT_LOPROC ... PT_HIPROC:
...
I think this could overflow: n_namesz can be u64 for elf64_note.
quoted
+ size = round_up(size + n->n_descsz, align);
Same here. You may want to use check_add_overflow(), etc, an u64 types.
Note->n_namesz is always four-byte. I should have used u32.
quoted
+
+ if (buf + size < buf)
+ return NULL;
I don't understand this. You want to check size not exceeding the
allocation, which isn't passed into this function. Checking for a full
unsigned address wrap around is not sufficient to detect overflow.
Here we only detect the warp around. After this returns we then check other
types of overflow in scan().
Again, this "< buf" test doesn't look at all correct to me.
quoted
+ (pr->pr_type > GNU_PROPERTY_X86_FEATURE_1_AND) ||
+ (pr->pr_type > max_type))
+ return NULL;
+ else
+ return (buf + sizeof(*pr) + pr->pr_datasz);
+}
+
+/*
+ * Scan 'buf' for a pattern; return true if found.
+ * *pos is the distance from the beginning of buf to where
+ * the searched item or the next item is located.
+ */
+static int scan(u8 *buf, u32 buf_size, int item_size,
+ test_fn test, next_fn next, u32 *arg, u32 *pos)
I'm not a fan of the short "scan", "test" and "next" names, and I
really don't like an arg named "arg". Something slightly more
descriptive for all of these would be nice, please.
I need to work on that :-) What would you suggest?
quoted
+{
+ int found = 0;
+ u8 *p, *max;
+
+ max = buf + buf_size;
+ if (max < buf)
+ return 0;
+
+ p = buf;
+
+ while ((p + item_size < max) && (p + item_size > buf)) {
These comparisons are safe due to the BUF_SIZE limit of buf_size and
the only used size of item_size, but if this becomes more generic, it
should be more defensive on the size calculations (e.g. make sure than
"item_size < max" and then here "p < max - item_size", etc).
I'd kind of rather this code walked the base type and check each for
the matching feature. What is the general specification for what
NT_GNU_PROPERTY_TYPE_0 contains?
There are other property types, but the kernel does not look at most of them.
If the kernel needs to look at others, we need to rewrite this.
[...]
quoted
+
+/*
+ * Search a PT_NOTE segment for the first NT_GNU_PROPERTY_TYPE_0.
+ */
+static int find_note_type_0(struct file *file, unsigned long note_size,
+ loff_t file_offset, u32 align, u32 *feature)
+{
+ u8 *buf;
+ u32 buf_pos;
+ unsigned long read_size;
+ unsigned long done;
+ int found = 0;
+ int ret = 0;
+
+ buf = kmalloc(BUF_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
Why kmalloc over stack variable? (Or, does BUF_SIZE here really need
to be 1024?)
BUF_SIZE can be smaller, for example 64. If it is too small, we need to do
kernel_read() too often.
I'd like to be using this code for a few other cases too (not just
x86-specific). For example, for marking KASan binaries as needing a
"legacy" memory layouts[1]. Others might be setting things like
no_new_privs at exec time, etc.
If the item is a bit of GNU_PROPERTY_X86_FEATURE_1_AND, then this code would
work. Has it been finalized?
Yu-cheng