[RFC PATCH 0/2] riscv: Expose orig_a0 to userspace for ptrace to set the actual a0

[RFC PATCH 0/2] riscv: Expose orig_a0 to userspace for ptrace to set the actual a0

[zhouquan]

From: Quan Zhou <zhouquan@iscas.ac.cn>

Due to the path that modifies a0 in syscall_enter_from_user_mode before the
actual execution of syscall_handler [1], the kernel currently saves a0 to
orig_a0 at the entry point of do_trap_ecall_u as an original copy of a0.
Once the syscall is interrupted and later resumed, the restarted syscall
will use orig_a0 to continue execution.

The above rules generally apply except for ptrace(PTRACE_SETREGSET,),
where the kernel will ignore the tracer's setting of tracee/a0 and
will restart with the tracee's original a0 value. For the current
kernel implementation of ptrace, projects like CRIU/Proot will
encounter issues where the a0 setting becomes ineffective when
performing ptrace(PTRACE_{SET/GET}REGSET,).

Here is a suggested solution, expose orig_a0 to userspace so that ptrace
can choose whether to set orig_a0 based on the actual scenario. In fact,
x86/orig_eax and loongArch/orig_a0 have adopted similar solutions.

[1] link: https://lore.kernel.org/lkml/20230403-crisping-animosity-04ed8a45c625@spud/T/

Quan Zhou (2):
  riscv: Expose orig_a0 in the user_regs_struct structure
  riscv: selftests: Add a ptrace test to check a0 of restarted syscall

 arch/riscv/include/asm/ptrace.h               |   4 +-
 arch/riscv/include/uapi/asm/ptrace.h          |   2 +
 tools/testing/selftests/riscv/Makefile        |   2 +-
 tools/testing/selftests/riscv/abi/.gitignore  |   1 +
 tools/testing/selftests/riscv/abi/Makefile    |  12 ++
 .../riscv/abi/ptrace_restart_syscall.c        | 148 ++++++++++++++++++
 6 files changed, 166 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/riscv/abi/.gitignore
 create mode 100644 tools/testing/selftests/riscv/abi/Makefile
 create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c


base-commit: 6ba59ff4227927d3a8530fc2973b80e94b54d58f
-- 
2.34.1

[RFC PATCH 1/2] riscv: Expose orig_a0 in the user_regs_struct structure

[zhouquan]

From: Quan Zhou <zhouquan@iscas.ac.cn>

Expose orig_a0 to userspace to ensure that users can modify
the actual value of `a0` in the traced process through the
ptrace(PTRACE_SETREGSET, ...) path. Since user_regs_struct is
a subset of pt_regs, we also need to adjust the position of
the orig_a0 field in pt_regs to achieve the correct copy.

Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
---
 arch/riscv/include/asm/ptrace.h      | 4 ++--
 arch/riscv/include/uapi/asm/ptrace.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
index b5b0adcc85c1..37f48d40ae46 100644
--- a/arch/riscv/include/asm/ptrace.h
+++ b/arch/riscv/include/asm/ptrace.h
@@ -45,12 +45,12 @@ struct pt_regs {
 	unsigned long t4;
 	unsigned long t5;
 	unsigned long t6;
+	/* a0 value before the syscall */
+	unsigned long orig_a0;
 	/* Supervisor/Machine CSRs */
 	unsigned long status;
 	unsigned long badaddr;
 	unsigned long cause;
-	/* a0 value before the syscall */
-	unsigned long orig_a0;
 };
 
 #define PTRACE_SYSEMU			0x1f
diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
index a38268b19c3d..3e37f80cb3e8 100644
--- a/arch/riscv/include/uapi/asm/ptrace.h
+++ b/arch/riscv/include/uapi/asm/ptrace.h
@@ -54,6 +54,8 @@ struct user_regs_struct {
 	unsigned long t4;
 	unsigned long t5;
 	unsigned long t6;
+	/* a0 value before the syscall */
+	unsigned long orig_a0;
 };
 
 struct __riscv_f_ext_state {
-- 
2.34.1

[RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[zhouquan]

From: Quan Zhou <zhouquan@iscas.ac.cn>

This test creates two processes: a tracer and a tracee. The tracer actively
sends a SIGUSR1 signal in user mode to interrupt the read syscall being
executed by the tracee. We will reset a0/orig_a0 and then observe the value
of a0 held by the restarted read syscall.

Compared to the test program, a more common scenario is the use of the
exece syscall, which sends a signal in the kernel path to restart
the syscall.

Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
---
 tools/testing/selftests/riscv/Makefile        |   2 +-
 tools/testing/selftests/riscv/abi/.gitignore  |   1 +
 tools/testing/selftests/riscv/abi/Makefile    |  12 ++
 .../riscv/abi/ptrace_restart_syscall.c        | 148 ++++++++++++++++++
 4 files changed, 162 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/riscv/abi/.gitignore
 create mode 100644 tools/testing/selftests/riscv/abi/Makefile
 create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c

diff --git a/tools/testing/selftests/riscv/Makefile b/tools/testing/selftests/riscv/Makefile
index 7ce03d832b64..98541dc2f164 100644
--- a/tools/testing/selftests/riscv/Makefile
+++ b/tools/testing/selftests/riscv/Makefile
@@ -5,7 +5,7 @@
 ARCH ?= $(shell uname -m 2>/dev/null || echo not)
 
 ifneq (,$(filter $(ARCH),riscv))
-RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn
+RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn abi
 else
 RISCV_SUBTARGETS :=
 endif
diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
new file mode 100644
index 000000000000..e1e00ffb9db9
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/.gitignore
@@ -0,0 +1 @@
+abi
diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
new file mode 100644
index 000000000000..634fa7de74e6
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 ARM Limited
+# Originally tools/testing/arm64/abi/Makefile
+
+CFLAGS += -I$(top_srcdir)/tools/include
+
+TEST_GEN_PROGS := ptrace_restart_syscall
+
+include ../../lib.mk
+
+$(OUTPUT)/ptrace_restart_syscall: ptrace_restart_syscall.c
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
new file mode 100644
index 000000000000..3e25548cb95e
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/ptrace.h>
+#include <sys/stat.h>
+#include <sys/user.h>
+#include <sys/wait.h>
+#include <sys/uio.h>
+#include <linux/elf.h>
+#include <linux/unistd.h>
+#include <asm/ptrace.h>
+
+#include "../../kselftest_harness.h"
+
+#define ORIG_A0_AFTER_MODIFIED  0x5
+#define MODIFY_A0               0x01
+#define MODIFY_ORIG_A0          0x02
+
+#define perr_and_exit(fmt, ...) do {			\
+	char buf[256];					\
+	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);	\
+	perror(buf);						\
+	exit(-1);						\
+} while (0)
+
+static inline void resume_and_wait_tracee(pid_t pid, int flag)
+{
+	int status;
+
+	if (ptrace(flag, pid, 0, 0))
+		perr_and_exit("failed to resume the tracee %d", pid);
+
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee %d", pid);
+}
+
+static void ptrace_restart_syscall(int opt, int *result)
+{
+	int status;
+	int p[2], fd_zero;
+	pid_t pid;
+
+	struct user_regs_struct regs;
+	struct iovec iov = {
+		.iov_base = &regs,
+		.iov_len = sizeof(regs),
+	};
+
+	if (pipe(p))
+		perr_and_exit("failed to create a pipe");
+
+	fd_zero = open("/dev/zero", O_RDONLY);
+	if (fd_zero < 0)
+		perr_and_exit("failed to open /dev/zero");
+
+	pid = fork();
+	if (pid == 0) {
+		char c;
+
+		/* Mark oneself being traced */
+		if (ptrace(PTRACE_TRACEME, 0, 0, 0))
+			perr_and_exit("failed to request for tracer to trace me");
+
+		kill(getpid(), SIGSTOP);
+
+		if (read(p[0], &c, 1) != 1)
+			exit(1);
+
+		exit(0);
+	} else if (pid < 0)
+		exit(1);
+
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee %d\n", pid);
+
+	/* Resume the tracee until the next syscall */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Deliver a signal to interrupt the syscall */
+	kill(pid, SIGUSR1);
+
+	/* The tracee stops at syscall exit */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Check tracee orig_a0 before syscall restart */
+	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to get tracee registers");
+	if (regs.orig_a0 != p[0])
+		perr_and_exit("unexpected a0");
+
+	/* Modify a0/orig_a0 for the restarted syscall */
+	switch (opt) {
+	case MODIFY_A0:
+		regs.a0 = fd_zero;
+		break;
+	case MODIFY_ORIG_A0:
+		regs.orig_a0 = fd_zero;
+		break;
+	}
+
+	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to set tracee registers");
+
+	/* Ignore SIGUSR1 signal */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Stop at the entry point of the restarted syscall */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Now, check regs.a0 of the restarted syscall */
+	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to get tracee registers");
+	*result = regs.a0;
+
+	/* Resume the tracee */
+	ptrace(PTRACE_CONT, pid, 0, 0);
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee");
+}
+
+TEST(ptrace_modify_a0)
+{
+	int result;
+
+	ptrace_restart_syscall(MODIFY_A0, &result);
+
+	/* The tracer's modification of a0 cannot affect the restarted tracee */
+	EXPECT_NE(ORIG_A0_AFTER_MODIFIED, result);
+}
+
+TEST(ptrace_modify_orig_a0)
+{
+	int result;
+
+	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
+
+	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
+	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);
+}
+
+TEST_HARNESS_MAIN
-- 
2.34.1

