Re: [PATCH bpf-next v5] m68k, bpf: Add initial BPF JIT compiler support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardcoded jump offset in tail call implementation corrupts BPF frame pointer on ColdFire processors.
- [High] Silent failure during the dry run of an extra_pass leaves the program in an inconsistent state, deceiving the BPF core subsystem.
--

--- Patch [1]: [PATCH bpf-next v5] m68k, bpf: Add initial BPF JIT compiler support ---
commit 1c48a3daa4134e7c3b2c0ae3d4113bebafb9922b
Author: Kuan-Wei Chiu <[email protected]>

    m68k, bpf: Add initial BPF JIT compiler support

    Add a BPF JIT compiler for the m68k architecture.
    
    The JIT generates m68k machine code targeting m68020+ processors and
    coldfire parts with an mmu (ISA_B/ISA_C). It excludes plain 68000/68010
    and non-mmu coldfire processors, as it relies on 32-bit branch
    displacements (b<cc>.l) to handle large bpf programs.

> diff --git a/arch/m68k/net/bpf_jit_comp.c b/arch/m68k/net/bpf_jit_comp.c
> new file mode 100644
> index 0000000000000..e3a5dca70a3f3
> --- /dev/null
> +++ b/arch/m68k/net/bpf_jit_comp.c

[ ... ]

> +static void emit_tail_call(const struct bpf_insn *insn, struct jit_ctx *ctx)
> +{
[ ... ]
> +	emit_16(ctx, 0x2068);					/* movea.l d16(%a0), %a0 */
> +	emit_16(ctx, offsetof(struct bpf_prog, bpf_func));
> +
> +	emit_16(ctx, 0x4ee8);					/* jmp d16(%a0) */
> +	emit_16(ctx, 44);

[Severity: High]
Can this jump offset corrupt the BPF frame pointer on ColdFire CPUs?

The hardcoded offset assumes a 44-byte prologue. While standard m68k
uses a 44-byte prologue, looking at build_prologue(), ColdFire CPUs use a
48-byte prologue because the 4-byte movem.l sequence is replaced by an
8-byte sequence:

        if (CPU_IS_COLDFIRE) {
                emit_16(ctx, 0x4fef);                           /* lea -16(%sp), %sp */
                emit_16(ctx, 0xfff0);
                emit_16(ctx, 0x48d7);                           /* movem.l d2-d5, (%sp) */
                emit_16(ctx, 0x003c);
        } else {

Jumping to offset 44 on ColdFire jumps into the middle of the last
prologue instruction, skipping the initialization of the lower 32 bits
and zeroing of %d0. This stores uninitialized data into the upper 32
bits of the BPF frame pointer (BPF_REG_10), which can cause out-of-bounds
memory accesses and kernel panics on stack access.

> +
> +	if (ctx->target) {
> +		ctx->target[jmp_out_1] = (ctx->idx - jmp_out_1) * 2;
> +		ctx->target[jmp_out_2] = (ctx->idx - jmp_out_2) * 2;
> +		ctx->target[jmp_out_3] = (ctx->idx - jmp_out_3) * 2;
> +	}
> +}

[ ... ]

> +struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_prog *prog)
> +{
[ ... ]
> +	build_prologue(&ctx);
> +	if (build_body(&ctx) < 0)
> +		goto out_off;

[Severity: High]
Does this error path leave the program in an inconsistent state if it
fails during an extra pass?

During an extra_pass (used for resolving subprogram addresses),
prog->jited is already set to 1 from the previous pass. The error
path jumps directly to out_off:

> +	build_epilogue(&ctx);
[ ... ]
> +out_off:
> +	kvfree(ctx.offsets);
> +	return prog;
> +}

This return does not clear prog->jited or prog->bpf_func. The BPF core
checks if (!prog->jited) and incorrectly assumes the compilation succeeded.

During the first pass, bpf_jit_get_func_addr() sets func_addr = NULL
without returning an error, successfully emitting an image with calls
to NULL. If the extra_pass fails (e.g. invalid subprogram offset), the
core will not abort and will execute the BPF program, calling a NULL
pointer and leading to a kernel panic.

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