These patches fix type mismatches in arm64 syscall wrapper
definitions, which trip indirect call checks with Control-Flow
Integrity.
Changes in v2:
- more informative commit message for the syscall_fn_t change
- added a patch for fixing sys_ni_syscall
Sami Tolvanen (3):
arm64: fix syscall_fn_t type
arm64: use the correct function type in SYSCALL_DEFINE0
arm64: use the correct function type for __arm64_sys_ni_syscall
arch/arm64/include/asm/syscall.h | 2 +-
arch/arm64/include/asm/syscall_wrapper.h | 18 +++++++++---------
arch/arm64/kernel/sys.c | 14 +++++++++-----
arch/arm64/kernel/sys32.c | 12 ++++++++----
4 files changed, 27 insertions(+), 19 deletions(-)
--
2.21.0.1020.gf2820cf01a-goog
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 <[email protected]>
---
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();
+}
+
/*
* 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
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 <[email protected]>
---
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.1020.gf2820cf01a-goog
Syscall wrappers in <asm/syscall_wrapper.h> use const struct pt_regs *
as the argument type. Use const in syscall_fn_t as well to fix indirect
call type mismatches with Control-Flow Integrity checking.
Signed-off-by: Sami Tolvanen <[email protected]>
Reviewed-by: Mark Rutland <[email protected]>
---
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.1020.gf2820cf01a-goog
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 <[email protected]>
> ---
> 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
>
On Tue, May 07, 2019 at 06:25:12PM +0100, Mark Rutland wrote:
> 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.
s390 already has the following in arch/s390/kernel/sys_s390.c:
SYSCALL_DEFINE0(ni_syscall)
{
return -ENOSYS;
}
Which, I suppose, is cleaner than calling sys_ni_syscall.
> I took a quick look, and it looks like it's messy but possible to fix
> up the core.
OK. How would you propose fixing this?
Sami
On Tue, May 07, 2019 at 11:32:27AM -0700, Sami Tolvanen wrote:
> On Tue, May 07, 2019 at 06:25:12PM +0100, Mark Rutland wrote:
> > 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.
>
> s390 already has the following in arch/s390/kernel/sys_s390.c:
>
> SYSCALL_DEFINE0(ni_syscall)
> {
> return -ENOSYS;
> }
>
> Which, I suppose, is cleaner than calling sys_ni_syscall.
>
> > I took a quick look, and it looks like it's messy but possible to fix
> > up the core.
>
> OK. How would you propose fixing this?
In the absence of a patch from Mark, I'd suggest just adding a SYS_NI macro
to our asm/syscall_wrapper.h file which avoids the error injection stuff. It
doesn't preclude moving this to the core later on, but it unblocks the CFI
work.
Will
Hi Sami,
On Wed, May 15, 2019 at 12:40:39PM +0100, Will Deacon wrote:
> On Tue, May 07, 2019 at 11:32:27AM -0700, Sami Tolvanen wrote:
> > On Tue, May 07, 2019 at 06:25:12PM +0100, Mark Rutland wrote:
> > > 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.
> >
> > s390 already has the following in arch/s390/kernel/sys_s390.c:
> >
> > SYSCALL_DEFINE0(ni_syscall)
> > {
> > return -ENOSYS;
> > }
> >
> > Which, I suppose, is cleaner than calling sys_ni_syscall.
> >
> > > I took a quick look, and it looks like it's messy but possible to fix
> > > up the core.
> >
> > OK. How would you propose fixing this?
>
> In the absence of a patch from Mark, I'd suggest just adding a SYS_NI macro
> to our asm/syscall_wrapper.h file which avoids the error injection stuff. It
> doesn't preclude moving this to the core later on, but it unblocks the CFI
> work.
Do you plan to repost this?
Will
On Fri, May 24, 2019 at 07:35:51PM +0100, Will Deacon wrote:
> > In the absence of a patch from Mark, I'd suggest just adding a SYS_NI macro
> > to our asm/syscall_wrapper.h file which avoids the error injection stuff.
If we don't want to use SYSCALL_DEFINE0, I don't think we need a macro
at all. I believe it's cleaner to just define __arm64_sys_ni_syscall with
the correct type in sys.c.
> Do you plan to repost this?
Yes. Sorry for the delay. I'll post v3 shortly.
Sami