Skip to contentSkip to conversation

x86/entry: A compat syscall bugfix and some test stuff

PATCH Superseded Andy Lutomirski 4 participants 7 messages Feb 23, 2021

Subsystems (6)
Stable branch x86 Kselftest Scheduler +2 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
author maintainer
#2 lore raw

PATCH 2/3 x86/entry: Fix entry/exit mismatch on failed fast 32-bit syscalls

In reply to Andy Lutomirski

On a 32-bit fast syscall that fails to read its arguments from user memory, the kernel currently does syscall exit work but not syscall exit work. This would confuse audit and ptrace.

This is a minimal fix intended for ease of backporting. A more complete cleanup is coming.

Cc [email protected]
Fixes 0b085e68f407 ("x86/entry: Consolidate 32/64 bit syscall entry")
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/entry/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/entry/common.c +2 -1
index 0904f5676e4d..cf4dcf346ca8 100644
@@ -128,7 +128,8 @@ static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
 		regs->ax = -EFAULT;
 
 		instrumentation_end();
-		syscall_exit_to_user_mode(regs);
+		local_irq_disable();
+		exit_to_user_mode();
 		return false;
 	}
Signature · 1 line
2.29.2
author maintainer
#6 lore raw

PATCH 3/3 selftests/x86: Add a missing .note.GNU-stack section to thunks_32.S

In reply to Andy Lutomirski

test_syscall_vdso_32 ended up with an executable stacks because the asm was missing the annotation that says that it is modern and doesn't need an executable stack. Add the annotation.

This was missed in commit aeaaf005da1d ("selftests/x86: Add missing .note.GNU-stack sections").

Signed-off-by Andy Lutomirski <[email protected]>
---
 tools/testing/selftests/x86/thunks_32.S | 2 ++
 1 file changed, 2 insertions(+)
tools/testing/selftests/x86/thunks_32.S +2 -0
index a71d92da8f46..f3f56e681e9f 100644
@@ -45,3 +45,5 @@ call64_from_32:
 	ret
 
 .size call64_from_32, .-call64_from_32
+
+.section .note.GNU-stack,"",%progbits
Signature · 1 line
2.29.2

Andy Lutomirski

author maintainer
#0 lore raw

cover x86/entry: A compat syscall bugfix and some test stuff

The compat syscall argument fixup error path is wrong. Fix it.

This also adds some sanity checks to the kernel that catch the bug when running selftests.

Andy Lutomirski (3):
entry: Check that syscall entries and syscall exits match x86/entry: Fix entry/exit mismatch on failed fast 32-bit syscalls selftests/x86: Add a missing .note.GNU-stack section to thunks_32.S

 arch/x86/entry/common.c                 |  3 ++-
 include/linux/entry-common.h            | 11 +++++++++++
 include/linux/sched.h                   |  1 +
 init/init_task.c                        |  9 +++++++++
 kernel/entry/common.c                   | 25 ++++++++++++++++++++++++-
 tools/testing/selftests/x86/thunks_32.S |  2 ++
 6 files changed, 49 insertions(+), 2 deletions(-)
Signature · 1 line
2.29.2
author maintainer
#1 lore raw

PATCH 1/3 entry: Check that syscall entries and syscall exits match

If arch code calls the wrong kernel entry helpers, syscall entries and exits can get out of sync. Add a new field to task_struct to track the syscall state and validate that it transitions correctly.

Signed-off-by Andy Lutomirski <[email protected]>

---

I'm not in love with this patch. I'm imagining moving TS_COMPAT and such into the new kentry_syscall_state field. What do you all think?

 include/linux/entry-common.h | 11 +++++++++++
 include/linux/sched.h        |  1 +
 init/init_task.c             |  9 +++++++++
 kernel/entry/common.c        | 25 ++++++++++++++++++++++++-
 4 files changed, 45 insertions(+), 1 deletion(-)
include/linux/entry-common.h +11 -0
index ca86a00abe86..c2463b6b71fe 100644
@@ -60,6 +60,17 @@
 	 _TIF_NEED_RESCHED | _TIF_PATCH_PENDING | _TIF_NOTIFY_SIGNAL |	\
 	 ARCH_EXIT_TO_USER_MODE_WORK)
 
