Re: [PATCH bpf v2 1/1] bpf: reject raw iteration over managed map fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: ERR_PTR dereference on map->record in map iterator prog checks
--

commit 5c7120e7a68cabfb0dcb935074cfe787d4f05fb6
Author: Zhiling Zou <zhilinz@nebusec.ai>

bpf: reject raw iteration over managed map fields

This commit restricts BPF map iterators (bpf_map_elem and bpf_sk_storage_map)
to accessing only the prefix before the first BTF-managed field in a map
value, and revalidates replacement programs during bpf_link_update().

Link: https://lore.kernel.org/all/1fa798d444efac579e91ee0716e228902a8f9afe.1791980149.git.zhilinz@nebusec.ai/

> diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> index c19b360bad9ea..04cf255587aa8 100644
> --- a/kernel/bpf/map_iter.c
> +++ b/kernel/bpf/map_iter.c
> @@ -135,10 +125,39 @@ static int bpf_iter_attach_map(struct bpf_prog *prog,
>  	else
>  		value_size = round_up(map->value_size, 8) * num_possible_cpus();
>  
> -	if (key_acc_size > key_size || value_acc_size > value_size) {
> -		err = -EACCES;
> +	if (key_acc_size > key_size || value_acc_size > value_size)
> +		return -EACCES;
> +
> +	/* Fields are sorted by offset; max_rdwr_access is an exclusive end. */
> +	if (map->record && value_acc_size > map->record->fields[0].offset)
> +		return -EACCES;

[Severity: Critical]
Can this dereference an ERR_PTR in bpf_iter_map_elem_check_prog() when
attaching or updating a map iterator on a BTF-enabled map whose value type
is not a struct or datasec (for example, an int or __u32)?

During BPF_MAP_CREATE, map_check_btf() in kernel/bpf/syscall.c sets
map->record = btf_parse_fields(...):

	map->record = btf_parse_fields(btf, value_type,
				       BPF_SPIN_LOCK | BPF_RES_SPIN_LOCK | BPF_TIMER | BPF_KPTR | BPF_LIST_HEAD |
				       BPF_RB_ROOT | BPF_REFCOUNT | BPF_WORKQUEUE | BPF_UPTR |
				       BPF_TASK_WORK,
				       map->value_size);
	if (!IS_ERR_OR_NULL(map->record)) {

When value_type is a scalar (or any non-struct/non-datasec type),
btf_parse_fields() -> btf_find_field() returns ERR_PTR(-EINVAL) (and can
also return ERR_PTR(-E2BIG) when a struct exceeds BTF_FIELDS_MAX). Because
map_check_btf() and the per-map-type check callbacks guard special-field
handling with !IS_ERR_OR_NULL(map->record), map creation succeeds while
map->record holds a non-NULL error pointer.

When BPF_LINK_CREATE or BPF_LINK_UPDATE later calls
bpf_iter_map_elem_check_prog(), checking only if (map->record) evaluates to
true and dereferences map->record->fields[0].offset.

Should this check !IS_ERR_OR_NULL(map->record) instead, matching
check_map_access() and other map->record users?

[ ... ]

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 1d295a8769fad..ffbcfab9a71c3 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -846,12 +846,35 @@ static void bpf_iter_fini_sk_storage_map(void *priv_data)
>  	bpf_map_put_with_uref(seq_info->map);
>  }
>  
> +static int bpf_iter_sk_storage_map_check_prog(struct bpf_prog *prog,
> +					      struct bpf_map *map)
> +{
> +	if (map->map_type != BPF_MAP_TYPE_SK_STORAGE)
> +		return -EINVAL;
> +
> +	if (prog->aux->max_rdwr_access > map->value_size)
> +		return -EACCES;
> +
> +	/* Fields are sorted by offset; max_rdwr_access is an exclusive end. */
> +	if (map->record &&
> +	    prog->aux->max_rdwr_access > map->record->fields[0].offset)
> +		return -EACCES;

[Severity: Critical]
Similarly, in bpf_iter_sk_storage_map_check_prog(), won't this crash when
map->record is an ERR_PTR (such as on sk_stg_map in the existing
test_bpf_sk_storage_map selftest, which uses __type(value, int))? Should
this also use !IS_ERR_OR_NULL(map->record)?

[ ... ]

完美结束。 (Wait - no extra text! Let me make sure there is no extra text at the end).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792217106.git.zhilinz@nebusec.ai?part=1