netdev - Re: [PATCH bpf-next 1/2] libbpf: Fix accessing first syscall argument on RV64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzbvUQ1HA6GPSF23piqbEBNVDZKZC0rB-X4LgeMpp9svYA@mail.gmail.com>
Date: Fri, 30 Aug 2024 12:34:13 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Pu Lehui <pulehui@...weicloud.com>
Cc: bpf@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	netdev@...r.kernel.org, Björn Töpel <bjorn@...nel.org>, 
	Ilya Leoshkevich <iii@...ux.ibm.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Puranjay Mohan <puranjay@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf-next 1/2] libbpf: Fix accessing first syscall argument
 on RV64

On Thu, Aug 29, 2024 at 6:32 AM Pu Lehui <pulehui@...weicloud.com> wrote:
>
> From: Pu Lehui <pulehui@...wei.com>
>
> On RV64, as Ilya mentioned before [0], the first syscall parameter should be
> accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
> otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
> test_lsm, etc. to fail on RV64. Let's fix it by using the struct pt_regs
> style to provide access to it only through PT_REGS_PARM1_CORE_SYSCALL().
>
> Link: https://lore.kernel.org/bpf/20220209021745.2215452-1-iii@linux.ibm.com [0]
> Signed-off-by: Pu Lehui <pulehui@...wei.com>
> ---
>  tools/lib/bpf/bpf_tracing.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 9314fa95f04e..388f30cf7914 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -351,6 +351,10 @@ struct pt_regs___arm64 {
>   * https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#risc-v-calling-conventions
>   */
>
> +struct pt_regs___riscv {
> +       unsigned long orig_a0;
> +};
> +
>  /* riscv provides struct user_regs_struct instead of struct pt_regs to userspace */
>  #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
>  #define __PT_PARM1_REG a0
> @@ -362,12 +366,15 @@ struct pt_regs___arm64 {
>  #define __PT_PARM7_REG a6
>  #define __PT_PARM8_REG a7
>
> -#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG
> +#define __PT_PARM1_SYSCALL_REG orig_a0
>  #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG
>  #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG
>  #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
>  #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
>  #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
> +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
> +#define PT_REGS_PARM1_CORE_SYSCALL(x) \
> +       BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)

I feel like what we did for s390x is a bit suboptimal, so let's (try
to) improve that and then do the same for RV64.

What I mean is that PT_REGS_PARMn_SYSCALL macros are used to read
pt_regs coming directly from context, right? In that case we don't
need to pay the price of BPF_CORE_READ(), we can just access memory
directly (but we still need CO-RE relocation!).

So I think what we should do is

1) mark pt_regs___riscv {} with __attribute__((preserve_access_index))
so that normal field accesses are CO-RE-relocated
2) change PT_REGS_PARM1_SYSCALL(x) to be `((const
struct_pt_regs___riscv *)(x))->orig_a0`, which will directly read
memory
3) keep PT_REGS_PARM1_CORE_SYSCALL() as is


But having written all the above, I'm not sure whether we allow CO-RE
relocated direct context accesses (verifier might complain about
modifying ctx register offset or something). So can you please check
it either on s390 or RV64 and let me know? I'm not marking it as
"Changes Requested" for that reason, because that might not work and
we'll have to do BPF_CORE_READ().

>
>  #define __PT_RET_REG ra
>  #define __PT_FP_REG s0
> --
> 2.34.1
>