+/*
+ * task_struct::kentry_syscall_state
+ *
+ * kentry_syscall_state is reset to zero on syscall exit.  For efficiency
+ * reasons, if CONFIG_PROVE_LOCKING=n, kentry_syscall_state is permitted
+ * to remain 0 even inside a syscall.
+ */
+#ifdef CONFIG_PROVE_LOCKING
+# define KENTRY_SYSCALL_STATE_IN_SYSCALL		1
+#endif
+
 /**
  * arch_check_user_regs - Architecture specific sanity check for user mode regs
  * @regs:	Pointer to currents pt_regs
include/linux/sched.h +1 -0
index 6e3a5eeec509..691057a3b48d 100644
@@ -1002,6 +1002,7 @@ struct task_struct {
 #endif
 	struct seccomp			seccomp;
 	struct syscall_user_dispatch	syscall_dispatch;
+	u32				kentry_syscall_state;
 
 	/* Thread group tracking: */
 	u64				parent_exec_id;
init/init_task.c +9 -0
index 8a992d73e6fb..91050c4f0bb3 100644
@@ -12,6 +12,7 @@
 #include <linux/audit.h>
 #include <linux/numa.h>
 #include <linux/scs.h>
+#include <linux/entry-common.h>
 
 #include <linux/uaccess.h>
 

@@ -212,6 +213,14 @@ struct task_struct init_task
 #ifdef CONFIG_SECCOMP
 	.seccomp	= { .filter_count = ATOMIC_INIT(0) },
 #endif
+#ifdef CONFIG_PROVE_LOCKING
+	/*
+	 * The init task, and kernel threads in general, are considered
+	 * to be "in a syscall".  This way they can execve() and then exit
+	 * the supposed syscall that they were in to go to user mode.
+	 */
+	.kentry_syscall_state = KENTRY_SYSCALL_STATE_IN_SYSCALL,
+#endif
 };
 EXPORT_SYMBOL(init_task);
kernel/entry/common.c +24 -1
index 378341642f94..7e971b866ad2 100644
@@ -83,7 +83,16 @@ static long syscall_trace_enter(struct pt_regs *regs, long syscall,
 static __always_inline long
 __syscall_enter_from_user_work(struct pt_regs *regs, long syscall)
 {
-	unsigned long work = READ_ONCE(current_thread_info()->syscall_work);
+	unsigned long work;
+
+	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
+		WARN_ONCE(current->kentry_syscall_state,
+			  "entering syscall %ld while already in a syscall",
+			  syscall);
+		current->kentry_syscall_state = KENTRY_SYSCALL_STATE_IN_SYSCALL;
+	}
+
+	work = READ_ONCE(current_thread_info()->syscall_work);
 
 	if (work & SYSCALL_WORK_ENTER)
 		syscall = syscall_trace_enter(regs, syscall, work);

@@ -195,6 +204,12 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
 {
 	unsigned long ti_work = READ_ONCE(current_thread_info()->flags);
 
+	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
+		WARN_ONCE(current->kentry_syscall_state &
+			  KENTRY_SYSCALL_STATE_IN_SYSCALL,
+			  "exiting to user mode while in syscall context");
+	}
+
 	lockdep_assert_irqs_disabled();
 
 	if (unlikely(ti_work & EXIT_TO_USER_MODE_WORK))

@@ -282,6 +297,14 @@ static void syscall_exit_to_user_mode_prepare(struct pt_regs *regs)
 	 */
 	if (unlikely(work & SYSCALL_WORK_EXIT))
 		syscall_exit_work(regs, work);
+
+	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
+		WARN_ONCE(!(current->kentry_syscall_state &
+			    KENTRY_SYSCALL_STATE_IN_SYSCALL),
+			  "exiting syscall %lu without entering first", nr);
+	}
+
+	current->kentry_syscall_state = 0;
 }
 
 static __always_inline void __syscall_exit_to_user_mode_work(struct pt_regs *regs)
Signature · 1 line
2.29.2
#3 lore raw

Re: [PATCH 2/3] x86/entry: Fix entry/exit mismatch on failed fast 32-bit syscalls

In reply to Andy Lutomirski
Quoted 3 lines · Andy Lutomirski

