[PATCH v2 0/2] riscv/ptrace: add new regset to access original a0 register

[PATCH v2 0/2] riscv/ptrace: add new regset to access original a0 register

[Celeste Liu]

The orig_a0 is missing in struct user_regs_struct of riscv, and there is
no way to add it without breaking UAPI. (See Link tag below)

Like NT_ARM_SYSTEM_CALL do, we add a new regset name NT_RISCV_ORIG_A0 to
access original a0 register from userspace via ptrace API.

Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/

Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
---
Changes in v2:
- Fix integer width.
- Add selftest.
- Link to v1: https://lore.kernel.org/r/20241201-riscv-new-regset-v1-1-c83c58abcc7b@coelacanthus.name

---
Celeste Liu (1):
      riscv/ptrace: add new regset to access original a0 register

Charlie Jenkins (1):
      riscv: selftests: Add a ptrace test to verify syscall parameter modification

 arch/riscv/kernel/ptrace.c                   |  32 +++++++
 include/uapi/linux/elf.h                     |   1 +
 tools/testing/selftests/riscv/abi/.gitignore |   1 +
 tools/testing/selftests/riscv/abi/Makefile   |   5 +-
 tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
 5 files changed, 172 insertions(+), 1 deletion(-)
---
base-commit: 0e287d31b62bb53ad81d5e59778384a40f8b6f56
change-id: 20241201-riscv-new-regset-d529b952ad0d

Best regards,
-- 
Celeste Liu <uwu@coelacanthus.name>

[PATCH v2 1/2] riscv/ptrace: add new regset to access original a0 register

[Celeste Liu]

The orig_a0 is missing in struct user_regs_struct of riscv, and there is
no way to add it without breaking UAPI. (See Link tag below)

Like NT_ARM_SYSTEM_CALL do, we add a new regset name NT_RISCV_ORIG_A0 to
access original a0 register from userspace via ptrace API.

Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
---
 arch/riscv/kernel/ptrace.c | 32 ++++++++++++++++++++++++++++++++
 include/uapi/linux/elf.h   |  1 +
 2 files changed, 33 insertions(+)

diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index ea67e9fb7a583683b922fe2c017ea61f3bc848db..18ce07ffb27bb1180667769eed800f6fdf96c083 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -31,6 +31,7 @@ enum riscv_regset {
 #ifdef CONFIG_RISCV_ISA_SUPM
 	REGSET_TAGGED_ADDR_CTRL,
 #endif
+	REGSET_ORIG_A0,
 };
 
 static int riscv_gpr_get(struct task_struct *target,
@@ -184,6 +185,29 @@ static int tagged_addr_ctrl_set(struct task_struct *target,
 }
 #endif
 
+static int riscv_orig_a0_get(struct task_struct *target,
+			     const struct user_regset *regset,
+			     struct membuf to)
+{
+	return membuf_store(&to, task_pt_regs(target)->orig_a0);
+}
+
+static int riscv_orig_a0_set(struct task_struct *target,
+			     const struct user_regset *regset,
+			     unsigned int pos, unsigned int count,
+			     const void *kbuf, const void __user *ubuf)
+{
+	unsigned long orig_a0 = task_pt_regs(target)->orig_a0;
+	int ret;
+
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &orig_a0, 0, -1);
+	if (ret)
+		return ret;
+
+	task_pt_regs(target)->orig_a0 = orig_a0;
+	return ret;
+}
+
 static const struct user_regset riscv_user_regset[] = {
 	[REGSET_X] = {
 		.core_note_type = NT_PRSTATUS,
@@ -224,6 +248,14 @@ static const struct user_regset riscv_user_regset[] = {
 		.set = tagged_addr_ctrl_set,
 	},
 #endif
+	[REGSET_ORIG_A0] = {
+		.core_note_type = NT_RISCV_ORIG_A0,
+		.n = 1,
+		.size = sizeof(elf_greg_t),
+		.align = sizeof(elf_greg_t),
+		.regset_get = riscv_orig_a0_get,
+		.set = riscv_orig_a0_set,
+	},
 };
 
 static const struct user_regset_view riscv_user_native_view = {
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index b44069d29cecc0f9de90ee66bfffd2137f4275a8..390060229601631da2fb27030d9fa2142e676c14 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -452,6 +452,7 @@ typedef struct elf64_shdr {
 #define NT_RISCV_CSR	0x900		/* RISC-V Control and Status Registers */
 #define NT_RISCV_VECTOR	0x901		/* RISC-V vector registers */
 #define NT_RISCV_TAGGED_ADDR_CTRL 0x902	/* RISC-V tagged address control (prctl()) */
+#define NT_RISCV_ORIG_A0	  0x903	/* RISC-V original a0 register */
 #define NT_LOONGARCH_CPUCFG	0xa00	/* LoongArch CPU config registers */
 #define NT_LOONGARCH_CSR	0xa01	/* LoongArch control and status registers */
 #define NT_LOONGARCH_LSX	0xa02	/* LoongArch Loongson SIMD Extension registers */

-- 
2.47.0

[PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Celeste Liu]

From: Charlie Jenkins <charlie@rivosinc.com>

This test checks that orig_a0 allows a syscall argument to be modified,
and that changing a0 does not change the syscall argument.

Cc: stable@vger.kernel.org
Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 tools/testing/selftests/riscv/abi/.gitignore |   1 +
 tools/testing/selftests/riscv/abi/Makefile   |   5 +-
 tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
--- a/tools/testing/selftests/riscv/abi/.gitignore
+++ b/tools/testing/selftests/riscv/abi/.gitignore
@@ -1 +1,2 @@
 pointer_masking
+ptrace
diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
--- a/tools/testing/selftests/riscv/abi/Makefile
+++ b/tools/testing/selftests/riscv/abi/Makefile
@@ -2,9 +2,12 @@
 
 CFLAGS += -I$(top_srcdir)/tools/include
 
-TEST_GEN_PROGS := pointer_masking
+TEST_GEN_PROGS := pointer_masking ptrace
 
 include ../../lib.mk
 
 $(OUTPUT)/pointer_masking: pointer_masking.c
 	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
+
+$(OUTPUT)/ptrace: ptrace.c
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
new file mode 100644
index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/ptrace.c
@@ -0,0 +1,134 @@
+// 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_MODIFY      0x01
+#define A0_MODIFY           0x02
+#define A0_OLD              0x03
+#define A0_NEW              0x04
+
+#define perr_and_exit(fmt, ...)						\
+	({								\
+		char buf[256];						\
+		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		perror(buf);						\
+		exit(-1);						\
+	})
+
+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_test(int opt, int *result)
+{
+	int status;
+	pid_t pid;
+	struct user_regs_struct regs;
+	struct iovec iov = {
+		.iov_base = &regs,
+		.iov_len = sizeof(regs),
+	};
+
+	unsigned long orig_a0;
+	struct iovec a0_iov = {
+		.iov_base = &orig_a0,
+		.iov_len = sizeof(orig_a0),
+	};
+
+	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(A0_OLD);
+	}
+
+	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 syscall */
+	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
+
+	/* Check tracee regs before the syscall */
+	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+		perr_and_exit("failed to get tracee registers\n");
+	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
+		perr_and_exit("failed to get tracee registers\n");
+	if (orig_a0 != A0_OLD)
+		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
+
+	/* Modify a0/orig_a0 for the syscall */
+	switch (opt) {
+	case A0_MODIFY:
+		regs.a0 = A0_NEW;
+		break;
+	case ORIG_A0_MODIFY:
+		orig_a0 = A0_NEW;
+		break;
+	}
+
+	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
+
+	/* The modification of a0 cannot affect the first argument of the syscall */
+	EXPECT_EQ(A0_OLD, result);
+}
+
+TEST(ptrace_modify_orig_a0)
+{
+	int result;
+
+	ptrace_test(ORIG_A0_MODIFY, &result);
+
+	/* Only modify orig_a0 to change the first argument of the syscall */
+	EXPECT_EQ(A0_NEW, result);
+}
+
+TEST_HARNESS_MAIN

-- 
2.47.0

Re: [PATCH v2 1/2] riscv/ptrace: add new regset to access original a0 register

[Björn Töpel]

Celeste Liu <uwu@coelacanthus.name> writes:

> The orig_a0 is missing in struct user_regs_struct of riscv, and there is
> no way to add it without breaking UAPI. (See Link tag below)
>
> Like NT_ARM_SYSTEM_CALL do, we add a new regset name NT_RISCV_ORIG_A0 to
> access original a0 register from userspace via ptrace API.
>
> Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> ---
>  arch/riscv/kernel/ptrace.c | 32 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/elf.h   |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index ea67e9fb7a583683b922fe2c017ea61f3bc848db..18ce07ffb27bb1180667769eed800f6fdf96c083 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -31,6 +31,7 @@ enum riscv_regset {
>  #ifdef CONFIG_RISCV_ISA_SUPM
>  	REGSET_TAGGED_ADDR_CTRL,
>  #endif
> +	REGSET_ORIG_A0,
>  };
>  
>  static int riscv_gpr_get(struct task_struct *target,
> @@ -184,6 +185,29 @@ static int tagged_addr_ctrl_set(struct task_struct *target,
>  }
>  #endif
>  
> +static int riscv_orig_a0_get(struct task_struct *target,
> +			     const struct user_regset *regset,
> +			     struct membuf to)
> +{
> +	return membuf_store(&to, task_pt_regs(target)->orig_a0);
> +}
> +
> +static int riscv_orig_a0_set(struct task_struct *target,
> +			     const struct user_regset *regset,
> +			     unsigned int pos, unsigned int count,
> +			     const void *kbuf, const void __user *ubuf)
> +{
> +	unsigned long orig_a0 = task_pt_regs(target)->orig_a0;
> +	int ret;
> +
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &orig_a0, 0, -1);
> +	if (ret)
> +		return ret;
> +
> +	task_pt_regs(target)->orig_a0 = orig_a0;
> +	return ret;

Nit: Could be return 0, for readability.

Regardless,
Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Björn Töpel]

Celeste Liu <uwu@coelacanthus.name> writes:

> From: Charlie Jenkins <charlie@rivosinc.com>
>
> This test checks that orig_a0 allows a syscall argument to be modified,
> and that changing a0 does not change the syscall argument.
>
> Cc: stable@vger.kernel.org
> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

Nice with a selftest! (Don't think the Cc: stable applies... ;-))

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Andrew Jones]

On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote:
> From: Charlie Jenkins <charlie@rivosinc.com>
> 
> This test checks that orig_a0 allows a syscall argument to be modified,
> and that changing a0 does not change the syscall argument.
> 
> Cc: stable@vger.kernel.org
> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
>  tools/testing/selftests/riscv/abi/Makefile   |   5 +-
>  tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
>  3 files changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
> --- a/tools/testing/selftests/riscv/abi/.gitignore
> +++ b/tools/testing/selftests/riscv/abi/.gitignore
> @@ -1 +1,2 @@
>  pointer_masking
> +ptrace
> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
> --- a/tools/testing/selftests/riscv/abi/Makefile
> +++ b/tools/testing/selftests/riscv/abi/Makefile
> @@ -2,9 +2,12 @@
>  
>  CFLAGS += -I$(top_srcdir)/tools/include
>  
> -TEST_GEN_PROGS := pointer_masking
> +TEST_GEN_PROGS := pointer_masking ptrace
>  
>  include ../../lib.mk
>  
>  $(OUTPUT)/pointer_masking: pointer_masking.c
>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> +
> +$(OUTPUT)/ptrace: ptrace.c
> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
> @@ -0,0 +1,134 @@
> +// 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_MODIFY      0x01
> +#define A0_MODIFY           0x02
> +#define A0_OLD              0x03
> +#define A0_NEW              0x04

Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very
unique (we could have those values by accident)? And should we include
setting bits over 31 for 64-bit targets?

> +
> +#define perr_and_exit(fmt, ...)						\
> +	({								\
> +		char buf[256];						\
> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
> +			__func__, __LINE__, ##__VA_ARGS__);		\
> +		perror(buf);						\
> +		exit(-1);						\
> +	})

Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to
consolidate testing/selftests/riscv/* tests on a common format for
errors and exit codes and we're already using other kselftest stuff.

> +
> +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_test(int opt, int *result)
> +{
> +	int status;
> +	pid_t pid;
> +	struct user_regs_struct regs;
> +	struct iovec iov = {
> +		.iov_base = &regs,
> +		.iov_len = sizeof(regs),
> +	};
> +
> +	unsigned long orig_a0;
> +	struct iovec a0_iov = {
> +		.iov_base = &orig_a0,
> +		.iov_len = sizeof(orig_a0),
> +	};
> +
> +	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(A0_OLD);
> +	}
> +
> +	if (pid < 0)
> +		exit(1);

This unexpected error condition deserves a message, so I'd use
ksft_exit_fail_perror() here.

> +
> +	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 syscall */
> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> +
> +	/* Check tracee regs before the syscall */
> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +		perr_and_exit("failed to get tracee registers\n");
> +	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
> +		perr_and_exit("failed to get tracee registers\n");
> +	if (orig_a0 != A0_OLD)
> +		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
> +
> +	/* Modify a0/orig_a0 for the syscall */
> +	switch (opt) {
> +	case A0_MODIFY:
> +		regs.a0 = A0_NEW;
> +		break;
> +	case ORIG_A0_MODIFY:
> +		orig_a0 = A0_NEW;
> +		break;
> +	}
> +
> +	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
> +
> +	/* The modification of a0 cannot affect the first argument of the syscall */
> +	EXPECT_EQ(A0_OLD, result);

What about checking that we actually set regs.a0 to A0_NEW? We'd need
A0_NEW to be more unique than 4, though.

> +}
> +
> +TEST(ptrace_modify_orig_a0)
> +{
> +	int result;
> +
> +	ptrace_test(ORIG_A0_MODIFY, &result);
> +
> +	/* Only modify orig_a0 to change the first argument of the syscall */

If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW
and can't check with this test that we don't set it to A0_NEW. We should
probably have two different test values, one for regs.a0 and one for
orig_a0 and ensure on both tests that we aren't writing both.

> +	EXPECT_EQ(A0_NEW, result);
> +}
> +
> +TEST_HARNESS_MAIN
> 
> -- 
> 2.47.0
>

Thanks,
drew

Re: [PATCH v2 1/2] riscv/ptrace: add new regset to access original a0 register

[Alexandre Ghiti]

Hi Celeste,

On 03/12/2024 10:30, Celeste Liu wrote:
> The orig_a0 is missing in struct user_regs_struct of riscv, and there is
> no way to add it without breaking UAPI. (See Link tag below)
>
> Like NT_ARM_SYSTEM_CALL do, we add a new regset name NT_RISCV_ORIG_A0 to
> access original a0 register from userspace via ptrace API.
>
> Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> ---
>   arch/riscv/kernel/ptrace.c | 32 ++++++++++++++++++++++++++++++++
>   include/uapi/linux/elf.h   |  1 +
>   2 files changed, 33 insertions(+)
>
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index ea67e9fb7a583683b922fe2c017ea61f3bc848db..18ce07ffb27bb1180667769eed800f6fdf96c083 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -31,6 +31,7 @@ enum riscv_regset {
>   #ifdef CONFIG_RISCV_ISA_SUPM
>   	REGSET_TAGGED_ADDR_CTRL,
>   #endif
> +	REGSET_ORIG_A0,
>   };
>   
>   static int riscv_gpr_get(struct task_struct *target,
> @@ -184,6 +185,29 @@ static int tagged_addr_ctrl_set(struct task_struct *target,
>   }
>   #endif
>   
> +static int riscv_orig_a0_get(struct task_struct *target,
> +			     const struct user_regset *regset,
> +			     struct membuf to)
> +{
> +	return membuf_store(&to, task_pt_regs(target)->orig_a0);
> +}
> +
> +static int riscv_orig_a0_set(struct task_struct *target,
> +			     const struct user_regset *regset,
> +			     unsigned int pos, unsigned int count,
> +			     const void *kbuf, const void __user *ubuf)
> +{
> +	unsigned long orig_a0 = task_pt_regs(target)->orig_a0;
> +	int ret;
> +
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &orig_a0, 0, -1);
> +	if (ret)
> +		return ret;
> +
> +	task_pt_regs(target)->orig_a0 = orig_a0;
> +	return ret;
> +}
> +
>   static const struct user_regset riscv_user_regset[] = {
>   	[REGSET_X] = {
>   		.core_note_type = NT_PRSTATUS,
> @@ -224,6 +248,14 @@ static const struct user_regset riscv_user_regset[] = {
>   		.set = tagged_addr_ctrl_set,
>   	},
>   #endif
> +	[REGSET_ORIG_A0] = {
> +		.core_note_type = NT_RISCV_ORIG_A0,
> +		.n = 1,
> +		.size = sizeof(elf_greg_t),
> +		.align = sizeof(elf_greg_t),
> +		.regset_get = riscv_orig_a0_get,
> +		.set = riscv_orig_a0_set,
> +	},
>   };
>   
>   static const struct user_regset_view riscv_user_native_view = {
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index b44069d29cecc0f9de90ee66bfffd2137f4275a8..390060229601631da2fb27030d9fa2142e676c14 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -452,6 +452,7 @@ typedef struct elf64_shdr {
>   #define NT_RISCV_CSR	0x900		/* RISC-V Control and Status Registers */
>   #define NT_RISCV_VECTOR	0x901		/* RISC-V vector registers */
>   #define NT_RISCV_TAGGED_ADDR_CTRL 0x902	/* RISC-V tagged address control (prctl()) */
> +#define NT_RISCV_ORIG_A0	  0x903	/* RISC-V original a0 register */
>   #define NT_LOONGARCH_CPUCFG	0xa00	/* LoongArch CPU config registers */
>   #define NT_LOONGARCH_CSR	0xa01	/* LoongArch control and status registers */
>   #define NT_LOONGARCH_LSX	0xa02	/* LoongArch Loongson SIMD Extension registers */
>

Do you know how far this should be backported? Does the following fixes 
tag make sense?

Fixes: e2c0cdfba7f6 ("RISC-V: User-facing API")

Thanks,

Alex

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Charlie Jenkins]

On Tue, Dec 03, 2024 at 01:55:07PM +0100, Andrew Jones wrote:
> On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote:
> > From: Charlie Jenkins <charlie@rivosinc.com>
> > 
> > This test checks that orig_a0 allows a syscall argument to be modified,
> > and that changing a0 does not change the syscall argument.
> > 
> > Cc: stable@vger.kernel.org
> > Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> > Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> > Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
> > Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  tools/testing/selftests/riscv/abi/.gitignore |   1 +
> >  tools/testing/selftests/riscv/abi/Makefile   |   5 +-
> >  tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
> >  3 files changed, 139 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> > index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
> > --- a/tools/testing/selftests/riscv/abi/.gitignore
> > +++ b/tools/testing/selftests/riscv/abi/.gitignore
> > @@ -1 +1,2 @@
> >  pointer_masking
> > +ptrace
> > diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> > index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
> > --- a/tools/testing/selftests/riscv/abi/Makefile
> > +++ b/tools/testing/selftests/riscv/abi/Makefile
> > @@ -2,9 +2,12 @@
> >  
> >  CFLAGS += -I$(top_srcdir)/tools/include
> >  
> > -TEST_GEN_PROGS := pointer_masking
> > +TEST_GEN_PROGS := pointer_masking ptrace
> >  
> >  include ../../lib.mk
> >  
> >  $(OUTPUT)/pointer_masking: pointer_masking.c
> >  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> > +
> > +$(OUTPUT)/ptrace: ptrace.c
> > +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> > diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
> > --- /dev/null
> > +++ b/tools/testing/selftests/riscv/abi/ptrace.c
> > @@ -0,0 +1,134 @@
> > +// 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_MODIFY      0x01
> > +#define A0_MODIFY           0x02
> > +#define A0_OLD              0x03
> > +#define A0_NEW              0x04
> 
> Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very
> unique (we could have those values by accident)? And should we include
> setting bits over 31 for 64-bit targets?
> 
> > +
> > +#define perr_and_exit(fmt, ...)						\
> > +	({								\
> > +		char buf[256];						\
> > +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
> > +			__func__, __LINE__, ##__VA_ARGS__);		\
> > +		perror(buf);						\
> > +		exit(-1);						\
> > +	})
> 
> Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to
> consolidate testing/selftests/riscv/* tests on a common format for
> errors and exit codes and we're already using other kselftest stuff.
> 
> > +
> > +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_test(int opt, int *result)
> > +{
> > +	int status;
> > +	pid_t pid;
> > +	struct user_regs_struct regs;
> > +	struct iovec iov = {
> > +		.iov_base = &regs,
> > +		.iov_len = sizeof(regs),
> > +	};
> > +
> > +	unsigned long orig_a0;
> > +	struct iovec a0_iov = {
> > +		.iov_base = &orig_a0,
> > +		.iov_len = sizeof(orig_a0),
> > +	};
> > +
> > +	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(A0_OLD);
> > +	}
> > +
> > +	if (pid < 0)
> > +		exit(1);
> 
> This unexpected error condition deserves a message, so I'd use
> ksft_exit_fail_perror() here.
> 
> > +
> > +	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 syscall */
> > +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> > +
> > +	/* Check tracee regs before the syscall */
> > +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> > +		perr_and_exit("failed to get tracee registers\n");
> > +	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
> > +		perr_and_exit("failed to get tracee registers\n");
> > +	if (orig_a0 != A0_OLD)
> > +		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
> > +
> > +	/* Modify a0/orig_a0 for the syscall */
> > +	switch (opt) {
> > +	case A0_MODIFY:
> > +		regs.a0 = A0_NEW;
> > +		break;
> > +	case ORIG_A0_MODIFY:
> > +		orig_a0 = A0_NEW;
> > +		break;
> > +	}
> > +
> > +	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
> > +
> > +	/* The modification of a0 cannot affect the first argument of the syscall */
> > +	EXPECT_EQ(A0_OLD, result);
> 
> What about checking that we actually set regs.a0 to A0_NEW? We'd need
> A0_NEW to be more unique than 4, though.
> 
> > +}
> > +
> > +TEST(ptrace_modify_orig_a0)
> > +{
> > +	int result;
> > +
> > +	ptrace_test(ORIG_A0_MODIFY, &result);
> > +
> > +	/* Only modify orig_a0 to change the first argument of the syscall */
> 
> If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW
> and can't check with this test that we don't set it to A0_NEW. We should
> probably have two different test values, one for regs.a0 and one for
> orig_a0 and ensure on both tests that we aren't writing both.
> 

Celeste, do you want to fix this up or are you waiting for me to?

- Charlie

> > +	EXPECT_EQ(A0_NEW, result);
> > +}
> > +
> > +TEST_HARNESS_MAIN
> > 
> > -- 
> > 2.47.0
> >
> 
> Thanks,
> drew

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Celeste Liu]

On 2024-12-20 02:26, Charlie Jenkins wrote:
> On Tue, Dec 03, 2024 at 01:55:07PM +0100, Andrew Jones wrote:
>> On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote:
>>> From: Charlie Jenkins <charlie@rivosinc.com>
>>>
>>> This test checks that orig_a0 allows a syscall argument to be modified,
>>> and that changing a0 does not change the syscall argument.
>>>
>>> Cc: stable@vger.kernel.org
>>> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
>>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>>> Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
>>> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
>>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>>> ---
>>>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
>>>  tools/testing/selftests/riscv/abi/Makefile   |   5 +-
>>>  tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
>>>  3 files changed, 139 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
>>> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
>>> --- a/tools/testing/selftests/riscv/abi/.gitignore
>>> +++ b/tools/testing/selftests/riscv/abi/.gitignore
>>> @@ -1 +1,2 @@
>>>  pointer_masking
>>> +ptrace
>>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
>>> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
>>> --- a/tools/testing/selftests/riscv/abi/Makefile
>>> +++ b/tools/testing/selftests/riscv/abi/Makefile
>>> @@ -2,9 +2,12 @@
>>>  
>>>  CFLAGS += -I$(top_srcdir)/tools/include
>>>  
>>> -TEST_GEN_PROGS := pointer_masking
>>> +TEST_GEN_PROGS := pointer_masking ptrace
>>>  
>>>  include ../../lib.mk
>>>  
>>>  $(OUTPUT)/pointer_masking: pointer_masking.c
>>>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>>> +
>>> +$(OUTPUT)/ptrace: ptrace.c
>>> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>>> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
>>> @@ -0,0 +1,134 @@
>>> +// 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_MODIFY      0x01
>>> +#define A0_MODIFY           0x02
>>> +#define A0_OLD              0x03
>>> +#define A0_NEW              0x04
>>
>> Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very
>> unique (we could have those values by accident)? And should we include
>> setting bits over 31 for 64-bit targets?
>>
>>> +
>>> +#define perr_and_exit(fmt, ...)						\
>>> +	({								\
>>> +		char buf[256];						\
>>> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
>>> +			__func__, __LINE__, ##__VA_ARGS__);		\
>>> +		perror(buf);						\
>>> +		exit(-1);						\
>>> +	})
>>
>> Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to
>> consolidate testing/selftests/riscv/* tests on a common format for
>> errors and exit codes and we're already using other kselftest stuff.
>>
>>> +
>>> +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_test(int opt, int *result)
>>> +{
>>> +	int status;
>>> +	pid_t pid;
>>> +	struct user_regs_struct regs;
>>> +	struct iovec iov = {
>>> +		.iov_base = &regs,
>>> +		.iov_len = sizeof(regs),
>>> +	};
>>> +
>>> +	unsigned long orig_a0;
>>> +	struct iovec a0_iov = {
>>> +		.iov_base = &orig_a0,
>>> +		.iov_len = sizeof(orig_a0),
>>> +	};
>>> +
>>> +	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(A0_OLD);
>>> +	}
>>> +
>>> +	if (pid < 0)
>>> +		exit(1);
>>
>> This unexpected error condition deserves a message, so I'd use
>> ksft_exit_fail_perror() here.
>>
>>> +
>>> +	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 syscall */
>>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>>> +
>>> +	/* Check tracee regs before the syscall */
>>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>>> +		perr_and_exit("failed to get tracee registers\n");
>>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
>>> +		perr_and_exit("failed to get tracee registers\n");
>>> +	if (orig_a0 != A0_OLD)
>>> +		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
>>> +
>>> +	/* Modify a0/orig_a0 for the syscall */
>>> +	switch (opt) {
>>> +	case A0_MODIFY:
>>> +		regs.a0 = A0_NEW;
>>> +		break;
>>> +	case ORIG_A0_MODIFY:
>>> +		orig_a0 = A0_NEW;
>>> +		break;
>>> +	}
>>> +
>>> +	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
>>> +
>>> +	/* The modification of a0 cannot affect the first argument of the syscall */
>>> +	EXPECT_EQ(A0_OLD, result);
>>
>> What about checking that we actually set regs.a0 to A0_NEW? We'd need
>> A0_NEW to be more unique than 4, though.
>>
>>> +}
>>> +
>>> +TEST(ptrace_modify_orig_a0)
>>> +{
>>> +	int result;
>>> +
>>> +	ptrace_test(ORIG_A0_MODIFY, &result);
>>> +
>>> +	/* Only modify orig_a0 to change the first argument of the syscall */
>>
>> If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW
>> and can't check with this test that we don't set it to A0_NEW. We should
>> probably have two different test values, one for regs.a0 and one for
>> orig_a0 and ensure on both tests that we aren't writing both.
>>
> 
> Celeste, do you want to fix this up or are you waiting for me to?

Sorry for delay. I was busy with household affairs in the past few weeks.
v3 will be sent tomorrow or the day after tomorrow.

I am deeply sorry for this.

> 
> - Charlie
> 
>>> +	EXPECT_EQ(A0_NEW, result);
>>> +}
>>> +
>>> +TEST_HARNESS_MAIN
>>>
>>> -- 
>>> 2.47.0
>>>
>>
>> Thanks,
>> drew

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Charlie Jenkins]

