Re: [PATCH bpf-next v1 09/14] bpf, loongarch: Convert struct_ops arena arguments in trampolines

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `cast_user` JIT implementation on LoongArch fails to preserve NULL pointers due to an incorrect branch offset.
--

--- Patch [9]: [PATCH bpf-next v1 09/14] bpf, loongarch: Convert struct_ops arena arguments in trampolines ---
commit ef7bc691896c4955d9e0b94ff8e49c81e26f20ef
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf, loongarch: Convert struct_ops arena arguments in trampolines
    
This patch allows an arena BPF program on LoongArch to receive
native kernel addresses converted to 32-bit arena offsets for
struct_ops callbacks. It introduces emit_arena_arg_conv() to
handle the conversion while preserving native NULL pointers.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 33cabaa7353f9..5020414bfd9f2 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
[ ... ]
> @@ -1680,21 +1685,55 @@ int bpf_arch_text_invalidate(void *dst, size_t len)
>  	return ret;
>  }
>  
> -static void store_args(struct jit_ctx *ctx, int nr_arg_slots, int args_off, bool is_struct_ops)
> +/*
> + * Convert an arena kernel address into a 32-bit arena offset while copying it
> + * into the BPF ctx. A nullable argument preserves a native NULL.
> + */
> +static void emit_arena_arg_conv(struct jit_ctx *ctx, int dst, int src, bool nullable, int base)
> +{
> +	if (dst != src)
> +		move_reg(ctx, dst, src);
> +	if (nullable)
> +		emit_insn(ctx, beq, dst, LOONGARCH_GPR_ZERO, 2);

[Severity: High]
This is a pre-existing issue, but seeing the correct offset of 2 used here for
the beq instruction made me notice a potential bug in build_insn() when
handling cast_user:

arch/loongarch/net/bpf_jit.c:build_insn() {
...
    emit_insn(ctx, beq, t1, LOONGARCH_GPR_ZERO, 1);
    emit_insn(ctx, or, t1, dst, t1);
    move_reg(ctx, dst, t1);
...
}

Does this pre-existing offset of 1 cause the branch to jump to the immediately
next instruction (PC + 4), failing to skip the 'or' instruction?

If so, would a NULL user pointer casted via cast_user incorrectly receive the
value of user_vm_start instead of remaining NULL?

Could this lead to invalid pointers being passed to userspace, potentially
causing logic errors or data corruption if userspace believes the pointer is
valid and writes to the start of the arena?

> +	emit_insn(ctx, subd, dst, dst, base);
> +	emit_zext_32(ctx, dst, true);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9