Issue while patching syscalls #1171

liu-song-6 · 2021-04-12T23:16:17Z

Hi,

I am testing a simple patch to a syscall:

diff --git i/kernel/sys.c w/kernel/sys.c
index d325f3ab624a..ae31c7cd76cb 100644
--- i/kernel/sys.c
+++ w/kernel/sys.c
@@ -1266,7 +1266,7 @@ SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
                return -EFAULT;

        down_read(&uts_sem);
-       memcpy(&tmp, utsname(), sizeof(tmp));
+       memcpy(&tmp, "hello", sizeof(tmp));
        up_read(&uts_sem);
        if (copy_to_user(name, &tmp, sizeof(tmp)))
                return -EFAULT;

create-diff-object failed on this:

sys.o: function __do_sys_uname has no fentry/mcount call, unable to patch

which is true. On the other hand, we have __x64_sys_uname with proper fentry

(gdb) disassem __x64_sys_uname
Dump of assembler code for function __x64_sys_uname:
   0xffffffff81482fb0 <+0>:     callq  0xffffffff81201360 <__fentry__>
   0xffffffff81482fb5 <+5>:     mov    0x70(%rdi),%rdi
   0xffffffff81482fb9 <+9>:     jmpq   0xffffffff81482eb0 <__do_sys_uname>

So I guess we can just patch __x64_sys_uname instead?

Do we have better solution for this?

The text was updated successfully, but these errors were encountered:

jpoimboe · 2021-04-13T16:56:56Z

I'm not sure we've ever patched a syscall before. The problem is, that in __SYSCALL_DEFINEx() in arch/x86/include/asm/syscall_wrapper.h, __do_sys##name has inline. And currently, in the kernel inline is defined to include notrace. I've discussed changing that with Steven Rostedt, such that inline no longer includes notrace.. But in the meantime that means that you can't patch "inline" functions, even if they aren't actually inlined by the compiler.

It's possible to work around this limitation, but it will be a bit tricky. We'll need to mess with the syscall macros a bit... let me play around with it.

jpoimboe · 2021-04-13T18:14:41Z

Here's a patch which seems like it would work for patching the syscall. However it seems to have triggered a bug in kpatch-build:

/home/jpoimboe/git/kpatch/kpatch-build/create-diff-object: ERROR: sys.o: kpatch_check_relocations: 2550: out-of-range relocation .rodata.__kpatch_do_sys_uname.str1.1+139 in .rela.text.__kpatch_do_sys_uname

Looking into it...

syscall.patch.txt

jpoimboe · 2021-04-13T18:46:11Z

Ah, that's because of this funny code:

        memcpy(&tmp, "hello", sizeof(tmp));

which tries to access far beyond the bounds of the "hello" string.

Otherwise, try out the above patch and let me know if it works for you.

jpoimboe · 2021-04-13T19:08:08Z

I opened #1172 to slightly improve the message for the error I saw. It should have said ".LC7+139" instead of ".rodata.__kpatch_do_sys_uname.str1.1+139".

liu-song-6 · 2021-04-13T20:54:17Z

With syscall.patch.txt, I am getting errors like

kernel/sys.c:1265:31: error: expected declaration specifiers or ‘...’ before numeric constant
        KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
                               ^
kernel/sys.c:1271:18: note: in definition of macro ‘__KPATCH_SYSCALL_DEFINEx’
  __X64_SYS_STUBx(x, name, __VA_ARGS__)                           \
                  ^
kernel/sys.c:1265:8: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINEx’
        KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
        ^~~~~~~~~~~~~~~~~~~~~~
kernel/sys.c:1282:1: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINE1’
 KPATCH_SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 ^~~~~~~~~~~~~~~~~~~~~~
kernel/sys.c:1265:34: error: unknown type name ‘_uname’; did you mean ‘ifr_name’?
        KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
                                  ^
kernel/sys.c:1271:21: note: in definition of macro ‘__KPATCH_SYSCALL_DEFINEx’
  __X64_SYS_STUBx(x, name, __VA_ARGS__)                           \
                     ^~~~
kernel/sys.c:1265:8: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINEx’
        KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
        ^~~~~~~~~~~~~~~~~~~~~~
kernel/sys.c:1282:1: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINE1’
 KPATCH_SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 ^~~~~~~~~~~~~~~~~~~~~~
kernel/sys.c:1282:60: error: unknown type name ‘name’
 KPATCH_SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
                                                            ^~~~
kernel/sys.c:1271:27: note: in definition of macro ‘__KPATCH_SYSCALL_DEFINEx’
  __X64_SYS_STUBx(x, name, __VA_ARGS__)                           \
                           ^~~~~~~~~~~
