From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:36:13
This patchset improves several overlapping issues around stale TLB
entries and W^X violations. It is combined from a slightly tweaked
"x86/alternative: text_poke() enhancements v7" [1] and a next version of
the "Don’t leave executable TLB entries to freed pages v2" [2]
patchsets that were conflicting.
The related issues that this fixes:
1. Fixmap PTEs that are used for patching are available for access from
other cores and might be exploited. They are not even flushed from
the TLB in remote cores, so the risk is even higher. Address this
issue by introducing a temporary mm that is only used during
patching. Unfortunately, due to init ordering, fixmap is still used
during boot-time patching. Future patches can eliminate the need for
it.
2. Missing lockdep assertion to ensure text_mutex is taken. It is
actually not always taken, so fix the instances that were found not
to take the lock (although they should be safe even without taking
the lock).
3. Module_alloc returning memory that is RWX until a module is finished
loading.
4. Sometimes when memory is freed via the module subsystem, an
executable permissioned TLB entry can remain to a freed page. If the
page is re-used to back an address that will receive data from
userspace, it can result in user data being mapped as executable in
the kernel. The root of this behavior is vfree lazily flushing the
TLB, but not lazily freeing the underlying pages.
The new changes from "Don’t leave executable TLB entries to freed pages
v2":
- Add support for case of hibernate trying to save an unmapped page
on the directmap. (Ard Biesheuvel)
- No week arch breakout for vfree-ing special memory (Andy Lutomirski)
- Avoid changing deferred free code by moving modules init free to work
queue (Andy Lutomirski)
- Plug in new flag for kprobes and ftrace
- More arch generic names for set_pages functions (Ard Biesheuvel)
- Fix for TLB not always flushing the directmap (Nadav Amit)
New changes from from "x86/alternative: text_poke() enhancements v7"
- Fix build failure on CONFIG_RANDOMIZE_BASE=n (Rick)
- Remove text_poke usage from ftrace (Nadav)
[1] https://lkml.org/lkml/2018/12/5/200
[2] https://lkml.org/lkml/2018/12/11/1571
Andy Lutomirski (1):
x86/mm: temporary mm struct
Nadav Amit (12):
Fix "x86/alternatives: Lockdep-enforce text_mutex in text_poke*()"
x86/jump_label: Use text_poke_early() during early init
fork: provide a function for copying init_mm
x86/alternative: initializing temporary mm for patching
x86/alternative: use temporary mm for text poking
x86/kgdb: avoid redundant comparison of patched code
x86/ftrace: set trampoline pages as executable
x86/kprobes: Instruction pages initialization enhancements
x86: avoid W^X being broken during modules loading
x86/jump-label: remove support for custom poker
x86/alternative: Remove the return value of text_poke_*()
module: Prevent module removal racing with text_poke()
Rick Edgecombe (4):
Add set_alias_ function and x86 implementation
mm: Make hibernate handle unmapped pages
vmalloc: New flags for safe vfree on special perms
Plug in new special vfree flag
arch/Kconfig | 4 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/fixmap.h | 2 -
arch/x86/include/asm/mmu_context.h | 32 +++++
arch/x86/include/asm/pgtable.h | 3 +
arch/x86/include/asm/set_memory.h | 3 +
arch/x86/include/asm/text-patching.h | 7 +-
arch/x86/kernel/alternative.c | 197 ++++++++++++++++++++-------
arch/x86/kernel/ftrace.c | 15 +-
arch/x86/kernel/jump_label.c | 19 ++-
arch/x86/kernel/kgdb.c | 25 +---
arch/x86/kernel/kprobes/core.c | 19 ++-
arch/x86/kernel/module.c | 2 +-
arch/x86/mm/init_64.c | 36 +++++
arch/x86/mm/pageattr.c | 16 ++-
arch/x86/xen/mmu_pv.c | 2 -
include/linux/filter.h | 18 +--
include/linux/mm.h | 18 +--
include/linux/sched/task.h | 1 +
include/linux/set_memory.h | 10 ++
include/linux/vmalloc.h | 13 ++
init/main.c | 3 +
kernel/bpf/core.c | 1 -
kernel/fork.c | 24 +++-
kernel/module.c | 87 ++++++------
mm/page_alloc.c | 6 +-
mm/vmalloc.c | 122 ++++++++++++++---
27 files changed, 497 insertions(+), 189 deletions(-)
--
2.17.1
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:33:50
This adds two new functions set_alias_default_noflush and set_alias_nv_noflush
for setting the alias mapping for the page to its default valid permissions
and to an invalid state that cannot be cached in a TLB, respectively. These
functions to not flush the TLB.
Note, __kernel_map_pages does something similar but flushes the TLB and doesn't
reset the permission bits to default on all architectures.
There is also an ARCH config ARCH_HAS_SET_ALIAS for specifying whether these
have an actual implementation or a default empty one.
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/Kconfig | 4 ++++
arch/x86/Kconfig | 1 +
arch/x86/include/asm/set_memory.h | 3 +++
arch/x86/mm/pageattr.c | 14 +++++++++++---
include/linux/set_memory.h | 10 ++++++++++
5 files changed, 29 insertions(+), 3 deletions(-)
@@ -249,6 +249,10 @@ config ARCH_HAS_FORTIFY_SOURCEconfigARCH_HAS_SET_MEMORYbool+# Select if arch has all set_alias_nv/default() functions+configARCH_HAS_SET_ALIAS+bool+# Select if arch init_task must go in the __init_task_data sectionconfigARCH_TASK_STRUCT_ON_STACKbool
@@ -85,6 +85,9 @@ int set_pages_nx(struct page *page, int numpages);intset_pages_ro(structpage*page,intnumpages);intset_pages_rw(structpage*page,intnumpages);+intset_alias_nv_noflush(structpage*page);+intset_alias_default_noflush(structpage*page);+externintkernel_set_to_readonly;voidset_kernel_text_rw(void);voidset_kernel_text_ro(void);
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:33:52
This adds a new flags VM_HAS_SPECIAL_PERMS, for enabling vfree operations to
immediately clear executable TLB entries to freed pages, and handle freeing
memory with special permissions. It also takes care of reseting the direct map
permissions for the pages being unmapped. So this flag is useful for any kind
of memory with elevated permissions, or where there can be related permissions
changes on the directmap. Today this is RO+X and RO memory.
Although this enables directly vfreeing RO memory now, RO memory cannot be
freed in an interrupt because the allocation itself is used as a node on
deferred free list. So when RO memory needs to be freed in an interrupt the
code doing the vfree needs to have its own work queue, as was the case before
the deferred vfree list handling was added. Today there is only one case where
this happens.
For architectures with set_alias_ implementations this whole operation can be
done with one TLB flush when centralized like this. For others with directmap
permissions, currently only arm64, a backup method using set_memory functions
is used to reset the directmap. When arm64 adds set_alias_ functions, this
backup can be removed.
When the TLB is flushed to both remove TLB entries for the vmalloc range
mapping and the direct map permissions, the lazy purge operation could be done
to try to save a TLB flush later. However today vm_unmap_aliases could flush a
TLB range that does not include the directmap. So a helper is added with extra
parameters that can allow both the vmalloc address and the direct mapping to be
flushed during this operation. The behavior of the normal vm_unmap_aliases
function is unchanged.
Suggested-by: Dave Hansen <redacted>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Will Deacon <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
include/linux/vmalloc.h | 13 +++++
mm/vmalloc.c | 122 +++++++++++++++++++++++++++++++++-------
2 files changed, 116 insertions(+), 19 deletions(-)
@@ -1494,6 +1503,79 @@ struct vm_struct *remove_vm_area(const void *addr)returnNULL;}+staticinlinevoidset_area_alias(conststructvm_struct*area,+int(*set_alias)(structpage*page))+{+inti;++for(i=0;i<area->nr_pages;i++){+unsignedlongaddr=+(unsignedlong)page_address(area->pages[i]);++if(addr)+set_alias(area->pages[i]);+}+}++/* This handles removing and resetting vm mappings related to the vm_struct. */+staticvoidvm_remove_mappings(structvm_struct*area,intdeallocate_pages)+{+unsignedlongaddr=(unsignedlong)area->addr;+unsignedlongstart=ULONG_MAX,end=0;+intspecial=area->flags&VM_HAS_SPECIAL_PERMS;+inti;++/*+*Thebelowblockcanberemovedwhenallarchitecturesthathave+*directmappermissionsalsohaveset_alias_implementations.Thisis+*todoresettingonthedirectmapforanyspecialpermissions(today+*onlyX),withoutleavingaRW+Xwindow.+*/+if(special&&!IS_ENABLED(CONFIG_ARCH_HAS_SET_ALIAS)){+set_memory_nx(addr,area->nr_pages);+set_memory_rw(addr,area->nr_pages);+}++remove_vm_area(area->addr);++/* If this is not special memory, we can skip the below. */+if(!special)+return;++/*+*Ifwearenotdeallocatingpages,wecanjustdotheflushoftheVM+*areaandreturn.+*/+if(!deallocate_pages){+vm_unmap_aliases();+return;+}++/*+*Ifwearehere,weneedtoflushthevmmappingandresetthedirect+*map.+*Firstfindthestartandendrangeofthedirectmappingstomake+*surethevm_unmap_aliasesflushincludesthedirectmap.+*/+for(i=0;i<area->nr_pages;i++){+unsignedlongaddr=+(unsignedlong)page_address(area->pages[i]);+if(addr){+start=min(addr,start);+end=max(addr,end);+}+}++/*+*Firstwesetdirectmaptosomethingnotvalidsothatitwon'tbe+*cachedifthereareanyaccessesaftertheTLBflush,thenweflush+*theTLB,andresetthedirectmappermissionstothedefault.+*/+set_area_alias(area,set_alias_nv_noflush);+_vm_unmap_aliases(start,end,1);+set_area_alias(area,set_alias_default_noflush);+}+staticvoid__vunmap(constvoid*addr,intdeallocate_pages){structvm_struct*area;
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:33:57
From: Nadav Amit <redacted>
To prevent improper use of the PTEs that are used for text patching, we
want to use a temporary mm struct. We initailize it by copying the init
mm.
The address that will be used for patching is taken from the lower area
that is usually used for the task memory. Doing so prevents the need to
frequently synchronize the temporary-mm (e.g., when BPF programs are
installed), since different PGDs are used for the task memory.
Finally, we randomize the address of the PTEs to harden against exploits
that use these PTEs.
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/pgtable.h | 3 +++
arch/x86/include/asm/text-patching.h | 2 ++
arch/x86/kernel/alternative.c | 3 +++
arch/x86/mm/init_64.c | 36 ++++++++++++++++++++++++++++
init/main.c | 3 +++
5 files changed, 47 insertions(+)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:05
From: Nadav Amit <redacted>
The return value of text_poke_early() and text_poke_bp() is useless.
Remove it.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 4 ++--
arch/x86/kernel/alternative.c | 11 ++++-------
2 files changed, 6 insertions(+), 9 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:08
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
kernel/module.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -2157,6 +2158,9 @@ static void free_module(struct module *mod)synchronize_rcu();mutex_unlock(&module_mutex);+/* Protect against patching of the module while it is being removed */+mutex_lock(&text_mutex);+/* This may be empty, but that's OK */module_arch_freeing_init(mod);module_memfree(mod->init_layout.base);
@@ -2168,6 +2172,7 @@ static void free_module(struct module *mod)/* Finally, free the core (containing the module structure) */module_memfree(mod->core_layout.base);+mutex_unlock(&text_mutex);}void*__symbol_get(constchar*symbol)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:13
From: Nadav Amit <redacted>
text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.
Moreover, since remote TLBs are not flushed after the temporary PTEs are
removed, the time-window in which the code is writable is not limited if
the fixmap PTEs - maliciously or accidentally - are cached in the TLB.
To address these potential security hazards, we use a temporary mm for
patching the code.
Finally, text_poke() is also not conservative enough when mapping pages,
as it always tries to map 2 pages, even when a single one is sufficient.
So try to be more conservative, and do not map more than needed.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/fixmap.h | 2 -
arch/x86/kernel/alternative.c | 109 +++++++++++++++++++++++++++-------
arch/x86/xen/mmu_pv.c | 2 -
3 files changed, 87 insertions(+), 26 deletions(-)
@@ -103,8 +103,6 @@ enum fixed_addresses {#ifdef CONFIG_PARAVIRTFIX_PARAVIRT_BOOTMAP,#endif-FIX_TEXT_POKE1,/* reserve 2 pages for text_poke() */-FIX_TEXT_POKE0,/* first page is last, because allocation is backward */#ifdef CONFIG_X86_INTEL_MIDFIX_LNW_VRTC,#endif
@@ -683,41 +684,105 @@ __ro_after_init unsigned long poking_addr;staticvoid*__text_poke(void*addr,constvoid*opcode,size_tlen){+boolcross_page_boundary=offset_in_page(addr)+len>PAGE_SIZE;+temporary_mm_state_tprev;+structpage*pages[2]={NULL};unsignedlongflags;-char*vaddr;-structpage*pages[2];-inti;+pte_tpte,*ptep;+spinlock_t*ptl;/*-*Whilebootmemoryallocatorisrunnigwecannotusestruct-*pagesastheyarenotyetinitialized.+*Whilebootmemoryallocatorisrunningwecannotusestructpagesas+*theyarenotyetinitialized.*/BUG_ON(!after_bootmem);if(!core_kernel_text((unsignedlong)addr)){pages[0]=vmalloc_to_page(addr);-pages[1]=vmalloc_to_page(addr+PAGE_SIZE);+if(cross_page_boundary)+pages[1]=vmalloc_to_page(addr+PAGE_SIZE);}else{pages[0]=virt_to_page(addr);WARN_ON(!PageReserved(pages[0]));-pages[1]=virt_to_page(addr+PAGE_SIZE);+if(cross_page_boundary)+pages[1]=virt_to_page(addr+PAGE_SIZE);}-BUG_ON(!pages[0]);+BUG_ON(!pages[0]||(cross_page_boundary&&!pages[1]));+local_irq_save(flags);-set_fixmap(FIX_TEXT_POKE0,page_to_phys(pages[0]));-if(pages[1])-set_fixmap(FIX_TEXT_POKE1,page_to_phys(pages[1]));-vaddr=(char*)fix_to_virt(FIX_TEXT_POKE0);-memcpy(&vaddr[(unsignedlong)addr&~PAGE_MASK],opcode,len);-clear_fixmap(FIX_TEXT_POKE0);-if(pages[1])-clear_fixmap(FIX_TEXT_POKE1);-local_flush_tlb();-sync_core();-/* Could also do a CLFLUSH here to speed up CPU recovery; but-thatcauseshangsonsomeVIACPUs.*/-for(i=0;i<len;i++)-BUG_ON(((char*)addr)[i]!=((char*)opcode)[i]);++/*+*Thelockisnotreallyneeded,butthisallowstoavoidopen-coding.+*/+ptep=get_locked_pte(poking_mm,poking_addr,&ptl);++/*+*Thismustnotfail;preallocatedinpoking_init().+*/+VM_BUG_ON(!ptep);++pte=mk_pte(pages[0],PAGE_KERNEL);+set_pte_at(poking_mm,poking_addr,ptep,pte);++if(cross_page_boundary){+pte=mk_pte(pages[1],PAGE_KERNEL);+set_pte_at(poking_mm,poking_addr+PAGE_SIZE,ptep+1,pte);+}++/*+*Loadingthetemporarymmbehavesasacompilerbarrier,which+*guaranteesthatthePTEwillbesetatthetimememcpy()isdone.+*/+prev=use_temporary_mm(poking_mm);++kasan_disable_current();+memcpy((u8*)poking_addr+offset_in_page(addr),opcode,len);+kasan_enable_current();++/*+*EnsurethatthePTEisonlyclearedaftertheinstructionsofmemcpy+*wereissuedbyusingacompilerbarrier.+*/+barrier();++pte_clear(poking_mm,poking_addr,ptep);++/*+*__flush_tlb_one_user()performsaredundantTLBflushwhenPTIison,+*asitalsoflushesthecorresponding"user"addressspaces,which+*doesnotexist.+*+*Poking,however,isalreadyveryinefficientsinceitdoesnottryto+*batchupdates,soweignorethisproblemforthetimebeing.+*+*SincethePTEsdonotexistinotherkerneladdress-spaces,wedo+*notuse__flush_tlb_one_kernel(),whichwhenPTIisonwouldcause+*moreunwarrantedTLBflushes.+*+*Thereisaslightanomalyhere:thePTEisasupervisor-onlyand+*(potentially)globalandweuse__flush_tlb_one_user()butthis+*shouldbefine.+*/+__flush_tlb_one_user(poking_addr);+if(cross_page_boundary){+pte_clear(poking_mm,poking_addr+PAGE_SIZE,ptep+1);+__flush_tlb_one_user(poking_addr+PAGE_SIZE);+}++/*+*Loadingthepreviouspage-tablehierarchyrequiresaserializing+*instructionthatalreadyallowsthecoretoseetheupdatedversion.+*Xen-PVisassumedtoserializeexecutioninasimilarmanner.+*/+unuse_temporary_mm(prev);++pte_unmap_unlock(ptep,ptl);+/*+*Ifthetextdoesn'tmatchwhatwejustwrote;somethingis+*fundamentallyscrewy,there'snothingwecanreallydoaboutthat.+*/+BUG_ON(memcmp(addr,opcode,len));+local_irq_restore(flags);returnaddr;}
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:18
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not configured.
So this changes kernel_map_pages and kernel_page_present to be defined when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented when
CONFIG_DEBUG_PAGEALLOC is defined.
So now when CONFIG_ARCH_HAS_SET_ALIAS=y, hibernate will handle not present
page when saving. Previously this was already done when CONFIG_DEBUG_PAGEALLOC
was configured. It does not appear to have a big hibernating performance
impact.
Before:
[ 4.670938] PM: Wrote 171996 kbytes in 0.21 seconds (819.02 MB/s)
After:
[ 4.504714] PM: Wrote 178932 kbytes in 0.22 seconds (813.32 MB/s)
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <redacted>
Cc: Pavel Machek <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/mm/pageattr.c | 4 ----
include/linux/mm.h | 18 ++++++------------
mm/page_alloc.c | 6 ++++--
3 files changed, 10 insertions(+), 18 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:28
Add new flag for handling freeing of special permissioned memory in vmalloc
and remove places where memory was set RW before freeing which is no longer
needed.
In kprobes, bpf and ftrace this just adds the flag, and removes the now
unneeded set_memory_ calls before calling vfree.
In modules, the freeing of init sections is moved to a work queue, since
freeing of RO memory is not supported in an interrupt by vmalloc.
Instead of call_rcu, it now uses synchronize_rcu() in the work queue.
Cc: Rusty Russell <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Paul E. McKenney <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 +--
arch/x86/kernel/kprobes/core.c | 7 +---
include/linux/filter.h | 16 ++-----
kernel/bpf/core.c | 1 -
kernel/module.c | 77 +++++++++++++++++-----------------
5 files changed, 45 insertions(+), 62 deletions(-)
@@ -692,10 +692,6 @@ static inline void *alloc_tramp(unsigned long size)}staticinlinevoidtramp_free(void*tramp,intsize){-intnpages=PAGE_ALIGN(size)>>PAGE_SHIFT;--set_memory_nx((unsignedlong)tramp,npages);-set_memory_rw((unsignedlong)tramp,npages);module_memfree(tramp);}#else
@@ -820,6 +816,8 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)/* ALLOC_TRAMP flags lets us know we created it */ops->flags|=FTRACE_OPS_FL_ALLOC_TRAMP;+set_vm_special(trampoline);+/**Moduleallocationneedstobecompletedbymakingthepage*executable.Thepageisstillwritable,whichisasecurityhazard,
@@ -452,12 +453,6 @@ void *alloc_insn_page(void)/* Recover page to RW mode before releasing it */voidfree_insn_page(void*page){-/*-*Firstmakethepagenon-executable,andthenonlythenmakeit-*writabletopreventitfrombeingW+Xinbetween.-*/-set_memory_nx((unsignedlong)page,1);-set_memory_rw((unsignedlong)page,1);module_memfree(page);}
@@ -483,7 +484,6 @@ struct bpf_prog {u16pages;/* Number of allocated pages */u16jited:1,/* Is our filter JIT'ed? */jit_requested:1,/* archs need to JIT the prog */-undo_set_mem:1,/* Passed set_memory_ro() checkpoint */gpl_compatible:1,/* Is filter GPL compatible? */cb_access:1,/* Is control block accessed? */dst_needed:1,/* Do we need dst entry? */
@@ -98,6 +98,10 @@ DEFINE_MUTEX(module_mutex);EXPORT_SYMBOL_GPL(module_mutex);staticLIST_HEAD(modules);+/* Work queue for freeing init sections in success case */+staticstructwork_structinit_free_wq;+staticstructllist_headinit_free_list;+#ifdef CONFIG_MODULES_TREE_LOOKUP/*
@@ -1972,15 +1978,6 @@ static void module_enable_nx(const struct module *mod)frob_writable_data(&mod->init_layout,set_memory_nx);}-staticvoidmodule_disable_nx(conststructmodule*mod)-{-frob_rodata(&mod->core_layout,set_memory_x);-frob_ro_after_init(&mod->core_layout,set_memory_x);-frob_writable_data(&mod->core_layout,set_memory_x);-frob_rodata(&mod->init_layout,set_memory_x);-frob_writable_data(&mod->init_layout,set_memory_x);-}-/* Iterate through all modules and set each module's text as RW */voidset_all_modules_text_rw(void){
@@ -2171,7 +2158,6 @@ static void free_module(struct module *mod)mutex_unlock(&module_mutex);/* This may be empty, but that's OK */-disable_ro_nx(&mod->init_layout);module_arch_freeing_init(mod);module_memfree(mod->init_layout.base);kfree(mod->args);
@@ -2181,7 +2167,6 @@ static void free_module(struct module *mod)lockdep_free_key_range(mod->core_layout.base,mod->core_layout.size);/* Finally, free the core (containing the module structure) */-disable_ro_nx(&mod->core_layout);module_memfree(mod->core_layout.base);}
@@ -3424,17 +3409,34 @@ static void do_mod_ctors(struct module *mod)/* For freeing module_init on success, in case kallsyms traversing */structmod_initfree{-structrcu_headrcu;+structllist_nodenode;void*module_init;};-staticvoiddo_free_init(structrcu_head*head)+staticvoiddo_free_init(structwork_struct*w){-structmod_initfree*m=container_of(head,structmod_initfree,rcu);-module_memfree(m->module_init);-kfree(m);+structllist_node*pos,*n,*list;+structmod_initfree*initfree;++list=llist_del_all(&init_free_list);++synchronize_rcu();++llist_for_each_safe(pos,n,list){+initfree=container_of(pos,structmod_initfree,node);+module_memfree(initfree->module_init);+kfree(initfree);+}}+staticint__initmodules_wq_init(void)+{+INIT_WORK(&init_free_wq,do_free_init);+init_llist_head(&init_free_list);+return0;+}+module_init(modules_wq_init);+/**Thisiswheretherealworkhappens.*
@@ -3511,7 +3513,6 @@ static noinline int do_init_module(struct module *mod)#endifmodule_enable_ro(mod,true);mod_tree_remove_init(mod);-disable_ro_nx(&mod->init_layout);module_arch_freeing_init(mod);mod->init_layout.base=NULL;mod->init_layout.size=0;
@@ -3522,14 +3523,18 @@ static noinline int do_init_module(struct module *mod)*Wewanttofreemodule_init,butbeawarethatkallsymsmaybe*walkingthiswithpreemptdisabled.Inallthefailurepaths,we*callsynchronize_rcu(),butwedon'twanttoslowdownthesuccess-*path,souseactualRCUhere.+*path.Wecan'tdomodule_memfreeinaninterrupt,sowedothework+*andcallsynchronize_rcu()inaworkqueue.+**Notethatmodule_alloc()onmostarchitecturescreatesW+Xpage*mappingswhichwon'tbecleanedupuntildo_free_init()runs.Any*codesuchasmark_rodata_ro()whichdependsonthosemappingsto*becleanedupneedstosyncwiththequeuedwork-ie*rcu_barrier()*/-call_rcu(&freeinit->rcu,do_free_init);+if(llist_add(&freeinit->node,&init_free_list))+schedule_work(&init_free_wq);+mutex_unlock(&module_mutex);wake_up_all(&module_wq);
@@ -3826,10 +3831,6 @@ static int load_module(struct load_info *info, const char __user *uargs,module_bug_cleanup(mod);mutex_unlock(&module_mutex);-/* we can't deallocate the module until we clear memory protection */-module_disable_ro(mod);-module_disable_nx(mod);-ddebug_cleanup:ftrace_release_mod(mod);dynamic_debug_remove(mod,info->debug);
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:45
From: Nadav Amit <redacted>
When modules and BPF filters are loaded, there is a time window in
which some memory is both writable and executable. An attacker that has
already found another vulnerability (e.g., a dangling pointer) might be
able to exploit this behavior to overwrite kernel code. This patch
prevents having writable executable PTEs in this stage.
In addition, avoiding having R+X mappings can also slightly simplify the
patching of modules code on initialization (e.g., by alternatives and
static-key), as would be done in the next patch. This was actually the
main motivation for this patch.
To avoid having W+X mappings, set them initially as RW (NX) and after
they are set as RO set them as X as well. Setting them as executable is
done as a separate step to avoid one core in which the old PTE is cached
(hence writable), and another which sees the updated PTE (executable),
which would break the W^X protection.
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Thomas Gleixner <redacted>
Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/alternative.c | 28 +++++++++++++++++++++-------
arch/x86/kernel/module.c | 2 +-
include/linux/filter.h | 4 ++--
kernel/module.c | 5 +++++
4 files changed, 29 insertions(+), 10 deletions(-)
@@ -667,15 +667,29 @@ void __init alternative_instructions(void)*handlersseeinganinconsistentinstructionwhileyoupatch.*/void*__init_or_moduletext_poke_early(void*addr,constvoid*opcode,-size_tlen)+size_tlen){unsignedlongflags;-local_irq_save(flags);-memcpy(addr,opcode,len);-local_irq_restore(flags);-sync_core();-/* Could also do a CLFLUSH here to speed up CPU recovery; but-thatcauseshangsonsomeVIACPUs.*/++if(static_cpu_has(X86_FEATURE_NX)&&+is_module_text_address((unsignedlong)addr)){+/*+*Modulestextismarkedinitiallyasnon-executable,sothe+*codecannotberunningandspeculativecode-fetchesare+*prevented.Wecanjustchangethecode.+*/+memcpy(addr,opcode,len);+}else{+local_irq_save(flags);+memcpy(addr,opcode,len);+local_irq_restore(flags);+sync_core();++/*+*CouldalsodoaCLFLUSHheretospeedupCPUrecovery;but+*thatcauseshangsonsomeVIACPUs.+*/+}returnaddr;}
@@ -483,7 +483,7 @@ struct bpf_prog {u16pages;/* Number of allocated pages */u16jited:1,/* Is our filter JIT'ed? */jit_requested:1,/* archs need to JIT the prog */-undo_set_mem:1,/* Passed set_memory_ro() checkpoint */+undo_set_mem:1,/* Passed set_memory_ro() checkpoint */gpl_compatible:1,/* Is filter GPL compatible? */cb_access:1,/* Is control block accessed? */dst_needed:1,/* Do we need dst entry? */
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:48
From: Nadav Amit <redacted>
This patch is a preparatory patch for a following patch that makes
module allocated pages non-executable. The patch sets the page as
executable after allocation.
In the future, we may get better protection of executables. For example,
by using hypercalls to request the hypervisor to protect VM executable
pages from modifications using nested page-tables. This would allow
us to ensure the executable has not changed between allocation and
its write-protection.
While at it, do some small cleanup of what appears to be unnecessary
masking.
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/kprobes/core.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -440,8 +452,12 @@ void *alloc_insn_page(void)/* Recover page to RW mode before releasing it */voidfree_insn_page(void*page){-set_memory_nx((unsignedlong)page&PAGE_MASK,1);-set_memory_rw((unsignedlong)page&PAGE_MASK,1);+/*+*Firstmakethepagenon-executable,andthenonlythenmakeit+*writabletopreventitfrombeingW+Xinbetween.+*/+set_memory_nx((unsignedlong)page,1);+set_memory_rw((unsignedlong)page,1);module_memfree(page);}
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:56
From: Nadav Amit <redacted>
Since alloc_module() will not set the pages as executable soon, we need
to do so for ftrace trampoline pages after they are allocated.
For the time being, we do not change ftrace to use the text_poke()
interface. As a result, ftrace breaks still breaks W^X.
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -742,6 +742,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)unsignedlongend_offset;unsignedlongop_offset;unsignedlongoffset;+unsignedlongnpages;unsignedlongsize;unsignedlongretq;unsignedlong*ptr;
@@ -774,6 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)return0;*tramp_size=size+RET_SIZE+sizeof(void*);+npages=DIV_ROUND_UP(*tramp_size,PAGE_SIZE);/* Copy ftrace_caller onto the trampoline memory */ret=probe_kernel_read(trampoline,(void*)start_offset,size);
@@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)/* ALLOC_TRAMP flags lets us know we created it */ops->flags|=FTRACE_OPS_FL_ALLOC_TRAMP;+/*+*Moduleallocationneedstobecompletedbymakingthepage+*executable.Thepageisstillwritable,whichisasecurityhazard,+*butanyhowftracebreaksW^Xcompletely.+*/+set_memory_x((unsignedlong)trampoline,npages);+return(unsignedlong)trampoline;fail:tramp_free(trampoline,*tramp_size);
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:34:58
From: Nadav Amit <redacted>
text_poke() already ensures that the written value is the correct one
and fails if that is not the case. There is no need for an additional
comparison. Remove it.
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/kgdb.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:35:14
From: Nadav Amit <redacted>
Provide a function for copying init_mm. This function will be later used
for setting a temporary mm.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
include/linux/sched/task.h | 1 +
kernel/fork.c | 24 ++++++++++++++++++------
2 files changed, 19 insertions(+), 6 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:35:23
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other core
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely clear
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to something
meaningful. One day, callers might actually respect it and the existing
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in text_poke*()")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <redacted>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 52 ++++++++++++++++++++--------
arch/x86/kernel/kgdb.c | 11 +++---
3 files changed, 45 insertions(+), 19 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:35:28
From: Nadav Amit <redacted>
There are only two types of poking: early and breakpoint based. The use
of a function pointer to perform poking complicates the code and is
probably inefficient due to the use of indirect branches.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/jump_label.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:35:47
From: Andy Lutomirski <luto@kernel.org>
Sometimes we want to set a temporary page-table entries (PTEs) in one of
the cores, without allowing other cores to use - even speculatively -
these mappings. There are two benefits for doing so:
(1) Security: if sensitive PTEs are set, temporary mm prevents their use
in other cores. This hardens the security as it prevents exploding a
dangling pointer to overwrite sensitive data using the sensitive PTE.
(2) Avoiding TLB shootdowns: the PTEs do not need to be flushed in
remote page-tables.
To do so a temporary mm_struct can be used. Mappings which are private
for this mm can be set in the userspace part of the address-space.
During the whole time in which the temporary mm is loaded, interrupts
must be disabled.
The first use-case for temporary PTEs, which will follow, is for poking
the kernel text.
[ Commit message was written by Nadav ]
Cc: Kees Cook <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <redacted>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/mmu_context.h | 32 ++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-01-17 00:36:01
From: Nadav Amit <redacted>
There is no apparent reason not to use text_poke_early() while we are
during early-init and we do not patch code that might be on the stack
(i.e., we'll return to the middle of the patched code). This appears to
be the case of jump-labels, so do so.
This is required for the next patches that would set a temporary mm for
patching, which is initialized after some static-keys are
enabled/disabled.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/jump_label.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
On Wed, 16 Jan 2019 16:32:43 -0800
Rick Edgecombe [off-list ref] wrote:
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other core
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely clear
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to something
meaningful. One day, callers might actually respect it and the existing
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
quoted hunk
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in text_poke*()")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <redacted>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 52 ++++++++++++++++++++--------
arch/x86/kernel/kgdb.c | 11 +++---
3 files changed, 45 insertions(+), 19 deletions(-)
On Wed, 16 Jan 2019 16:32:51 -0800
Rick Edgecombe [off-list ref] wrote:
From: Nadav Amit <redacted>
This patch is a preparatory patch for a following patch that makes
module allocated pages non-executable. The patch sets the page as
executable after allocation.
In the future, we may get better protection of executables. For example,
by using hypercalls to request the hypervisor to protect VM executable
pages from modifications using nested page-tables. This would allow
us to ensure the executable has not changed between allocation and
its write-protection.
While at it, do some small cleanup of what appears to be unnecessary
masking.
OK, then this should be done.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!
@@ -440,8 +452,12 @@ void *alloc_insn_page(void)/* Recover page to RW mode before releasing it */voidfree_insn_page(void*page){-set_memory_nx((unsignedlong)page&PAGE_MASK,1);-set_memory_rw((unsignedlong)page&PAGE_MASK,1);+/*+*Firstmakethepagenon-executable,andthenonlythenmakeit+*writabletopreventitfrombeingW+Xinbetween.+*/+set_memory_nx((unsignedlong)page,1);+set_memory_rw((unsignedlong)page,1);module_memfree(page);}
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
Thank you,
quoted hunk
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
kernel/module.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -2157,6 +2158,9 @@ static void free_module(struct module *mod)synchronize_rcu();mutex_unlock(&module_mutex);+/* Protect against patching of the module while it is being removed */+mutex_lock(&text_mutex);+/* This may be empty, but that's OK */module_arch_freeing_init(mod);module_memfree(mod->init_layout.base);
@@ -2168,6 +2172,7 @@ static void free_module(struct module *mod)/* Finally, free the core (containing the module structure) */module_memfree(mod->core_layout.base);+mutex_unlock(&text_mutex);}void*__symbol_get(constchar*symbol)
From: Pavel Machek <hidden> Date: 2019-01-17 09:39:57
Hi!
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not configured.
So this changes kernel_map_pages and kernel_page_present to be defined when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented when
CONFIG_DEBUG_PAGEALLOC is defined.
Which architectures are that?
Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
don't want broken hibernation in between....
From: Nadav Amit <hidden> Date: 2019-01-17 18:07:11
On Jan 16, 2019, at 11:54 PM, Masami Hiramatsu [off-list ref] wrote:
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
So it appears that you are right and all the users of text_poke() and
text_poke_bp() do install module notifiers, and remove the module from their
internal data structure when they are done (*). As long as they prevent
text_poke*() to be called concurrently (e.g., using jump_label_lock()),
everything is fine.
Having said that, the question is whether you “trust” text_poke*() users to
do so. text_poke() description does not day explicitly that you need to
prevent modules from being removed.
What do you say?
(*) I am not sure about kgdb, but it probably does not matter much
From: Andy Lutomirski <luto@kernel.org> Date: 2019-01-17 20:27:41
On Wed, Jan 16, 2019 at 4:33 PM Rick Edgecombe
[off-list ref] wrote:
From: Nadav Amit <redacted>
text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.
i think this may be sufficient, but barely.
+ pte_clear(poking_mm, poking_addr, ptep);
+
+ /*
+ * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+ * as it also flushes the corresponding "user" address spaces, which
+ * does not exist.
+ *
+ * Poking, however, is already very inefficient since it does not try to
+ * batch updates, so we ignore this problem for the time being.
+ *
+ * Since the PTEs do not exist in other kernel address-spaces, we do
+ * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+ * more unwarranted TLB flushes.
+ *
+ * There is a slight anomaly here: the PTE is a supervisor-only and
+ * (potentially) global and we use __flush_tlb_one_user() but this
+ * should be fine.
+ */
+ __flush_tlb_one_user(poking_addr);
+ if (cross_page_boundary) {
+ pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+ __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+ }
In principle, another CPU could still have the old translation. Your
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do that
with IRQs off. Hmm. I think you should add an inc_mm_tlb_gen() here.
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.
If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.
Also, you're using PAGE_KERNEL. Please tell me that the global bit
isn't set in there.
--Andy
From: Andy Lutomirski <luto@kernel.org> Date: 2019-01-17 20:47:54
On Thu, Jan 17, 2019 at 12:27 PM Andy Lutomirski [off-list ref] wrote:
On Wed, Jan 16, 2019 at 4:33 PM Rick Edgecombe
[off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.
i think this may be sufficient, but barely.
quoted
+ pte_clear(poking_mm, poking_addr, ptep);
+
+ /*
+ * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+ * as it also flushes the corresponding "user" address spaces, which
+ * does not exist.
+ *
+ * Poking, however, is already very inefficient since it does not try to
+ * batch updates, so we ignore this problem for the time being.
+ *
+ * Since the PTEs do not exist in other kernel address-spaces, we do
+ * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+ * more unwarranted TLB flushes.
+ *
+ * There is a slight anomaly here: the PTE is a supervisor-only and
+ * (potentially) global and we use __flush_tlb_one_user() but this
+ * should be fine.
+ */
+ __flush_tlb_one_user(poking_addr);
+ if (cross_page_boundary) {
+ pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+ __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+ }
In principle, another CPU could still have the old translation. Your
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do that
with IRQs off. Hmm. I think you should add an inc_mm_tlb_gen() here.
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.
If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.
Also, you're using PAGE_KERNEL. Please tell me that the global bit
isn't set in there.
Much better solution: do unuse_temporary_mm() and *then*
flush_tlb_mm_range(). This is entirely non-sketchy and should be just
about optimal, too.
--Andy
On January 16, 2019 10:47:01 PM PST, Masami Hiramatsu [off-list ref] wrote:
On Wed, 16 Jan 2019 16:32:43 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other
core
quoted
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely
clear
quoted
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to
something
quoted
meaningful. One day, callers might actually respect it and the
existing
quoted
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
quoted
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in
text_poke*()")
quoted
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <redacted>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 52
return addr;
}
-/**
- * text_poke - Update instructions on a live kernel
- * @addr: address to modify
- * @opcode: source of the copy
- * @len: length to copy
- *
- * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
- * It means the size must be writable atomically and the address
must be aligned
quoted
- * in a way that permits an atomic write. It also makes sure we fit
return addr;
}
+/**
+ * text_poke - Update instructions on a live kernel
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we fit
on a single
quoted
+ * page.
+ */
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+ lockdep_assert_held(&text_mutex);
+
+ return __text_poke(addr, opcode, len);
+}
+
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we fit
on a single
quoted
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other core
is running,
quoted
+ * despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+ return __text_poke(addr, opcode, len);
+}
+
static void do_sync_core(void *info)
{
sync_core();
@@ -758,13 +758,13 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt
*bpt)
quoted
if (!err)
return err;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
@@ -783,12 +783,13 @@ int kgdb_arch_remove_breakpoint(struct
kgdb_bkpt *bpt)
quoted
if (bpt->type != BP_POKE_BREAKPOINT)
goto knl_write;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
locked.
quoted
*/
if (mutex_is_locked(&text_mutex))
goto knl_write;
- text_poke((void *)bpt->bpt_addr, bpt->saved_instr,
if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
goto knl_write;
--
2.17.1
If you are reorganizing this code, please do so so that the caller doesn't have to worry about if it should call text_poke_bp() or text_poke_early(). Right now the caller had to know that, which makes no sense.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Nadav Amit <hidden> Date: 2019-01-17 21:44:01
On Jan 17, 2019, at 12:47 PM, Andy Lutomirski [off-list ref] wrote:
On Thu, Jan 17, 2019 at 12:27 PM Andy Lutomirski [off-list ref] wrote:
quoted
On Wed, Jan 16, 2019 at 4:33 PM Rick Edgecombe
[off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.
i think this may be sufficient, but barely.
quoted
+ pte_clear(poking_mm, poking_addr, ptep);
+
+ /*
+ * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+ * as it also flushes the corresponding "user" address spaces, which
+ * does not exist.
+ *
+ * Poking, however, is already very inefficient since it does not try to
+ * batch updates, so we ignore this problem for the time being.
+ *
+ * Since the PTEs do not exist in other kernel address-spaces, we do
+ * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+ * more unwarranted TLB flushes.
+ *
+ * There is a slight anomaly here: the PTE is a supervisor-only and
+ * (potentially) global and we use __flush_tlb_one_user() but this
+ * should be fine.
+ */
+ __flush_tlb_one_user(poking_addr);
+ if (cross_page_boundary) {
+ pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+ __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+ }
In principle, another CPU could still have the old translation. Your
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do that
with IRQs off. Hmm. I think you should add an inc_mm_tlb_gen() here.
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.
If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.
Also, you're using PAGE_KERNEL. Please tell me that the global bit
isn't set in there.
Much better solution: do unuse_temporary_mm() and *then*
flush_tlb_mm_range(). This is entirely non-sketchy and should be just
about optimal, too.
This solution sounds nice and clean. The fact the global-bit was set didn’t
matter before (since __flush_tlb_one_user would get rid of it no matter
what), but would matter now, so I’ll change it too.
Thanks!
Nadav
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-01-17 22:16:53
On Thu, 2019-01-17 at 10:39 +0100, Pavel Machek wrote:
Hi!
quoted
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
configured.
So this changes kernel_map_pages and kernel_page_present to be defined when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented when
CONFIG_DEBUG_PAGEALLOC is defined.
Which architectures are that?
Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
don't want broken hibernation in between....
Thanks for taking a look. It was added for x86 for patch 13 in this patchset and
there was interest expressed for adding for arm64. If you didn't get the whole
set and want to see let me know and I can send it.
This will break build AFAICT. _debug_pagealloc_enabled variable does
not exist in !CONFIG_DEBUG_PAGEALLOC case.
Pavel
After adding in the CONFIG_ARCH_HAS_SET_ALIAS condition to the ifdefs in this
area it looked a little hard to read to me, so I moved debug_pagealloc_enabled
and extern bool _debug_pagealloc_enabled outside to make it easier. I think you
are right, the actual non-extern variable can not be there, but the reference
here gets optimized out in that case.
Just double checked and it builds for both CONFIG_DEBUG_PAGEALLOC=n and
CONFIG_DEBUG_PAGEALLOC=y for me.
Thanks,
Rick
From: Nadav Amit <hidden> Date: 2019-01-17 22:29:44
On Jan 17, 2019, at 1:43 PM, Nadav Amit [off-list ref] wrote:
quoted
On Jan 17, 2019, at 12:47 PM, Andy Lutomirski [off-list ref] wrote:
On Thu, Jan 17, 2019 at 12:27 PM Andy Lutomirski [off-list ref] wrote:
quoted
On Wed, Jan 16, 2019 at 4:33 PM Rick Edgecombe
[off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.
i think this may be sufficient, but barely.
quoted
+ pte_clear(poking_mm, poking_addr, ptep);
+
+ /*
+ * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+ * as it also flushes the corresponding "user" address spaces, which
+ * does not exist.
+ *
+ * Poking, however, is already very inefficient since it does not try to
+ * batch updates, so we ignore this problem for the time being.
+ *
+ * Since the PTEs do not exist in other kernel address-spaces, we do
+ * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+ * more unwarranted TLB flushes.
+ *
+ * There is a slight anomaly here: the PTE is a supervisor-only and
+ * (potentially) global and we use __flush_tlb_one_user() but this
+ * should be fine.
+ */
+ __flush_tlb_one_user(poking_addr);
+ if (cross_page_boundary) {
+ pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+ __flush_tlb_one_user(poking_addr + PAGE_SIZE);
+ }
In principle, another CPU could still have the old translation. Your
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do that
with IRQs off. Hmm. I think you should add an inc_mm_tlb_gen() here.
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.
If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.
Also, you're using PAGE_KERNEL. Please tell me that the global bit
isn't set in there.
Much better solution: do unuse_temporary_mm() and *then*
flush_tlb_mm_range(). This is entirely non-sketchy and should be just
about optimal, too.
This solution sounds nice and clean. The fact the global-bit was set didn’t
matter before (since __flush_tlb_one_user would get rid of it no matter
what), but would matter now, so I’ll change it too.
Err.. so actually text_poke() might be called with disabled IRQs (by kgdb).
flush_tlb_mm_range() should still work fine even with disabled IRQs since no
core would use poking_mm at this point. I can add a comment to
flush_tlb_mm_range(), but all in all it is actually not very pretty.
In principle, another CPU could still have the old translation.
Your
quoted
quoted
mutex probably makes this impossible, but it makes me nervous.
Ideally you'd use flush_tlb_mm_range(), but I guess you can't do
that
quoted
quoted
with IRQs off. Hmm. I think you should add an inc_mm_tlb_gen()
here.
quoted
quoted
Arguably, if you did that, you could omit the flushes, but maybe
that's silly.
If we start getting new users of use_temporary_mm(), we should give
some serious thought to the SMP semantics.
Also, you're using PAGE_KERNEL. Please tell me that the global bit
isn't set in there.
Much better solution: do unuse_temporary_mm() and *then*
flush_tlb_mm_range(). This is entirely non-sketchy and should be
just
quoted
about optimal, too.
This solution sounds nice and clean. The fact the global-bit was set
didn’t
matter before (since __flush_tlb_one_user would get rid of it no matter
what), but would matter now, so I’ll change it too.
Thanks!
Nadav
You can just disable the global bit at the top level, obviously.
This approach also should make it far easier to do batching if desired.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Nadav Amit <hidden> Date: 2019-01-17 22:39:26
On Jan 17, 2019, at 1:15 PM, hpa@zytor.com wrote:
On January 16, 2019 10:47:01 PM PST, Masami Hiramatsu [off-list ref] wrote:
quoted
On Wed, 16 Jan 2019 16:32:43 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other
core
quoted
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely
clear
quoted
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to
something
quoted
meaningful. One day, callers might actually respect it and the
existing
quoted
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
quoted
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in
text_poke*()")
quoted
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <redacted>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 52
return addr;
}
-/**
- * text_poke - Update instructions on a live kernel
- * @addr: address to modify
- * @opcode: source of the copy
- * @len: length to copy
- *
- * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
- * It means the size must be writable atomically and the address
must be aligned
quoted
- * in a way that permits an atomic write. It also makes sure we fit
return addr;
}
+/**
+ * text_poke - Update instructions on a live kernel
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we fit
on a single
quoted
+ * page.
+ */
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+ lockdep_assert_held(&text_mutex);
+
+ return __text_poke(addr, opcode, len);
+}
+
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we fit
on a single
quoted
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other core
is running,
quoted
+ * despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+ return __text_poke(addr, opcode, len);
+}
+
static void do_sync_core(void *info)
{
sync_core();
@@ -758,13 +758,13 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt
*bpt)
quoted
if (!err)
return err;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
@@ -783,12 +783,13 @@ int kgdb_arch_remove_breakpoint(struct
kgdb_bkpt *bpt)
quoted
if (bpt->type != BP_POKE_BREAKPOINT)
goto knl_write;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
locked.
quoted
*/
if (mutex_is_locked(&text_mutex))
goto knl_write;
- text_poke((void *)bpt->bpt_addr, bpt->saved_instr,
if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
goto knl_write;
--
2.17.1
If you are reorganizing this code, please do so so that the caller doesn’t
have to worry about if it should call text_poke_bp() or text_poke_early().
Right now the caller had to know that, which makes no sense.
Did you look at "[11/17] x86/jump-label: remove support for custom poker”?
https://lore.kernel.org/patchwork/patch/1032857/
If this is not what you regard, please be more concrete. text_poke_early()
is still used directly on init and while modules are loaded, which might not
be great, but is outside of the scope of this patch-set.
On January 17, 2019 2:39:15 PM PST, Nadav Amit [off-list ref] wrote:
quoted
On Jan 17, 2019, at 1:15 PM, hpa@zytor.com wrote:
On January 16, 2019 10:47:01 PM PST, Masami Hiramatsu
[off-list ref] wrote:
quoted
quoted
On Wed, 16 Jan 2019 16:32:43 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead
*supposedly*
quoted
quoted
quoted
ensures the lock is not taken and will not be acquired by any other
core
quoted
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely
clear
quoted
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to
something
quoted
meaningful. One day, callers might actually respect it and the
existing
quoted
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
return addr;
}
-/**
- * text_poke - Update instructions on a live kernel
- * @addr: address to modify
- * @opcode: source of the copy
- * @len: length to copy
- *
- * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
- * It means the size must be writable atomically and the address
must be aligned
quoted
- * in a way that permits an atomic write. It also makes sure we
return addr;
}
+/**
+ * text_poke - Update instructions on a live kernel
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we
fit
quoted
quoted
on a single
quoted
+ * page.
+ */
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+ lockdep_assert_held(&text_mutex);
+
+ return __text_poke(addr, opcode, len);
+}
+
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we
fit
quoted
quoted
on a single
quoted
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other
core
quoted
quoted
is running,
quoted
+ * despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+ return __text_poke(addr, opcode, len);
+}
+
static void do_sync_core(void *info)
{
sync_core();
@@ -758,13 +758,13 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt
*bpt)
quoted
if (!err)
return err;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
@@ -783,12 +783,13 @@ int kgdb_arch_remove_breakpoint(struct
kgdb_bkpt *bpt)
quoted
if (bpt->type != BP_POKE_BREAKPOINT)
goto knl_write;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
locked.
quoted
*/
if (mutex_is_locked(&text_mutex))
goto knl_write;
- text_poke((void *)bpt->bpt_addr, bpt->saved_instr,
if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
goto knl_write;
--
2.17.1
If you are reorganizing this code, please do so so that the caller
doesn’t
quoted
have to worry about if it should call text_poke_bp() or
text_poke_early().
quoted
Right now the caller had to know that, which makes no sense.
Did you look at "[11/17] x86/jump-label: remove support for custom
poker”?
https://lore.kernel.org/patchwork/patch/1032857/
If this is not what you regard, please be more concrete.
text_poke_early()
is still used directly on init and while modules are loaded, which
might not
be great, but is outside of the scope of this patch-set.
I don't think it is out of scope, although that patch is a huge step in the right direction.
text_poke_{early,bp,...}, however, should be fully internal, that is, static functions, and we should present a single interface, preferably called text_poke(), to the outside world.
I think we have three subcases:
1. Early, UP, or under stop_machine();
2. Atomic and aligned;
3. Breakpoint.
My proposed algorithm should remove the need for a fixup which should help this interface, too.
The specific alignment needed for #2 is started by the hardware people to be not crossing 16 bytes (NOT a cache line) on any CPU we support SMP on and, of course, being possible to do atomically do on the specific CPU (note that we *can* do a redundantly large store of existing bytes, which adds flexibility.)
To the best of my knowledge any CPU supporting SSE can do an atomic (for our purposes) aligned 16-byte store via MOVAPS; of course any CPU with cx16 can do it without SSE registers. For older CPUs we may be limited to 8-byte stores (cx8) or even 4-byte stores before we need to use the breakpoint algorithm.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Nadav Amit <hidden> Date: 2019-01-17 23:14:38
On Jan 17, 2019, at 2:59 PM, hpa@zytor.com wrote:
On January 17, 2019 2:39:15 PM PST, Nadav Amit [off-list ref] wrote:
quoted
quoted
On Jan 17, 2019, at 1:15 PM, hpa@zytor.com wrote:
On January 16, 2019 10:47:01 PM PST, Masami Hiramatsu
[off-list ref] wrote:
quoted
quoted
On Wed, 16 Jan 2019 16:32:43 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead
*supposedly*
quoted
quoted
quoted
ensures the lock is not taken and will not be acquired by any other
core
quoted
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely
clear
quoted
that this would be the case if gdb_do_roundup is zero.
This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to
something
quoted
meaningful. One day, callers might actually respect it and the
existing
quoted
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
return addr;
}
-/**
- * text_poke - Update instructions on a live kernel
- * @addr: address to modify
- * @opcode: source of the copy
- * @len: length to copy
- *
- * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
- * It means the size must be writable atomically and the address
must be aligned
quoted
- * in a way that permits an atomic write. It also makes sure we
return addr;
}
+/**
+ * text_poke - Update instructions on a live kernel
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we
fit
quoted
quoted
on a single
quoted
+ * page.
+ */
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+ lockdep_assert_held(&text_mutex);
+
+ return __text_poke(addr, opcode, len);
+}
+
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing
early
quoted
quoted
patching.
quoted
+ * It means the size must be writable atomically and the address
must be aligned
quoted
+ * in a way that permits an atomic write. It also makes sure we
fit
quoted
quoted
on a single
quoted
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other
core
quoted
quoted
is running,
quoted
+ * despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+ return __text_poke(addr, opcode, len);
+}
+
static void do_sync_core(void *info)
{
sync_core();
@@ -758,13 +758,13 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt
*bpt)
quoted
if (!err)
return err;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
@@ -783,12 +783,13 @@ int kgdb_arch_remove_breakpoint(struct
kgdb_bkpt *bpt)
quoted
if (bpt->type != BP_POKE_BREAKPOINT)
goto knl_write;
/*
- * It is safe to call text_poke() because normal kernel execution
+ * It is safe to call text_poke_kgdb() because normal kernel
execution
quoted
* is stopped on all cores, so long as the text_mutex is not
locked.
quoted
*/
if (mutex_is_locked(&text_mutex))
goto knl_write;
- text_poke((void *)bpt->bpt_addr, bpt->saved_instr,
if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
goto knl_write;
--
2.17.1
If you are reorganizing this code, please do so so that the caller
doesn’t
quoted
have to worry about if it should call text_poke_bp() or
text_poke_early().
quoted
Right now the caller had to know that, which makes no sense.
Did you look at "[11/17] x86/jump-label: remove support for custom
poker”?
https://lore.kernel.org/patchwork/patch/1032857/
If this is not what you regard, please be more concrete.
text_poke_early()
is still used directly on init and while modules are loaded, which
might not
be great, but is outside of the scope of this patch-set.
I don't think it is out of scope, although that patch is a huge step in the right direction.
text_poke_{early,bp,...}, however, should be fully internal, that is, static functions, and we should present a single interface, preferably called text_poke(), to the outside world.
I think we have three subcases:
1. Early, UP, or under stop_machine();
2. Atomic and aligned;
3. Breakpoint.
My proposed algorithm should remove the need for a fixup which should help this interface, too.
That’s another reason why such change might be done later (after your
changes are merged). The main reason is that Rick was kind enough to
deal with the whole patch-set.
The specific alignment needed for #2 is started by the hardware people to be not crossing 16 bytes (NOT a cache line) on any CPU we support SMP on and, of course, being possible to do atomically do on the specific CPU (note that we *can* do a redundantly large store of existing bytes, which adds flexibility.)
To the best of my knowledge any CPU supporting SSE can do an atomic (for our purposes) aligned 16-byte store via MOVAPS; of course any CPU with cx16 can do it without SSE registers. For older CPUs we may be limited to 8-byte stores (cx8) or even 4-byte stores before we need to use the breakpoint algorithm.
So the last time we had this discussion, I could not be convinced that
hypervisors (e.g, KVM), which do not follow this undocumented behavior,
would not break. I also don’t remember an official confirmation of this
behavior on Intel and AMD CPUs.
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-01-17 23:20:02
I think we have three subcases:
1. Early, UP, or under stop_machine();
2. Atomic and aligned;
3. Breakpoint.
My proposed algorithm should remove the need for a fixup which should help this interface, too.
The specific alignment needed for #2 is started by the hardware people to be not crossing 16 bytes (NOT a cache line) on any CPU we support SMP on and, of course, being possible to do atomically do on the specific CPU (note that we *can* do a redundantly large store of existing bytes, which adds flexibility.)
To the best of my knowledge any CPU supporting SSE can do an atomic (for our purposes) aligned 16-byte store via MOVAPS; of course any CPU with cx16 can do it without SSE registers. For older CPUs we may be limited to 8-byte stores (cx8) or even 4-byte stores before we need to use the breakpoint algorithm.
Sending to a restricted list, because I don't actually know how publicly
known this is, but it is known there are operating systems in the field
already which rely on the 16-byte atomicity guarantee.
-hpa
From: Pavel Machek <hidden> Date: 2019-01-17 23:41:18
Hi!
quoted
quoted
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
configured.
So this changes kernel_map_pages and kernel_page_present to be defined when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented when
CONFIG_DEBUG_PAGEALLOC is defined.
Which architectures are that?
Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
don't want broken hibernation in between....
Thanks for taking a look. It was added for x86 for patch 13 in this patchset and
there was interest expressed for adding for arm64. If you didn't get the whole
set and want to see let me know and I can send it.
I googled in in the meantime.
Anyway, if something is broken between patch 13 and 14, then they
should be same patch.
This will break build AFAICT. _debug_pagealloc_enabled variable does
not exist in !CONFIG_DEBUG_PAGEALLOC case.
Pavel
After adding in the CONFIG_ARCH_HAS_SET_ALIAS condition to the ifdefs in this
area it looked a little hard to read to me, so I moved debug_pagealloc_enabled
and extern bool _debug_pagealloc_enabled outside to make it easier. I think you
are right, the actual non-extern variable can not be there, but the reference
here gets optimized out in that case.
Just double checked and it builds for both CONFIG_DEBUG_PAGEALLOC=n and
CONFIG_DEBUG_PAGEALLOC=y for me.
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-01-17 23:45:29
On 1/17/19 10:07 AM, Nadav Amit wrote:
quoted
On Jan 16, 2019, at 11:54 PM, Masami Hiramatsu [off-list ref] wrote:
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
So it appears that you are right and all the users of text_poke() and
text_poke_bp() do install module notifiers, and remove the module from their
internal data structure when they are done (*). As long as they prevent
text_poke*() to be called concurrently (e.g., using jump_label_lock()),
everything is fine.
Having said that, the question is whether you “trust” text_poke*() users to
do so. text_poke() description does not day explicitly that you need to
prevent modules from being removed.
What do you say?
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-01-17 23:48:37
On Fri, 2019-01-18 at 00:41 +0100, Pavel Machek wrote:
Hi!
quoted
quoted
quoted
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
configured.
So this changes kernel_map_pages and kernel_page_present to be defined
when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented
when
CONFIG_DEBUG_PAGEALLOC is defined.
Which architectures are that?
Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
don't want broken hibernation in between....
Thanks for taking a look. It was added for x86 for patch 13 in this patchset
and
there was interest expressed for adding for arm64. If you didn't get the
whole
set and want to see let me know and I can send it.
I googled in in the meantime.
Anyway, if something is broken between patch 13 and 14, then they
should be same patch.
Great. It should be ok because the new functions are not used anywhere until
after this patch.
Thanks,
Rick
This will break build AFAICT. _debug_pagealloc_enabled variable does
not exist in !CONFIG_DEBUG_PAGEALLOC case.
Pavel
After adding in the CONFIG_ARCH_HAS_SET_ALIAS condition to the ifdefs in
this
area it looked a little hard to read to me, so I moved
debug_pagealloc_enabled
and extern bool _debug_pagealloc_enabled outside to make it easier. I think
you
are right, the actual non-extern variable can not be there, but the
reference
here gets optimized out in that case.
Just double checked and it builds for both CONFIG_DEBUG_PAGEALLOC=n and
CONFIG_DEBUG_PAGEALLOC=y for me.
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-01-17 23:59:09
On 1/16/19 11:54 PM, Masami Hiramatsu wrote:
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
The issue isn't the module list, but rather when it is safe to free the
contents, so we don't clobber anything. We absolutely need to enforce
that we can't text_poke() something that might have already been freed.
That being said, we *also* really would prefer to enforce that we can't
text_poke() memory that doesn't actually contain code; as far as I can
tell we don't currently do that check.
This, again, is a good use for a separate mm context. We can enforce
that that context will only ever contain valid page mappings for actual
code pages.
(Note: in my proposed algorithm, with a separate mm, replace INVLPG with
switching CR3 if we have to do a rollback or roll forward in the
breakpoint handler.)
-hpa
From: Nadav Amit <hidden> Date: 2019-01-18 01:15:34
On Jan 17, 2019, at 3:58 PM, H. Peter Anvin [off-list ref] wrote:
On 1/16/19 11:54 PM, Masami Hiramatsu wrote:
quoted
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
The issue isn't the module list, but rather when it is safe to free the
contents, so we don't clobber anything. We absolutely need to enforce
that we can't text_poke() something that might have already been freed.
That being said, we *also* really would prefer to enforce that we can't
text_poke() memory that doesn't actually contain code; as far as I can
tell we don't currently do that check.
Yes, that what the mutex was supposed to achieve. It’s not supposed just
to check whether it is a code page, but also that it is the same code
page that you wanted to patch.
This, again, is a good use for a separate mm context. We can enforce
that that context will only ever contain valid page mappings for actual
code pages.
This will not tell you that you have the *right* code-page. The module
notifiers help to do so, since they synchronize the text poking with
the module removal.
(Note: in my proposed algorithm, with a separate mm, replace INVLPG with
switching CR3 if we have to do a rollback or roll forward in the
breakpoint handler.)
I really need to read your patches more carefully to see what you mean.
Anyhow, so what do you prefer? I’m ok with either one:
1. Keep this patch
2. Remove this patch and change into a comment on text_poke()
3. Just drop the patch
From: Nadav Amit <hidden> Date: 2019-01-18 02:40:57
On Jan 17, 2019, at 3:19 PM, H. Peter Anvin [off-list ref] wrote:
quoted
I think we have three subcases:
1. Early, UP, or under stop_machine();
2. Atomic and aligned;
3. Breakpoint.
My proposed algorithm should remove the need for a fixup which should help this interface, too.
The specific alignment needed for #2 is started by the hardware people to be not crossing 16 bytes (NOT a cache line) on any CPU we support SMP on and, of course, being possible to do atomically do on the specific CPU (note that we *can* do a redundantly large store of existing bytes, which adds flexibility.)
To the best of my knowledge any CPU supporting SSE can do an atomic (for our purposes) aligned 16-byte store via MOVAPS; of course any CPU with cx16 can do it without SSE registers. For older CPUs we may be limited to 8-byte stores (cx8) or even 4-byte stores before we need to use the breakpoint algorithm.
Sending to a restricted list, because I don't actually know how publicly
known this is, but it is known there are operating systems in the field
already which rely on the 16-byte atomicity guarantee.
Hmm. I don’t know how restricted the list is, considering you left "LSM list”.
;-)
Anyhow, I don’t know whether it changes things much. If you patch an MMIO
accessing instruction, which is executed on top of KVM - things might break.
IMHO, the very least hypervisors vendors should be informed before
implementing such change.
From: Pavel Machek <hidden> Date: 2019-01-18 08:16:14
On Thu 2019-01-17 23:48:30, Edgecombe, Rick P wrote:
On Fri, 2019-01-18 at 00:41 +0100, Pavel Machek wrote:
quoted
Hi!
quoted
quoted
quoted
For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
configured.
So this changes kernel_map_pages and kernel_page_present to be defined
when
CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
(page_alloc.c) where those functions are assumed to only be implemented
when
CONFIG_DEBUG_PAGEALLOC is defined.
Which architectures are that?
Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
don't want broken hibernation in between....
Thanks for taking a look. It was added for x86 for patch 13 in this patchset
and
there was interest expressed for adding for arm64. If you didn't get the
whole
set and want to see let me know and I can send it.
I googled in in the meantime.
Anyway, if something is broken between patch 13 and 14, then they
should be same patch.
Great. It should be ok because the new functions are not used anywhere until
after this patch.
On Thu, 17 Jan 2019 18:07:03 +0000
Nadav Amit [off-list ref] wrote:
quoted
On Jan 16, 2019, at 11:54 PM, Masami Hiramatsu [off-list ref] wrote:
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
So it appears that you are right and all the users of text_poke() and
text_poke_bp() do install module notifiers, and remove the module from their
internal data structure when they are done (*). As long as they prevent
text_poke*() to be called concurrently (e.g., using jump_label_lock()),
everything is fine.
Having said that, the question is whether you “trust” text_poke*() users to
do so. text_poke() description does not day explicitly that you need to
prevent modules from being removed.
What do you say?
I agreed, but in that case, this is just a fool proof. I think we should
prevent this kind of bug by review, and should comment it on text_poke(),
instead of locking text_mutex.
What I thought was even if we take text_mutex here, such user can modify
the (released) module code right after we exit this section.
Maybe we'd better make text_poke() more smart?
(*) I am not sure about kgdb, but it probably does not matter much
I think we don't need to care about kgdb. It is a tool which should be able
to shoot your feet and we can not prevent it. Only expert can avoid it. :)
Thank you,
--
Masami Hiramatsu [off-list ref]
On Thu, 17 Jan 2019 17:15:27 -0800
Nadav Amit [off-list ref] wrote:
quoted
On Jan 17, 2019, at 3:58 PM, H. Peter Anvin [off-list ref] wrote:
On 1/16/19 11:54 PM, Masami Hiramatsu wrote:
quoted
On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.
At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?
The issue isn't the module list, but rather when it is safe to free the
contents, so we don't clobber anything. We absolutely need to enforce
that we can't text_poke() something that might have already been freed.
That being said, we *also* really would prefer to enforce that we can't
text_poke() memory that doesn't actually contain code; as far as I can
tell we don't currently do that check.
Yes, that what the mutex was supposed to achieve. It’s not supposed just
to check whether it is a code page, but also that it is the same code
page that you wanted to patch.
quoted
This, again, is a good use for a separate mm context. We can enforce
that that context will only ever contain valid page mappings for actual
code pages.
This will not tell you that you have the *right* code-page. The module
notifiers help to do so, since they synchronize the text poking with
the module removal.
quoted
(Note: in my proposed algorithm, with a separate mm, replace INVLPG with
switching CR3 if we have to do a rollback or roll forward in the
breakpoint handler.)
I really need to read your patches more carefully to see what you mean.
Anyhow, so what do you prefer? I’m ok with either one:
1. Keep this patch
2. Remove this patch and change into a comment on text_poke()
3. Just drop the patch
I would prefer 2. so at least we should add a comment to text_poke().
Thank you,
--
Masami Hiramatsu [off-list ref]
On Wed, Jan 16, 2019 at 04:32:43PM -0800, Rick Edgecombe wrote:
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other core
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely clear
that this would be the case if gdb_do_roundup is zero.
I guess that variable name is "kgdb_do_roundup" ?
This patch creates two wrapper functions, text_poke() and
Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.
Also, do
$ git grep 'This patch' Documentation/process
for more details.
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.
While we are at it, change the return code of text_poke() to something
meaningful. One day, callers might actually respect it and the existing
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in text_poke*()")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <redacted>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 52 ++++++++++++++++++++--------
arch/x86/kernel/kgdb.c | 11 +++---
3 files changed, 45 insertions(+), 19 deletions(-)
...
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early patching.
+ * It means the size must be writable atomically and the address must be aligned
+ * in a way that permits an atomic write. It also makes sure we fit on a single
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other core is running,
+ * despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
text_poke_unlocked() I guess. I don't think kgdb is that special that it
needs its own function flavor.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
From: Nadav Amit <hidden> Date: 2019-01-25 18:28:20
On Jan 25, 2019, at 1:30 AM, Borislav Petkov [off-list ref] wrote:
On Wed, Jan 16, 2019 at 04:32:43PM -0800, Rick Edgecombe wrote:
quoted
From: Nadav Amit <redacted>
text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other core
while text_poke() is running.
The reason for the "supposedly" comment is that it is not entirely clear
that this would be the case if gdb_do_roundup is zero.
I guess that variable name is "kgdb_do_roundup” ?
Yes. Will fix.
quoted
This patch creates two wrapper functions, text_poke() and
Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.
Also, do
$ git grep 'This patch' Documentation/process
for more details.
text_poke_unlocked() I guess. I don't think kgdb is that special that it
needs its own function flavor.
Tglx suggested this naming to prevent anyone from misusing text_poke_kdgb().
This is a very specific use-case that nobody else should need.
Regards,
Nadav
From: Steven Rostedt <rostedt@goodmis.org> Date: 2019-02-06 16:22:23
On Wed, 16 Jan 2019 16:32:50 -0800
Rick Edgecombe [off-list ref] wrote:
quoted hunk
From: Nadav Amit <redacted>
Since alloc_module() will not set the pages as executable soon, we need
to do so for ftrace trampoline pages after they are allocated.
For the time being, we do not change ftrace to use the text_poke()
interface. As a result, ftrace breaks still breaks W^X.
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -742,6 +742,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)unsignedlongend_offset;unsignedlongop_offset;unsignedlongoffset;+unsignedlongnpages;unsignedlongsize;unsignedlongretq;unsignedlong*ptr;
@@ -774,6 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)return0;*tramp_size=size+RET_SIZE+sizeof(void*);+npages=DIV_ROUND_UP(*tramp_size,PAGE_SIZE);/* Copy ftrace_caller onto the trampoline memory */ret=probe_kernel_read(trampoline,(void*)start_offset,size);
@@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)/* ALLOC_TRAMP flags lets us know we created it */ops->flags|=FTRACE_OPS_FL_ALLOC_TRAMP;+/*+*Moduleallocationneedstobecompletedbymakingthepage+*executable.Thepageisstillwritable,whichisasecurityhazard,+*butanyhowftracebreaksW^Xcompletely.+*/
Perhaps we should set the page to non writable after the page is
updated? And set it to writable only when we need to update it.
As for this patch:
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2019-02-06 16:24:03
On Wed, 16 Jan 2019 16:32:58 -0800
Rick Edgecombe [off-list ref] wrote:
Add new flag for handling freeing of special permissioned memory in vmalloc
and remove places where memory was set RW before freeing which is no longer
needed.
In kprobes, bpf and ftrace this just adds the flag, and removes the now
unneeded set_memory_ calls before calling vfree.
In modules, the freeing of init sections is moved to a work queue, since
freeing of RO memory is not supported in an interrupt by vmalloc.
Instead of call_rcu, it now uses synchronize_rcu() in the work queue.
Cc: Rusty Russell <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Paul E. McKenney <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 +--
For the ftrace code.
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
From: Nadav Amit <hidden> Date: 2019-02-06 17:33:44
On Feb 6, 2019, at 8:22 AM, Steven Rostedt [off-list ref] wrote:
On Wed, 16 Jan 2019 16:32:50 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
From: Nadav Amit <redacted>
Since alloc_module() will not set the pages as executable soon, we need
to do so for ftrace trampoline pages after they are allocated.
For the time being, we do not change ftrace to use the text_poke()
interface. As a result, ftrace breaks still breaks W^X.
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -742,6 +742,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
unsigned long end_offset;
unsigned long op_offset;
unsigned long offset;
+ unsigned long npages;
unsigned long size;
unsigned long retq;
unsigned long *ptr;
@@ -774,6 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
@@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
/* ALLOC_TRAMP flags lets us know we created it */
ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
+ /*
+ * Module allocation needs to be completed by making the page
+ * executable. The page is still writable, which is a security hazard,
+ * but anyhow ftrace breaks W^X completely.
+ */
Perhaps we should set the page to non writable after the page is
updated? And set it to writable only when we need to update it.
You remember that I sent you a patch that changed all these writes into
text_poke() and you said that I should defer it until this series is merged?
As for this patch:
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
From: Steven Rostedt <rostedt@goodmis.org> Date: 2019-02-06 17:41:17
On Wed, 6 Feb 2019 09:33:35 -0800
Nadav Amit [off-list ref] wrote:
quoted
quoted
/* Copy ftrace_caller onto the trampoline memory */
ret = probe_kernel_read(trampoline, (void *)start_offset, size);
@@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
/* ALLOC_TRAMP flags lets us know we created it */
ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
+ /*
+ * Module allocation needs to be completed by making the page
+ * executable. The page is still writable, which is a security hazard,
+ * but anyhow ftrace breaks W^X completely.
+ */
Perhaps we should set the page to non writable after the page is
updated? And set it to writable only when we need to update it.
You remember that I sent you a patch that changed all these writes into
text_poke() and you said that I should defer it until this series is merged?
And I notice that it is set to RO after this call anyway.
-- Steve
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-02-07 17:33:50
On Wed, 2019-02-06 at 11:23 -0500, Steven Rostedt wrote:
On Wed, 16 Jan 2019 16:32:58 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
Add new flag for handling freeing of special permissioned memory in vmalloc
and remove places where memory was set RW before freeing which is no longer
needed.
In kprobes, bpf and ftrace this just adds the flag, and removes the now
unneeded set_memory_ calls before calling vfree.
In modules, the freeing of init sections is moved to a work queue, since
freeing of RO memory is not supported in an interrupt by vmalloc.
Instead of call_rcu, it now uses synchronize_rcu() in the work queue.
Cc: Rusty Russell <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Paul E. McKenney <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 +--
For the ftrace code.
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-02-07 18:20:53
On Thu, 2019-02-07 at 12:49 -0500, Steven Rostedt wrote:
On Thu, 7 Feb 2019 17:33:37 +0000
"Edgecombe, Rick P" [off-list ref] wrote:
quoted
quoted
quoted
---
arch/x86/kernel/ftrace.c | 6 +--
For the ftrace code.
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
Thanks!
I just noticed that the subject is incorrect; It is missing the
"subsystem:" part. See Documentation/process/submitting-patches.rst
-- Steve
Sorry about that. There is actually v2 of this patchset out there, where there
are no code changes for this patch, but it is split into separate patches for
each subsystem. It has "x86/ftrace: " for the ftrace patch.
Rick