Re: [RFC PATCH 1/2] riscv: Expose orig_a0 in the user_regs_struct structure

[Charlie Jenkins]

On Wed, Jun 19, 2024 at 10:01:27AM +0800, zhouquan@iscas.ac.cn wrote:
> From: Quan Zhou <zhouquan@iscas.ac.cn>
> 
> Expose orig_a0 to userspace to ensure that users can modify
> the actual value of `a0` in the traced process through the
> ptrace(PTRACE_SETREGSET, ...) path. Since user_regs_struct is
> a subset of pt_regs, we also need to adjust the position of
> the orig_a0 field in pt_regs to achieve the correct copy.
> 
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> ---
>  arch/riscv/include/asm/ptrace.h      | 4 ++--
>  arch/riscv/include/uapi/asm/ptrace.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
> index b5b0adcc85c1..37f48d40ae46 100644
> --- a/arch/riscv/include/asm/ptrace.h
> +++ b/arch/riscv/include/asm/ptrace.h
> @@ -45,12 +45,12 @@ struct pt_regs {
>  	unsigned long t4;
>  	unsigned long t5;
>  	unsigned long t6;
> +	/* a0 value before the syscall */
> +	unsigned long orig_a0;
>  	/* Supervisor/Machine CSRs */
>  	unsigned long status;
>  	unsigned long badaddr;
>  	unsigned long cause;
> -	/* a0 value before the syscall */
> -	unsigned long orig_a0;
>  };
>  
>  #define PTRACE_SYSEMU			0x1f
> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
> index a38268b19c3d..3e37f80cb3e8 100644
> --- a/arch/riscv/include/uapi/asm/ptrace.h
> +++ b/arch/riscv/include/uapi/asm/ptrace.h
> @@ -54,6 +54,8 @@ struct user_regs_struct {
>  	unsigned long t4;
>  	unsigned long t5;
>  	unsigned long t6;
> +	/* a0 value before the syscall */
> +	unsigned long orig_a0;
>  };
>  
>  struct __riscv_f_ext_state {
> -- 
> 2.34.1
> 

This is a good addition!

Since orig_a0 is no longer at the bottom of pt_regs, MAX_REG_OFFSET is
now incorrect.

Can you adjust the value of:

#define MAX_REG_OFFSET offsetof(struct pt_regs, orig_a0)

in arch/riscv/include/asm/ptrace.h to be:

#define MAX_REG_OFFSET offsetof(struct pt_regs, cause)

This is something that is very easy to miss. I think it would be
valuable to leave a comment at the top of struct pt_regs pointing out
that MAX_REG_OFFSET needs to be adjusted if struct pt_regs changes.

- Charlie

Re: [RFC PATCH 1/2] riscv: Expose orig_a0 in the user_regs_struct structure

[Quan Zhou]

On 2024/6/20 09:05, Charlie Jenkins wrote:
> On Wed, Jun 19, 2024 at 10:01:27AM +0800, zhouquan@iscas.ac.cn wrote:
>> From: Quan Zhou <zhouquan@iscas.ac.cn>
>>
>> Expose orig_a0 to userspace to ensure that users can modify
>> the actual value of `a0` in the traced process through the
>> ptrace(PTRACE_SETREGSET, ...) path. Since user_regs_struct is
>> a subset of pt_regs, we also need to adjust the position of
>> the orig_a0 field in pt_regs to achieve the correct copy.
>>
>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>> ---
>>   arch/riscv/include/asm/ptrace.h      | 4 ++--
>>   arch/riscv/include/uapi/asm/ptrace.h | 2 ++
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
>> index b5b0adcc85c1..37f48d40ae46 100644
>> --- a/arch/riscv/include/asm/ptrace.h
>> +++ b/arch/riscv/include/asm/ptrace.h
>> @@ -45,12 +45,12 @@ struct pt_regs {
>>   	unsigned long t4;
>>   	unsigned long t5;
>>   	unsigned long t6;
>> +	/* a0 value before the syscall */
>> +	unsigned long orig_a0;
>>   	/* Supervisor/Machine CSRs */
>>   	unsigned long status;
>>   	unsigned long badaddr;
>>   	unsigned long cause;
>> -	/* a0 value before the syscall */
>> -	unsigned long orig_a0;
>>   };
>>   
>>   #define PTRACE_SYSEMU			0x1f
>> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
>> index a38268b19c3d..3e37f80cb3e8 100644
>> --- a/arch/riscv/include/uapi/asm/ptrace.h
>> +++ b/arch/riscv/include/uapi/asm/ptrace.h
>> @@ -54,6 +54,8 @@ struct user_regs_struct {
>>   	unsigned long t4;
>>   	unsigned long t5;
>>   	unsigned long t6;
>> +	/* a0 value before the syscall */
>> +	unsigned long orig_a0;
>>   };
>>   
>>   struct __riscv_f_ext_state {
>> -- 
>> 2.34.1
>>
> This is a good addition!
>
> Since orig_a0 is no longer at the bottom of pt_regs, MAX_REG_OFFSET is
> now incorrect.
>
> Can you adjust the value of:
>
> #define MAX_REG_OFFSET offsetof(struct pt_regs, orig_a0)
>
> in arch/riscv/include/asm/ptrace.h to be:
>
> #define MAX_REG_OFFSET offsetof(struct pt_regs, cause)
>
> This is something that is very easy to miss. I think it would be
> valuable to leave a comment at the top of struct pt_regs pointing out
> that MAX_REG_OFFSET needs to be adjusted if struct pt_regs changes.
>
> - Charlie
>
Oh, good idea!  I'm sorry for missing such an important point earlier.

Once enough information is gathered from this RFC, I will include what 
you mentioned in the V1 series.

Thanks,
Quan
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[Charlie Jenkins]

On Wed, Jun 19, 2024 at 10:01:47AM +0800, zhouquan@iscas.ac.cn wrote:
> From: Quan Zhou <zhouquan@iscas.ac.cn>
> 
> This test creates two processes: a tracer and a tracee. The tracer actively
> sends a SIGUSR1 signal in user mode to interrupt the read syscall being
> executed by the tracee. We will reset a0/orig_a0 and then observe the value
> of a0 held by the restarted read syscall.

I don't quite follow what the goal of this test is. With orig_a0 being
added to the previous patch for ptrace, a more constrained test could
ensure that this value is respected.

> 
> Compared to the test program, a more common scenario is the use of the
> exece syscall, which sends a signal in the kernel path to restart
> the syscall.
> 
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> ---
>  tools/testing/selftests/riscv/Makefile        |   2 +-
>  tools/testing/selftests/riscv/abi/.gitignore  |   1 +
>  tools/testing/selftests/riscv/abi/Makefile    |  12 ++
>  .../riscv/abi/ptrace_restart_syscall.c        | 148 ++++++++++++++++++
>  4 files changed, 162 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/riscv/abi/.gitignore
>  create mode 100644 tools/testing/selftests/riscv/abi/Makefile
>  create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> 
> diff --git a/tools/testing/selftests/riscv/Makefile b/tools/testing/selftests/riscv/Makefile
> index 7ce03d832b64..98541dc2f164 100644
> --- a/tools/testing/selftests/riscv/Makefile
> +++ b/tools/testing/selftests/riscv/Makefile
> @@ -5,7 +5,7 @@
>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>  
>  ifneq (,$(filter $(ARCH),riscv))
> -RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn
> +RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn abi
>  else
>  RISCV_SUBTARGETS :=
>  endif
> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> new file mode 100644
> index 000000000000..e1e00ffb9db9
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/.gitignore
> @@ -0,0 +1 @@
> +abi

The gitignore should contain a list of all of the generated binaries
that should be ignored. Can you put ptrace_restart_syscall in here
instead of abi?

> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> new file mode 100644
> index 000000000000..634fa7de74e6
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/Makefile
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2021 ARM Limited
> +# Originally tools/testing/arm64/abi/Makefile
> +
> +CFLAGS += -I$(top_srcdir)/tools/include
> +
> +TEST_GEN_PROGS := ptrace_restart_syscall
> +
> +include ../../lib.mk
> +
> +$(OUTPUT)/ptrace_restart_syscall: ptrace_restart_syscall.c
> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> new file mode 100644
> index 000000000000..3e25548cb95e
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/ptrace.h>
> +#include <sys/stat.h>
> +#include <sys/user.h>
> +#include <sys/wait.h>
> +#include <sys/uio.h>
> +#include <linux/elf.h>
> +#include <linux/unistd.h>
> +#include <asm/ptrace.h>
> +
> +#include "../../kselftest_harness.h"
> +
> +#define ORIG_A0_AFTER_MODIFIED  0x5
> +#define MODIFY_A0               0x01
> +#define MODIFY_ORIG_A0          0x02
> +
> +#define perr_and_exit(fmt, ...) do {			\
> +	char buf[256];					\
> +	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
> +			__func__, __LINE__, ##__VA_ARGS__);	\
> +	perror(buf);						\
> +	exit(-1);						\
> +} while (0)
> +
> +static inline void resume_and_wait_tracee(pid_t pid, int flag)
> +{
> +	int status;
> +
> +	if (ptrace(flag, pid, 0, 0))
> +		perr_and_exit("failed to resume the tracee %d", pid);
> +
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee %d", pid);
> +}
> +
> +static void ptrace_restart_syscall(int opt, int *result)
> +{
> +	int status;
> +	int p[2], fd_zero;
> +	pid_t pid;
> +
> +	struct user_regs_struct regs;
> +	struct iovec iov = {
> +		.iov_base = &regs,
> +		.iov_len = sizeof(regs),
> +	};
> +
> +	if (pipe(p))
> +		perr_and_exit("failed to create a pipe");
> +
> +	fd_zero = open("/dev/zero", O_RDONLY);
> +	if (fd_zero < 0)
> +		perr_and_exit("failed to open /dev/zero");
> +
> +	pid = fork();
> +	if (pid == 0) {
> +		char c;
> +
> +		/* Mark oneself being traced */
> +		if (ptrace(PTRACE_TRACEME, 0, 0, 0))
> +			perr_and_exit("failed to request for tracer to trace me");
> +
> +		kill(getpid(), SIGSTOP);
> +
> +		if (read(p[0], &c, 1) != 1)
> +			exit(1);
> +
> +		exit(0);
> +	} else if (pid < 0)
> +		exit(1);
> +
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee %d\n", pid);
> +
> +	/* Resume the tracee until the next syscall */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Deliver a signal to interrupt the syscall */
> +	kill(pid, SIGUSR1);
> +
> +	/* The tracee stops at syscall exit */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Check tracee orig_a0 before syscall restart */
> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to get tracee registers");
> +	if (regs.orig_a0 != p[0])
> +		perr_and_exit("unexpected a0");
> +
> +	/* Modify a0/orig_a0 for the restarted syscall */
> +	switch (opt) {
> +	case MODIFY_A0:
> +		regs.a0 = fd_zero;
> +		break;
> +	case MODIFY_ORIG_A0:
> +		regs.orig_a0 = fd_zero;
> +		break;
> +	}
> +
> +	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to set tracee registers");
> +
> +	/* Ignore SIGUSR1 signal */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Stop at the entry point of the restarted syscall */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Now, check regs.a0 of the restarted syscall */
> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to get tracee registers");
> +	*result = regs.a0;
> +
> +	/* Resume the tracee */
> +	ptrace(PTRACE_CONT, pid, 0, 0);
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee");
> +}
> +
> +TEST(ptrace_modify_a0)
> +{
> +	int result;
> +
> +	ptrace_restart_syscall(MODIFY_A0, &result);
> +
> +	/* The tracer's modification of a0 cannot affect the restarted tracee */
> +	EXPECT_NE(ORIG_A0_AFTER_MODIFIED, result);
> +}
> +
> +TEST(ptrace_modify_orig_a0)
> +{
> +	int result;
> +
> +	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
> +
> +	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
> +	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);

How does the value end up being 5?

- Charlie

> +}
> +
> +TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 