kernel/sys.c:1265:8: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINEx’
        KPATCH_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
        ^~~~~~~~~~~~~~~~~~~~~~
kernel/sys.c:1282:1: note: in expansion of macro ‘KPATCH_SYSCALL_DEFINE1’
 KPATCH_SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 ^~~~~~~~~~~~~~~~~~~~~~
In file included from ./include/linux/error-injection.h:6,
                 from ./include/linux/module.h:25,
                 from ./include/linux/kallsyms.h:13,
                 from ./include/linux/ftrace.h:11,
                 from ./include/linux/perf_event.h:49,
                 from kernel/sys.c:17:
./arch/x86/include/asm/syscall_wrapper.h:51:24: error: ‘__ia32_sys_uname’ undeclared here (not in a function); did you mean ‘__ia32_sys_newuname’?
  ALLOW_ERROR_INJECTION(__ia32_sys##name, ERRNO);   \
                        ^~~~~~~~~~
./include/asm-generic/error-injection.h:30:26: note: in definition of macro ‘ALLOW_ERROR_INJECTION’
   .addr = (unsigned long)fname,    \
                          ^~~~~

Is this triggered by certain gcc? I am using gcc version 8.4.1 20200928 (Red Hat 8.4.1-1) (GCC)

jpoimboe · 2021-04-13T21:23:47Z

Hm, it works for me with kernel 5.11 and gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC).

I'm having trouble making sense of the error. It could be the gcc version or the kernel version (most of the macros were copy/pasted from the kernel with slight changes).

liu-song-6 · 2021-04-13T21:42:33Z

The error also confuses me. I am using a 5.6 based kernel, which might be the reason. Let me debug more.

Once this is all figured out, shall we include KPATCH_SYSCALL_DEFINEx() etc. in kpatch-macros.h?

jpoimboe · 2021-04-13T21:52:48Z

Once this is all figured out, shall we include KPATCH_SYSCALL_DEFINEx() etc. in kpatch-macros.h?

Yes, I think so. We might end up needing to have multiple versions of the macros based on kernel version and arch. And we can add an integration test to hopefully catch if/when the macros no longer work for future kernels.

liu-song-6 · 2021-04-13T22:03:47Z

Have we considered adding kpatch-macros.h to the kernel tree? I guess this would reduce version/arch checks within kpatch-macros.h.

liu-song-6 · 2021-04-13T22:07:50Z

This version works for the 5.6 kernel.

diff --git a/kernel/sys.c b/kernel/sys.c
index d325f3ab624a..229946fe59a3 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1258,7 +1258,34 @@ SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
 /*
  * Old cruft
  */
-SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
+#include "kpatch-macros.h"
+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_DEFINEx(x, sname, ...)                          \
+       __KPATCH_SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
+
+#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 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__))
+
+KPATCH_SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 {
        struct old_utsname tmp;

jpoimboe · 2021-04-13T22:54:41Z

Have we considered adding kpatch-macros.h to the kernel tree? I guess this would reduce version/arch checks within kpatch-macros.h.

That would be nice, but it would be NAKed with a vengeance.

Long term we do hope to have a unified patch creation solution which can live completely in the kernel tree.

liu-song-6 · 2021-04-16T00:05:35Z

The change by @jpoimboe works great on a 5.12 based kernel. Thanks again.

jpoimboe · 2021-04-16T15:30:48Z

@liu-song-6 Glad that worked! Reopening so I don't forget to add the macro.

On x86, attempting to patch a syscall 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'. Add some kpatch-specific syscall definition macros which can be used for patching a syscall. 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 use replace the use of the SYSCALL_DEFINE1 (or similar) macro with the "KPATCH_" prefixed version. (This isn't needed on powerpc because the __do_sys##name() function is inlined by the compiler because it only has a single call site.) Fixes: dynup#1171 Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

liu-song-6 closed this as completed Apr 16, 2021

jpoimboe reopened this Apr 16, 2021

jpoimboe closed this as completed in e9c0b67 Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue while patching syscalls #1171

Issue while patching syscalls #1171

liu-song-6 commented Apr 12, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 16, 2021

jpoimboe commented Apr 16, 2021

Issue while patching syscalls #1171

Issue while patching syscalls #1171

Comments

liu-song-6 commented Apr 12, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

liu-song-6 commented Apr 13, 2021

jpoimboe commented Apr 13, 2021

liu-song-6 commented Apr 16, 2021

jpoimboe commented Apr 16, 2021