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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Latent NULL Pointer Dereference in process_map_ptr_arg
- [Low] Incorrect error reporting for non-adjacent object and map arguments
- [Low] Misleading error message when a map contains both a timer and a workqueue
--

--- Patch [2]: [PATCH bpf-next v3 02/18] bpf: Unify const map ptr argument checking for helpers and kfuncs ---
commit 28f78cdb3fca90eedfb38c316b9b1e6ce726d4a2
Author: Amery Hung <[email protected]>

bpf: Unify const map ptr argument checking for helpers and kfuncs

This patch refactors the verifier to use a single process_map_ptr_arg()
function for validating constant map pointer arguments in both helpers and
kfuncs. The new logic attempts to dynamically derive the bound object name
from the map's BTF record and computes the register numbers relative to
the map argument position.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 616194f25dfb7..4a5b542192106 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:
> +	 * inner_map1 = bpf_map_lookup_elem(outer_map, key1)
> +	 * inner_map2 = bpf_map_lookup_elem(outer_map, key2)
> +	 * if (inner_map1 && inner_map2) {
> +	 *     timer = bpf_map_lookup_elem(inner_map1);
> +	 *     if (timer)
> +	 *         // mismatch would have been allowed
> +	 *         bpf_timer_init(timer, inner_map2);
> +	 * }
> +	 *
> +	 * Comparing map_ptr is enough to distinguish normal and outer maps.
> +	 */
> +	if (meta->map.ptr &&
> +	    (meta->map.ptr != reg->map_ptr || meta->map.uid != reg->map_uid)) {
> +		argno_t obj_argno = argno_from_reg(reg_from_argno(argno) - 1);

[Severity: Low]
Does this incorrectly assume the bound object strictly precedes the map
argument in all function signatures?

If a custom module kfunc interleaves arguments (e.g., passing a flag between
the object and the map) or if the map is passed on the stack, this arithmetic
will yield the wrong register or a negative value.  For instance, if the map
is the first argument, it might calculate negative registers and print
garbage like fp-1992 in the verifier log.

> +		struct btf_record *rec = meta->map.ptr->record;
> +		const char *obj_name = "workqueue";
> +
> +		if (rec->timer_off >= 0)

[Severity: Medium]
Can this unconditionally dereference rec when it is NULL?

If the verifier processes a kfunc that takes multiple map pointers (e.g., in a
custom module), the first map passed is tracked in meta->map.ptr. If that first
map is a simple array or hash map without any special BTF fields, its record
will be NULL. When the verifier processes the second map argument and it
mismatches, it will hit this check and crash due to a NULL pointer dereference.

[Severity: Low]
Will this produce misleading error messages if a map contains both a timer and
a workqueue?

A BPF map value can validly contain both a bpf_timer and a bpf_wq, meaning its
btf_record will have both timer_off >= 0 and wq_off >= 0 true. 

If a user calls bpf_wq_init() but accidentally passes a mismatched map, this
conditional guesses the object type purely from the map's record and prioritizes
the timer field. It will incorrectly print an error about a timer pointer
mismatch, which would mislead the user who was explicitly working with a
workqueue.

> +			obj_name = "timer";
> +		else if (rec->task_work_off >= 0)
> +			obj_name = "bpf_task_work";
> +
> +		verbose(env, "%s pointer in %s map_uid=%d ",
> +			obj_name, reg_arg_name(env, obj_argno), meta->map.uid);
> +		verbose(env, "doesn't match map pointer in %s map_uid=%d\n",
> +			reg_arg_name(env, argno), reg->map_uid);
> +		return -EINVAL;
> +	}
> +
> +	meta->map.ptr = reg->map_ptr;
> +	meta->map.uid = reg->map_uid;
> +	return 0;
> +}

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