ARM: ptrace: Restore syscall skipping and restart while tracing

PATCH Superseded Kees Cook 2 participants 5 messages Aug 4, 2023 – Aug 10, 2023

Subsystems (3)

Arm (32-bit) Arm MediaTek ptrace support

lore

Share & tools

lore

Kees Cook

author

2023-08-04 07:11 UTC #0 lore raw

PATCH 1/1 ARM: ptrace: Restore syscall skipping and restart while tracing

Since commit 4e57a4ddf6b0 ("ARM: 9107/1: syscall: always store thread_info->abi_syscall"), the seccomp selftests "syscall_errno", "syscall_faked", and "syscall_restart" have been broken. This was related to two issues:

- seccomp and PTRACE depend on using the special value of "-1" for skipping syscalls. This value wasn't working because it was getting masked by __NR_SYSCALL_MASK in both PTRACE_SET_SYSCALL and get_syscall_nr().

- the syscall entry label "local_restart" is used for resuming syscalls interrupted by signals, but the updated syscall number (in scno) was not being stored in current_thread_info()->abi_syscall, causing traced syscall restarting to fail.

Explicitly test for -1 in PTRACE_SET_SYSCALL and get_syscall_nr(), leaving it exposed when present, allowing tracers to skip syscalls again.

Move the AEABI-only assignment of current_thread_info()->abi_syscall after the "local_restart" label to allow tracers to survive syscall restarting.

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Lecopzer Chen <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc [email protected]

Fixes 4e57a4ddf6b0 ("ARM: 9107/1: syscall: always store thread_info->abi_syscall")

Signed-off-by Kees Cook <[email protected]>

---
Note that I haven't tested OABI at all, and AEABI+OABI_COMPAT doesn't work with seccomp. I booted an AEABI system under AEABI+OABI_COMPAT, but I wasn't able to test tracing...

---
 arch/arm/include/asm/syscall.h | 3 +++
 arch/arm/kernel/entry-common.S | 5 +++--
 arch/arm/kernel/ptrace.c       | 5 +++--
 3 files changed, 9 insertions(+), 4 deletions(-)

arch/arm/include/asm/syscall.h +3 -0

index dfeed440254a..fe4326d938c1 100644
@@ -25,6 +25,9 @@ static inline int syscall_get_nr(struct task_struct *task,
 	if (IS_ENABLED(CONFIG_AEABI) && !IS_ENABLED(CONFIG_OABI_COMPAT))
 		return task_thread_info(task)->abi_syscall;
 
+	if (task_thread_info(task)->abi_syscall == -1)
+		return -1;
+
 	return task_thread_info(task)->abi_syscall & __NR_SYSCALL_MASK;
 }

arch/arm/kernel/entry-common.S +3 -2

