Re: [PATCH bpf v2] bpf: Fix off-by-one boundary validation in arena direct-value access

On Sun, Apr 26, 2026 at 12:16 PM Emil Tsalapatis <[email protected]> wrote:
>
> On Sun Apr 26, 2026 at 2:17 PM EDT, sashiko-bot wrote:
> > Sashiko AI review found 1 potential issue(s):
> > - [Medium] Data race and bounds check bypass in arena_map_direct_value_addr() due to lockless reads
> > --
> >
> > commit a9b5d3016475abf10be02a320dc627c95b306d92
> > Author: Junyoung Jang <[email protected]>
> >
> > bpf: Fix off-by-one boundary validation in arena direct-value access
> >
> > This commit fixes an off-by-one error in the arena map boundary check
> > where offsets exactly at the end of the mapping were incorrectly accepted.
> >
> >> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> >> index 802656c6fd3c..49a8f7b1beef 100644
> >> --- a/kernel/bpf/arena.c
> >> +++ b/kernel/bpf/arena.c
> >> @@ -511,7 +511,7 @@ static int arena_map_direct_value_addr(const struct bpf_map *map, u64 *imm, u32
> >>  {
> >>      struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> >>
> >> -    if ((u64)off > arena->user_vm_end - arena->user_vm_start)
> >> +    if ((u64)off >= arena->user_vm_end - arena->user_vm_start)
> >
> > Could this evaluation be vulnerable to a data race?
> >
> > This function reads arena->user_vm_end and arena->user_vm_start without
> > holding arena->lock or using READ_ONCE().
> >
> > Concurrently, a user-space process calling mmap() on the arena map FD
> > can invoke arena_map_mmap(), which updates these fields under arena->lock.
> > Because user_vm_start is updated first, a concurrent reader here might
> > observe the new user_vm_start (a high address) and the old user_vm_end
> > (which might be 0).
> >
> > In that scenario, arena->user_vm_end - arena->user_vm_start would
> > underflow to a large 64-bit value, which would allow arbitrary offsets
> > to bypass this bounds check.
>
> This seems like a valid concern, though it is unrelated to the fix here
> which seems correct. Can you sent a followup patch using an arena lock
> guard in map_direct_value_addr? I don't see any issues with taking the
> lock since the method is only used at verification time.

No. sashk is wrong here.
In general I suggest to ignore sashk's medium and low suggestions.
'critical' are often correct.
'high' should be considered, but acting on them needs thought.