On Fri, 2022-12-02 at 11:36 +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@xxxxxxxxxxxx>
>
> A BPF call instruction can be, correctly, marked with zext_dst set to
> true. An example of this can be found in the BPF selftests
> progs/bpf_cubic.c:
>
> ...
> extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym;
>
> __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
> {
> return tcp_reno_undo_cwnd(sk);
> }
> ...
>
> which compiles to:
> 0: r1 = *(u64 *)(r1 + 0x0)
> 1: call -0x1
> 2: exit
>
> The call will be marked as zext_dst set to true, and for some
> backends
> (bpf_jit_needs_zext() returns true) expanded to:
> 0: r1 = *(u64 *)(r1 + 0x0)
> 1: call -0x1
> 2: w0 = w0
> 3: exit
In the verifier, the marking is done by check_kfunc_call() (added in
e6ac2450d6de), right? So the problem occurs only for kfuncs?
/* Check return type */
t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
...
if (btf_type_is_scalar(t)) {
mark_reg_unknown(env, regs, BPF_REG_0);
mark_btf_func_reg_size(env, BPF_REG_0, t->size);
I tried to find some official information whether the eBPF calling
convention requires sign- or zero- extending return values and
arguments, but unfortunately [1] doesn't mention this.
LLVM's lib/Target/BPF/BPFCallingConv.td mentions both R* and W*
registers, but since assigning to W* leads to zero-extension, it seems
to me that this is the case.
If the above is correct, then shouldn't we rather use sizeof(void *) in
the mark_btf_func_reg_size() call above?
> The opt_subreg_zext_lo32_rnd_hi32() function which is responsible for
> the zext patching, relies on insn_def_regno() to fetch the register
> to
> zero-extend. However, this function does not handle call instructions
> correctly, and opt_subreg_zext_lo32_rnd_hi32() fails the
> verification.
>
> Make sure that R0 is correctly resolved for (BPF_JMP | BPF_CALL)
> instructions.
>
> Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in
> insn_has_def32()")
> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
> ---
> I'm not super happy about the additional special case -- first
> cmpxchg, and now call. :-( A more elegant/generic solution is
> welcome!
> ---
> kernel/bpf/verifier.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 264b3dc714cc..4f9660eafc72 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13386,6 +13386,9 @@ static int
> opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn))
> continue;
>
> + if (insn.code == (BPF_JMP | BPF_CALL))
> + load_reg = BPF_REG_0;
> +
> if (WARN_ON(load_reg == -1)) {
> verbose(env, "verifier bug. zext_dst is set,
> but no reg is defined\n");
> return -EFAULT;
>
> base-commit: 01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1
[1]
https://docs.kernel.org/bpf/instruction-set.html#registers-and-calling-convention
