diff --git a/doc/patch-author-guide.md b/doc/patch-author-guide.md index f3dcf73..383b799 100644 --- a/doc/patch-author-guide.md +++ b/doc/patch-author-guide.md @@ -9,7 +9,27 @@ There are many pitfalls that can be encountered when creating a live patch. This document attempts to guide the patch creation process. It's a work in progress. If you find it useful, please contribute! -Patch Analysis +Table of contents +================= + +- [Patch analysis](#patch-analysis) +- [kpatch vs livepatch vs kGraft](#kpatch-vs-livepatch-vs-kgraft) +- [Patch upgrades](#patch-upgrades) +- [Data structure changes](#data-structure-changes) +- [Data semantic changes](#data-semantic-changes) +- [Init code changes](#init-code-changes) +- [Header file changes](#header-file-changes) +- [Dealing with unexpected changed functions](#dealing-with-unexpected-changed-functions) +- [Removing references to static local variables](#removing-references-to-static-local-variables) +- [Code removal](#code-removal) +- [Once macros](#once-macros) +- [inline implies notrace](#inline-implies-notrace) +- [Jump labels](#jump-labels) +- [Sibling calls](#sibling-calls) +- [Exported symbol versioning](#exported-symbol-versioning) +- [System calls](#system-calls) + +Patch analysis -------------- kpatch provides _some_ guarantees, but it does not guarantee that all patches @@ -759,6 +779,10 @@ if (static_key_enabled(&false_key)) if (likely(static_key_enabled(&key))) ``` +Note that with Linux 5.8+, jump labels in patched functions are now supported +when the static key was originally defined in the kernel proper (vmlinux). The +above error will not be seen unless the static key lives in a module. + Sibling calls ------------- @@ -870,3 +894,14 @@ To track down specifically what caused a symbol CRC change, tools like detailed symbol definition report. For a kpatch-build, kabi-dw can be modified to operate on .o object files (not just .ko and vmlinux files) and the `$CACHEDIR/tmp/{orig, patched}` directories compared. + +System calls +------------ + +Attempting to patch a syscall typically results in an error, due to a missing +fentry hook in the inner `__do_sys##name()` function. The fentry hook is +missing because of the 'inline' annotation, which invokes 'notrace'. + +This problem can be worked around by adding `#include "kpatch-syscall.h"` and +replacing the use of the `SYSCALL_DEFINE1` (or similar) macro with the +`KPATCH_` prefixed version. diff --git a/kmod/patch/kpatch-macros.h b/kmod/patch/kpatch-macros.h index caaadbc..8e09702 100644 --- a/kmod/patch/kpatch-macros.h +++ b/kmod/patch/kpatch-macros.h @@ -4,6 +4,7 @@ #include #include #include +#include "kpatch-syscall.h" /* upstream 33def8498fdd "treewide: Convert macro and uses of __section(foo) to __section("foo")" */ #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 10, 0) diff --git a/kmod/patch/kpatch-syscall.h b/kmod/patch/kpatch-syscall.h new file mode 100644 index 0000000..07a1cec --- /dev/null +++ b/kmod/patch/kpatch-syscall.h @@ -0,0 +1,155 @@ +#ifndef __KPATCH_SYSCALL_H_ +#define __KPATCH_SYSCALL_H_ + +#include "kpatch-macros.h" + +/* + * These kpatch-specific syscall definition macros can be used for patching a + * syscall. + * + * Attempting to patch a syscall typically results in an error, due to a + * missing fentry hook in the inner __do_sys##name() function. The fentry hook + * is missing because of the 'inline' annotation, which invokes 'notrace'. + * + * These macros are copied almost verbatim from the kernel, the main difference + * being a 'kpatch' prefix added to the __do_sys##name() function name. This + * causes kpatch-build to treat it as a new function (due to + * its new name), and its caller __se_sys##name() function is inlined by its own + * caller __x64_sys##name() function, which has an fentry hook. + + * To patch a syscall, just replace the use of the SYSCALL_DEFINE1 (or similar) + * macro with the "KPATCH_" prefixed version. + */ + +#define KPATCH_IGNORE_SYSCALL_SECTIONS \ + KPATCH_IGNORE_SECTION("__syscalls_metadata"); \ + KPATCH_IGNORE_SECTION("_ftrace_events") + +#define KPATCH_SYSCALL_DEFINE1(name, ...) KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__) +#define KPATCH_SYSCALL_DEFINE2(name, ...) KPATCH_SYSCALL_DEFINEx(2, _##name, __VA_ARGS__) +#define KPATCH_SYSCALL_DEFINE3(name, ...) KPATCH_SYSCALL_DEFINEx(3, _##name, __VA_ARGS__) +#define KPATCH_SYSCALL_DEFINE4(name, ...) KPATCH_SYSCALL_DEFINEx(4, _##name, __VA_ARGS__) +#define KPATCH_SYSCALL_DEFINE5(name, ...) KPATCH_SYSCALL_DEFINEx(5, _##name, __VA_ARGS__) +#define KPATCH_SYSCALL_DEFINE6(name, ...) KPATCH_SYSCALL_DEFINEx(6, _##name, __VA_ARGS__) + +#define KPATCH_SYSCALL_DEFINEx(x, sname, ...) \ + KPATCH_IGNORE_SYSCALL_SECTIONS; \ + __KPATCH_SYSCALL_DEFINEx(x, sname, __VA_ARGS__) + +#ifdef CONFIG_X86_64 + +#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 7, 0) + +#define __KPATCH_SYSCALL_DEFINEx(x, name, ...) \ + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\ + __X64_SYS_STUBx(x, name, __VA_ARGS__) \ + __IA32_SYS_STUBx(x, name, __VA_ARGS__) \ + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \ + { \ + long ret = __kpatch_do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \ + return ret; \ + } \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) + +#elif LINUX_VERSION_CODE >= KERNEL_VERSION(4, 17, 0) + +#define __KPATCH_SYSCALL_DEFINEx(x, name, ...) \ + asmlinkage long __x64_sys##name(const struct pt_regs *regs); \ + ALLOW_ERROR_INJECTION(__x64_sys##name, ERRNO); \ + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\ + asmlinkage long __x64_sys##name(const struct pt_regs *regs) \ + { \ + return __se_sys##name(SC_X86_64_REGS_TO_ARGS(x,__VA_ARGS__));\ + } \ + __IA32_SYS_STUBx(x, name, __VA_ARGS__) \ + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \ + { \ + long ret = __kpatch_do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \ + return ret; \ + } \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) + +#else /* LINUX_VERSION_CODE < KERNEL_VERSION(4, 17, 0) */ + +#define __KPATCH_SYSCALL_DEFINEx(x, name, ...) \ + asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \ + static inline long __kpatch_SYSC##name(__MAP(x,__SC_DECL,__VA_ARGS__));\ + asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \ + { \ + long ret = __kpatch_SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));\ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \ + return ret; \ + } \ + SYSCALL_ALIAS(sys##name, SyS##name); \ + static inline long __kpatch_SYSC##name(__MAP(x,__SC_DECL,__VA_ARGS__)) + +#endif /* LINUX_VERSION_CODE */ + + +#elif defined(CONFIG_PPC64) + +#define __KPATCH_SYSCALL_DEFINEx(x, name, ...) \ + __diag_push(); \ + __diag_ignore(GCC, 8, "-Wattribute-alias", \ + "Type aliasing is used to sanitize syscall arguments");\ + asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \ + __attribute__((alias(__stringify(__se_sys##name)))); \ + ALLOW_ERROR_INJECTION(sys##name, ERRNO); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\ + asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \ + asmlinkage long __attribute__((optimize("-fno-optimize-sibling-calls")))\ + __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \ + { \ + long ret = __kpatch_do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \ + return ret; \ + } \ + __diag_pop(); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) + + +#elif defined(CONFIG_S390) + +#define __KPATCH_S390_SYS_STUBx(x, name, ...) \ + long __s390_sys##name(struct pt_regs *regs); \ + ALLOW_ERROR_INJECTION(__s390_sys##name, ERRNO); \ + long __s390_sys##name(struct pt_regs *regs) \ + { \ + long ret = __kpatch_do_sys##name(SYSCALL_PT_ARGS(x, regs, \ + __SC_COMPAT_CAST, __MAP(x, __SC_TYPE, __VA_ARGS__))); \ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + return ret; \ + } + +#define __KPATCH_SYSCALL_DEFINEx(x, name, ...) \ + __diag_push(); \ + __diag_ignore(GCC, 8, "-Wattribute-alias", \ + "Type aliasing is used to sanitize syscall arguments"); \ + long __s390x_sys##name(struct pt_regs *regs) \ + __attribute__((alias(__stringify(__se_sys##name)))); \ + ALLOW_ERROR_INJECTION(__s390x_sys##name, ERRNO); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \ + long __se_sys##name(struct pt_regs *regs); \ + __KPATCH_S390_SYS_STUBx(x, name, __VA_ARGS__) \ + long __se_sys##name(struct pt_regs *regs) \ + { \ + long ret = __kpatch_do_sys##name(SYSCALL_PT_ARGS(x, regs, \ + __SC_CAST, __MAP(x, __SC_TYPE, __VA_ARGS__))); \ + __MAP(x,__SC_TEST,__VA_ARGS__); \ + return ret; \ + } \ + __diag_pop(); \ + static inline long __kpatch_do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) + + +#endif /* CONFIG_X86_64 */ + +#endif /* __KPATCH_SYSCALL_H_ */ diff --git a/test/integration/linux-5.10.11/syscall-LOADED.test b/test/integration/linux-5.10.11/syscall-LOADED.test new file mode 100755 index 0000000..3a2fd88 --- /dev/null +++ b/test/integration/linux-5.10.11/syscall-LOADED.test @@ -0,0 +1,3 @@ +#!/bin/bash + +uname -s | grep -q kpatch diff --git a/test/integration/linux-5.10.11/syscall.patch b/test/integration/linux-5.10.11/syscall.patch new file mode 100644 index 0000000..f86d695 --- /dev/null +++ b/test/integration/linux-5.10.11/syscall.patch @@ -0,0 +1,21 @@ +diff --git a/kernel/sys.c b/kernel/sys.c +index 2e2e3f378d97..05271fe26c89 100644 +--- a/kernel/sys.c ++++ b/kernel/sys.c +@@ -1250,13 +1250,15 @@ static int override_release(char __user *release, size_t len) + return ret; + } + +-SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) ++#include "kpatch-syscall.h" ++KPATCH_SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) + { + struct new_utsname tmp; + + down_read(&uts_sem); + memcpy(&tmp, utsname(), sizeof(tmp)); + up_read(&uts_sem); ++ strcat(tmp.sysname, ".kpatch"); + if (copy_to_user(name, &tmp, sizeof(tmp))) + return -EFAULT; + diff --git a/test/integration/rhel-7.9/syscall-LOADED.test b/test/integration/rhel-7.9/syscall-LOADED.test new file mode 100755 index 0000000..3a2fd88 --- /dev/null +++ b/test/integration/rhel-7.9/syscall-LOADED.test @@ -0,0 +1,3 @@ +#!/bin/bash + +uname -s | grep -q kpatch diff --git a/test/integration/rhel-7.9/syscall.patch b/test/integration/rhel-7.9/syscall.patch new file mode 100644 index 0000000..729340d --- /dev/null +++ b/test/integration/rhel-7.9/syscall.patch @@ -0,0 +1,26 @@ +diff --git a/kernel/sys.c b/kernel/sys.c +index 1fbf388279832..b5186aa83adfa 100644 +--- a/kernel/sys.c ++++ b/kernel/sys.c +@@ -1392,14 +1392,18 @@ static int override_release(char __user *release, size_t len) + return ret; + } + +-SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) ++#include "kpatch-syscall.h" ++KPATCH_SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) + { ++ struct new_utsname tmp; + int errno = 0; + + down_read(&uts_sem); +- if (copy_to_user(name, utsname(), sizeof *name)) +- errno = -EFAULT; ++ memcpy(&tmp, utsname(), sizeof(tmp)); + up_read(&uts_sem); ++ strcat(tmp.sysname, ".kpatch"); ++ if (copy_to_user(name, &tmp, sizeof(tmp))) ++ errno = -EFAULT; + + if (!errno && override_release(name->release, sizeof(name->release))) + errno = -EFAULT; diff --git a/test/integration/rhel-8.4/syscall-LOADED.test b/test/integration/rhel-8.4/syscall-LOADED.test new file mode 100755 index 0000000..3a2fd88 --- /dev/null +++ b/test/integration/rhel-8.4/syscall-LOADED.test @@ -0,0 +1,3 @@ +#!/bin/bash + +uname -s | grep -q kpatch diff --git a/test/integration/rhel-8.4/syscall.patch b/test/integration/rhel-8.4/syscall.patch new file mode 100644 index 0000000..4679d01 --- /dev/null +++ b/test/integration/rhel-8.4/syscall.patch @@ -0,0 +1,26 @@ +diff --git a/kernel/sys.c b/kernel/sys.c +index 871c0848f05c8..479bf8725d2e6 100644 +--- a/kernel/sys.c ++++ b/kernel/sys.c +@@ -1241,14 +1241,18 @@ static int override_release(char __user *release, size_t len) + return ret; + } + +-SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) ++#include "kpatch-syscall.h" ++KPATCH_SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name) + { ++ struct new_utsname tmp; + int errno = 0; + + down_read(&uts_sem); +- if (copy_to_user(name, utsname(), sizeof *name)) +- errno = -EFAULT; ++ memcpy(&tmp, utsname(), sizeof(tmp)); + up_read(&uts_sem); ++ strcat(tmp.sysname, ".kpatch"); ++ if (copy_to_user(name, &tmp, sizeof(tmp))) ++ errno = -EFAULT; + + if (!errno && override_release(name->release, sizeof(name->release))) + errno = -EFAULT;