Re: [PATCH] bpf: Fix signedness bug

[Amery Hung]

On Fri, Jun 5, 2026 at 8:25 PM <bot+bpf-ci@xxxxxxxxxx> wrote:
>
> > commit cc27f12c12ff54b8eace339c3e130317bf205277
> > Author:     Ethan Tidmore <ethantidmore06@xxxxxxxxx>
> >
> >     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;

Hi,

As pointed out by the bot, the fix to the issue has been merged as
part of the following patchset.
https://lore.kernel.org/bpf/20260605202056.1780352-1-ameryhung@xxxxxxxxx/

Thanks,
Amery

>
> 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@xxxxxxxxx>
> >
> > 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