From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:50:55
This patchset improves several overlapping issues around stale TLB entries and
W^X violations. It is combined from "x86/alternative: text_poke() enhancements
v7" [1] and "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.
Changes v2 to v3:
- Fix commit messages and comments [Boris]
- Rename VM_HAS_SPECIAL_PERMS [Boris]
- Remove unnecessary local variables [Boris]
- Rename set_alias_*() functions [Boris, Andy]
- Save/restore DR registers when using temporary mm
- Move line deletion from patch 10 to patch 17
Changes v1 to v2:
- Adding “Reviewed-by tag” [Masami]
- Comment instead of code to warn against module removal while
patching [Masami]
- Avoiding open-coded TLB flush [Andy]
- Remove "This patch" [Borislav Petkov]
- Not set global bit during text poking [Andy, hpa]
- Add Ack from [Pavel Machek]
- Split patch 16 "Plug in new special vfree flag" into 4 patches (16-19)
to make it easier to review. There were no code changes.
The changes from "Don’t leave executable TLB entries to freed pages
v2" to v1:
- 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)
Changes from "x86/alternative: text_poke() enhancements v7" to v1
- 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: Introduce temporary mm structs
Nadav Amit (12):
x86/jump_label: Use text_poke_early() during early init
x86/mm: Save DRs when loading a temporary mm
fork: Provide a function for copying init_mm
x86/alternative: Initialize 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: Set instruction page as executable
x86/module: Avoid breaking W^X while loading modules
x86/jump-label: Remove support for custom poker
x86/alternative: Remove the return value of text_poke_*()
x86/alternative: Comment about module removal races
Rick Edgecombe (7):
x86/mm/cpa: Add set_direct_map_ functions
mm: Make hibernate handle unmapped pages
vmalloc: Add flag for free of special permsissions
modules: Use vmalloc special flag
bpf: Use vmalloc special flag
x86/ftrace: Use vmalloc special flag
x86/kprobes: Use vmalloc special flag
arch/Kconfig | 4 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/fixmap.h | 2 -
arch/x86/include/asm/mmu_context.h | 58 ++++++++++
arch/x86/include/asm/pgtable.h | 3 +
arch/x86/include/asm/set_memory.h | 3 +
arch/x86/include/asm/text-patching.h | 6 +-
arch/x86/kernel/alternative.c | 153 +++++++++++++++++++++------
arch/x86/kernel/ftrace.c | 14 ++-
arch/x86/kernel/jump_label.c | 21 ++--
arch/x86/kernel/kgdb.c | 14 +--
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 | 82 +++++++-------
kernel/power/snapshot.c | 5 +-
mm/page_alloc.c | 7 +-
mm/vmalloc.c | 113 ++++++++++++++++----
28 files changed, 475 insertions(+), 174 deletions(-)
--
2.17.1
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:00
From: Nadav Amit <redacted>
There is no apparent reason not to use text_poke_early() during
early-init, since no patching of code that might be on the stack is done
and only a single core is running.
This is required for the next patches that would set a temporary mm for
text poking, and this mm is only 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>
Acked-by: Peter Zijlstra (Intel) <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(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:03
From: Nadav Amit <redacted>
To prevent improper use of the PTEs that are used for text patching, the
next patches will use a temporary mm struct. 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, randomize the address of the PTEs to harden against exploits
that use these PTEs.
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
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-02-21 23:51:05
From: Nadav Amit <redacted>
Since alloc_module() will not set the pages as executable soon, set
ftrace trampoline pages as executable after they are allocated.
For the time being, do not change ftrace to use the text_poke()
interface. As a result, ftrace still breaks W^X.
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 8 ++++++++
1 file changed, 8 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,12 @@ 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-02-21 23:51:06
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.
While at it, do some small cleanup of what appears to be unnecessary
masking.
Acked-by: 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,andonlythenmakeitwritableto+*preventitfrombeingW+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-02-21 23:51:14
Use new flag VM_FLUSH_RESET_PERMS for handling freeing of special
permissioned memory in vmalloc and remove places where memory was set NX
and RW before freeing which is no longer needed.
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 ++----
1 file changed, 2 insertions(+), 4 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_flush_reset_perms(trampoline);+/**Moduleallocationneedstobecompletedbymakingthepage*executable.Thepageisstillwritable,whichisasecurityhazard,
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:18
From: Nadav Amit <redacted>
Add a comment to clarify that users of text_poke() must ensure that
no races with module removal take place.
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/alternative.c | 5 +++++
1 file changed, 5 insertions(+)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:23
Use 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.
Since freeing of VM_FLUSH_RESET_PERMS memory is not supported in an
interrupt by vmalloc, the freeing of init sections is moved to a work
queue. Instead of call_rcu it now uses synchronize_rcu() in the work
queue.
Lastly, there is now a WARN_ON in module_memfree since it should not be
called in an interrupt with special memory as is required for
VM_FLUSH_RESET_PERMS.
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
kernel/module.c | 77 +++++++++++++++++++++++++------------------------
1 file changed, 39 insertions(+), 38 deletions(-)
@@ -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.module_memfree()cannotbecalledinaninterrupt,sodothe+*workandcallsynchronize_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-02-21 23:51:26
Use new flag VM_FLUSH_RESET_PERMS for handling freeing of special
permissioned memory in vmalloc and remove places where memory was set NX
and RW before freeing which is no longer needed.
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/kprobes/core.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -452,12 +453,6 @@ void *alloc_insn_page(void)/* Recover page to RW mode before releasing it */voidfree_insn_page(void*page){-/*-*Firstmakethepagenon-executable,andonlythenmakeitwritableto-*preventitfrombeingW+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-02-21 23:51:33
Use new flag VM_FLUSH_RESET_PERMS for handling freeing of special
permissioned memory in vmalloc and remove places where memory was set RW
before freeing which is no longer needed. Don't track if the memory is RO
anymore because it is now tracked in vmalloc.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
include/linux/filter.h | 17 +++--------------
kernel/bpf/core.c | 1 -
2 files changed, 3 insertions(+), 15 deletions(-)
@@ -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? */
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:38
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: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.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-02-21 23:51:43
Add a new flag VM_FLUSH_RESET_PERMS, for enabling vfree operations to
immediately clear executable TLB entries before freeing pages, and handle
resetting permissions on the directmap. 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 non-writeable memory now,
non-writable 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 was added to
vmalloc.
For architectures with set_direct_map_ 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_direct_map_ 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.
Cc: Borislav Petkov <bp@alien8.de>
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 | 113 +++++++++++++++++++++++++++++++++-------
2 files changed, 107 insertions(+), 19 deletions(-)
@@ -1494,6 +1501,72 @@ struct vm_struct *remove_vm_area(const void *addr)returnNULL;}+staticinlinevoidset_area_direct_map(conststructvm_struct*area,+int(*set_direct_map)(structpage*page))+{+inti;++for(i=0;i<area->nr_pages;i++)+if(page_address(area->pages[i]))+set_direct_map(area->pages[i]);+}++/* Handle 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;+intflush_reset=area->flags&VM_FLUSH_RESET_PERMS;+inti;++/*+*Thebelowblockcanberemovedwhenallarchitecturesthathave+*directmappermissionsalsohaveset_direct_map_()implementations.+*Thisisconcernedwithresettingthedirectmapanyanvmaliaswith+*executepermissions,withoutleavingaRW+Xwindow.+*/+if(flush_reset&&!IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)){+set_memory_nx(addr,area->nr_pages);+set_memory_rw(addr,area->nr_pages);+}++remove_vm_area(area->addr);++/* If this is not VM_FLUSH_RESET_PERMS memory, no need for the below. */+if(!flush_reset)+return;++/*+*Ifnotdeallocatingpages,justdotheflushoftheVMareaand+*return.+*/+if(!deallocate_pages){+vm_unmap_aliases();+return;+}++/*+*Ifexecutiongetshere,flushthevmmappingandresetthedirect+*map.Findthestartandendrangeofthedirectmappingstomakesure+*thevm_unmap_aliases()flushincludesthedirectmap.+*/+for(i=0;i<area->nr_pages;i++){+if(page_address(area->pages[i])){+start=min(addr,start);+end=max(addr,end);+}+}++/*+*Setdirectmaptosomethinginvalidsothatitwon'tbecachedif+*thereareanyaccessesaftertheTLBflush,thenflushtheTLBand+*resetthedirectmappermissionstothedefault.+*/+set_area_direct_map(area,set_direct_map_invalid_noflush);+_vm_unmap_aliases(start,end,1);+set_area_direct_map(area,set_direct_map_default_noflush);+}+staticvoid__vunmap(constvoid*addr,intdeallocate_pages){structvm_struct*area;
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:51
Make hibernate handle unmapped pages on the direct map when
CONFIG_ARCH_HAS_SET_ALIAS is set. These functions allow for setting pages
to invalid configurations, so now hibernate should check if the pages have
valid mappings and handle if they are unmapped when doing a hibernate
save operation.
Previously this checking was already done when CONFIG_DEBUG_PAGEALLOC
was configured. It does not appear to have a big hibernating performance
impact. The speed of the saving operation before this change was measured
as 819.02 MB/s, and after was measured at 813.32 MB/s.
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>
Cc: Borislav Petkov <bp@alien8.de>
Acked-by: Pavel Machek <redacted>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/mm/pageattr.c | 4 ----
include/linux/mm.h | 18 ++++++------------
kernel/power/snapshot.c | 5 +++--
mm/page_alloc.c | 7 +++++--
4 files changed, 14 insertions(+), 20 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:51:57
Add two new functions set_direct_map_default_noflush() and
set_direct_map_invalid_noflush() for setting the direct map alias for the
page to its default valid permissions and to an invalid state that cannot
be cached in a TLB, respectively. These functions do 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.
Also add an ARCH config ARCH_HAS_SET_DIRECT_MAP 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_direct_map_invalid/default() functions+configARCH_HAS_SET_DIRECT_MAP+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_direct_map_invalid_noflush(structpage*page);+intset_direct_map_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-02-21 23:52:07
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 W+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>
Cc: Jessica Yu <jeyu@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 | 1 +
kernel/module.c | 5 +++++
4 files changed, 28 insertions(+), 8 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(boot_cpu_has(X86_FEATURE_NX)&&+is_module_text_address((unsignedlong)addr)){+/*+*Modulestextismarkedinitiallyasnon-executable,sothe+*codecannotberunningandspeculativecode-fetchesare+*prevented.Justchangethecode.+*/+memcpy(addr,opcode,len);+}else{+local_irq_save(flags);+memcpy(addr,opcode,len);+local_irq_restore(flags);+sync_core();++/*+*CouldalsodoaCLFLUSHheretospeedupCPUrecovery;but+*thatcauseshangsonsomeVIACPUs.+*/+}returnaddr;}
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:52:11
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: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Peter Zijlstra (Intel) <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 | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:52:21
From: Nadav Amit <redacted>
text_poke() can potentially compromise 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, 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: Dave Hansen <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.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 | 108 +++++++++++++++++++++++++++-------
arch/x86/xen/mmu_pv.c | 2 -
3 files changed, 86 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,104 @@ __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;+structpage*pages[2]={NULL};+temp_mm_state_tprev;unsignedlongflags;-char*vaddr;-structpage*pages[2];-inti;+pte_tpte,*ptep;+spinlock_t*ptl;+pgprot_tpgprot;/*-*Whilebootmemoryallocatorisrunnigwecannotusestruct-*pagesastheyarenotyetinitialized.+*Whilebootmemoryallocatorisrunningwecannotusestructpagesas+*theyarenotyetinitialized.Thereisnowaytorecover.*/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]);+/*+*Ifsomethingwentwrong,crashandburnsincerecoverypathsarenot+*implemented.+*/+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]);++/*+*Mapthepagewithouttheglobalbit,asTLBflushingisdonewith+*flush_tlb_mm_range(),whichisintendedfornon-globalPTEs.+*/+pgprot=__pgprot(pgprot_val(PAGE_KERNEL)&~_PAGE_GLOBAL);++/*+*Thelockisnotreallyneeded,butthisallowstoavoidopen-coding.+*/+ptep=get_locked_pte(poking_mm,poking_addr,&ptl);++/*+*Thismustnotfail;preallocatedinpoking_init().+*/+VM_BUG_ON(!ptep);++pte=mk_pte(pages[0],pgprot);+set_pte_at(poking_mm,poking_addr,ptep,pte);++if(cross_page_boundary){+pte=mk_pte(pages[1],pgprot);+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);+if(cross_page_boundary)+pte_clear(poking_mm,poking_addr+PAGE_SIZE,ptep+1);++/*+*Loadingthepreviouspage-tablehierarchyrequiresaserializing+*instructionthatalreadyallowsthecoretoseetheupdatedversion.+*Xen-PVisassumedtoserializeexecutioninasimilarmanner.+*/+unuse_temporary_mm(prev);++/*+*FlushingtheTLBmightinvolveIPIs,whichwouldrequireenabled+*IRQs,butnotifthemmisnotused,asitisinthispoint.+*/+flush_tlb_mm_range(poking_mm,poking_addr,poking_addr++(cross_page_boundary?2:1)*PAGE_SIZE,+PAGE_SHIFT,false);++/*+*Ifthetextdoesnotmatchwhatwejustwrotethensomethingis+*fundamentallyscrewy;there'snothingwecanreallydoaboutthat.+*/+BUG_ON(memcmp(addr,opcode,len));++pte_unmap_unlock(ptep,ptl);local_irq_restore(flags);returnaddr;}
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:52:24
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.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
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-02-21 23:52:40
From: Nadav Amit <redacted>
Prevent user watchpoints from mistakenly firing while the temporary mm
is being used. As the addresses that of the temporary mm might overlap
those of the user-process, this is necessary to prevent wrong signals
or worse things from happening.
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
---
arch/x86/include/asm/mmu_context.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Date: 2019-02-21 23:52:45
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: Dave Hansen <redacted>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
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-02-21 23:52:51
From: Andy Lutomirski <luto@kernel.org>
Using a dedicated page-table for temporary PTEs prevents other cores
from using - even speculatively - these PTEs, thereby providing two
benefits:
(1) Security hardening: an attacker that gains kernel memory writing
abilities cannot easily overwrite sensitive data.
(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 mm struct, which will follow, is for
poking the kernel text.
[ Commit message was written by Nadav Amit ]
Cc: Kees Cook <redacted>
Cc: Dave Hansen <redacted>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
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 | 33 ++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
From: Sean Christopherson <hidden> Date: 2019-02-22 00:07:35
On Thu, Feb 21, 2019 at 03:44:34PM -0800, Rick Edgecombe wrote:
quoted hunk
From: Nadav Amit <redacted>
Prevent user watchpoints from mistakenly firing while the temporary mm
is being used. As the addresses that of the temporary mm might overlap
those of the user-process, this is necessary to prevent wrong signals
or worse things from happening.
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
---
arch/x86/include/asm/mmu_context.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
Pretty sure caching hw_breakpoint_active() is unnecessary. It queries a
per-cpu value, not hardware's DR7 register, and that same value is
consumed by hw_breakpoint_restore(). No idea if breakpoints can be
disabled while using a temp mm, but even if that can happen, there's no
need to restore breakpoints if they've all been disabled, i.e. if
hw_breakpoint_active() returns false in unuse_temporary_mm().
quoted hunk
+ if (state.bp_enabled)
+ hw_breakpoint_disable();
+
return state;
}
@@ -387,6 +405,13 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev) { lockdep_assert_irqs_disabled(); switch_mm_irqs_off(NULL, prev.prev, current);++ /*+ * Restore the breakpoints if they were disabled before the temporary mm+ * was loaded.+ */+ if (prev.bp_enabled)+ hw_breakpoint_restore(); } #endif /* _ASM_X86_MMU_CONTEXT_H */
From: Nadav Amit <hidden> Date: 2019-02-22 00:17:31
On Feb 21, 2019, at 4:07 PM, Sean Christopherson [off-list ref] wrote:
On Thu, Feb 21, 2019 at 03:44:34PM -0800, Rick Edgecombe wrote:
quoted
From: Nadav Amit <redacted>
Prevent user watchpoints from mistakenly firing while the temporary mm
is being used. As the addresses that of the temporary mm might overlap
those of the user-process, this is necessary to prevent wrong signals
or worse things from happening.
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
---
arch/x86/include/asm/mmu_context.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
lockdep_assert_irqs_disabled();
state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
switch_mm_irqs_off(NULL, mm, current);
+
+ /*
+ * If breakpoints are enabled, disable them while the temporary mm is
+ * used. Userspace might set up watchpoints on addresses that are used
+ * in the temporary mm, which would lead to wrong signals being sent or
+ * crashes.
+ *
+ * Note that breakpoints are not disabled selectively, which also causes
+ * kernel breakpoints (e.g., perf's) to be disabled. This might be
+ * undesirable, but still seems reasonable as the code that runs in the
+ * temporary mm should be short.
+ */
+ state.bp_enabled = hw_breakpoint_active();
Pretty sure caching hw_breakpoint_active() is unnecessary. It queries a
per-cpu value, not hardware's DR7 register, and that same value is
consumed by hw_breakpoint_restore(). No idea if breakpoints can be
disabled while using a temp mm, but even if that can happen, there's no
need to restore breakpoints if they've all been disabled, i.e. if
hw_breakpoint_active() returns false in unuse_temporary_mm().
Good point. I will fix it for next version.
Thanks,
Nadav
From: Steven Rostedt <rostedt@goodmis.org> Date: 2019-02-22 00:22:16
On Thu, 21 Feb 2019 15:44:49 -0800
Rick Edgecombe [off-list ref] wrote:
quoted hunk
Use new flag VM_FLUSH_RESET_PERMS for handling freeing of special
permissioned memory in vmalloc and remove places where memory was set NX
and RW before freeing which is no longer needed.
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
@@ -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_flush_reset_perms(trampoline);+ /* * Module allocation needs to be completed by making the page * executable. The page is still writable, which is a security hazard,
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-02-22 00:55:53
On Thu, 2019-02-21 at 19:22 -0500, Steven Rostedt wrote:
On Thu, 21 Feb 2019 15:44:49 -0800
Rick Edgecombe [off-list ref] wrote:
quoted
Use new flag VM_FLUSH_RESET_PERMS for handling freeing of special
permissioned memory in vmalloc and remove places where memory was set NX
and RW before freeing which is no longer needed.
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/kernel/ftrace.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
On Thu, Feb 21, 2019 at 03:44:31PM -0800, Rick Edgecombe wrote:
Changes v2 to v3:
- Fix commit messages and comments [Boris]
- Rename VM_HAS_SPECIAL_PERMS [Boris]
- Remove unnecessary local variables [Boris]
- Rename set_alias_*() functions [Boris, Andy]
- Save/restore DR registers when using temporary mm
- Move line deletion from patch 10 to patch 17
In your previous submission there was a patch called
Subject: [PATCH v2 01/20] Fix "x86/alternatives: Lockdep-enforce text_mutex in text_poke*()"
What happened to it?
It did introduce a function text_poke_kgdb(), a.o., and I see this
function in the diff contexts in some of the patches in this submission
so it looks to me like you missed that first patch when submitting v3?
Or am *I* missing something?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2019-02-22 18:32:26
On Fri, 2019-02-22 at 17:14 +0100, Borislav Petkov wrote:
On Thu, Feb 21, 2019 at 03:44:31PM -0800, Rick Edgecombe wrote:
quoted
Changes v2 to v3:
- Fix commit messages and comments [Boris]
- Rename VM_HAS_SPECIAL_PERMS [Boris]
- Remove unnecessary local variables [Boris]
- Rename set_alias_*() functions [Boris, Andy]
- Save/restore DR registers when using temporary mm
- Move line deletion from patch 10 to patch 17
In your previous submission there was a patch called
Subject: [PATCH v2 01/20] Fix "x86/alternatives: Lockdep-enforce text_mutex in
text_poke*()"
What happened to it?
It did introduce a function text_poke_kgdb(), a.o., and I see this
function in the diff contexts in some of the patches in this submission
so it looks to me like you missed that first patch when submitting v3?
Or am *I* missing something?
Thx.
Oh, you are right! Sorry about that. I'll just send a new version with fixes for
other comments instead of a resend of this one.
Thanks,
Rick