Re: [RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[Quan Zhou]

On 2024/6/20 10:55, Charlie Jenkins wrote:
> On Wed, Jun 19, 2024 at 10:01:47AM +0800, zhouquan@iscas.ac.cn wrote:
>> From: Quan Zhou <zhouquan@iscas.ac.cn>
>>
>> This test creates two processes: a tracer and a tracee. The tracer actively
>> sends a SIGUSR1 signal in user mode to interrupt the read syscall being
>> executed by the tracee. We will reset a0/orig_a0 and then observe the value
>> of a0 held by the restarted read syscall.
> 
> I don't quite follow what the goal of this test is. With orig_a0 being
> added to the previous patch for ptrace, a more constrained test could
> ensure that this value is respected.
> 

Sry, I may not have described the patch clearly enough. This patch 
provides a channel for modifying a0 in user-space ptrace via orig_a0. 
Here, I will try to outline the whole situation:

1、When the tracer calls ptrace to modify regs->a0, can the tracee's a0 
be correctly modified?

Through testing, if the user only modifies regs->a0, it doesn't work. Why?

The execution flow of the tracee in the test program is as follows.Prior 
to this explanation:

- PTRACE_SYSCALL can make the tracee block before and after executing
   a syscall.
- The tracer sends SIGUSR1 to interrupt read, and the kernel will
   restart it.
- Please note the point marked with (*), which I believe is the cause
   of the issue.

user     kernel
     |
     |
     |
      read
         | +-> regs->orig_a0 = regs->a0; //(*1)
         |                                       <=tracer:PTRACE_SYSCALL
         | +-> syscall_enter_from_user_mode
               +-> ptrace_report_syscall_entry
                   +-> ptrace_stop
         | //stopped
         |                                       <= tracer:SIGUSR1
         |
         | //resume                              <= tracer:PTRACE_SYSCALL
         | syscall_handler...
         |
         | +-> syscall_exit_to_user_mode
               +-> syscall_exit_to_user_mode_prepare
                   +-> ptrace_report_syscall_exit
                       +-> ptrace_stop
     | //stopped
     |
     | /* Change a0/orig_a0 here and observe the restarted syscall */
     | regs->{a0/orig_a0} = fd_zero; //(*2)
     | ptrace(PTRACE_SETREGSET, ...);
         |                                      <= tracer:PTRACE_SYSCALL
         | //restarting..., skip SIGUSR1
         |
         | +-> exit_to_user_mode_loop
               +-> arch_do_signal_or_restart
                   +-> /* Settings for syscall restart */
                       regs->a0 = regs->orig_a0; //(*3)
     | //stopped
     | //and block before the syscall again, get current regs->a0
     | *result = regs->a0;
     |
     | /* Now, Check regs->a0 of restarted syscall */
     | EXPECT_NE(0x5, result); //for PTRACE_SETREGSSET a0, failed
     | EXPECT_EQ(0x5, result); //for PTRACE_SETREGSSET orig_a0, succeed

If I'm wrong, please let me know. 🙂

2、Actually, I discovered the issue while using the execve function.
When I tried to modify the first parameter of execve in the tracer,
I found it didn't work.

As for why not use execve for testing, there are two reasons:

1) The root cause of this issue is that when a syscall is interrupted
and then resumed, it restarts with orig_a0 instead of a0, so modifying
a0 doesn't work. I want to focus the test on the "restarted syscall".