On Fri, Dec 20, 2024 at 05:29:45AM +0800, Celeste Liu wrote:
> 
> On 2024-12-20 02:26, Charlie Jenkins wrote:
> > On Tue, Dec 03, 2024 at 01:55:07PM +0100, Andrew Jones wrote:
> >> On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote:
> >>> From: Charlie Jenkins <charlie@rivosinc.com>
> >>>
> >>> This test checks that orig_a0 allows a syscall argument to be modified,
> >>> and that changing a0 does not change the syscall argument.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> >>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> >>> Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
> >>> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> >>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> >>> ---
> >>>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
> >>>  tools/testing/selftests/riscv/abi/Makefile   |   5 +-
> >>>  tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
> >>>  3 files changed, 139 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> >>> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
> >>> --- a/tools/testing/selftests/riscv/abi/.gitignore
> >>> +++ b/tools/testing/selftests/riscv/abi/.gitignore
> >>> @@ -1 +1,2 @@
> >>>  pointer_masking
> >>> +ptrace
> >>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> >>> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
> >>> --- a/tools/testing/selftests/riscv/abi/Makefile
> >>> +++ b/tools/testing/selftests/riscv/abi/Makefile
> >>> @@ -2,9 +2,12 @@
> >>>  
> >>>  CFLAGS += -I$(top_srcdir)/tools/include
> >>>  
> >>> -TEST_GEN_PROGS := pointer_masking
> >>> +TEST_GEN_PROGS := pointer_masking ptrace
> >>>  
> >>>  include ../../lib.mk
> >>>  
> >>>  $(OUTPUT)/pointer_masking: pointer_masking.c
> >>>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> >>> +
> >>> +$(OUTPUT)/ptrace: ptrace.c
> >>> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> >>> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
> >>> @@ -0,0 +1,134 @@
> >>> +// 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_MODIFY      0x01
> >>> +#define A0_MODIFY           0x02
> >>> +#define A0_OLD              0x03
> >>> +#define A0_NEW              0x04
> >>
> >> Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very
> >> unique (we could have those values by accident)? And should we include
> >> setting bits over 31 for 64-bit targets?
> >>
> >>> +
> >>> +#define perr_and_exit(fmt, ...)						\
> >>> +	({								\
> >>> +		char buf[256];						\
> >>> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
> >>> +			__func__, __LINE__, ##__VA_ARGS__);		\
> >>> +		perror(buf);						\
> >>> +		exit(-1);						\
> >>> +	})
> >>
> >> Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to
> >> consolidate testing/selftests/riscv/* tests on a common format for
> >> errors and exit codes and we're already using other kselftest stuff.
> >>
> >>> +
> >>> +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_test(int opt, int *result)
> >>> +{
> >>> +	int status;
> >>> +	pid_t pid;
> >>> +	struct user_regs_struct regs;
> >>> +	struct iovec iov = {
> >>> +		.iov_base = &regs,
> >>> +		.iov_len = sizeof(regs),
> >>> +	};
> >>> +
> >>> +	unsigned long orig_a0;
> >>> +	struct iovec a0_iov = {
> >>> +		.iov_base = &orig_a0,
> >>> +		.iov_len = sizeof(orig_a0),
> >>> +	};
> >>> +
> >>> +	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(A0_OLD);
> >>> +	}
> >>> +
> >>> +	if (pid < 0)
> >>> +		exit(1);
> >>
> >> This unexpected error condition deserves a message, so I'd use
> >> ksft_exit_fail_perror() here.
> >>
> >>> +
> >>> +	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 syscall */
> >>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
> >>> +
> >>> +	/* Check tracee regs before the syscall */
> >>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> >>> +		perr_and_exit("failed to get tracee registers\n");
> >>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
> >>> +		perr_and_exit("failed to get tracee registers\n");
> >>> +	if (orig_a0 != A0_OLD)
> >>> +		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
> >>> +
> >>> +	/* Modify a0/orig_a0 for the syscall */
> >>> +	switch (opt) {
> >>> +	case A0_MODIFY:
> >>> +		regs.a0 = A0_NEW;
> >>> +		break;
> >>> +	case ORIG_A0_MODIFY:
> >>> +		orig_a0 = A0_NEW;
> >>> +		break;
> >>> +	}
> >>> +
> >>> +	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
> >>> +
> >>> +	/* The modification of a0 cannot affect the first argument of the syscall */
> >>> +	EXPECT_EQ(A0_OLD, result);
> >>
> >> What about checking that we actually set regs.a0 to A0_NEW? We'd need
> >> A0_NEW to be more unique than 4, though.
> >>
> >>> +}
> >>> +
> >>> +TEST(ptrace_modify_orig_a0)
> >>> +{
> >>> +	int result;
> >>> +
> >>> +	ptrace_test(ORIG_A0_MODIFY, &result);
> >>> +
> >>> +	/* Only modify orig_a0 to change the first argument of the syscall */
> >>
> >> If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW
> >> and can't check with this test that we don't set it to A0_NEW. We should
> >> probably have two different test values, one for regs.a0 and one for
> >> orig_a0 and ensure on both tests that we aren't writing both.
> >>
> > 
> > Celeste, do you want to fix this up or are you waiting for me to?
> 
> Sorry for delay. I was busy with household affairs in the past few weeks.
> v3 will be sent tomorrow or the day after tomorrow.
> 
> I am deeply sorry for this.

No need to apologize! Just wanted to make sure you weren't expected me
to update the test :)

