Re: [PATCH 3/4] x86: use the correct function type for sys_ni_syscall

[Will Deacon]

On Fri, Sep 13, 2019 at 05:27:40PM -0700, Andy Lutomirski wrote:
> > On Sep 13, 2019, at 4:26 PM, Sami Tolvanen  wrote:
> >> On Fri, Sep 13, 2019 at 3:45 PM Andy Lutomirski  wrote:
> >> Should this be SYSCALL_DEFINE0?
> > 
> > It can be, and that would also fix the issue. However, it does result
> > in unnecessary error injection to be hooked up here, which is why
> > arm64 preferred to avoid the macro when I fixed it there. S390 uses
> > SYSCALL_DEFINE0 for this though and since sys_ni_syscall always
> > returns -ENOSYS, it shouldn't be a huge problem. Thoughts?
> > 
> 
> I don’t see why all syscalls except these  few should have error injection
> hooked up.  It’s also IMO nicer from a maintenance perspective to have all
> syscalls use the same macros.
> 
> Will, is there something I’m missing?

There was a reasonable request from Mark (CC'd) not to allow error injection
for unimplemented system calls, so that's why we took the approach that we
did. There was also a vague plan to fix this for everybody [1] but evidently
nobody found the time :(

Will

[1] 
https://lore.kernel.org/lkml/20190524215821.ga37...@google.com/T/#m6519b2aad06d8c384de1f55256f08687c83d8796

Re: [PATCH 3/4] x86: use the correct function type for sys_ni_syscall

[Andy Lutomirski]

> On Sep 13, 2019, at 4:26 PM, Sami Tolvanen  wrote:
> 
>> On Fri, Sep 13, 2019 at 3:45 PM Andy Lutomirski  wrote:
>> Should this be SYSCALL_DEFINE0?
> 
> It can be, and that would also fix the issue. However, it does result
> in unnecessary error injection to be hooked up here, which is why
> arm64 preferred to avoid the macro when I fixed it there. S390 uses
> SYSCALL_DEFINE0 for this though and since sys_ni_syscall always
> returns -ENOSYS, it shouldn't be a huge problem. Thoughts?
> 


I don’t see why all syscalls except these  few should have error injection 
hooked up.  It’s also IMO nicer from a maintenance perspective to have all 
syscalls use the same macros.

Will, is there something I’m missing?

Re: [PATCH 3/4] x86: use the correct function type for sys_ni_syscall

[Sami Tolvanen]

On Fri, Sep 13, 2019 at 3:45 PM Andy Lutomirski  wrote:
> Should this be SYSCALL_DEFINE0?

It can be, and that would also fix the issue. However, it does result
in unnecessary error injection to be hooked up here, which is why
arm64 preferred to avoid the macro when I fixed it there. S390 uses
SYSCALL_DEFINE0 for this though and since sys_ni_syscall always
returns -ENOSYS, it shouldn't be a huge problem. Thoughts?

Sami

Re: [PATCH 3/4] x86: use the correct function type for sys_ni_syscall

[Andy Lutomirski]

On Fri, Sep 13, 2019 at 2:00 PM Sami Tolvanen  wrote:
>
> Use the correct function type for sys_ni_syscall in system
> call tables to fix indirect call mismatches with Control-Flow
> Integrity (CFI) checking.

Should this be SYSCALL_DEFINE0?

[PATCH 3/4] x86: use the correct function type for sys_ni_syscall

[Sami Tolvanen]

Use the correct function type for sys_ni_syscall in system
call tables to fix indirect call mismatches with Control-Flow
Integrity (CFI) checking.

Signed-off-by: Sami Tolvanen 
---
 arch/x86/entry/syscall_32.c| 13 ++---
 arch/x86/entry/syscall_64.c| 12 +---
 arch/x86/entry/syscalls/syscall_32.tbl |  4 ++--
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
index aa3336a7cb15..1cbdfff116d1 100644
--- a/arch/x86/entry/syscall_32.c
+++ b/arch/x86/entry/syscall_32.c
@@ -11,12 +11,19 @@
 /* On X86_64, we use struct pt_regs * to pass parameters to syscalls */
 #define __SYSCALL_I386(nr, sym, qual) extern asmlinkage long sym(const struct 
pt_regs *);
 
-/* this is a lie, but it does not hurt as sys_ni_syscall just returns -EINVAL 
*/
-extern asmlinkage long sys_ni_syscall(const struct pt_regs *);
+extern asmlinkage long sys_ni_syscall(void);
+
+asmlinkage long __ia32_sys_ni_syscall(const struct pt_regs *__unused)
+{
+   return sys_ni_syscall();
+}
+
+#define __sys_ni_syscall __ia32_sys_ni_syscall
 
 #else /* CONFIG_IA32_EMULATION */
 #define __SYSCALL_I386(nr, sym, qual) extern asmlinkage long sym(unsigned 
long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned 
long);
 extern asmlinkage long sys_ni_syscall(unsigned long, unsigned long, unsigned 
long, unsigned long, unsigned long, unsigned long);
+#define __sys_ni_syscall sys_ni_syscall
 #endif /* CONFIG_IA32_EMULATION */
 
 #include 
