Issue while patching syscalls

[liu-song-6]

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?

[jpoimboe]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[jpoimboe]

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