[PATCH 0/2] fix function type mismatches in syscall wrappers

[PATCH 0/2] fix function type mismatches in syscall wrappers

[Sami Tolvanen]

These patches fix type mismatches in arm64 syscall wrapper
definitions, which trip indirect call checks with Control-Flow
Integrity.

Sami Tolvanen (2):
  arm64: fix syscall_fn_t type
  arm64: use the correct function type in SYSCALL_DEFINE0

 arch/arm64/include/asm/syscall.h         |  2 +-
 arch/arm64/include/asm/syscall_wrapper.h | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.21.0.593.g511ec345e18-goog

[PATCH 1/2] arm64: fix syscall_fn_t type

[Sami Tolvanen]

Use const struct pt_regs * instead of struct pt_regs * as
the argument type to fix indirect call type mismatches with
Control-Flow Integrity checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/include/asm/syscall.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index a179df3674a1a..6206ab9bfcfc5 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -20,7 +20,7 @@
 #include <linux/compat.h>
 #include <linux/err.h>
 
-typedef long (*syscall_fn_t)(struct pt_regs *regs);
+typedef long (*syscall_fn_t)(const struct pt_regs *regs);
 
 extern const syscall_fn_t sys_call_table[];
 
-- 
2.21.0.593.g511ec345e18-goog

[PATCH 2/2] arm64: use the correct function type in SYSCALL_DEFINE0

[Sami Tolvanen]

Although a syscall defined using SYSCALL_DEFINE0 doesn't accept
parameters, use the correct function type to avoid indirect call
type mismatches with Control-Flow Integrity checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/include/asm/syscall_wrapper.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/syscall_wrapper.h b/arch/arm64/include/asm/syscall_wrapper.h
index a4477e515b798..507d0ee6bc690 100644
--- a/arch/arm64/include/asm/syscall_wrapper.h
+++ b/arch/arm64/include/asm/syscall_wrapper.h
@@ -30,10 +30,10 @@
 	}										\
 	static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
 
