| Message ID | 20190503191225.6684-4-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix function type mismatches in syscall wrappers | expand |
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@google.com> > --- > 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
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> };
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@google.com> --- arch/arm64/kernel/sys.c | 14 +++++++++----- arch/arm64/kernel/sys32.c | 12 ++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-)