[PATCH 0/4] seccomp: remove the 'sd' argument from __secure_computing()

[PATCH 0/4] seccomp: remove the 'sd' argument from __secure_computing()

[Oleg Nesterov]

Hello,

I know nothing about arch/mips and I don't have a mips machine, 1/4 wasn't
even compile tested.

Oleg.
---

 arch/mips/kernel/ptrace.c           | 20 ++------------------
 arch/powerpc/kernel/ptrace/ptrace.c |  2 +-
 include/linux/seccomp.h             | 12 ++++--------
 kernel/entry/common.c               |  2 +-
 kernel/seccomp.c                    | 26 ++++++++++----------------
 5 files changed, 18 insertions(+), 44 deletions(-)

[PATCH 1/4] seccomp/mips: change syscall_trace_enter() to use secure_computing()

[Oleg Nesterov]

arch/mips/Kconfig selects HAVE_ARCH_SECCOMP_FILTER so syscall_trace_enter()
can just use __secure_computing(NULL) and rely on populate_seccomp_data(sd)
and "sd == NULL" checks in __secure_computing(sd) paths.

With the change above syscall_trace_enter() can just use secure_computing()
and avoid #ifdef + test_thread_flag(TIF_SECCOMP). CONFIG_GENERIC_ENTRY is
not defined, so test_syscall_work(SECCOMP) will check TIF_SECCOMP.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/mips/kernel/ptrace.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 61503a36067e..f7107479c7fa 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -1326,24 +1326,8 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs)
 			return -1;
 	}
 
-#ifdef CONFIG_SECCOMP
-	if (unlikely(test_thread_flag(TIF_SECCOMP))) {
-		int ret, i;
-		struct seccomp_data sd;
-		unsigned long args[6];
-
-		sd.nr = current_thread_info()->syscall;
-		sd.arch = syscall_get_arch(current);
-		syscall_get_arguments(current, regs, args);
-		for (i = 0; i < 6; i++)
-			sd.args[i] = args[i];
-		sd.instruction_pointer = KSTK_EIP(current);
-
-		ret = __secure_computing(&sd);
-		if (ret == -1)
-			return ret;
-	}
-#endif
+	if (secure_computing())
+		return -1;
 
 	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
 		trace_sys_enter(regs, regs->regs[2]);
-- 
2.25.1.362.g51ebf55

Re: [PATCH 1/4] seccomp/mips: change syscall_trace_enter() to use secure_computing()

[Kees Cook]

On Mon, Jan 20, 2025 at 02:44:45PM +0100, Oleg Nesterov wrote:
> arch/mips/Kconfig selects HAVE_ARCH_SECCOMP_FILTER so syscall_trace_enter()
> can just use __secure_computing(NULL) and rely on populate_seccomp_data(sd)
> and "sd == NULL" checks in __secure_computing(sd) paths.
> 
> With the change above syscall_trace_enter() can just use secure_computing()
> and avoid #ifdef + test_thread_flag(TIF_SECCOMP). CONFIG_GENERIC_ENTRY is
> not defined, so test_syscall_work(SECCOMP) will check TIF_SECCOMP.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/mips/kernel/ptrace.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 61503a36067e..f7107479c7fa 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -1326,24 +1326,8 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs)
>  			return -1;
>  	}
>  
> -#ifdef CONFIG_SECCOMP
> -	if (unlikely(test_thread_flag(TIF_SECCOMP))) {

Yup, this test works out the same as what secure_computing() does.

> -		int ret, i;
> -		struct seccomp_data sd;
> -		unsigned long args[6];
> -
> -		sd.nr = current_thread_info()->syscall;

This matches MIPS's syscall_get_nr() in populate_seccomp_data().

> -		sd.arch = syscall_get_arch(current);
> -		syscall_get_arguments(current, regs, args);
> -		for (i = 0; i < 6; i++)
> -			sd.args[i] = args[i];
> -		sd.instruction_pointer = KSTK_EIP(current);

Rest matches the rest of populate_seccomp_data().

> -
> -		ret = __secure_computing(&sd);
> -		if (ret == -1)
> -			return ret;
> -	}
> -#endif
> +	if (secure_computing())
> +		return -1;
>  
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>  		trace_sys_enter(regs, regs->regs[2]);
> -- 

