Re: [PATCH v16 01/18] seccomp: Convert __secure_computing() to return boolean
Jinjie Ruan <[email protected]> Mon, 6 Jul 2026 10:41:16 +0800
| Newsgroups | org.kernel.vger.linux-alpha,dev.linux.lists.loongarch,org.infradead.lists.linux-riscv,org.infradead.lists.linux-um,org.kernel.vger.linux-csky,org.kernel.vger.linux-kernel,org.kernel.vger.linux-m68k,org.kernel.vger.linux-mips,org.kernel.vger.linux-parisc,org.kernel.vger.linux-s390,org.kernel.vger.linux-sh,org.kvack.linux-mm,org.ozlabs.lists.linuxppc-dev |
|---|---|
| Message-ID | <[email protected]> |
On 7/1/2026 12:37 AM, Ada Couprie Diaz wrote: > Hi Jinjie, > > On 29/06/2026 14:05, Jinjie Ruan wrote: >> The return value of __secure_computing() currently uses 0 to indicate >> that a system call should be allowed, and -1 to indicate that it should >> be blocked/killed. This 0/-1 pattern is non-intuitive for a security >> check function and makes the control flow at the call sites less >> readable. >> >> Furthermore, any potential future changes to these return values would >> require a high-risk, error-prone audit of all its users across different >> architectures. >> >> Sanitize this logic by converting the return type of __secure_computing() >> to a proper boolean, where 'true' explicitly means 'allow' and 'false' >> means 'fail/deny'. >> >> Update all the two dozen or so call sites across the tree to align with >> this new boolean semantic. No functional changes are intended, as the >> callers still return -1 to the lower-level assembly entry code upon >> seccomp denial. > Would it be relevant to mention that this fixes the unsound return value of > `syscall_trace_enter()` in generic entry, which motivated the patch > initially[0] ? That's a very good point. It's definitely worth mentioning >> Suggested-by: Thomas Gleixner <[email protected]> >> Signed-off-by: Jinjie Ruan <[email protected]> >> --- >> arch/alpha/kernel/ptrace.c | 2 +- >> arch/arm/kernel/ptrace.c | 2 +- >> arch/arm64/kernel/ptrace.c | 2 +- >> arch/csky/kernel/ptrace.c | 2 +- >> arch/m68k/kernel/ptrace.c | 2 +- >> arch/mips/kernel/ptrace.c | 2 +- >> arch/parisc/kernel/ptrace.c | 2 +- >> arch/sh/kernel/ptrace_32.c | 2 +- >> arch/um/kernel/skas/syscall.c | 2 +- >> arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- >> arch/xtensa/kernel/ptrace.c | 3 +-- >> include/linux/entry-common.h | 7 +++--- >> include/linux/seccomp.h | 10 ++++---- >> kernel/seccomp.c | 34 +++++++++++++-------------- >> 14 files changed, 36 insertions(+), 38 deletions(-) > > This is missing an update to the Kconfig documentation, a possible > suggestion : Will update it. > > diff --git a/arch/Kconfig b/arch/Kconfig > index fa7507ac8e13..9e3d40088afb 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -637,7 +637,7 @@ config HAVE_ARCH_SECCOMP_FILTER > - syscall_set_return_value() > - SIGSYS siginfo_t support > - secure_computing is called from a ptrace_event()-safe context > - - secure_computing return value is checked and a return value > of -1 > + - secure_computing return value is checked and if false > results in the system call being skipped immediately. > - seccomp syscall wired up > - if !HAVE_SPARSE_SYSCALL_NR, have SECCOMP_ARCH_NATIVE, > >> [...] >> diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h >> index 416a3352261f..3f66320e46d3 100644 >> --- a/include/linux/entry-common.h >> +++ b/include/linux/entry-common.h >> @@ -100,9 +100,8 @@ static __always_inline long >> syscall_trace_enter(struct pt_regs *regs, unsigned l >> /* Do seccomp after ptrace, to catch any tracer changes. */ >> if (work & SYSCALL_WORK_SECCOMP) { >> - ret = __secure_computing(); >> - if (ret == -1L) >> - return ret; >> + if (!__secure_computing()) >> + return -1L; >> } >> /* Either of the above might have changed the syscall number */ >> @@ -113,7 +112,7 @@ static __always_inline long >> syscall_trace_enter(struct pt_regs *regs, unsigned l >> syscall_enter_audit(regs, syscall); >> - return ret ? : syscall; >> + return syscall; >> } >> [...] > > Otherwise this feels like a more appropriate change with regards to > "safeguarding against new `secure_computing()` return value" ! > > With the updated Kconfig : > Reviewed-by: Ada Couprie Diaz <[email protected]> > > Thanks, > Ada > > [0]: https://lore.kernel.org/r/20260511092103.1974980-2- > [email protected] > >