Re: [PATCH] bpf: Fix signedness bug

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

> commit cc27f12c12ff54b8eace339c3e130317bf205277
> Author:     Ethan Tidmore <ethantidmore06@gmail.com>
>
>     bpf: Fix signedness bug
>
>     The function acquire_reference() returns negative error codes and 'id'
>     is an unsigned integer, so the check (id < 0) is always impossible.

Is this commit message accurate? Looking at check_kfunc_call() in
kernel/bpf/verifier.c, the variable 'id' is declared as a signed int:

    const struct btf_param *args;
    u32 i, nargs, ptr_type_id;
    struct btf *desc_btf;
    int id;

Since acquire_reference() can return negative error codes like -ENOMEM,
wouldn't the original check (id < 0) work correctly?

The three other call sites of acquire_reference() in the verifier all use
the same signed-int pattern without issue:

    verifier.c:mark_stack_slots_dynptr():
        id = acquire_reference(env, insn_idx, dynptr_id);
        if (id < 0)
            return id;

    verifier.c:mark_stack_slots_iter():
        id = acquire_reference(env, insn_idx, ITER_TYPE_TO_ID(iter_type));
        if (id < 0)
            return id;

    verifier.c:check_helper_call():
        id = acquire_reference(env, insn_idx, fn->arg_type[arg_num]);
        if (id < 0)
            return id;

It looks like the Smatch warning might be a false positive, and the original
code was already handling the error properly.

>
>     Detected by Smatch:
>     kernel/bpf/verifier.c:13115 check_kfunc_call()
>     warn: unsigned 'id' is never less than zero.
>
>     Fixes: 308c7a0ae8859 ("bpf: Refactor object relationship tracking and fix dynptr UAF bug")
>     Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 935595138aa0..be528a0be13c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13167,9 +13167,11 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		}
>  		mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
>  		if (is_kfunc_acquire(&meta)) {
> -			id = acquire_reference(env, insn_idx, 0);
> -			if (id < 0)
> -				return id;
> +			err = acquire_reference(env, insn_idx, 0);
> +			if (err < 0)
> +				return err;
> +			id = err;
> +
>  			regs[BPF_REG_0].id = id;
>  		} else if (is_rbtree_node_type(ptr_type) || is_list_node_type(ptr_type)) {
>  			ref_set_non_owning(env, &regs[BPF_REG_0]);

The code change itself is behavior-equivalent since 'err' is also a signed
int, so the patch doesn't introduce a runtime regression. But if the premise
that 'id' is unsigned is wrong, should the commit message be corrected?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27050796130