On 2024-12-03 13:37, Charlie Jenkins wrote: > On Sun, Dec 01, 2024 at 05:47:13AM +0800, 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) > > We have had a patch sitting on the lists for a very long time to do this > which I guess didn't get enough attention. I am glad that we have more > eyes on this problem now so it can actually be fixed :) [1]. > > However that patch has the problem that it modifies the > user_regs_struct. It is super unfortunate that riscv didn't have the > foresight of loongarch to add padding. > > There is a nice test case in there that would be great to get added > alongside this commit with the appropriate changes. [2] > > [1] https://lore.kernel.org/linux-riscv/cover.1719408040.git.zhouquan@xxxxxxxxxxx/ > [2] https://lore.kernel.org/linux-riscv/1e9cbab1b0badc05592fce46717418930076a6ae.1719408040.git.zhouquan@xxxxxxxxxxx/ > > > Since I am familiar with the code I have gone ahead and made the > appropriate changes. Here is the diff: > > From f35184467cc7b319c2a5c5c034d18119c46f54c2 Mon Sep 17 00:00:00 2001 > From: Charlie Jenkins <charlie@xxxxxxxxxxxx> > Date: Mon, 2 Dec 2024 21:19:13 -0800 > Subject: [PATCH] riscv: selftests: Add a ptrace test to verify syscall > parameter modification > > This test checks that orig_a0 allows a syscall argument to be modified, > and that changing a0 does not change the syscall argument. > > Co-developed-by: Quan Zhou <zhouquan@xxxxxxxxxxx> > Signed-off-by: Charlie Jenkins <charlie@xxxxxxxxxxxx> > --- > arch/riscv/kernel/ptrace.c | 2 +- > tools/testing/selftests/riscv/abi/.gitignore | 1 + > tools/testing/selftests/riscv/abi/Makefile | 5 +- > tools/testing/selftests/riscv/abi/ptrace.c | 133 +++++++++++++++++++ > 4 files changed, 139 insertions(+), 2 deletions(-) > create mode 100644 tools/testing/selftests/riscv/abi/ptrace.c > > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c > index faa46de90003..025c22894d32 100644 > --- a/arch/riscv/kernel/ptrace.c > +++ b/arch/riscv/kernel/ptrace.c > @@ -197,7 +197,7 @@ static int riscv_orig_a0_set(struct task_struct *target, > unsigned int pos, unsigned int count, > const void *kbuf, const void __user *ubuf) > { > - int orig_a0 = task_pt_regs(target)->orig_a0; > + unsigned long orig_a0 = task_pt_regs(target)->orig_a0; > int ret; > > ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &orig_a0, 0, -1); > diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore > index b38358f91c4d..378c605919a3 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 ed82ff9c664e..3f74d059dfdc 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 000000000000..1c3ce40d6a34 > --- /dev/null > +++ b/tools/testing/selftests/riscv/abi/ptrace.c > @@ -0,0 +1,133 @@ > +// 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 = ®s, > + .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.34.1 > > >> >> 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@xxxxxxxxx/ >> Signed-off-by: Celeste Liu <uwu@xxxxxxxxxxxxxxxxx> >> --- >> arch/riscv/kernel/ptrace.c | 33 +++++++++++++++++++++++++++++++++ >> include/uapi/linux/elf.h | 1 + >> 2 files changed, 34 insertions(+) >> >> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c >> index ea67e9fb7a583683b922fe2c017ea61f3bc848db..faa46de9000376eb445a32d43a40210d7b846844 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,30 @@ 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) >> +{ >> + int orig_a0 = task_pt_regs(target)->orig_a0; > > The testcase above highlights that this should be of type "unsigned > long" instead of int! Otherwise 64-bit systems will only be able to set > the first 32 bits (as Björn pointed out in the other thread) :) > > This issue was found because the test case tries to set all 64 bits and > succeeds, but the extra bits corrupt the stack. Maybe the code here > should enforce that the count is equal to the size of an unsigned long? > Fortunately the extra bits ended up in the stack so it was determined to > be corrupted, but I suppose that will not necessarily always be the case > depending on kernel compiler optimizations and user_regset_copyin() > could end up overwritting other data in this function undetected. Yes. It's my mistake. v2 included fixes and the selftest. It has been sent. > > - Charlie > >> + 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 +249,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 */ >> >> --- >> base-commit: 0e287d31b62bb53ad81d5e59778384a40f8b6f56 >> change-id: 20241201-riscv-new-regset-d529b952ad0d >> >> Best regards, >> -- >> Celeste Liu <uwu@xxxxxxxxxxxxxxxxx> >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/linux-riscv