So this check out logically from what I can see. I can build test it,
but I don't have MIPS emulation set up. I'd love an Ack from a MIPS
maintainer...

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH 1/4] seccomp/mips: change syscall_trace_enter() to use secure_computing()

[Thomas Bogendoerfer]

On Mon, Jan 20, 2025 at 02:44:45PM +0100, Oleg Nesterov wrote:
> arch/mips/Kconfig selects HAVE_ARCH_SECCOMP_FILTER so syscall_trace_enter()
> can just use __secure_computing(NULL) and rely on populate_seccomp_data(sd)
> and "sd == NULL" checks in __secure_computing(sd) paths.
> 
> With the change above syscall_trace_enter() can just use secure_computing()
> and avoid #ifdef + test_thread_flag(TIF_SECCOMP). CONFIG_GENERIC_ENTRY is
> not defined, so test_syscall_work(SECCOMP) will check TIF_SECCOMP.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/mips/kernel/ptrace.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 61503a36067e..f7107479c7fa 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -1326,24 +1326,8 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs)
>  			return -1;
>  	}
>  
> -#ifdef CONFIG_SECCOMP
> -	if (unlikely(test_thread_flag(TIF_SECCOMP))) {
> -		int ret, i;
> -		struct seccomp_data sd;
> -		unsigned long args[6];
> -
> -		sd.nr = current_thread_info()->syscall;
> -		sd.arch = syscall_get_arch(current);
> -		syscall_get_arguments(current, regs, args);
> -		for (i = 0; i < 6; i++)
> -			sd.args[i] = args[i];
> -		sd.instruction_pointer = KSTK_EIP(current);
> -
> -		ret = __secure_computing(&sd);
> -		if (ret == -1)
> -			return ret;
> -	}
> -#endif
> +	if (secure_computing())
> +		return -1;
>  
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>  		trace_sys_enter(regs, regs->regs[2]);
> -- 
> 2.25.1.362.g51ebf55

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

[PATCH 2/4] seccomp: kill the dead code in the !CONFIG_HAVE_ARCH_SECCOMP_FILTER version of __secure_computing()

[Oleg Nesterov]

Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL)
will crash or not, this is not consistent/safe.

Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing()
has no callers, these architectures use secure_computing_strict().

Also, after the previous change __secure_computing(sd) is always called
with sd == NULL, so it is clear that we can remove the code which makes
no sense.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/seccomp.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index e45531455d3b..e01dfe57a884 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -32,11 +32,7 @@ static inline int secure_computing(void)
 }
 #else
 extern void secure_computing_strict(int this_syscall);
-static inline int __secure_computing(const struct seccomp_data *sd)
-{
-	secure_computing_strict(sd->nr);
-	return 0;
-}
+static inline int __secure_computing(const struct seccomp_data *sd) { return 0; }
 #endif
 
 extern long prctl_get_seccomp(void);
-- 
2.25.1.362.g51ebf55

Re: [PATCH 2/4] seccomp: kill the dead code in the !CONFIG_HAVE_ARCH_SECCOMP_FILTER version of __secure_computing()

[Kees Cook]

On Mon, Jan 20, 2025 at 02:44:52PM +0100, Oleg Nesterov wrote:
> Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL)
> will crash or not, this is not consistent/safe.

Right now this never happens because there are no callers.

> Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing()
> has no callers, these architectures use secure_computing_strict().

As you say here.

> Also, after the previous change __secure_computing(sd) is always called
> with sd == NULL, so it is clear that we can remove the code which makes
> no sense.

However, after this change, if someone were to *add* a caller, it would
bypass strict mode. Instead of "return 0", it seems like it'd be better
to remove the function entirely (and maybe add a comment about calling
secure_computing_strict() directly)?

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/seccomp.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index e45531455d3b..e01dfe57a884 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -32,11 +32,7 @@ static inline int secure_computing(void)
>  }
>  #else
>  extern void secure_computing_strict(int this_syscall);
> -static inline int __secure_computing(const struct seccomp_data *sd)
> -{
> -	secure_computing_strict(sd->nr);
> -	return 0;
> -}
> +static inline int __secure_computing(const struct seccomp_data *sd) { return 0; }
>  #endif
>  
>  extern long prctl_get_seccomp(void);
> -- 
> 2.25.1.362.g51ebf55
> 

-- 
Kees Cook

Re: [PATCH 2/4] seccomp: kill the dead code in the !CONFIG_HAVE_ARCH_SECCOMP_FILTER version of __secure_computing()