2) Compared to the current test scenario, execve is terminated by ptrace
earlier, so I chose a later point. In fact, setting regs->a0 in the path
between (*1) and (*3) is ineffective because it will eventually be 
overwritten by orig_a0, correct?

The current test may not intuitively reflect the issue. If possible, I 
will provide a more comprehensive test based on everyone's suggestions.

Thanks,
Quan

>>
>> Compared to the test program, a more common scenario is the use of the
>> exece syscall, which sends a signal in the kernel path to restart
>> the syscall.
>>
>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>> ---
>>   tools/testing/selftests/riscv/Makefile        |   2 +-
>>   tools/testing/selftests/riscv/abi/.gitignore  |   1 +
>>   tools/testing/selftests/riscv/abi/Makefile    |  12 ++
>>   .../riscv/abi/ptrace_restart_syscall.c        | 148 ++++++++++++++++++
>>   4 files changed, 162 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/testing/selftests/riscv/abi/.gitignore
>>   create mode 100644 tools/testing/selftests/riscv/abi/Makefile
>>   create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
>>
>> diff --git a/tools/testing/selftests/riscv/Makefile b/tools/testing/selftests/riscv/Makefile
>> index 7ce03d832b64..98541dc2f164 100644
>> --- a/tools/testing/selftests/riscv/Makefile
>> +++ b/tools/testing/selftests/riscv/Makefile
>> @@ -5,7 +5,7 @@
>>   ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>   
>>   ifneq (,$(filter $(ARCH),riscv))
>> -RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn
>> +RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn abi
>>   else
>>   RISCV_SUBTARGETS :=
>>   endif
>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
>> new file mode 100644
>> index 000000000000..e1e00ffb9db9
>> --- /dev/null
>> +++ b/tools/testing/selftests/riscv/abi/.gitignore
>> @@ -0,0 +1 @@
>> +abi
> 
> The gitignore should contain a list of all of the generated binaries
> that should be ignored. Can you put ptrace_restart_syscall in here
> instead of abi?
> 

...yeah, I will fix it later.

