dynup / kpatch

kpatch - live kernel patching
GNU General Public License v2.0
1.49k stars 304 forks source link

kmod: maintain syscall metadata sections in kpatch syscall macros #1376

Closed joe-lawrence closed 8 months ago

joe-lawrence commented 8 months ago

The KPATCH_SYSCALL_DEFINEn macros in kpatch-syscall.h do not provide the same syscall metadata (saved in the __syscalls_metadata and _ftrace_events ELF sections) as the kernel. These same macros also instruct kpatch-build to ignore changes to these sections. This works fine as long as there are other unmodified syscalls present in the object file. However, if not, the kpatch syscall macros may result in either metadata ELF sections not appearing in the patched object file. The create-diff-object program expects to encounter any ELF section that has been marked by KPATCH_IGNORE_SECTION in the patched object file.

To avoid this limitation, create dummy __syscalls_metadata and _ftrace_events entries for the kpatch-modified syscall. The specific values shouldn't matter since their sections will still be marked with KPATCH_IGNORE_SECTION and now their presence will be guarenteed for create-diff-object.

Closes: #1375 ("kpatch-build error when modifying an object file's only syscall")

joe-lawrence commented 8 months ago

A few follow ups:

jpoimboe commented 8 months ago

Since the kernel's include/linux/syscalls.h only defines SYSCALL_METADATA inside an #ifdef CONFIG_FTRACE_SYSCALLS block, should we do the same for KPATCH_SYSCALL_METADATA, too?

I think either way is fine.

In my limited testing, at least on ppc64le, I have observed that the dummy entries do ensure a __syscalls_metadata section, but not an associated rela section. This can SEGFAULT create-diff-object's __kpatch_correlate_section() code as it's passed a NULL sec_patched pointer. Should we assign the dummy metadata entries a pointer to something real (like &jiffies) to force the rela section, too?

Hm, how does it get passed a NULL sec_patched pointer?

joe-lawrence commented 8 months ago

In my limited testing, at least on ppc64le, I have observed that the dummy entries do ensure a __syscalls_metadata section, but not an associated rela section. This can SEGFAULT create-diff-object's __kpatch_correlate_section() code as it's passed a NULL sec_patched pointer. Should we assign the dummy metadata entries a pointer to something real (like &jiffies) to force the rela section, too?

Hm, how does it get passed a NULL sec_patched pointer?

Program received signal SIGSEGV, Segmentation fault.
0x0000000010004cf8 in __kpatch_correlate_section (sec_orig=0x10057060, sec_patched=0x0) at create-diff-object.c:1005
1005            CORRELATE_ELEMENT(sec_orig, sec_patched, "section");

(gdb) bt
#0  0x0000000010004cf8 in __kpatch_correlate_section (sec_orig=0x10057060, sec_patched=0x0) at create-diff-object.c:1005
#1  0x000000001000523c in kpatch_correlate_section (sec_orig=0x10056fc0, sec_patched=0x10111cf0) at create-diff-object.c:1032
#2  0x000000001000541c in kpatch_correlate_sections (seclist_orig=0x10054100, seclist_patched=0x1010f3f0) at create-diff-object.c:1071
#3  0x0000000010006d20 in kpatch_correlate_elfs (kelf_orig=0x100540f0, kelf_patched=0x1010f3e0) at create-diff-object.c:1505
#4  0x000000001000f6d4 in main (argc=8, argv=0x7fffffffec48) at create-diff-object.c:3992

(gdb) up
#1  0x000000001000523c in kpatch_correlate_section (sec_orig=0x10056fc0, sec_patched=0x10111cf0) at create-diff-object.c:1032
1032                    __kpatch_correlate_section(sec_orig->rela, sec_patched->rela);

(gdb) list
1027            if (is_rela_section(sec_orig)) {
1028                    __kpatch_correlate_section(sec_orig->base, sec_patched->base);
1029                    sec_orig = sec_orig->base;
1030                    sec_patched = sec_patched->base;
1031            } else if (sec_orig->rela) {
1032                    __kpatch_correlate_section(sec_orig->rela, sec_patched->rela);
1033            }
1034
1035            if (sec_orig->secsym)
1036                    kpatch_correlate_symbol(sec_orig->secsym, sec_patched->secsym);

(gdb) p sec_orig->name
$1 = 0x7ffff7c5ff75 "__syscalls_metadata"
(gdb) p sec_orig->rela
$2 = (struct section *) 0x10057060
(gdb) p sec_patched->name
$3 = 0x7ffff7bef861 "__syscalls_metadata"
(gdb) p sec_patched->rela
$4 = (struct section *) 0x0
jpoimboe commented 8 months ago

Your version of the code seems to be missing the fix for that: 9fac261e ?

joe-lawrence commented 8 months ago

Your version of the code seems to be missing the fix for that: 9fac261 ?

Ah yes, good find, my older kpatch-build was indeed missing that commit (which fixes the problem).

Anything more to tweak for this PR?