[Oleg Nesterov]

On 01/20, Kees Cook wrote:
>
> On Mon, Jan 20, 2025 at 02:44:52PM +0100, Oleg Nesterov wrote:
> > Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL)
> > will crash or not, this is not consistent/safe.
>
> Right now this never happens because there are no callers.
>
> > Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing()
> > has no callers, these architectures use secure_computing_strict().
>
> As you say here.
>
> > Also, after the previous change __secure_computing(sd) is always called
> > with sd == NULL, so it is clear that we can remove the code which makes
> > no sense.
>
> However, after this change, if someone were to *add* a caller, it would
> bypass strict mode.

OK, thanks, I agree this is not consistent, even if I think that
!CONFIG_HAVE_ARCH_SECCOMP_FILTER arches should not add a new caller.

> Instead of "return 0", it seems like it'd be better
> to remove the function entirely (and maybe add a comment about calling
> secure_computing_strict() directly)?

This means that __secure_computing() will be defined even if !CONFIG_SECCOMP,
but it won't be defined if CONFIG_SECCOMP && !CONFIG_HAVE_ARCH_SECCOMP_FILTER.

How about

	__secure_computing()
	{
		return secure_computing_strict(syscall_get_nr(...));
	}

in the "#ifndef CONFIG_HAVE_ARCH_SECCOMP_FILTER" section near
secure_computing_strict() in kernel/seccomp.c ?

Oleg.

Re: [PATCH 2/4] seccomp: kill the dead code in the !CONFIG_HAVE_ARCH_SECCOMP_FILTER version of __secure_computing()

[Kees Cook]

On Tue, Jan 21, 2025 at 03:30:39PM +0100, Oleg Nesterov wrote:
> How about
> 
> 	__secure_computing()
> 	{
> 		return secure_computing_strict(syscall_get_nr(...));
> 	}
> 
> in the "#ifndef CONFIG_HAVE_ARCH_SECCOMP_FILTER" section near
> secure_computing_strict() in kernel/seccomp.c ?

Yeah, that should be good.

-- 
Kees Cook

[PATCH 3/4] seccomp: remove the 'sd' argument from __secure_computing()

[Oleg Nesterov]

After the previous changes 'sd' is always NULL.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/powerpc/kernel/ptrace/ptrace.c | 2 +-
 include/linux/seccomp.h             | 8 ++++----
 kernel/entry/common.c               | 2 +-
 kernel/seccomp.c                    | 7 +++----
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 727ed4a14545..c6997df63287 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -215,7 +215,7 @@ static int do_seccomp(struct pt_regs *regs)
 	 * have already loaded -ENOSYS into r3, or seccomp has put
 	 * something else in r3 (via SECCOMP_RET_ERRNO/TRACE).
 	 */
-	if (__secure_computing(NULL))
+	if (__secure_computing())
 		return -1;
 
 	/*
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index e01dfe57a884..6125baa96b76 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -23,16 +23,16 @@
 #include <asm/seccomp.h>
 
 #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
-extern int __secure_computing(const struct seccomp_data *sd);
+extern int __secure_computing(void);
 static inline int secure_computing(void)
 {
 	if (unlikely(test_syscall_work(SECCOMP)))
-		return  __secure_computing(NULL);
+		return  __secure_computing();
 	return 0;
 }
 #else
 extern void secure_computing_strict(int this_syscall);
-static inline int __secure_computing(const struct seccomp_data *sd) { return 0; }
+static inline int __secure_computing(void) { return 0; }
 #endif
 
 extern long prctl_get_seccomp(void);
@@ -54,7 +54,7 @@ static inline int secure_computing(void) { return 0; }
 #else
 static inline void secure_computing_strict(int this_syscall) { return; }
 #endif
-static inline int __secure_computing(const struct seccomp_data *sd) { return 0; }
+static inline int __secure_computing(void) { return 0; }
 
 static inline long prctl_get_seccomp(void)
 {
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index e33691d5adf7..20154572ede9 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -49,7 +49,7 @@ long syscall_trace_enter(struct pt_regs *regs, long syscall,
 
 	/* Do seccomp after ptrace, to catch any tracer changes. */
 	if (work & SYSCALL_WORK_SECCOMP) {
-		ret = __secure_computing(NULL);
+		ret = __secure_computing();
 		if (ret == -1L)
 			return ret;
 	}
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 385d48293a5f..c29dfe82139e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1347,7 +1347,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 }
 #endif
 