-#define COMPAT_SYSCALL_DEFINE0(sname)					\
-	asmlinkage long __arm64_compat_sys_##sname(void);		\
-	ALLOW_ERROR_INJECTION(__arm64_compat_sys_##sname, ERRNO);	\
-	asmlinkage long __arm64_compat_sys_##sname(void)
+#define COMPAT_SYSCALL_DEFINE0(sname)							\
+	asmlinkage long __arm64_compat_sys_##sname(const struct pt_regs *__unused);	\
+	ALLOW_ERROR_INJECTION(__arm64_compat_sys_##sname, ERRNO);			\
+	asmlinkage long __arm64_compat_sys_##sname(const struct pt_regs *__unused)
 
 #define COND_SYSCALL_COMPAT(name) \
 	cond_syscall(__arm64_compat_sys_##name);
@@ -62,11 +62,11 @@
 	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
 
 #ifndef SYSCALL_DEFINE0
-#define SYSCALL_DEFINE0(sname)					\
-	SYSCALL_METADATA(_##sname, 0);				\
-	asmlinkage long __arm64_sys_##sname(void);		\
-	ALLOW_ERROR_INJECTION(__arm64_sys_##sname, ERRNO);	\
-	asmlinkage long __arm64_sys_##sname(void)
+#define SYSCALL_DEFINE0(sname)							\
+	SYSCALL_METADATA(_##sname, 0);						\
+	asmlinkage long __arm64_sys_##sname(const struct pt_regs *__unused);	\
+	ALLOW_ERROR_INJECTION(__arm64_sys_##sname, ERRNO);			\
+	asmlinkage long __arm64_sys_##sname(const struct pt_regs *__unused)
 #endif
 
 #ifndef COND_SYSCALL
-- 
2.21.0.593.g511ec345e18-goog

Re: [PATCH 1/2] arm64: fix syscall_fn_t type

[Mark Rutland]

On Wed, May 01, 2019 at 01:04:50PM -0700, Sami Tolvanen wrote:
> Use const struct pt_regs * instead of struct pt_regs * as
> the argument type to fix indirect call type mismatches with
> Control-Flow Integrity checking.

It's probably worth noting that in <asm/syscall_wrapper.h> all syscall
wrappers take a const struct pt_regs *, which is where the mismatch
comes from.

> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/syscall.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
> index a179df3674a1a..6206ab9bfcfc5 100644
> --- a/arch/arm64/include/asm/syscall.h
> +++ b/arch/arm64/include/asm/syscall.h
> @@ -20,7 +20,7 @@
>  #include <linux/compat.h>
>  #include <linux/err.h>
>  
> -typedef long (*syscall_fn_t)(struct pt_regs *regs);
> +typedef long (*syscall_fn_t)(const struct pt_regs *regs);

For a second I was worried that we modify the regs to assign the return
value, but I see we do that in the syscall.c wrapper, where the pt_regs
argument isn't const.

We certainly chouldn't need to modify the regs when acquiring the
arguments, and as above this matches <asm/syscall_wrapper.h>, so this
looks sound to me.

FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH 2/2] arm64: use the correct function type in SYSCALL_DEFINE0

[Mark Rutland]

On Wed, May 01, 2019 at 01:04:51PM -0700, Sami Tolvanen wrote:
> Although a syscall defined using SYSCALL_DEFINE0 doesn't accept
> parameters, use the correct function type to avoid indirect call
> type mismatches with Control-Flow Integrity checking.

Generally, this makes sense, but I'm not sure that this is complete.

IIUC this introduces a new type mismatch with sys_ni_syscall() in some
cases. We probably need that to use SYSCALL_DEFINE0(), and maybe have a
ksys_ni_syscall() for in-kernel wrappers.

Thanks,
Mark.

> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/syscall_wrapper.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/syscall_wrapper.h b/arch/arm64/include/asm/syscall_wrapper.h
> index a4477e515b798..507d0ee6bc690 100644
> --- a/arch/arm64/include/asm/syscall_wrapper.h
> +++ b/arch/arm64/include/asm/syscall_wrapper.h
> @@ -30,10 +30,10 @@
>  	}										\
>  	static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
>  
> -#define COMPAT_SYSCALL_DEFINE0(sname)					\
> -	asmlinkage long __arm64_compat_sys_##sname(void);		\
> -	ALLOW_ERROR_INJECTION(__arm64_compat_sys_##sname, ERRNO);	\
> -	asmlinkage long __arm64_compat_sys_##sname(void)
> +#define COMPAT_SYSCALL_DEFINE0(sname)							\
> +	asmlinkage long __arm64_compat_sys_##sname(const struct pt_regs *__unused);	\
> +	ALLOW_ERROR_INJECTION(__arm64_compat_sys_##sname, ERRNO);			\
> +	asmlinkage long __arm64_compat_sys_##sname(const struct pt_regs *__unused)
>  
>  #define COND_SYSCALL_COMPAT(name) \
>  	cond_syscall(__arm64_compat_sys_##name);
> @@ -62,11 +62,11 @@
>  	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
>  
>  #ifndef SYSCALL_DEFINE0
> -#define SYSCALL_DEFINE0(sname)					\
> -	SYSCALL_METADATA(_##sname, 0);				\
> -	asmlinkage long __arm64_sys_##sname(void);		\
> -	ALLOW_ERROR_INJECTION(__arm64_sys_##sname, ERRNO);	\
> -	asmlinkage long __arm64_sys_##sname(void)
> +#define SYSCALL_DEFINE0(sname)							\
> +	SYSCALL_METADATA(_##sname, 0);						\
> +	asmlinkage long __arm64_sys_##sname(const struct pt_regs *__unused);	\
> +	ALLOW_ERROR_INJECTION(__arm64_sys_##sname, ERRNO);			\
> +	asmlinkage long __arm64_sys_##sname(const struct pt_regs *__unused)
>  #endif
>  
>  #ifndef COND_SYSCALL
> -- 
> 2.21.0.593.g511ec345e18-goog
> 

Re: [PATCH 2/2] arm64: use the correct function type in SYSCALL_DEFINE0

[Sami Tolvanen]

Hi Mark,

On Fri, May 03, 2019 at 11:21:28AM +0100, Mark Rutland wrote:
> Generally, this makes sense, but I'm not sure that this is complete.
> 
> IIUC this introduces a new type mismatch with sys_ni_syscall() in some
> cases.

Thanks for the review. You're correct, sys_ni_syscall needs to be fixed
too. I'll include this in v2.

> We probably need that to use SYSCALL_DEFINE0(), and maybe have a
> ksys_ni_syscall() for in-kernel wrappers.

Why would we need ksys_ni_syscall? It seems something like this should
be sufficient:

  asmlinkage long sys_ni_syscall(void);

  SYSCALL_DEFINE0(ni_syscall)
  {
          return sys_ni_syscall();
  }

Sami