index bcc4c9ec3aa4..08bd624e4c6f 100644
@@ -246,8 +246,6 @@ ENTRY(vector_swi)
 	bic	scno, scno, #0xff000000		@ mask off SWI op-code
 	str	scno, [tsk, #TI_ABI_SYSCALL]
 	eor	scno, scno, #__NR_SYSCALL_BASE	@ check OS number
-#else
-	str	scno, [tsk, #TI_ABI_SYSCALL]
 #endif
 	/*
 	 * Reload the registers that may have been corrupted on entry to

@@ -256,6 +254,9 @@ ENTRY(vector_swi)
  TRACE(	ldmia	sp, {r0 - r3}		)
 
 local_restart:
+#if defined(CONFIG_AEABI) && !defined(CONFIG_OABI_COMPAT)
+	str	scno, [tsk, #TI_ABI_SYSCALL]	@ store scno for syscall restart
+#endif
 	ldr	r10, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	stmdb	sp!, {r4, r5}			@ push fifth and sixth args

arch/arm/kernel/ptrace.c +3 -2

index 2d8e2516906b..fef32d73f912 100644
@@ -783,8 +783,9 @@ long arch_ptrace(struct task_struct *child, long request,
 			break;
 
 		case PTRACE_SET_SYSCALL:
-			task_thread_info(child)->abi_syscall = data &
-							__NR_SYSCALL_MASK;
+			if (data != -1)
+				data &= __NR_SYSCALL_MASK;
+			task_thread_info(child)->abi_syscall = data;
 			ret = 0;
 			break;

Signature · 1 line

2.34.1

Arnd Bergmann

2023-08-09 19:48 UTC #1 lore raw

Quoted 4 lines · Kees Cook

Since commit 4e57a4ddf6b0 ("ARM: 9107/1: syscall: always store thread_info->abi_syscall"), the seccomp selftests "syscall_errno", "syscall_faked", and "syscall_restart" have been broken. This was related to two issues:

While it looks like my patch introduced both problems, it might be better to split your fix into two bits.

Quoted 8 lines

- seccomp and PTRACE depend on using the special value of "-1" for skipping syscalls. This value wasn't working because it was getting masked by __NR_SYSCALL_MASK in both PTRACE_SET_SYSCALL and get_syscall_nr().
Explicitly test for -1 in PTRACE_SET_SYSCALL and get_syscall_nr(), leaving it exposed when present, allowing tracers to skip syscalls again.

This part looks good to me, at least it seems to be one of multiple ways of doing this, depending on how we want to encode the syscall skipping in the variable.

Quoted 8 lines

- the syscall entry label "local_restart" is used for resuming syscalls interrupted by signals, but the updated syscall number (in scno) was not being stored in current_thread_info()->abi_syscall, causing traced syscall restarting to fail.
Move the AEABI-only assignment of current_thread_info()->abi_syscall after the "local_restart" label to allow tracers to survive syscall restarting.

I'm not following exactly what you are doing here yet, but I suspect this part is wrong:

Quoted 14 lines

arch/arm/kernel/entry-common.S +0 -2

index bcc4c9ec3aa4..08bd624e4c6f 100644
@@ -246,8 +246,6 @@ ENTRY(vector_swi)
 	bic	scno, scno, #0xff000000		@ mask off SWI op-code
 	str	scno, [tsk, #TI_ABI_SYSCALL]
 	eor	scno, scno, #__NR_SYSCALL_BASE	@ check OS number
-#else
-	str	scno, [tsk, #TI_ABI_SYSCALL]
 #endif
 	/*
 	 * Reload the registers that may have been corrupted on entry to

arch/arm/kernel/entry-common.S ENTRY(vector_swi) · line 262

Quoted 9 lines

arch/arm/kernel/entry-common.S +3 -0

@@ -256,6 +254,9 @@ ENTRY(vector_swi)
  TRACE(	ldmia	sp, {r0 - r3}		)
 
 local_restart:
+#if defined(CONFIG_AEABI) && !defined(CONFIG_OABI_COMPAT)
+	str	scno, [tsk, #TI_ABI_SYSCALL]	@ store scno for syscall restart
+#endif
 	ldr	r10, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	stmdb	sp!, {r4, r5}			@ push fifth and sixth args

If the local_restart code has to store the syscall number for an EABI-only kernel, wouldn't it have to also do this for a kernel with OABI-only or OABI_COMPAT support?

Arnd

Kees Cook

author

2023-08-10 19:32 UTC #2 lore raw

Quoted 8 lines · Arnd Bergmann

Quoted 4 lines · Kees Cook
Since commit 4e57a4ddf6b0 ("ARM: 9107/1: syscall: always store thread_info->abi_syscall"), the seccomp selftests "syscall_errno", "syscall_faked", and "syscall_restart" have been broken. This was related to two issues:
While it looks like my patch introduced both problems, it might be better to split your fix into two bits.

Okay, sounds good.

Quoted 41 lines

Quoted 8 lines
- seccomp and PTRACE depend on using the special value of "-1" for skipping syscalls. This value wasn't working because it was getting masked by __NR_SYSCALL_MASK in both PTRACE_SET_SYSCALL and get_syscall_nr().
Explicitly test for -1 in PTRACE_SET_SYSCALL and get_syscall_nr(), leaving it exposed when present, allowing tracers to skip syscalls again.
This part looks good to me, at least it seems to be one of multiple ways of doing this, depending on how we want to encode the syscall skipping in the variable.
Quoted 8 lines
- the syscall entry label "local_restart" is used for resuming syscalls interrupted by signals, but the updated syscall number (in scno) was not being stored in current_thread_info()->abi_syscall, causing traced syscall restarting to fail.
Move the AEABI-only assignment of current_thread_info()->abi_syscall after the "local_restart" label to allow tracers to survive syscall restarting.
I'm not following exactly what you are doing here yet, but I suspect this part is wrong:
Quoted 16 lines
arch/arm/kernel/entry-common.S +0 -2
index bcc4c9ec3aa4..08bd624e4c6f 100644
@@ -246,8 +246,6 @@ ENTRY(vector_swi)
 	bic	scno, scno, #0xff000000		@ mask off SWI op-code
 	str	scno, [tsk, #TI_ABI_SYSCALL]
 	eor	scno, scno, #__NR_SYSCALL_BASE	@ check OS number
-#else
-	str	scno, [tsk, #TI_ABI_SYSCALL]
 #endif
 	/*
 	 * Reload the registers that may have been corrupted on entry to
@@ -256,6 +254,9 @@ ENTRY(vector_swi)
  TRACE(	ldmia	sp, {r0 - r3}		)
 

Quoted 11 lines

arch/arm/kernel/entry-common.S ENTRY(vector_swi) · line 262
Quoted 7 lines
arch/arm/kernel/entry-common.S +3 -0
 local_restart:
+#if defined(CONFIG_AEABI) && !defined(CONFIG_OABI_COMPAT)
+	str	scno, [tsk, #TI_ABI_SYSCALL]	@ store scno for syscall restart
+#endif
 	ldr	r10, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	stmdb	sp!, {r4, r5}			@ push fifth and sixth args
 
If the local_restart code has to store the syscall number for an EABI-only kernel, wouldn't it have to also do this for a kernel with OABI-only or OABI_COMPAT support?

This is the part I wasn't sure about. Initially I was thinking it didn't matter because it's only a problem for a seccomp tracer, but I realize it might be exposed to a PTRACE tracer too. I was only able to test with EABI since seccomp is disabled for OABI_COMPAT.

Anyway, syscall restart is done this way:

movlt scno, #(__NR_restart_syscall - __NR_SYSCALL_BASE)

Can a EABI call restart an OABI syscall? I think so?

So maybe we just need to add:

str scno, [tsk, #TI_ABI_SYSCALL] @ store scno for syscall restart

after that instead of moving it like I did originally?

Let me test that...

Signature · 1 line

Kees Cook

Arnd Bergmann

2023-08-10 20:10 UTC #3 lore raw

Quoted 11 lines · Kees Cook

Quoted 3 lines · Arnd Bergmann
If the local_restart code has to store the syscall number for an EABI-only kernel, wouldn't it have to also do this for a kernel with OABI-only or OABI_COMPAT support?
This is the part I wasn't sure about. Initially I was thinking it didn't matter because it's only a problem for a seccomp tracer, but I realize it might be exposed to a PTRACE tracer too. I was only able to test with EABI since seccomp is disabled for OABI_COMPAT.

Quoted 5 lines

Anyway, syscall restart is done this way:
movlt scno, #(__NR_restart_syscall - __NR_SYSCALL_BASE)
Can a EABI call restart an OABI syscall? I think so?

There are very few differences between oabi and eabi syscalls, I think it basically comes down to

- the syscall number, and register in which it is passed to the kernel
- a few syscalls that exist for OABI backward compatibility and were deprecated before EABI was added
- a few syscalls that pass a struct with different alignment rules
- epoll_wait() uses a runtime check for the output format

It also seems like the __NR_restart_syscall path is only relevant for syscalls using restart_block for restarting, and that means it's only poll(), futex(), nanosleep(), clock_nanosleep() and their time64 counterparts. All of these are handled by the same entry points for OABI and EABI, i.e. there is no overlap with the exceptions above. Crucially, epoll does not use restart_block, unlike poll().

Quoted 5 lines

So maybe we just need to add:
str scno, [tsk, #TI_ABI_SYSCALL] @ store scno for syscall restart
after that instead of moving it like I did originally?

Yes, I think that works!

For pure EABI and pure OABI kernels, this just does the right thing, storing a plain __NR_restart_syscall in the field without an ABI marker. For an OABI compat task running on an EABI kernel, it will call the EABI version of restart_syscall(), but that is exactly the same as the OABI version, as shown above.

Arnd

Kees Cook

author

2023-08-10 20:15 UTC #4 lore raw

Quoted 27 lines · Arnd Bergmann

Quoted 11 lines · Kees Cook
Quoted 3 lines · Arnd Bergmann
If the local_restart code has to store the syscall number for an EABI-only kernel, wouldn't it have to also do this for a kernel with OABI-only or OABI_COMPAT support?
This is the part I wasn't sure about. Initially I was thinking it didn't matter because it's only a problem for a seccomp tracer, but I realize it might be exposed to a PTRACE tracer too. I was only able to test with EABI since seccomp is disabled for OABI_COMPAT.
Quoted 5 lines
Anyway, syscall restart is done this way:
movlt scno, #(__NR_restart_syscall - __NR_SYSCALL_BASE)
Can a EABI call restart an OABI syscall? I think so?
There are very few differences between oabi and eabi syscalls, I think it basically comes down to
- the syscall number, and register in which it is passed to the kernel
- a few syscalls that exist for OABI backward compatibility and were deprecated before EABI was added
- a few syscalls that pass a struct with different alignment rules
- epoll_wait() uses a runtime check for the output format

Quoted 4 lines

It also seems like the __NR_restart_syscall path is only relevant for syscalls using restart_block for restarting, and that means it's only poll(), futex(), nanosleep(), clock_nanosleep() and their time64 counterparts. All of these are handled by the same entry

Right -- it's a tiny corner case I tripped over years ago while building seccomp filters, so it got added to the selftests. :)

Quoted 10 lines

points for OABI and EABI, i.e. there is no overlap with the exceptions above. Crucially, epoll does not use restart_block, unlike poll().
Quoted 5 lines
So maybe we just need to add:
str scno, [tsk, #TI_ABI_SYSCALL] @ store scno for syscall restart
after that instead of moving it like I did originally?

Quoted 7 lines

Yes, I think that works!
For pure EABI and pure OABI kernels, this just does the right thing, storing a plain __NR_restart_syscall in the field without an ABI marker. For an OABI compat task running on an EABI kernel, it will call the EABI version of restart_syscall(), but that is exactly the same as the OABI version, as shown above.

Okay, excellent. I came to the same conclusion. Patch 1 in the v2 addresses this and tested okay for me.

Thanks for looking at this!

-Kees

Signature · 1 line

Kees Cook