[PATCH 0/2] Fixes for the generic entry/exit framework

[PATCH 0/2] Fixes for the generic entry/exit framework

[Ingo Molnar]

This fixes a !CONFIG_SECCOMP build bug found during CI testing,
plus adds a couple of 'noinstr' attribute ordering corrections
similar to 7f6fa101dfac and previous commits.

I resolved the CONFIG_SECCOMP problem by also making GENERIC_ENTRY
depend on HAVE_ARCH_SECCOMP_FILTER. This dependency was implicit
in the new code by virtue of x86 being a modern seccomp-filter
architecture.

The patch makes this explicit. I think it's reasonable to assume
any architecture that wants to make use of the generic code to
have modern seccomp framework. If they don't they'd have to
port it to the old secure_computing_strict() API anyway.

It's on top of the latest tip:x86/entry.

Thanks,

	Ingo

Ingo Molnar (2):
  entry: Fix CONFIG_SECCOMP assumption
  entry: Correct 'noinstr' attributes

 arch/Kconfig          | 6 ++++--
 kernel/entry/common.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] entry: Fix CONFIG_SECCOMP assumption

[Ingo Molnar]

The __secure_computing() callback only exists on CONFIG_SECCOMP=y,
and on architectures that have CONFIG_HAVE_ARCH_SECCOMP_FILTER.

Instead of complicating the #ifdef within the generic entry code,
make the generic entry code depend on the availability of a modern
seccomp framework. This was implicit in the generic code due to
x86 being a modern seccomp-filter architecture.

Also move the Kconfig entry to after its HAVE_ARCH_SECCOMP_FILTER
dependency and fix minor whitespace damage while at it.

Fixes: 142781e108b1: ("entry: Provide generic syscall entry functionality")
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/Kconfig          | 6 ++++--
 kernel/entry/common.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 852a527f418f..c2b29cfc4796 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -27,8 +27,6 @@ config HAVE_IMA_KEXEC
 config HOTPLUG_SMT
 	bool
 
-config GENERIC_ENTRY
-       bool
 
 config OPROFILE
 	tristate "OProfile system profiling"
@@ -654,6 +652,10 @@ config HAVE_IRQ_EXIT_ON_IRQ_STACK
 	  This spares a stack switch and improves cache usage on softirq
 	  processing.
 
+config GENERIC_ENTRY
+	bool
+	depends on HAVE_ARCH_SECCOMP_FILTER
+
 config PGTABLE_LEVELS
 	int
 	default 2
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 495f5c051b03..49ed8b47773a 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -53,12 +53,14 @@ static long syscall_trace_enter(struct pt_regs *regs, long syscall,
 			return -1L;
 	}
 
+#ifdef CONFIG_SECCOMP
 	/* Do seccomp after ptrace, to catch any tracer changes. */
 	if (ti_work & _TIF_SECCOMP) {
 		ret = __secure_computing(NULL);
 		if (ret == -1L)
 			return ret;
 	}
+#endif
 
 	if (unlikely(ti_work & _TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, syscall);
-- 
2.25.1

[PATCH 2/2] entry: Correct 'noinstr' attributes

[Ingo Molnar]

The noinstr attribute is to be specified before the return type in the
same way 'inline' is used.

Similar cases were recently fixed for x86, via:

  7f6fa101dfac: ("x86: Correct noinstr qualifiers")

These 2 cases were carried over by the new generic entry code.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/entry/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 49ed8b47773a..a6da0e4adf93 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -258,7 +258,7 @@ noinstr void irqentry_exit_to_user_mode(struct pt_regs *regs)
 	exit_to_user_mode();
 }
 
-irqentry_state_t noinstr irqentry_enter(struct pt_regs *regs)
+noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
 {
 	irqentry_state_t ret = {
 		.exit_rcu = false,
@@ -335,7 +335,7 @@ void irqentry_exit_cond_resched(void)
 	}
 }
 
