A kernel crash happened in __do_softirq() in very rare cases when the
binary patch created from mainline commit be82485fbcbb
("netlink: fix an use-after-free issue for nlk groups") was unloaded.
Investigation has shown that the kernel tried to execute an RCU
callback, deferred_put_nlk_sk(), defined in the patch module after the
module had been unloaded.
The callback was set by the patched variant of netlink_release() and
the address of the patched deferred_put_nlk_sk() was used, rather than
the address of the original function.
Similar problems occur with workqueue functions as well.
As suggested in
https://github.com/dynup/kpatch/pull/755#issuecomment-344135224,
create-diff-object was modified so that the addresses of the original
functions were used in such situations, at least for x86 systems. A
similar fix for PowerPC was added as well.
Changes in v4:
* '#ifdef __x86_64__' was removed. It is not actually needed right now
because the constants for relocation types are different on different
architectures.
Changes in v3:
* Minor refactoring and a comment explaining what this all is about.
Quite lengthy, but the dynrela-related code is really far from obvious.
Changes in v2:
* Handle the nested functions the same way as before, because they are
unlikely to be used as asynchronous callbacks. Example: cmp() in
bch_cache_show() from drivers/md/bcache/sysfs.c in the kernel 4.4.
As the nested functions are local to the functions they are defined in,
the compiler names them in a similar way to static locals: <name>.<number>.
Currently, we filter out all functions with '.' in their names. If there
are any asynchronous callbacks in the kernel that have a dot in their
names too, they could be handled in the future patches. It is unclear
though, if the callbacks with such names can appear in the kernel.
Signed-off-by: Evgenii Shatokhin <eshatokhin@virtuozzo.com>
With kernel commit b1fca27d384 ("kernel debug: support resetting
WARN*_ONCE") the *_ONCE warnings are placed .data.once section.
Including .data.once section is valid, so add an check in
kpatch_verify_patchability() while checking for included invalid
sections.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Symbols with R_PPC64_REL24 relocation type are functions and it's
currently assumed that all functions are replaced with their respective
section symbols.
There are function whose reference are not straight forward section
symbol but section + offset. These function replacement should be
handled more like bundled sections. Remove the check, which imposes
the inital assumption.
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
.toc section entries are mostly place holder for relocation entries,
specified in .rela.toc section. Sometimes, .toc section may have
constants as entries. These constants are not reference to any symbols,
but plain instructions mostly due to some arthimetics in the functions
referring them.
They are referred by the functions like normal .toc entries, these
entries can not be resolved to any symbols. This patch creates a list
of constants if available for .toc sections and compares them in
rela_equal() to ensure their is no mismatch in the generated constants
for original and patched .o files.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
GCC 7.2.1 complains about the usage of the PPC64_LOCAL_ENTRY_OFFSET
macro:
create-diff-object.c: In function ‘is_gcc6_localentry_bundled_sym’:
create-diff-object.c:119:83: error: ‘<<’ in boolean context, did you mean ‘<’ ? [-Werror=int-in-bool-context]
(((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >> 2) << 2)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
create-diff-object.c:140:10: note: in expansion of macro ‘PPC64_LOCAL_ENTRY_OFFSET’
return (PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) &&
^~~~~~~~~~~~~~~~~~~~~~~~
Fix it by explicitly treating the macro as an integer instead of a bool.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Found in the scope of https://github.com/dynup/kpatch/pull/755 but not
related to the main problem discussed there.
kpatch_create_patches_sections() and kpatch_create_intermediate_sections()
used 'hint' in error messages.
However, the string 'hint' refers to is owned by 'kelf_base' and is
freed before kpatch_create_*_sections() are called. As a result, if
these functions try to output errors and print 'hint',
create-diff-object will crash.
As suggested in the mentioned PR, 'hint' is actually no longer needed at
that stage, so I have removed it from kpatch_create_*_sections().
When creating .kpatch.relocations, there's no reason to convert the
relocation destinations to symbols. In fact, it's actively harmful
because it makes it harder for create-klp-module to deal with the GCC 6+
8-byte localentry gap.
This also fixes a regression which was introduced in 5888f316e6, which
broke ppc64le relocations.
Fixes#754.
Fixes: 5888f316e6 ("create-klp-module: support unbundled symbols")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
kpatch_relocation's 'dest' addend and 'offset' fields are redundant. In
fact, the 'offset' field isn't always accurate because it doesn't have a
relocation, so its value doesn't adjust when multiple .o files are
combined. Just use the 'dest' addend instead.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
kpatch_replace_sections_syms() assumes that all bundled symbols start at
section offset zero. With ppc64le and GCC 6+, that assumption is no
longer accurate. When replacing a rela symbol section with its
corresponding symbol, adjust the addend as necessary.
Also, with this fix in place, the workaround in
create_klp_relasecs_and_syms() can be removed.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
On ppc64le, adding a printk to total_mapping_size() caused it to change
from non-localentry to localentry, presumably because it was no longer a
leaf function. With GCC 6, a localentry function is offset by 8 in the
section, so different st_values are ok.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The STT_FUNC and SHN_UNDEF checks aren't needed because they're already
implied by the localentry check.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
is_localentry_sym() isn't quite the right name, because it also checks
for the 8-byte gap introduced by GCC 6, and also checks that the
function is otherwise at the beginning of the section.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
padding, for a total of 16 bytes. However, when laying out the structs
in the .parainstructions section, the vmlinux script only aligns before
each struct's data, not after. So the last entry doesn't have the
4-byte padding, which breaks kpatch_regenerate_special_section()'s
assumption of a 16-byte struct, resulting in a memcpy past the end of
the section.
Fixes#747.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
kpatch-elf.c is used by binaries other than create-diff-object, but
create-diff-object is the only one that cares about "bundling". Move
the bundling to create-diff-object.
Fixes#700.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Normally, kpatch doesn't complain if you remove or rename a function.
This is a feature, because sometimes you have to rename a function in
order to patch it, if for example it doesn't have an fentry call. In
the object code, it's treated as a new function. You could get the same
result by copying/pasting the original function and giving the copy a
new name. But renaming it makes it much easier to review the patch.
In RHEL 7.4, I tried to rename l2cap_config_rsp() to
l2cap_config_rsp_kpatch(), but it failed with:
ERROR: l2cap_core.o: reference to static local variable CSWTCH.347 in l2cap_config_rsp was removed
This particular error is an easy fix, because the CSWTCH.* symbols are
read-only and are created by GCC. So they shouldn't be correlated
anyway.
In the future, we will need a more general fix to allow the removal of
functions which use *any* static local variables. Either automatically,
or by adding a manual annotation. This can be handled when we rewrite
the static local variable handling in #545.
If an .LCx symbol gets renamed or changes sections, or if its section
gets renamed, kpatch-build will get confused.
They aren't *real* symbols, just string constants. So no need to
correlate and compare them.
Fixes#714.
Fixes#727.
With gcc-6 the function prologue is changeg by
moving the toc base resolution func - 0x8 bytes:
.globl my_func
.type my_func, @function
.quad .TOC.-my_func
my_func:
.reloc ., R_PPC64_ENTRY ; optional
ld r2,-8(r12)
add r2,r2,r12
.localentry my_func, .-my_func
Add support for function prologue, along with gcc-5.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Add support for ppc64le specific special sections:
- __ftr_fixup
- __mmu_ftr_fixup
- __fw_ftr_fixup
- __lwsync_fixup
This patch also add #ifdef guards for architecture specific
special sections.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
This patch adds support for livepatch hook based module
creation for PPC64le. It introduces PPC64le architecture
bits:
- Add relocation type of R_PPC64_ADDR64 while parsing powerpc ELF.
- Introduce .toc sections mainpulation.
- Skip kpatch specific details for livepatch hook.
Also remove the definition of rela_insn() for powerpc. The only
call site is been guarded by #ifdef x86.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
symbol->has_fentry_call is x86 specfic. Rename it to more
generic name, representing the general idea of calling
profiling function at function entry.
This patch converts all instance of symbol->has_fentry_call
to symbol->has_func_profiling and also renames functions:
kpatch_check_fentry_calls() -> kpatch_check_func_profiling_calls()
kpatch_find_fentry_calls() -> kpatch_find_func_profiling_calls()
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
kpatch-build/insn provides x86 instruction analysis, disable
the analyzer support when build on powerpc.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Joe saw the following errors when loading Linux commit 128394eff343
("sg_write()/bsg_write() is not fit to be called under KERNEL_DS"):
Skipped dynrela for copy_user_generic_unrolled (0xffffffffa0475942 <- 0xffffffff813211e0): the instruction has been changed already.
Skipped dynrela for copy_user_generic_unrolled (0xffffffffa0475a57 <- 0xffffffff813211e0): the instruction has been changed already.
That is known issue #580, but it can be avoided by leaving
'copy_user_generic_unrolled' as a normal relocation instead of
converting it to a dynrela, because it's an exported symbol.
Also remove the manual check for '__fentry__' because it's covered by
the exported symbol check.
Also remove a duplicate comment about unexported global object symbols
being in another .o in the patch object.
Fixes#695.
Strip kpatch_ignore_func_* and __UNIQUE_ID_kpatch_ignore_section_*
symbols to prevent the inclusion of .kpatch.ignore.functions and
.kpatch.ignore.sections. Mark the symbols as SAME, otherwise they are
considered NEW and are recursively included. This includes the
corresponding ignore sections and rela sections and may also create new,
unnecessary dynrelas.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Normal correlated symbols are marked the SAME initially but static local
variables are correlated in a separate function. Also mark these the
SAME.
This fixes an issue where patching a function which called printk_once
(which uses a static local variable) would fail to build because the
static local variable was considered new and thus introduced a new data
member into .data..read_mostly which is not allowed to change.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
When CONFIG_DEBUG_ATOMIC_SLEEP is enabled, might_sleep calls will add
the line number to the instruction stream. Detect and ignore any such
changes.
Fixes: #657.
We use kelf_base->symbols to find a unique matching FILE+locals combination
when we call lookup_open(). If we can't find one matching or we find more
than one matching, we error out.
If we find a unique one, we setup table->local_syms in lookup_open(),
so later lookup_local_symbol() could do its lookup based on table->local_syms.
Fixes#604.
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
In addition to .kpatch.relocations and .kpatch.symbols, have
create-diff-object create an .kpatch.arch section. This section can be used
to create .klp.arch. sections that are required for klp modules built for
versions >= 4.9. Each entry in the .kpatch.arch section represents an
arch-specific section (.altinstructions or .parainstructions) and contains
a pointer to the arch-specific section itself (see kpatch_arch struct
member 'sec') and a pointer to the objname string (see kpatch_arch struct
member 'objname'). This is enough information to be able to build
.klp.arch. sections in a later phase of kpatch-build.
Instead of creating dynrela sections, have create-diff-object create
intermediate sections .kpatch.relocations and .kpatch.symbols which can
then be used to build (depending on kernel version) either dynrela sections
or klp rela/klp arch sections + klp symbols in a later phase of kpatch-build.
Have lookup_open() also parse Module.symvers and add the resulting symbols
and their objnames to the lookup table. This code was essentially
cherry-picked from Josh Poimboeuf's lookup code found here:
8cdca59c88
That patch was modified to fix a bug in obj_read() (calling elf_end()
without strdup'ing the symbol name strings, which was causing null
dereferences) and to fix up the module name after reading it from
Module.symvers (replacing '-' with '_' and stripping the path prefixes).
Also, add lookup_exported_symbol_objname(), which looks up the objname of
an exported symbol by making use of the objname information obtained from
Module.symvers.
Move functions kpatch_reindex_elements() and kpatch_rebuild_rela_section_data()
from create-diff-object.c to kpatch-elf.c. These functions will be used
to rebuild kpatch elf data in create-klp-module and create-kpatch-module,
i.e. during the second "phase" of kpatch-build.
Fixes sparse complaints:
create-diff-object.c:2302:24: warning: Using plain integer as NULL pointer
create-diff-object.c:2303:11: warning: Using plain integer as NULL pointer
create-diff-object.c:2334:59: warning: Using plain integer as NULL pointer
create-diff-object.c:2347:43: warning: Using plain integer as NULL pointer
GCC with KASAN instrumentation creates section ".rodata" with some static strings (i.e. some of them go to ".rodata.str1.1" for release build).
This change makes possible to build patch and check if it fixes issue found with KASAN, such as CVE-2016-9555.
Give a slightly better error message for the dup file+symbol issue.
It's still cryptic but it's good enough to at least give us kpatch
developers a better idea about what went wrong. This would have helped
diagnose issue #633 much more quickly.
When pruning entries from the fixup table, update the offsets in
.rela__ex_table otherwise the relas might point to the wrong fixup entry
or even out of the .fixup section.
Fixes#615.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
This fixes the detection of WARN_ON_ONCE, WARN_ONCE, and WARN_TAINT_ONCE
on Linux 4.6 and newer.
The signature for those macros changed with upstream Linux commit
dfbf2897d004 ("bug: set warn variable before calling WARN()").
Fixes#602.
Introduce a common kpatch elf api by moving all functions and struct
declarations related to manipulating kpatch_elf objects from
create-diff-object to kpatch-elf.{h,c}. Move logging macros to a separate
file log.h, and have kpatch-elf.h include it. These changes will generalize
the kpatch-elf and logging api and make it available to other kpatch-build
tools.