- Charlie

> 
> > 
> > - Charlie
> > 
> >>> +	EXPECT_EQ(A0_NEW, result);
> >>> +}
> >>> +
> >>> +TEST_HARNESS_MAIN
> >>>
> >>> -- 
> >>> 2.47.0
> >>>
> >>
> >> Thanks,
> >> drew
> 

Re: [PATCH v2 2/2] riscv: selftests: Add a ptrace test to verify syscall parameter modification

[Celeste Liu]

On 2024-12-20 05:36, Charlie Jenkins wrote:
> On Fri, Dec 20, 2024 at 05:29:45AM +0800, Celeste Liu wrote:
>>
>> On 2024-12-20 02:26, Charlie Jenkins wrote:
>>> On Tue, Dec 03, 2024 at 01:55:07PM +0100, Andrew Jones wrote:
>>>> On Tue, Dec 03, 2024 at 05:30:05PM +0800, Celeste Liu wrote:
>>>>> From: Charlie Jenkins <charlie@rivosinc.com>
>>>>>
>>>>> This test checks that orig_a0 allows a syscall argument to be modified,
>>>>> and that changing a0 does not change the syscall argument.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
>>>>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>>>>> Co-developed-by: Celeste Liu <uwu@coelacanthus.name>
>>>>> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
>>>>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>>>>> ---
>>>>>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
>>>>>  tools/testing/selftests/riscv/abi/Makefile   |   5 +-
>>>>>  tools/testing/selftests/riscv/abi/ptrace.c   | 134 +++++++++++++++++++++++++++
>>>>>  3 files changed, 139 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
>>>>> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
>>>>> --- a/tools/testing/selftests/riscv/abi/.gitignore
>>>>> +++ b/tools/testing/selftests/riscv/abi/.gitignore
>>>>> @@ -1 +1,2 @@
>>>>>  pointer_masking
>>>>> +ptrace
>>>>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
>>>>> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..3f74d059dfdcbce4d45d8ff618781ccea1419061 100644
>>>>> --- a/tools/testing/selftests/riscv/abi/Makefile
>>>>> +++ b/tools/testing/selftests/riscv/abi/Makefile
>>>>> @@ -2,9 +2,12 @@
>>>>>  
>>>>>  CFLAGS += -I$(top_srcdir)/tools/include
>>>>>  
>>>>> -TEST_GEN_PROGS := pointer_masking
>>>>> +TEST_GEN_PROGS := pointer_masking ptrace
>>>>>  
>>>>>  include ../../lib.mk
>>>>>  
>>>>>  $(OUTPUT)/pointer_masking: pointer_masking.c
>>>>>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>>>>> +
>>>>> +$(OUTPUT)/ptrace: ptrace.c
>>>>> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>>>>> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..d192764b428d1f1c442f3957c6fedeb01a48d556
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
>>>>> @@ -0,0 +1,134 @@
>>>>> +// 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_MODIFY      0x01
>>>>> +#define A0_MODIFY           0x02
>>>>> +#define A0_OLD              0x03
>>>>> +#define A0_NEW              0x04
>>>>
>>>> Shouldn't A0_OLD and A0_NEW set more bits, since 3 and 4 aren't very
>>>> unique (we could have those values by accident)? And should we include
>>>> setting bits over 31 for 64-bit targets?
>>>>
>>>>> +
>>>>> +#define perr_and_exit(fmt, ...)						\
>>>>> +	({								\
>>>>> +		char buf[256];						\
>>>>> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
>>>>> +			__func__, __LINE__, ##__VA_ARGS__);		\
>>>>> +		perror(buf);						\
>>>>> +		exit(-1);						\
>>>>> +	})
>>>>
>>>> Can we use e.g. ksft_exit_fail_perror() instead? I'd prefer we try to
>>>> consolidate testing/selftests/riscv/* tests on a common format for
>>>> errors and exit codes and we're already using other kselftest stuff.
>>>>
>>>>> +
>>>>> +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_test(int opt, int *result)
>>>>> +{
>>>>> +	int status;
>>>>> +	pid_t pid;
>>>>> +	struct user_regs_struct regs;
>>>>> +	struct iovec iov = {
>>>>> +		.iov_base = &regs,
>>>>> +		.iov_len = sizeof(regs),
>>>>> +	};
>>>>> +
>>>>> +	unsigned long orig_a0;
>>>>> +	struct iovec a0_iov = {
>>>>> +		.iov_base = &orig_a0,
>>>>> +		.iov_len = sizeof(orig_a0),
>>>>> +	};
>>>>> +
>>>>> +	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(A0_OLD);
>>>>> +	}
>>>>> +
>>>>> +	if (pid < 0)
>>>>> +		exit(1);
>>>>
>>>> This unexpected error condition deserves a message, so I'd use
>>>> ksft_exit_fail_perror() here.
>>>>
>>>>> +
>>>>> +	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 syscall */
>>>>> +	resume_and_wait_tracee(pid, PTRACE_SYSCALL);
>>>>> +
>>>>> +	/* Check tracee regs before the syscall */
>>>>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>>>>> +		perr_and_exit("failed to get tracee registers\n");
>>>>> +	if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
>>>>> +		perr_and_exit("failed to get tracee registers\n");
>>>>> +	if (orig_a0 != A0_OLD)
>>>>> +		perr_and_exit("unexpected orig_a0: 0x%lx\n", orig_a0);
>>>>> +
>>>>> +	/* Modify a0/orig_a0 for the syscall */
>>>>> +	switch (opt) {
>>>>> +	case A0_MODIFY:
>>>>> +		regs.a0 = A0_NEW;
>>>>> +		break;
>>>>> +	case ORIG_A0_MODIFY:
>>>>> +		orig_a0 = A0_NEW;
>>>>> +		break;
>>>>> +	}
>>>>> +
>>>>> +	if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_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_test(A0_MODIFY, &result);
>>>>> +
>>>>> +	/* The modification of a0 cannot affect the first argument of the syscall */
>>>>> +	EXPECT_EQ(A0_OLD, result);
>>>>
>>>> What about checking that we actually set regs.a0 to A0_NEW? We'd need
>>>> A0_NEW to be more unique than 4, though.
>>>>
>>>>> +}
>>>>> +
>>>>> +TEST(ptrace_modify_orig_a0)
>>>>> +{
>>>>> +	int result;
>>>>> +
>>>>> +	ptrace_test(ORIG_A0_MODIFY, &result);
>>>>> +
>>>>> +	/* Only modify orig_a0 to change the first argument of the syscall */
>>>>
>>>> If we run ptrace_modify_a0 first then we've already set regs.a0 to A0_NEW
>>>> and can't check with this test that we don't set it to A0_NEW. We should
>>>> probably have two different test values, one for regs.a0 and one for
>>>> orig_a0 and ensure on both tests that we aren't writing both.
>>>>
>>>
>>> Celeste, do you want to fix this up or are you waiting for me to?
>>
>> Sorry for delay. I was busy with household affairs in the past few weeks.
>> v3 will be sent tomorrow or the day after tomorrow.
>>
>> I am deeply sorry for this.
> 
> No need to apologize! Just wanted to make sure you weren't expected me
> to update the test :)
> 
> - Charlie
> 
>>
>>>
>>> - Charlie
>>>
>>>>> +	EXPECT_EQ(A0_NEW, result);
>>>>> +}
>>>>> +
>>>>> +TEST_HARNESS_MAIN
>>>>>
>>>>> -- 
>>>>> 2.47.0
>>>>>
>>>>
>>>> Thanks,
>>>> drew
>>

v4 has been sent.