Re: [PATCH bpf-next v2 02/18] bpf: Unify const map ptr argument checking for helpers and kfuncs

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Fri Jul 24, 2026 at 10:49 PM CEST, Amery Hung wrote:
> On Fri, Jul 24, 2026 at 12:28 PM <sashiko-bot@kernel.org> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>
>> New issues:
>> - [Low] Multi-line comment does not follow the required BPF subsystem comment style.
>>
>> Pre-existing issues:
>> - [Critical] Pre-existing verifier panic when an array map is passed to kfuncs like `bpf_wq_init`.
>> - [High] Pre-existing logic bug allows mismatching maps to bypass safety checks in kfuncs.
>> --
>>
>> --- Patch [2]: [PATCH bpf-next v2 02/18] bpf: Unify const map ptr argument checking for helpers and kfuncs ---
>> commit 08bfe87a36be7126ec3db2d3ab4783e72b62e453
>> Author: Amery Hung <ameryhung@gmail.com>
>>
>> bpf: Unify const map ptr argument checking for helpers and kfuncs
>>
>> Both the helper ARG_CONST_MAP_PTR and the kfunc KF_ARG_PTR_TO_MAP
>> recorded the map pointer in meta->map and, when a map was already
>> bound by a preceding timer/workqueue/task_work argument, rejected a
>> mismatching map. This logic is factored into a single process_map_ptr_arg()
>> used by both paths.
>>
>> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> > index e8c76abbf76a2..a3429610b426e 100644
>> > --- a/kernel/bpf/verifier.c
>> > +++ b/kernel/bpf/verifier.c
>> > @@ -8274,6 +8274,44 @@ static int get_constant_map_key(struct bpf_verifier_env *env,
>> >
>> >  static bool can_elide_value_nullness(const struct bpf_map *map);
>> >
>> > +static int process_map_ptr_arg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>> > +                            argno_t argno, struct bpf_call_arg_meta *meta)
>> > +{
>> > +     /* Use map_uid (which is unique id of inner map) to reject:
>>
>> [Severity: Low]
>> This isn't a bug, but does this multi-line comment follow the preferred BPF
>> subsystem formatting?
>>
>> According to the subsystem guidelines, multi-line comments should have the
>> opening /* on its own line:
>>
>> /*
>>  * Use map_uid (which is unique id of inner map) to reject:
>>  * ...
>>  */
>>
>> [ ... ]
>>
>> > @@ -12122,36 +12140,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
>> >                                       reg_arg_name(env, argno));
>> >                               return -EINVAL;
>> >                       }
>> > -                     if (meta->map.ptr && (reg->map_ptr->record->wq_off >= 0 ||
>> > -                                           reg->map_ptr->record->task_work_off >= 0)) {
>>
>> [Severity: Critical]
>> This is a pre-existing issue that was not introduced by this patch (in fact,
>> the patch appears to fix it!), but could the old code crash the verifier here
>> if a BPF program called bpf_wq_init(wq, map, 0) and passed an array map
>> without BTF for the map argument?
>>
>> Since an array map without BTF has a NULL record, evaluating
>> reg->map_ptr->record->wq_off would result in an unconditional NULL pointer
>> dereference, leading to a panic. This could be triggered by any user with
>> privileges to load BPF programs.
>
> [...]
>
>>
>> [Severity: High]
>> This is also a pre-existing issue that was not introduced by this patch, but
>> did this original condition allow mismatching maps to bypass safety checks?
>>
>> If a BPF program called bpf_wq_init(wq, map, 0) with a valid workqueue but
>> passed a mismatched map (like a map containing only a bpf_timer), the
>> mismatched map would lack both wq_off and task_work_off. This would make the
>> condition evaluate to false, bypassing the mismatch rejection and potentially
>> leading to type confusion or memory corruption during runtime callbacks.
>>
>> > -                             /* Use map_uid (which is unique id of inner map) to reject:
>> > -                              * inner_map1 = bpf_map_lookup_elem(outer_map, key1)
>
> Yes and that is why this patch use meta->map.ptr->record that is
> already verified. I can also separate it into a standalone fix
> targeting bpf if needed.
>

I think keeping it in your set is fine. Since this is not reported I doubt
anyone hit this in practice, though I agree it's worth fixing.

>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260724190813.1458271-1-ameryhung@gmail.com?part=2