>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
>> new file mode 100644
>> index 000000000000..634fa7de74e6
>> --- /dev/null
>> +++ b/tools/testing/selftests/riscv/abi/Makefile
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021 ARM Limited
>> +# Originally tools/testing/arm64/abi/Makefile
>> +
>> +CFLAGS += -I$(top_srcdir)/tools/include
>> +
>> +TEST_GEN_PROGS := ptrace_restart_syscall
>> +
>> +include ../../lib.mk
>> +
>> +$(OUTPUT)/ptrace_restart_syscall: ptrace_restart_syscall.c
>> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>> diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
>> new file mode 100644
>> index 000000000000..3e25548cb95e
>> --- /dev/null
>> +++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <signal.h>
>> +#include <errno.h>
>> +#include <sys/types.h>
>> +#include <sys/ptrace.h>
>> +#include <sys/stat.h>
>> +#include <sys/user.h>
>> +#include <sys/wait.h>
>> +#include <sys/uio.h>
>> +#include <linux/elf.h>
>> +#include <linux/unistd.h>
>> +#include <asm/ptrace.h>
>> +
>> +#include "../../kselftest_harness.h"
>> +
>> +#define ORIG_A0_AFTER_MODIFIED  0x5
>> +#define MODIFY_A0               0x01
>> +#define MODIFY_ORIG_A0          0x02
>> +
>> +#define perr_and_exit(fmt, ...) do {			\
>> +	char buf[256];					\
>> +	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
>> +			__func__, __LINE__, ##__VA_ARGS__);	\
>> +	perror(buf);						\
>> +	exit(-1);						\
>> +} while (0)
>> +
>> +static inline void resume_and_wait_tracee(pid_t pid, int flag)
>> +{
>> +	int status;
>> +
>> +	if (ptrace(flag, pid, 0, 0))
>> +		perr_and_exit("failed to resume the tracee %d", pid);
>> +
>> +	if (waitpid(pid, &status, 0) != pid)
>> +		perr_and_exit("failed to wait for the tracee %d", pid);
>> +}
>> +
>> +static void ptrace_restart_syscall(int opt, int *result)
>> +{
>> +	int status;
>> +	int p[2], fd_zero;
>> +	pid_t pid;
>> +
>> +	struct user_regs_struct regs;
>> +	struct iovec iov = {
>> +		.iov_base = &regs,
>> +		.iov_len = sizeof(regs),
>> +	};
>> +
>> +	if (pipe(p))
>> +		perr_and_exit("failed to create a pipe");
>> +
>> +	fd_zero = open("/dev/zero", O_RDONLY);
>> +	if (fd_zero < 0)
>> +		perr_and_exit("failed to open /dev/zero");
>> +
>> +	pid = fork();
>> +	if (pid == 0) {
>> +		char c;
>> +
>> +		/* Mark oneself being traced */
>> +		if (ptrace(PTRACE_TRACEME, 0, 0, 0))
>> +			perr_and_exit("failed to request for tracer to trace me");
>> +
>> +		kill(getpid(), SIGSTOP);
>> +
>> +		if (read(p[0], &c, 1) != 1)
>> +			exit(1);
>> +
>> +		exit(0);
>> +	} else if (pid < 0)
>> +		exit(1);
>> +
>> +	if (waitpid(pid, &status, 0) != pid)
>> +		perr_and_exit("failed to wait for the tracee %d\n", pid);
>> +
>> +	/* Resume the tracee until the next syscall */
>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>> +
>> +	/* Deliver a signal to interrupt the syscall */
>> +	kill(pid, SIGUSR1);
>> +
>> +	/* The tracee stops at syscall exit */
>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>> +
>> +	/* Check tracee orig_a0 before syscall restart */
>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>> +		perr_and_exit("failed to get tracee registers");
>> +	if (regs.orig_a0 != p[0])
>> +		perr_and_exit("unexpected a0");
>> +
>> +	/* Modify a0/orig_a0 for the restarted syscall */
>> +	switch (opt) {
>> +	case MODIFY_A0:
>> +		regs.a0 = fd_zero;
>> +		break;
>> +	case MODIFY_ORIG_A0:
>> +		regs.orig_a0 = fd_zero;
>> +		break;
>> +	}
>> +
>> +	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
>> +		perr_and_exit("failed to set tracee registers");
>> +
>> +	/* Ignore SIGUSR1 signal */
>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>> +
>> +	/* Stop at the entry point of the restarted syscall */
>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>> +
>> +	/* Now, check regs.a0 of the restarted syscall */
>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>> +		perr_and_exit("failed to get tracee registers");
>> +	*result = regs.a0;
>> +
>> +	/* Resume the tracee */
>> +	ptrace(PTRACE_CONT, pid, 0, 0);
>> +	if (waitpid(pid, &status, 0) != pid)
>> +		perr_and_exit("failed to wait for the tracee");
>> +}
>> +
>> +TEST(ptrace_modify_a0)
>> +{
>> +	int result;
>> +
>> +	ptrace_restart_syscall(MODIFY_A0, &result);
>> +
>> +	/* The tracer's modification of a0 cannot affect the restarted tracee */
>> +	EXPECT_NE(ORIG_A0_AFTER_MODIFIED, result);
>> +}
>> +
>> +TEST(ptrace_modify_orig_a0)
>> +{
>> +	int result;
>> +
>> +	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
>> +
>> +	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
>> +	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);
> 
> How does the value end up being 5?
> 
> - Charlie
> 

The tracer ultimately sets `fd_zero` to the restarted syscall.

Since 0, 1, and 2 are standard input, output, and error, the file 
descriptors will be allocated in this order: `p[0] -> p[1] -> fd_zero`. 
Thus, fd_zero will be 5.

>> +}
>> +
>> +TEST_HARNESS_MAIN
>> -- 
>> 2.34.1
>>

Re: [RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[Charlie Jenkins]

On Fri, Jun 21, 2024 at 02:29:07PM +0800, Quan Zhou wrote:
> 
> 
> On 2024/6/20 10:55, Charlie Jenkins wrote:
> > On Wed, Jun 19, 2024 at 10:01:47AM +0800, zhouquan@iscas.ac.cn wrote:
> > > From: Quan Zhou <zhouquan@iscas.ac.cn>
> > > 
> > > This test creates two processes: a tracer and a tracee. The tracer actively
> > > sends a SIGUSR1 signal in user mode to interrupt the read syscall being
> > > executed by the tracee. We will reset a0/orig_a0 and then observe the value
> > > of a0 held by the restarted read syscall.
> > 
> > I don't quite follow what the goal of this test is. With orig_a0 being
> > added to the previous patch for ptrace, a more constrained test could
> > ensure that this value is respected.
> > 
> 
> Sry, I may not have described the patch clearly enough. This patch provides
> a channel for modifying a0 in user-space ptrace via orig_a0. Here, I will
> try to outline the whole situation:
> 
> 1、When the tracer calls ptrace to modify regs->a0, can the tracee's a0 be
> correctly modified?
> 
> Through testing, if the user only modifies regs->a0, it doesn't work. Why?
> 
> The execution flow of the tracee in the test program is as follows.Prior to
> this explanation:
> 
> - PTRACE_SYSCALL can make the tracee block before and after executing
>   a syscall.
> - The tracer sends SIGUSR1 to interrupt read, and the kernel will
>   restart it.
> - Please note the point marked with (*), which I believe is the cause
>   of the issue.
> 
> user     kernel
>     |
>     |
>     |
>      read
>         | +-> regs->orig_a0 = regs->a0; //(*1)
>         |                                       <=tracer:PTRACE_SYSCALL
>         | +-> syscall_enter_from_user_mode
>               +-> ptrace_report_syscall_entry
>                   +-> ptrace_stop
>         | //stopped
>         |                                       <= tracer:SIGUSR1
>         |
>         | //resume                              <= tracer:PTRACE_SYSCALL
>         | syscall_handler...
>         |
>         | +-> syscall_exit_to_user_mode
>               +-> syscall_exit_to_user_mode_prepare
>                   +-> ptrace_report_syscall_exit
>                       +-> ptrace_stop
>     | //stopped
>     |
>     | /* Change a0/orig_a0 here and observe the restarted syscall */
>     | regs->{a0/orig_a0} = fd_zero; //(*2)
>     | ptrace(PTRACE_SETREGSET, ...);
>         |                                      <= tracer:PTRACE_SYSCALL
>         | //restarting..., skip SIGUSR1
>         |
>         | +-> exit_to_user_mode_loop
>               +-> arch_do_signal_or_restart
>                   +-> /* Settings for syscall restart */
>                       regs->a0 = regs->orig_a0; //(*3)
>     | //stopped
>     | //and block before the syscall again, get current regs->a0
>     | *result = regs->a0;
>     |
>     | /* Now, Check regs->a0 of restarted syscall */
>     | EXPECT_NE(0x5, result); //for PTRACE_SETREGSSET a0, failed
>     | EXPECT_EQ(0x5, result); //for PTRACE_SETREGSSET orig_a0, succeed
> 
> If I'm wrong, please let me know. 🙂
> 
> 2、Actually, I discovered the issue while using the execve function.
> When I tried to modify the first parameter of execve in the tracer,
> I found it didn't work.
> 
> As for why not use execve for testing, there are two reasons:
> 
> 1) The root cause of this issue is that when a syscall is interrupted
> and then resumed, it restarts with orig_a0 instead of a0, so modifying
> a0 doesn't work. I want to focus the test on the "restarted syscall".
> 
> 2) Compared to the current test scenario, execve is terminated by ptrace
> earlier, so I chose a later point. In fact, setting regs->a0 in the path
> between (*1) and (*3) is ineffective because it will eventually be
> overwritten by orig_a0, correct?

