Re: [PATCH bpf] bpf: Reject writable map iterators on read-only and frozen maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: missing freeze synchronization in
  bpf_iter_map_check_value_write()
--

commit 1e3fa097f2efa9dcea1ba23069784fa975915ada
Author: Adethya Srinivasan <adethyasrinivasan@outlook.com>

bpf: Reject writable map iterators on read-only and frozen maps

This commit updates bpf_iter_map_check_value_write() to return -EACCES
when a writable map iterator program is attached to a map with
BPF_F_RDONLY_PROG set or with map->frozen set.

> diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> index a58c148692e6c..d74e1152c7f81 100644
> --- a/kernel/bpf/map_iter.c
> +++ b/kernel/bpf/map_iter.c
> @@ -99,8 +99,8 @@ static struct bpf_iter_reg bpf_map_reg_info = {
>  
>  /*
>   * For an iterator that hands the live map value to the program as a writable
> - * buffer: the verifier knows nothing of the map, so the special fields of the
> - * value have to lie outside of what the program may write.
> + * buffer: the verifier knows nothing of the map, so read-only / frozen maps
> + * and special fields of the value have to be protected from writes.
>   */
>  int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
>  				   const struct bpf_map *map)
> @@ -109,7 +109,13 @@ int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
>  	const struct btf_field *field;
>  	u32 i;
>  
> -	if (IS_ERR_OR_NULL(rec) || !prog->aux->max_rdwr_write)
> +	if (!prog->aux->max_rdwr_write)
> +		return 0;
> +
> +	if ((map->map_flags & BPF_F_RDONLY_PROG) || READ_ONCE(map->frozen))
> +		return -EACCES;

[Severity: Medium]
Can a writable iterator link attached before (or concurrently with)
map_freeze() still overwrite elements of a frozen map?

bpf_iter_map_check_value_write() is only called at link attach
(bpf_iter_attach_map()) and link replacement (bpf_iter_link_replace()).
Unlike bpf_map_mmap(), neither path takes map->freeze_mutex or increments
map->writecnt via bpf_map_write_active_inc(), and neither
prepare_seq_file() nor bpf_seq_read() re-checks map->frozen when an
iterator is opened or read.  For a map created without BPF_F_RDONLY_PROG,
attaching a writable iterator link and then calling BPF_MAP_FREEZE allows
map_freeze() to succeed while subsequent BPF_ITER_CREATE and read() calls
on that link still mutate the frozen map in place.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/LO2P302MB0197FAF0305A94F7575CA700BC752@LO2P302MB0197.GBRP302.PROD.OUTLOOK.COM?part=1