Re: [PATCH v2 3/3] arm64: use the correct function type for __arm64_sys_ni_syscall

[Mark Rutland]

On Fri, May 03, 2019 at 12:12:25PM -0700, Sami Tolvanen wrote:
> Calling sys_ni_syscall through a syscall_fn_t pointer trips indirect
> call Control-Flow Integrity checking due to a function type
> mismatch. Use SYSCALL_DEFINE0 for __arm64_sys_ni_syscall instead and
> remove the now unnecessary casts.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@xxxxxxxxxx>
> ---
>  arch/arm64/kernel/sys.c   | 14 +++++++++-----
>  arch/arm64/kernel/sys32.c | 12 ++++++++----
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> index b44065fb16160..4f8e8a7237a85 100644
> --- a/arch/arm64/kernel/sys.c
> +++ b/arch/arm64/kernel/sys.c
> @@ -47,22 +47,26 @@ SYSCALL_DEFINE1(arm64_personality, unsigned int, personality)
>  	return ksys_personality(personality);
>  }
>  
> +asmlinkage long sys_ni_syscall(void);
> +
> +SYSCALL_DEFINE0(ni_syscall)
> +{
> +	return sys_ni_syscall();
> +}

I strongly think that we cant to fix up the common definition in
kernel/sys_ni.c rather than having a point-hack in arm64. Other
architectures (e.g. x86, s390) will want the same for CFI, and I'd like
to ensure that our approached don't diverge.

I took a quick look, and it looks like it's messy but possible to fix
up the core.

I also suspect that using SYSCALL_DEFINE0() as it currently stands isn't
a great idea, since it'll allow fault injection for unimplemented
syscalls, which sounds dubious to me.

Thanks,
Mark.

> +
>  /*
>   * Wrappers to pass the pt_regs argument.
>   */
>  #define sys_personality		sys_arm64_personality
>  
> -asmlinkage long sys_ni_syscall(const struct pt_regs *);
> -#define __arm64_sys_ni_syscall	sys_ni_syscall
> -
>  #undef __SYSCALL
>  #define __SYSCALL(nr, sym)	asmlinkage long __arm64_##sym(const struct pt_regs *);
>  #include <asm/unistd.h>
>  
>  #undef __SYSCALL
> -#define __SYSCALL(nr, sym)	[nr] = (syscall_fn_t)__arm64_##sym,
> +#define __SYSCALL(nr, sym)	[nr] = __arm64_##sym,
>  
>  const syscall_fn_t sys_call_table[__NR_syscalls] = {
> -	[0 ... __NR_syscalls - 1] = (syscall_fn_t)sys_ni_syscall,
> +	[0 ... __NR_syscalls - 1] = __arm64_sys_ni_syscall,
>  #include <asm/unistd.h>
>  };
> diff --git a/arch/arm64/kernel/sys32.c b/arch/arm64/kernel/sys32.c
> index 0f8bcb7de7008..f8f6c26cfd326 100644
> --- a/arch/arm64/kernel/sys32.c
> +++ b/arch/arm64/kernel/sys32.c
> @@ -133,17 +133,21 @@ COMPAT_SYSCALL_DEFINE6(aarch32_fallocate, int, fd, int, mode,
>  	return ksys_fallocate(fd, mode, arg_u64(offset), arg_u64(len));
>  }
>  
> -asmlinkage long sys_ni_syscall(const struct pt_regs *);
> -#define __arm64_sys_ni_syscall	sys_ni_syscall
> +asmlinkage long sys_ni_syscall(void);
> +
> +COMPAT_SYSCALL_DEFINE0(ni_syscall)
> +{
> +	return sys_ni_syscall();
> +}
>  
>  #undef __SYSCALL
>  #define __SYSCALL(nr, sym)	asmlinkage long __arm64_##sym(const struct pt_regs *);
>  #include <asm/unistd32.h>
>  
>  #undef __SYSCALL
> -#define __SYSCALL(nr, sym)	[nr] = (syscall_fn_t)__arm64_##sym,
> +#define __SYSCALL(nr, sym)	[nr] = __arm64_##sym,
>  
>  const syscall_fn_t compat_sys_call_table[__NR_compat_syscalls] = {
> -	[0 ... __NR_compat_syscalls - 1] = (syscall_fn_t)sys_ni_syscall,
> +	[0 ... __NR_compat_syscalls - 1] = __arm64_sys_ni_syscall,
>  #include <asm/unistd32.h>
>  };
> -- 
> 2.21.0.1020.gf2820cf01a-goog
>