On a 32-bit fast syscall that fails to read its arguments from user memory, the kernel currently does syscall exit work but not syscall exit work.

and this sentence does not make any sense.

Quoted 1 line

This would confuse audit and ptrace.

Would? It does confuse it, right?

Quoted 15 lines

This is a minimal fix intended for ease of backporting. A more complete cleanup is coming.

Cc [email protected]
Fixes 0b085e68f407 ("x86/entry: Consolidate 32/64 bit syscall entry")
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/entry/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/entry/common.c +0 -0
index 0904f5676e4d..cf4dcf346ca8 100644

arch/x86/entry/common.c static noinstr bool __do_fast_syscall_32(struct pt_regs *regs) · line 132

Quoted 6 lines
arch/x86/entry/common.c +2 -1
@@ -128,7 +128,8 @@ static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
 		regs->ax = -EFAULT;
 
 		instrumentation_end();
-		syscall_exit_to_user_mode(regs);
+		local_irq_disable();
+		exit_to_user_mode();

While I suggested this with tired brain yesterday night, it's not really correct as it does not go through exit_to_user_mode_prepare() which it really has to.

Thanks,

tglx

maintainer
#4 lore raw

Re: [PATCH 2/3] x86/entry: Fix entry/exit mismatch on failed fast 32-bit syscalls

In reply to Andy Lutomirski
Quoted 19 lines · Andy Lutomirski

On a 32-bit fast syscall that fails to read its arguments from user memory, the kernel currently does syscall exit work but not syscall exit work. This would confuse audit and ptrace.

This is a minimal fix intended for ease of backporting. A more complete cleanup is coming.

Cc [email protected]
Fixes 0b085e68f407 ("x86/entry: Consolidate 32/64 bit syscall entry")
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/entry/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/entry/common.c +0 -0
index 0904f5676e4d..cf4dcf346ca8 100644

arch/x86/entry/common.c static noinstr bool __do_fast_syscall_32(struct pt_regs *regs) · line 134

Quoted 8 lines
arch/x86/entry/common.c +2 -1
@@ -128,7 +128,8 @@ static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
 		regs->ax = -EFAULT;
 
 		instrumentation_end();
-		syscall_exit_to_user_mode(regs);
+		local_irq_disable();
+		exit_to_user_mode();
 		return false;
 	}

I'm confused, twice. Once by your Changelog, and second by the actual patch. Shouldn't every return to userspace pass through exit_to_user_mode_prepare() ? We shouldn't ignore NEED_RESCHED or NOTIFY_RESUME, both of which can be set I think, even if the SYSCALL didn't actually do anything.

#5 lore raw

Re: [PATCH 2/3] x86/entry: Fix entry/exit mismatch on failed fast 32-bit syscalls

Quoted 26 lines · Peter Zijlstra
Quoted 21 lines · Andy Lutomirski

On a 32-bit fast syscall that fails to read its arguments from user memory, the kernel currently does syscall exit work but not syscall exit work. This would confuse audit and ptrace.

This is a minimal fix intended for ease of backporting. A more complete cleanup is coming.

Cc [email protected]
Fixes 0b085e68f407 ("x86/entry: Consolidate 32/64 bit syscall entry")
Signed-off-by Andy Lutomirski <[email protected]>

---
arch/x86/entry/common.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/entry/common.c +0 -0
index 0904f5676e4d..cf4dcf346ca8 100644
@@ -128,7 +128,8 @@ static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
       regs->ax = -EFAULT;
Quoted 12 lines

arch/x86/entry/common.c static noinstr bool __do_fast_syscall_32(struct pt_regs *regs) · line 134

Quoted 6 lines
arch/x86/entry/common.c +2 -1
       instrumentation_end();
-        syscall_exit_to_user_mode(regs);
+        local_irq_disable();
+        exit_to_user_mode();
       return false;
   }

I'm confused, twice. Once by your Changelog, and second by the actual patch. Shouldn't every return to userspace pass through exit_to_user_mode_prepare() ? We shouldn't ignore NEED_RESCHED or NOTIFY_RESUME, both of which can be set I think, even if the SYSCALL didn't actually do anything.

Aaaaahhhhhh! There are too many of these functions. I’ll poke around. I’ll also try to figure out why I didn’t catch this in testing.