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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] NULL pointer dereference in `process_map_ptr_arg` when a BPF function accepts multiple map arguments.
- [Low] Incorrect and misleading register numbers in `process_map_ptr_arg` verifier error message.
--

--- Patch [2]: [PATCH bpf-next v1 02/18] bpf: Unify const map ptr argument checking for helpers and kfuncs ---
commit e4d2232623c3b71f4ec65054cb7cebf7737a4120
Author: Amery Hung <ameryhung@gmail.com>

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

This commit factors out the map pointer argument checking logic for both
helpers and kfuncs into a single process_map_ptr_arg() function. It uses
the bound map's btf_record to derive the object name (timer, workqueue,
or bpf_task_work) for diagnostic messages.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e8c76abbf76a2..bff51aff25579 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -8274,6 +8274,50 @@ static int get_constant_map_key(struct bpf_verifier_env *env,
>  
>  static bool can_elide_value_nullness(const struct bpf_map *map);
>  
> +/* Record the bpf_map argument of a helper or kfunc call in meta->map.
> + *
> + * When meta->map is already set from an earlier argument (a timer, workqueue or
> + * task_work object that is bound to a map), reject a map pointer that does not
> + * match it. That object sits in the register right before the map argument, and
> + * its type is taken from the bound map's record for the diagnostic.
> + */
> +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)
> +{
> +	int map_regno = reg_from_argno(argno);
[ ... ]
> +	if (meta->map.ptr &&
> +	    (meta->map.ptr != reg->map_ptr || meta->map.uid != reg->map_uid)) {
> +		struct btf_record *rec = meta->map.ptr->record;
> +		const char *obj_name = "workqueue";
> +
> +		if (rec->timer_off >= 0)

[Severity: Medium]
Will this cause a NULL pointer dereference in the verifier if a kfunc
accepts multiple map pointers?

If a custom kfunc or future helper takes two map pointers, such as 
void *a__map and void *b__map, process_map_ptr_arg() will process the
second map and see meta->map.ptr already set by the first.

If the first map was a standard map without special fields, its record
pointer is NULL, causing rec->timer_off to crash.

Should we check if rec is NULL before accessing its fields?

> +			obj_name = "timer";
> +		else if (rec->task_work_off >= 0)
> +			obj_name = "bpf_task_work";
> +
> +		verbose(env, "%s pointer in R%d map_uid=%d doesn't match map pointer in R%d map_uid=%d\n",
> +			obj_name, map_regno - 1, meta->map.uid, map_regno, reg->map_uid);

[Severity: Low]
Can the hardcoded map_regno - 1 produce confusing or negative register
numbers in the verifier error message?

If the map argument is passed on the stack (e.g. more than 5 arguments),
reg_from_argno() returns -1, making the printed registers -2 and -1.

Additionally, if a future kfunc separates the object and map arguments,
map_regno - 1 will point to the wrong register.

Could we retrieve the exact register or stack slot number for the
bound object dynamically instead of assuming strict adjacency?

> +		return -EINVAL;
> +	}
> +
> +	meta->map.ptr = reg->map_ptr;
> +	meta->map.uid = reg->map_uid;
> +	return 0;
> +}

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