Re: [PATCH bpf-next v4 02/16] bpf: Add source and instruction diagnostic context

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf_disasm_kfunc_name() will trigger a NULL pointer dereference or mutate verifier state if diagnostic formatting is invoked before the add_kfuncs() pass completes.
--

--- Patch [2]: [PATCH bpf-next v4 02/16] bpf: Add source and instruction diagnostic context ---
commit ef91626322111071ba03bd2ef6640700c748dd5f
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Add source and instruction diagnostic context

Teach verifier diagnostics to annotate an instruction with BTF source
line information and nearby BPF instructions. The renderer keeps source
text in a fixed-width lane and prints instructions in a stable right-hand
gutter.

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c

[ ... ]

> @@ -35,6 +245,182 @@ static void diag_write(struct bpf_verifier_env *env, const char *fmt, ...)

[ ... ]

> +static const char *disasm_kfunc_name(void *private_data, const struct bpf_insn *insn)
> +{
> +	struct disasm_ctx *ctx = private_data;
> +
> +	return bpf_disasm_kfunc_name(ctx->env, insn);
> +}
> +
> +static void format_disasm_line(struct bpf_verifier_env *env, int insn_idx,
> +			       struct disasm_line *line)
> +{
> +	struct disasm_ctx ctx = { .env = env };
> +	struct bpf_insn *insn;
> +	const struct bpf_insn_cbs cbs = {
> +		.cb_call = disasm_kfunc_name,
> +		.cb_print = disasm_print,
> +		.private_data = &ctx,
> +	};

[Severity: High]
Can this sequence trigger a NULL pointer dereference or unsafe state
mutation?

If bpf_diag_source() handles an error during early structural validation
(before add_kfuncs() runs), it calls format_disasm_line() to print nearby
context instructions. 

If any instruction in this window is a module kfunc call, it invokes the
bpf_disasm_kfunc_name() callback shown above, which eventually cascades into
__find_kfunc_desc_btf() in kernel/bpf/verifier.c.

__find_kfunc_desc_btf() then executes:

tab = env->prog->aux->kfunc_btf_tab;
b = bsearch(&kf_btf, tab->descs, tab->nr_descs, ...);

If the kfunc BTF table hasn't been allocated yet, wouldn't dereferencing
tab cause a panic? 

Additionally, if the table is only partially initialized, wouldn't
__find_kfunc_desc_btf() mutate the table by appending and sorting new
entries during what is expected to be a read-only logging operation?

> +
> +	line->idx = insn_idx;
> +	line->valid = false;
> +	seq_buf_init(&ctx.seq, line->text, sizeof(line->text));

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