On Fri, Sep 06, 2024 at 03:56:03PM GMT, Daniel Borkmann wrote:
> Lonial found an issue that despite user- and BPF-side frozen BPF map
> (like in case of .rodata), it was still possible to write into it from
> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> as arguments.
>
> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> is never set. Later, check_helper_mem_access(), under the case of
> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> subsequent call to check_map_access_type() and given the BPF map is
> read-only it succeeds.
>
> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> when results are written into them as opposed to read out of them. The
> latter indicates that it's okay to pass a pointer to uninitialized memory
> as the memory is written to anyway.
>
> However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
> just with additional alignment requirement. So it is better to just get
> rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
> fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
> alignment given these helpers write directly into the args via *<ptr> = val.
> The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).
>
> MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
> argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
> the buffer size a priori and therefore cannot blindly write *<ptr> = val.
>
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Lonial Con <kongln9170@xxxxxxxxx>
> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
Agree with Andrii's suggestion in silbing thread. That said, logic-wise LGTM
Acked-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