Thank you for the thorough explanation! I feel like a test case like the
following achieves the same goal but without needing the pipes and the
file. What do you think?

From 5f13cdf8f7312b2b3cc98fbfbb3c95fcef62c0f0 Mon Sep 17 00:00:00 2001
From: Charlie Jenkins <charlie@rivosinc.com>
Date: Fri, 21 Jun 2024 12:58:29 -0700
Subject: [PATCH] riscv: selftests: Add a ptrace test to check a0 of restarted
 syscall

Add a riscv selftest that checks that a0 of syscalls are able to be
replaced. When entering a syscall, a0 contains the first argument to the
syscall and upon exiting, a0 contains the return value. To replace the
a0 argument instead of the a0 return value with ptrace after halting the
program with PTRACE_SYSCALL, orig_a0 must be used. This test checks that
orig_a0 allows a syscall argument to be modified, and that changing a0
does not change the syscall argument.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 .../riscv/abi/ptrace_restart_syscall.c        | 123 ++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c

diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
new file mode 100644
index 000000000000..e74ae02c6850
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/ptrace.h>
+#include <sys/stat.h>
+#include <sys/user.h>
+#include <sys/wait.h>
+#include <sys/uio.h>
+#include <linux/elf.h>
+#include <linux/unistd.h>
+#include <asm/ptrace.h>
+
+#include "../../kselftest_harness.h"
+
+#define DEFAULT_A0		0x6
+
+#define ORIG_A0_AFTER_MODIFIED  0x5
+#define MODIFY_A0               0x01
+#define MODIFY_ORIG_A0          0x02
+
+#define perr_and_exit(fmt, ...) do {			\
+	char buf[256];					\
+	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);	\
+	perror(buf);						\
+	exit(-1);						\
+} while (0)
+
+static inline void resume_and_wait_tracee(pid_t pid, int flag)
+{
+	int status;
+
+	if (ptrace(flag, pid, 0, 0))
+		perr_and_exit("failed to resume the tracee %d\n", pid);
+
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee %d\n", pid);
+}
+
+static void ptrace_restart_syscall(int opt, int *result)
+{
+	int status;
+	pid_t pid;
+
+	struct user_regs_struct regs;
+	struct iovec iov = {
+		.iov_base = &regs,
+		.iov_len = sizeof(regs),
+	};
+
+	pid = fork();
+	if (pid == 0) {
+		/* Mark oneself being traced */
+		long val = ptrace(PTRACE_TRACEME, 0, 0, 0);
+		if (val)
+			perr_and_exit("failed to request for tracer to trace me: %ld\n", val);
+
+		kill(getpid(), SIGSTOP);
+
+		/* Perform exit syscall that will be intercepted */
+		exit(DEFAULT_A0);
+	} else if (pid < 0) {
+		exit(1);
+	}
+
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee %d\n", pid);
+
+	/* Stop at the entry point of the restarted syscall */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Now, check regs.a0 of the restarted syscall */
+	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to get tracee registers\n");
+
+	/* Modify a0/orig_a0 for the restarted syscall */
+	switch (opt) {
+	case MODIFY_A0:
+		regs.a0 = ORIG_A0_AFTER_MODIFIED;
+		break;
+	case MODIFY_ORIG_A0:
+		regs.orig_a0 = ORIG_A0_AFTER_MODIFIED;
+		break;
+	}
+
+	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to set tracee registers\n");
+
+	/* Resume the tracee */
+	ptrace(PTRACE_CONT, pid, 0, 0);
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee\n");
+
+	*result = WEXITSTATUS(status);
+}
+
+TEST(ptrace_modify_a0)
+{
+	int result;
+
+	ptrace_restart_syscall(MODIFY_A0, &result);
+
+	/* The tracer's modification of a0 cannot affect the restarted tracee */
+	EXPECT_EQ(DEFAULT_A0, result);
+}
+
+TEST(ptrace_modify_orig_a0)
+{
+	int result;
+
+	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
+
+	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
+	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);
+}
+
+TEST_HARNESS_MAIN
-- 
2.44.0