-int __secure_computing(const struct seccomp_data *sd)
+int __secure_computing(void)
 {
 	int mode = current->seccomp.mode;
 	int this_syscall;
@@ -1356,15 +1356,14 @@ int __secure_computing(const struct seccomp_data *sd)
 	    unlikely(current->ptrace & PT_SUSPEND_SECCOMP))
 		return 0;
 
-	this_syscall = sd ? sd->nr :
-		syscall_get_nr(current, current_pt_regs());
+	this_syscall = syscall_get_nr(current, current_pt_regs());
 
 	switch (mode) {
 	case SECCOMP_MODE_STRICT:
 		__secure_computing_strict(this_syscall);  /* may call do_exit */
 		return 0;
 	case SECCOMP_MODE_FILTER:
-		return __seccomp_filter(this_syscall, sd, false);
+		return __seccomp_filter(this_syscall, NULL, false);
 	/* Surviving SECCOMP_RET_KILL_* must be proactively impossible. */
 	case SECCOMP_MODE_DEAD:
 		WARN_ON_ONCE(1);
-- 
2.25.1.362.g51ebf55

Re: [PATCH 3/4] seccomp: remove the 'sd' argument from __secure_computing()

[Kees Cook]

On Mon, Jan 20, 2025 at 02:44:59PM +0100, Oleg Nesterov wrote:
> After the previous changes 'sd' is always NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

[PATCH 4/4] seccomp: remove the 'sd' argument from __seccomp_filter()

[Oleg Nesterov]

After the previous change 'sd' is always NULL.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/seccomp.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index c29dfe82139e..75e293d3c1a1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1213,13 +1213,12 @@ static int seccomp_do_user_notification(int this_syscall,
 	return -1;
 }
 
-static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
-			    const bool recheck_after_trace)
+static int __seccomp_filter(int this_syscall, const bool recheck_after_trace)
 {
 	u32 filter_ret, action;
+	struct seccomp_data sd;
 	struct seccomp_filter *match = NULL;
 	int data;
-	struct seccomp_data sd_local;
 
 	/*
 	 * Make sure that any changes to mode from another thread have
@@ -1227,12 +1226,9 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 	 */
 	smp_rmb();
 
-	if (!sd) {
-		populate_seccomp_data(&sd_local);
-		sd = &sd_local;
-	}
+	populate_seccomp_data(&sd);
 
-	filter_ret = seccomp_run_filters(sd, &match);
+	filter_ret = seccomp_run_filters(&sd, &match);
 	data = filter_ret & SECCOMP_RET_DATA;
 	action = filter_ret & SECCOMP_RET_ACTION_FULL;
 
@@ -1290,13 +1286,13 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		 * a reload of all registers. This does not goto skip since
 		 * a skip would have already been reported.
 		 */
-		if (__seccomp_filter(this_syscall, NULL, true))
+		if (__seccomp_filter(this_syscall, true))
 			return -1;
 
 		return 0;
 
 	case SECCOMP_RET_USER_NOTIF:
-		if (seccomp_do_user_notification(this_syscall, match, sd))
+		if (seccomp_do_user_notification(this_syscall, match, &sd))
 			goto skip;
 
 		return 0;
@@ -1338,8 +1334,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 	return -1;
 }
 #else
-static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
-			    const bool recheck_after_trace)
+static int __seccomp_filter(int this_syscall, const bool recheck_after_trace)
 {
 	BUG();
 
@@ -1363,7 +1358,7 @@ int __secure_computing(void)
 		__secure_computing_strict(this_syscall);  /* may call do_exit */
 		return 0;
 	case SECCOMP_MODE_FILTER:
-		return __seccomp_filter(this_syscall, NULL, false);
+		return __seccomp_filter(this_syscall, false);
 	/* Surviving SECCOMP_RET_KILL_* must be proactively impossible. */
 	case SECCOMP_MODE_DEAD:
 		WARN_ON_ONCE(1);
-- 
2.25.1.362.g51ebf55

Re: [PATCH 4/4] seccomp: remove the 'sd' argument from __seccomp_filter()

[Kees Cook]

On Mon, Jan 20, 2025 at 02:45:05PM +0100, Oleg Nesterov wrote:
> After the previous change 'sd' is always NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook