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 --- 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; + 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,
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH] riscv/ptrace: add new regset to get original a0 register Link: https://lore.kernel.org/stable/20241201-riscv-new-regset-v1-1-c83c58abcc7b%4...
Thanks for working on this!
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/ Signed-off-by: Celeste Liu uwu@coelacanthus.name
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)
Use full 100 chars!
+{
- 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)
Dito!
+{
- int orig_a0 = task_pt_regs(target)->orig_a0;
64b regs on RV64.
- 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;
+}
Multiple blanks.
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),
...and sizeof(elf_greg_t) is 64b in RV64 -- mismatch above.
Björn
On 2024-12-02 23:44, Björn Töpel wrote:
Thanks for working on this!
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/ Signed-off-by: Celeste Liu uwu@coelacanthus.name
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)Use full 100 chars!
Linux code style prefer 80 limit.
+{
- 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)Dito!
+{
- int orig_a0 = task_pt_regs(target)->orig_a0;
64b regs on RV64.
Oh, my bad.
- 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;
+}
Multiple blanks.
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),...and sizeof(elf_greg_t) is 64b in RV64 -- mismatch above.
v2 has been sent.
Björn
Celeste Liu uwu@coelacanthus.name writes:
+static int riscv_orig_a0_get(struct task_struct *target,
const struct user_regset *regset,struct membuf to)Use full 100 chars!
Linux code style prefer 80 limit.
No, it has been increased to 100, since many years [1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scri...
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@iscas.ac.c... [2] https://lore.kernel.org/linux-riscv/1e9cbab1b0badc05592fce46717418930076a6ae...
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@rivosinc.com 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@iscas.ac.cn Signed-off-by: Charlie Jenkins charlie@rivosinc.com --- 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@gmail.com/ Signed-off-by: Celeste Liu uwu@coelacanthus.name
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.
- 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@coelacanthus.name
linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
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@iscas.ac.c... [2] https://lore.kernel.org/linux-riscv/1e9cbab1b0badc05592fce46717418930076a6ae...
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@rivosinc.com 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@iscas.ac.cn Signed-off-by: Charlie Jenkins charlie@rivosinc.com
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@gmail.com/ Signed-off-by: Celeste Liu uwu@coelacanthus.name
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@coelacanthus.name
linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
linux-stable-mirror@lists.linaro.org
-
Björn Töpel -
Celeste Liu -
Charlie Jenkins -
kernel test robot