> 
> The current test may not intuitively reflect the issue. If possible, I will
> provide a more comprehensive test based on everyone's suggestions.
> 
> Thanks,
> Quan
> 
> > > 
> > > Compared to the test program, a more common scenario is the use of the
> > > exece syscall, which sends a signal in the kernel path to restart
> > > the syscall.
> > > 
> > > Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> > > ---
> > >   tools/testing/selftests/riscv/Makefile        |   2 +-
> > >   tools/testing/selftests/riscv/abi/.gitignore  |   1 +
> > >   tools/testing/selftests/riscv/abi/Makefile    |  12 ++
> > >   .../riscv/abi/ptrace_restart_syscall.c        | 148 ++++++++++++++++++
> > >   4 files changed, 162 insertions(+), 1 deletion(-)
> > >   create mode 100644 tools/testing/selftests/riscv/abi/.gitignore
> > >   create mode 100644 tools/testing/selftests/riscv/abi/Makefile
> > >   create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> > > 
> > > diff --git a/tools/testing/selftests/riscv/Makefile b/tools/testing/selftests/riscv/Makefile
> > > index 7ce03d832b64..98541dc2f164 100644
> > > --- a/tools/testing/selftests/riscv/Makefile
> > > +++ b/tools/testing/selftests/riscv/Makefile
> > > @@ -5,7 +5,7 @@
> > >   ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> > >   ifneq (,$(filter $(ARCH),riscv))
> > > -RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn
> > > +RISCV_SUBTARGETS ?= hwprobe vector mm sigreturn abi
> > >   else
> > >   RISCV_SUBTARGETS :=
> > >   endif
> > > diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> > > new file mode 100644
> > > index 000000000000..e1e00ffb9db9
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/riscv/abi/.gitignore
> > > @@ -0,0 +1 @@
> > > +abi
> > 
> > The gitignore should contain a list of all of the generated binaries
> > that should be ignored. Can you put ptrace_restart_syscall in here
> > instead of abi?
> > 
> 
> ...yeah, I will fix it later.
> 
> > > diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> > > new file mode 100644
> > > index 000000000000..634fa7de74e6
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/riscv/abi/Makefile
> > > @@ -0,0 +1,12 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2021 ARM Limited
> > > +# Originally tools/testing/arm64/abi/Makefile
> > > +
> > > +CFLAGS += -I$(top_srcdir)/tools/include
> > > +
> > > +TEST_GEN_PROGS := ptrace_restart_syscall
> > > +
> > > +include ../../lib.mk
> > > +
> > > +$(OUTPUT)/ptrace_restart_syscall: ptrace_restart_syscall.c
> > > +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> > > diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> > > new file mode 100644
> > > index 000000000000..3e25548cb95e
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> > > @@ -0,0 +1,148 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +#include <fcntl.h>
> > > +#include <signal.h>
> > > +#include <errno.h>
> > > +#include <sys/types.h>
> > > +#include <sys/ptrace.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/user.h>
> > > +#include <sys/wait.h>
> > > +#include <sys/uio.h>
> > > +#include <linux/elf.h>
> > > +#include <linux/unistd.h>
> > > +#include <asm/ptrace.h>
> > > +
> > > +#include "../../kselftest_harness.h"
> > > +
> > > +#define ORIG_A0_AFTER_MODIFIED  0x5
> > > +#define MODIFY_A0               0x01
> > > +#define MODIFY_ORIG_A0          0x02
> > > +
> > > +#define perr_and_exit(fmt, ...) do {			\
> > > +	char buf[256];					\
> > > +	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
> > > +			__func__, __LINE__, ##__VA_ARGS__);	\
> > > +	perror(buf);						\
> > > +	exit(-1);						\
> > > +} while (0)
> > > +
> > > +static inline void resume_and_wait_tracee(pid_t pid, int flag)
> > > +{
> > > +	int status;
> > > +
> > > +	if (ptrace(flag, pid, 0, 0))
> > > +		perr_and_exit("failed to resume the tracee %d", pid);
> > > +
> > > +	if (waitpid(pid, &status, 0) != pid)
> > > +		perr_and_exit("failed to wait for the tracee %d", pid);
> > > +}
> > > +
> > > +static void ptrace_restart_syscall(int opt, int *result)
> > > +{
> > > +	int status;
> > > +	int p[2], fd_zero;
> > > +	pid_t pid;
> > > +
> > > +	struct user_regs_struct regs;
> > > +	struct iovec iov = {
> > > +		.iov_base = &regs,
> > > +		.iov_len = sizeof(regs),
> > > +	};
> > > +
> > > +	if (pipe(p))
> > > +		perr_and_exit("failed to create a pipe");
> > > +
> > > +	fd_zero = open("/dev/zero", O_RDONLY);
> > > +	if (fd_zero < 0)
> > > +		perr_and_exit("failed to open /dev/zero");
> > > +
> > > +	pid = fork();
> > > +	if (pid == 0) {
> > > +		char c;
> > > +
> > > +		/* Mark oneself being traced */
> > > +		if (ptrace(PTRACE_TRACEME, 0, 0, 0))
> > > +			perr_and_exit("failed to request for tracer to trace me");
> > > +
> > > +		kill(getpid(), SIGSTOP);
> > > +
> > > +		if (read(p[0], &c, 1) != 1)
> > > +			exit(1);
> > > +
> > > +		exit(0);
> > > +	} else if (pid < 0)
> > > +		exit(1);
> > > +
> > > +	if (waitpid(pid, &status, 0) != pid)
> > > +		perr_and_exit("failed to wait for the tracee %d\n", pid);
> > > +
> > > +	/* Resume the tracee until the next syscall */
> > > +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> > > +
> > > +	/* Deliver a signal to interrupt the syscall */
> > > +	kill(pid, SIGUSR1);
> > > +
> > > +	/* The tracee stops at syscall exit */
> > > +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> > > +
> > > +	/* Check tracee orig_a0 before syscall restart */
> > > +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> > > +		perr_and_exit("failed to get tracee registers");
> > > +	if (regs.orig_a0 != p[0])
> > > +		perr_and_exit("unexpected a0");
> > > +
> > > +	/* Modify a0/orig_a0 for the restarted syscall */
> > > +	switch (opt) {
> > > +	case MODIFY_A0:
> > > +		regs.a0 = fd_zero;
> > > +		break;
> > > +	case MODIFY_ORIG_A0:
> > > +		regs.orig_a0 = fd_zero;
> > > +		break;
> > > +	}
> > > +
> > > +	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
> > > +		perr_and_exit("failed to set tracee registers");
> > > +
> > > +	/* Ignore SIGUSR1 signal */
> > > +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> > > +
> > > +	/* Stop at the entry point of the restarted syscall */
> > > +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> > > +
> > > +	/* Now, check regs.a0 of the restarted syscall */
> > > +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> > > +		perr_and_exit("failed to get tracee registers");
> > > +	*result = regs.a0;
> > > +
> > > +	/* Resume the tracee */
> > > +	ptrace(PTRACE_CONT, pid, 0, 0);
> > > +	if (waitpid(pid, &status, 0) != pid)
> > > +		perr_and_exit("failed to wait for the tracee");
> > > +}
> > > +
> > > +TEST(ptrace_modify_a0)
> > > +{
> > > +	int result;
> > > +
> > > +	ptrace_restart_syscall(MODIFY_A0, &result);
> > > +
> > > +	/* The tracer's modification of a0 cannot affect the restarted tracee */
> > > +	EXPECT_NE(ORIG_A0_AFTER_MODIFIED, result);
> > > +}
> > > +
> > > +TEST(ptrace_modify_orig_a0)
> > > +{
> > > +	int result;
> > > +
> > > +	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
> > > +
> > > +	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
> > > +	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);
> > 
> > How does the value end up being 5?
> > 
> > - Charlie
> > 
> 
> The tracer ultimately sets `fd_zero` to the restarted syscall.
> 
> Since 0, 1, and 2 are standard input, output, and error, the file
> descriptors will be allocated in this order: `p[0] -> p[1] -> fd_zero`.
> Thus, fd_zero will be 5.
> 
> > > +}
> > > +
> > > +TEST_HARNESS_MAIN
> > > -- 
> > > 2.34.1
> > > 
> 

