netdev - Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAADnVQLhfN7f6AFxa_19E0g2_YADEkrfPPffi43HeH9VCi8MqQ@mail.gmail.com>
Date: Sun, 24 Mar 2024 11:40:20 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Pu Lehui <pulehui@...weicloud.com>
Cc: bpf <bpf@...r.kernel.org>, linux-riscv <linux-riscv@...ts.infradead.org>, 
	Network Development <netdev@...r.kernel.org>, Björn Töpel <bjorn@...nel.org>, 
	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 <yhs@...com>, 
	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>, 
	Palmer Dabbelt <palmer@...belt.com>, Luke Nelson <luke.r.nels@...il.com>, 
	Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility
 between bpf and riscv abi

On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@...weicloud.com> wrote:
>
> From: Pu Lehui <pulehui@...wei.com>
>
> We encountered a failing case when running selftest in no_alu32 mode:
>
> The failure case is `kfunc_call/kfunc_call_test4` and its source code is
> like bellow:
> ```
> long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> int kfunc_call_test4(struct __sk_buff *skb)
> {
>         ...
>         tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
>         ...
> }
> ```
>
> And its corresponding asm code is:
> ```
> 0: r1 = -3
> 1: r2 = -30
> 2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00
> 4: r4 = -1000
> 5: call bpf_kfunc_call_test4
> ```
>
> insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and
> converted to int type and then send to bpf_kfunc_call_test4. But since
> it is zero-extended in the bpf calling convention, riscv jit will
> directly treat it as an unsigned 32-bit int value, and then fails with
> the message "actual 4294966063 != expected -1234".
>
> The reason is the incompatibility between bpf and riscv abi, that is,
> bpf will do zero-extension on uint, but riscv64 requires sign-extension
> on int or uint. We can solve this problem by sign extending the 32-bit
> parameters in kfunc.
>
> The issue is related to [0], and thanks to Yonghong and Alexei.
>
> Link: https://github.com/llvm/llvm-project/pull/84874 [0]
> Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64")
> Signed-off-by: Pu Lehui <pulehui@...wei.com>
> ---
>  arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 869e4282a2c4..e3fc39370f7d 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>                 if (ret < 0)
>                         return ret;
>
> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +                       const struct btf_func_model *fm;
> +                       int idx;
> +
> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
> +                       if (!fm)
> +                               return -EINVAL;
> +
> +                       for (idx = 0; idx < fm->nr_args; idx++) {
> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
> +
> +                               if (fm->arg_size[idx] == sizeof(int))
> +                                       emit_sextw(reg, reg, ctx);
> +                       }
> +               }
> +

The btf_func_model usage looks good.
Glad that no new flags were necessary, since both int and uint
need to be sign extend the existing arg_size was enough.

Since we're at it. Do we need to do zero extension of return value ?
There is
__bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
but the selftest with it is too simple:
        return bpf_kfunc_call_test2((struct sock *)sk, 1, 2);

Could you extend this selftest with a return of large int/uint
with 31th bit set to force sign extension in native
kernel risc-v code ?
I suspect the bpf side will be confused.
Which would mean that risc-v JIT in addition to:
        if (insn->src_reg != BPF_PSEUDO_CALL)
            emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);

need to conditionally do:
 if (fm->ret_size == sizeof(int))
   emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
              bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
?