-void noinstr irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
+noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 {
 	lockdep_assert_irqs_disabled();
 
-- 
2.25.1

Re: [PATCH 1/2] entry: Fix CONFIG_SECCOMP assumption

[Thomas Gleixner]

Ingo Molnar <mingo@kernel.org> writes:
> The __secure_computing() callback only exists on CONFIG_SECCOMP=y,

No. There is a stub function for the SECCOMP=n case.

> and on architectures that have CONFIG_HAVE_ARCH_SECCOMP_FILTER.

which is a prerequiste for converting over.

> Instead of complicating the #ifdef within the generic entry code,

There is no #ifdef in the generic code and there is none required.

> make the generic entry code depend on the availability of a modern
> seccomp framework. This was implicit in the generic code due to
> x86 being a modern seccomp-filter architecture.
>
> Also move the Kconfig entry to after its HAVE_ARCH_SECCOMP_FILTER
> dependency and fix minor whitespace damage while at it.
>
> Fixes: 142781e108b1: ("entry: Provide generic syscall entry
> functionality")

I don't see what that fixes.

> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/Kconfig          | 6 ++++--
>  kernel/entry/common.c | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 852a527f418f..c2b29cfc4796 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -27,8 +27,6 @@ config HAVE_IMA_KEXEC
>  config HOTPLUG_SMT
>  	bool
>  
> -config GENERIC_ENTRY
> -       bool
>  
>  config OPROFILE
>  	tristate "OProfile system profiling"
> @@ -654,6 +652,10 @@ config HAVE_IRQ_EXIT_ON_IRQ_STACK
>  	  This spares a stack switch and improves cache usage on softirq
>  	  processing.
>  
> +config GENERIC_ENTRY
> +	bool
> +	depends on HAVE_ARCH_SECCOMP_FILTER

Any architecture which wants to switch over to this should have
that. Otherwise the build will just fail and rightfully so.

>  
> +#ifdef CONFIG_SECCOMP

And the exact point of that ifdef is? This code was carefully written
NOT to have ifdefs and all non-active things are optimized out by the
compiler.


>  	/* Do seccomp after ptrace, to catch any tracer changes. */
>  	if (ti_work & _TIF_SECCOMP) {
>  		ret = __secure_computing(NULL);
>  		if (ret == -1L)
>  			return ret;
>  	}
> +#endif

Thanks,

        tglx

Re: [PATCH 2/2] entry: Correct 'noinstr' attributes

[Thomas Gleixner]

Ingo Molnar <mingo@kernel.org> writes:
> The noinstr attribute is to be specified before the return type in the
> same way 'inline' is used.
>
> Similar cases were recently fixed for x86, via:
>
>   7f6fa101dfac: ("x86: Correct noinstr qualifiers")
>
> These 2 cases were carried over by the new generic entry code.

Yes, my bad. I merged the above and then forgot to carry it over.

[tip: core/entry] entry: Correct 'noinstr' attributes

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the core/entry branch of tip:

Commit-ID:     aadfc2f957cb470a5a7e52cc41a2fa86e784bcd2
Gitweb:        https://git.kernel.org/tip/aadfc2f957cb470a5a7e52cc41a2fa86e784bcd2
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Sat, 25 Jul 2020 11:19:51 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sun, 26 Jul 2020 15:42:20 +02:00

entry: Correct 'noinstr' attributes

The noinstr attribute is to be specified before the return type in the
same way 'inline' is used.

Similar cases were recently fixed for x86 in commit 7f6fa101dfac ("x86:
Correct noinstr qualifiers"), but the generic entry code was based on the
the original version and did not carry the fix over.

Fixes: a5497bab5f72 ("entry: Provide generic interrupt entry/exit code")
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200725091951.744848-3-mingo@kernel.org
---
 kernel/entry/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 495f5c0..9852e0d 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -256,7 +256,7 @@ noinstr void irqentry_exit_to_user_mode(struct pt_regs *regs)
 	exit_to_user_mode();
 }
 
-irqentry_state_t noinstr irqentry_enter(struct pt_regs *regs)
+noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
 {
 	irqentry_state_t ret = {
 		.exit_rcu = false,
@@ -333,7 +333,7 @@ void irqentry_exit_cond_resched(void)
 	}
 }
 
-void noinstr irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
+noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 {
 	lockdep_assert_irqs_disabled();
 

Re: [PATCH 1/2] entry: Fix CONFIG_SECCOMP assumption

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Ingo Molnar <mingo@kernel.org> writes:
> > The __secure_computing() callback only exists on CONFIG_SECCOMP=y,
> 
> No. There is a stub function for the SECCOMP=n case.

Which was buggy:

  static inline int __secure_computing(void) { return 0; }

Note the 'void' argument, while it should take an argument.

For example on x86-64 allnoconfig there's !CONFIG_SECCOMP.

> > and on architectures that have CONFIG_HAVE_ARCH_SECCOMP_FILTER.
> 
> which is a prerequiste for converting over.

Yeah, and the patch I sent makes this explicit instead of implicit.

> > Instead of complicating the #ifdef within the generic entry code,
> 
> There is no #ifdef in the generic code and there is none required.

I simply carried that #ifdef over from the x86 code, but indeed fixing 
the stub is a bit cleaner.

> > make the generic entry code depend on the availability of a modern
> > seccomp framework. This was implicit in the generic code due to
> > x86 being a modern seccomp-filter architecture.
> >
> > Also move the Kconfig entry to after its HAVE_ARCH_SECCOMP_FILTER
> > dependency and fix minor whitespace damage while at it.
> >
> > Fixes: 142781e108b1: ("entry: Provide generic syscall entry
> > functionality")
> 
> I don't see what that fixes.

A build bug.

Thanks,

	Ingo

Re: [PATCH 1/2] entry: Fix CONFIG_SECCOMP assumption

[Thomas Gleixner]

Ingo Molnar <mingo@kernel.org> writes:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> Ingo Molnar <mingo@kernel.org> writes:
>> > The __secure_computing() callback only exists on CONFIG_SECCOMP=y,
>> 
>> No. There is a stub function for the SECCOMP=n case.
>
> Which was buggy:
>
>   static inline int __secure_computing(void) { return 0; }

Yes. I screwed that up and fixing that is the right thing to do.