Re: [RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[Quan Zhou]

On 2024/6/22 04:20, Charlie Jenkins wrote:
> On Fri, Jun 21, 2024 at 02:29:07PM +0800, Quan Zhou wrote:
>>
>>
>> On 2024/6/20 10:55, Charlie Jenkins wrote:
>>> On Wed, Jun 19, 2024 at 10:01:47AM +0800, zhouquan@iscas.ac.cn wrote:
>>>> From: Quan Zhou <zhouquan@iscas.ac.cn>
>>>>
>>>> This test creates two processes: a tracer and a tracee. The tracer actively
>>>> sends a SIGUSR1 signal in user mode to interrupt the read syscall being
>>>> executed by the tracee. We will reset a0/orig_a0 and then observe the value
>>>> of a0 held by the restarted read syscall.
>>>
>>> I don't quite follow what the goal of this test is. With orig_a0 being
>>> added to the previous patch for ptrace, a more constrained test could
>>> ensure that this value is respected.
>>>
>>
>> Sry, I may not have described the patch clearly enough. This patch provides
>> a channel for modifying a0 in user-space ptrace via orig_a0. Here, I will
>> try to outline the whole situation:
>>
>> 1、When the tracer calls ptrace to modify regs->a0, can the tracee's a0 be
>> correctly modified?
>>
>> Through testing, if the user only modifies regs->a0, it doesn't work. Why?
>>
>> The execution flow of the tracee in the test program is as follows.Prior to
>> this explanation:
>>
>> - PTRACE_SYSCALL can make the tracee block before and after executing
>>    a syscall.
>> - The tracer sends SIGUSR1 to interrupt read, and the kernel will
>>    restart it.
>> - Please note the point marked with (*), which I believe is the cause
>>    of the issue.
>>
>> user     kernel
>>      |
>>      |
>>      |
>>       read
>>          | +-> regs->orig_a0 = regs->a0; //(*1)
>>          |                                       <=tracer:PTRACE_SYSCALL
>>          | +-> syscall_enter_from_user_mode
>>                +-> ptrace_report_syscall_entry
>>                    +-> ptrace_stop
>>          | //stopped
>>          |                                       <= tracer:SIGUSR1
>>          |
>>          | //resume                              <= tracer:PTRACE_SYSCALL
>>          | syscall_handler...
>>          |
>>          | +-> syscall_exit_to_user_mode
>>                +-> syscall_exit_to_user_mode_prepare
>>                    +-> ptrace_report_syscall_exit
>>                        +-> ptrace_stop
>>      | //stopped
>>      |
>>      | /* Change a0/orig_a0 here and observe the restarted syscall */
>>      | regs->{a0/orig_a0} = fd_zero; //(*2)
>>      | ptrace(PTRACE_SETREGSET, ...);
>>          |                                      <= tracer:PTRACE_SYSCALL
>>          | //restarting..., skip SIGUSR1
>>          |
>>          | +-> exit_to_user_mode_loop
>>                +-> arch_do_signal_or_restart
>>                    +-> /* Settings for syscall restart */
>>                        regs->a0 = regs->orig_a0; //(*3)
>>      | //stopped
>>      | //and block before the syscall again, get current regs->a0
>>      | *result = regs->a0;
>>      |
>>      | /* Now, Check regs->a0 of restarted syscall */
>>      | EXPECT_NE(0x5, result); //for PTRACE_SETREGSSET a0, failed
>>      | EXPECT_EQ(0x5, result); //for PTRACE_SETREGSSET orig_a0, succeed
>>
>> If I'm wrong, please let me know. 🙂
>>
>> 2、Actually, I discovered the issue while using the execve function.
>> When I tried to modify the first parameter of execve in the tracer,
>> I found it didn't work.
>>
>> As for why not use execve for testing, there are two reasons:
>>
>> 1) The root cause of this issue is that when a syscall is interrupted
>> and then resumed, it restarts with orig_a0 instead of a0, so modifying
>> a0 doesn't work. I want to focus the test on the "restarted syscall".
>>
>> 2) Compared to the current test scenario, execve is terminated by ptrace
>> earlier, so I chose a later point. In fact, setting regs->a0 in the path
>> between (*1) and (*3) is ineffective because it will eventually be
>> overwritten by orig_a0, correct?
> 
> Thank you for the thorough explanation! I feel like a test case like the
> following achieves the same goal but without needing the pipes and the
> file. What do you think?
> 
>>From 5f13cdf8f7312b2b3cc98fbfbb3c95fcef62c0f0 Mon Sep 17 00:00:00 2001
> From: Charlie Jenkins <charlie@rivosinc.com>
> Date: Fri, 21 Jun 2024 12:58:29 -0700
> Subject: [PATCH] riscv: selftests: Add a ptrace test to check a0 of restarted
>   syscall
> 
> Add a riscv selftest that checks that a0 of syscalls are able to be
> replaced. When entering a syscall, a0 contains the first argument to the
> syscall and upon exiting, a0 contains the return value. To replace the
> a0 argument instead of the a0 return value with ptrace after halting the
> program with PTRACE_SYSCALL, orig_a0 must be used. This test checks that
> orig_a0 allows a syscall argument to be modified, and that changing a0
> does not change the syscall argument.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>   .../riscv/abi/ptrace_restart_syscall.c        | 123 ++++++++++++++++++
>   1 file changed, 123 insertions(+)
>   create mode 100644 tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> 
> diff --git a/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> new file mode 100644
> index 000000000000..e74ae02c6850
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/ptrace_restart_syscall.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/ptrace.h>
> +#include <sys/stat.h>
> +#include <sys/user.h>
> +#include <sys/wait.h>
> +#include <sys/uio.h>
> +#include <linux/elf.h>
> +#include <linux/unistd.h>
> +#include <asm/ptrace.h>
> +
> +#include "../../kselftest_harness.h"
> +
> +#define DEFAULT_A0		0x6
> +
> +#define ORIG_A0_AFTER_MODIFIED  0x5
> +#define MODIFY_A0               0x01
> +#define MODIFY_ORIG_A0          0x02
> +
> +#define perr_and_exit(fmt, ...) do {			\
> +	char buf[256];					\
> +	snprintf(buf, sizeof(buf), "%s:%d: " fmt ": %m\n",	\
> +			__func__, __LINE__, ##__VA_ARGS__);	\
> +	perror(buf);						\
> +	exit(-1);						\
> +} while (0)
> +
> +static inline void resume_and_wait_tracee(pid_t pid, int flag)
> +{
> +	int status;
> +
> +	if (ptrace(flag, pid, 0, 0))
> +		perr_and_exit("failed to resume the tracee %d\n", pid);
> +
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee %d\n", pid);
> +}
> +
> +static void ptrace_restart_syscall(int opt, int *result)
> +{
> +	int status;
> +	pid_t pid;
> +
> +	struct user_regs_struct regs;
> +	struct iovec iov = {
> +		.iov_base = &regs,
> +		.iov_len = sizeof(regs),
> +	};
> +
> +	pid = fork();
> +	if (pid == 0) {
> +		/* Mark oneself being traced */
> +		long val = ptrace(PTRACE_TRACEME, 0, 0, 0);
> +		if (val)
> +			perr_and_exit("failed to request for tracer to trace me: %ld\n", val);
> +
> +		kill(getpid(), SIGSTOP);
> +
> +		/* Perform exit syscall that will be intercepted */
> +		exit(DEFAULT_A0);
> +	} else if (pid < 0) {
> +		exit(1);
> +	}
> +
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee %d\n", pid);
> +
> +	/* Stop at the entry point of the restarted syscall */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Now, check regs.a0 of the restarted syscall */
> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to get tracee registers\n");
> +
> +	/* Modify a0/orig_a0 for the restarted syscall */
> +	switch (opt) {
> +	case MODIFY_A0:
> +		regs.a0 = ORIG_A0_AFTER_MODIFIED;
> +		break;
> +	case MODIFY_ORIG_A0:
> +		regs.orig_a0 = ORIG_A0_AFTER_MODIFIED;
> +		break;
> +	}
> +
> +	if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to set tracee registers\n");
> +
> +	/* Resume the tracee */
> +	ptrace(PTRACE_CONT, pid, 0, 0);
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee\n");
> +
> +	*result = WEXITSTATUS(status);
> +}
> +
> +TEST(ptrace_modify_a0)
> +{
> +	int result;
> +
> +	ptrace_restart_syscall(MODIFY_A0, &result);
> +
> +	/* The tracer's modification of a0 cannot affect the restarted tracee */
> +	EXPECT_EQ(DEFAULT_A0, result);
> +}
> +
> +TEST(ptrace_modify_orig_a0)
> +{
> +	int result;
> +
> +	ptrace_restart_syscall(MODIFY_ORIG_A0, &result);
> +
> +	/* The tracer must modify orig_a0 to actually change the tracee's a0 */
> +	EXPECT_EQ(ORIG_A0_AFTER_MODIFIED, result);
> +}
> +
> +TEST_HARNESS_MAIN

Great, this test can reflect issues more concisely compared to the 
previous one. I will update it in the next PATCH.

Thanks a lot!