@@ -29,6 +36,6 @@ __visible const sys_call_ptr_t 
ia32_sys_call_table[__NR_syscall_compat_max+1] =
 * Smells like a compiler bug -- it doesn't work
 * when the & below is removed.
 */
-   [0 ... __NR_syscall_compat_max] = _ni_syscall,
+   [0 ... __NR_syscall_compat_max] = &__sys_ni_syscall,
 #include 
 };
diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
index d5252bc1e380..0341b3e7fede 100644
--- a/arch/x86/entry/syscall_64.c
+++ b/arch/x86/entry/syscall_64.c
@@ -4,11 +4,17 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
-/* this is a lie, but it does not hurt as sys_ni_syscall just returns -EINVAL 
*/
-extern asmlinkage long sys_ni_syscall(const struct pt_regs *);
+extern asmlinkage long sys_ni_syscall(void);
+
+asmlinkage long __x64_sys_ni_syscall(const struct pt_regs *__unused)
+{
+   return sys_ni_syscall();
+}
+
 #define __SYSCALL_64(nr, sym, qual) extern asmlinkage long sym(const struct 
pt_regs *);
 #include 
 #undef __SYSCALL_64
@@ -20,6 +26,6 @@ asmlinkage const sys_call_ptr_t 
sys_call_table[__NR_syscall_max+1] = {
 * Smells like a compiler bug -- it doesn't work
 * when the & below is removed.
 */
-   [0 ... __NR_syscall_max] = _ni_syscall,
+   [0 ... __NR_syscall_max] = &__x64_sys_ni_syscall,
 #include 
 };
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl 
b/arch/x86/entry/syscalls/syscall_32.tbl
index c00019abd076..9514f2fe456a 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -124,7 +124,7 @@
 110i386ioplsys_iopl
__ia32_sys_iopl
 111i386vhangup sys_vhangup 
__ia32_sys_vhangup
 112i386idle
-113i386vm86old sys_vm86old 
sys_ni_syscall
+113i386vm86old sys_vm86old 
__ia32_sys_ni_syscall
 114i386wait4   sys_wait4   
__ia32_compat_sys_wait4
 115i386swapoff sys_swapoff 
__ia32_sys_swapoff
 116i386sysinfo sys_sysinfo 
__ia32_compat_sys_sysinfo
@@ -177,7 +177,7 @@
 163i386mremap  sys_mremap  
__ia32_sys_mremap
 164i386setresuid   sys_setresuid16 
__ia32_sys_setresuid16
 165i386getresuid   sys_getresuid16 
__ia32_sys_getresuid16
-166i386vm86sys_vm86
sys_ni_syscall
+166i386vm86sys_vm86
__ia32_sys_ni_syscall
 167i386query_module
 168i386pollsys_poll
__ia32_sys_poll
 169i386nfsservctl
-- 
2.23.0.237.gc6a4ce50a0-goog