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

PATCH In review Andy Lutomirski 2 participants 5 messages Feb 23, 2021 – Mar 1, 2021

Subsystems (6)

Stable branch x86 Kselftest Scheduler +2 more subsystems

lore

Share & tools

lore

Andy Lutomirski

author maintainer

2021-02-23 18:16 UTC #2 lore raw

PATCH v2 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 entry work. This confuses audit and ptrace. For example:

    $ ./tools/testing/selftests/x86/syscall_arg_fault_32
    ...
    strace: pid 264258: entering, ptrace_syscall_info.op == 2
    ...

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

Andy Lutomirski

author maintainer

2021-02-23 18:16 UTC #0 lore raw

v2 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.

Changes from v1:
- The fix is actually correct this time, I hope

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

Andy Lutomirski

author maintainer

2021-02-23 18:17 UTC #1 lore raw

PATCH v2 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]>

---
 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

Andy Lutomirski

author maintainer

2021-02-23 18:17 UTC #4 lore raw

PATCH v2 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

Thomas Gleixner

2021-03-01 15:37 UTC #3 lore raw

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

In reply to Andy Lutomirski

Quoted 24 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 entry work. This confuses audit and ptrace. For example:
    $ ./tools/testing/selftests/x86/syscall_arg_fault_32
    ...
    strace: pid 264258: entering, ptrace_syscall_info.op == 2
    ...
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();

That's still the same as the previous version. The right function (while the name is misleading) to invoke here is irqentry_exit_to_user_mode() because that invokes exit_to_user_mode_prepare() before exit_to_user_mode(). We can rename that function afterwards.

Thanks,

tglx