Re: [PATCH bpf-next v1 11/14] bpf: Replace arena kfunc argument flags with suffixes

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing address translation in non-sleepable arena kfuncs leads to arena memory corruption.
--

--- Patch [11]: [PATCH bpf-next v1 11/14] bpf: Replace arena kfunc argument flags with suffixes ---
commit a7446937b514c39a83585718956aa0b543092b07
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Replace arena kfunc argument flags with suffixes

This commit replaces the KF_ARENA_ARG1 and KF_ARENA_ARG2 flags with
__arena and __arena__nullable BTF suffixes for arena kfuncs. It also
updates the main kfuncs to translate JIT-provided kernel addresses back to
user addresses using arena_kaddr_to_uaddr().

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b431..6c34a0d34b3f9 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
[ ... ]
> @@ -1044,18 +1044,28 @@ static void arena_free_irq(struct irq_work *iw)
>  	schedule_work(&arena->free_work);
>  }
>  
> +static long arena_kaddr_to_uaddr(struct bpf_arena *arena, const void *addr)
> +{
> +	if (!addr)
> +		return 0;
> +
> +	return (long)addr - bpf_arena_get_kern_vm_start(arena);
> +}
> +
>  __bpf_kfunc_start_defs();
>  
> -__bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__ign, u32 page_cnt,
> -					int node_id, u64 flags)
> +__bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__arena__nullable,
> +					u32 page_cnt, int node_id, u64 flags)
>  {
>  	struct bpf_map *map = p__map;
>  	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	long addr;
>  
>  	if (map->map_type != BPF_MAP_TYPE_ARENA || flags || !page_cnt)
>  		return NULL;
>  
> -	return (void *)arena_alloc_pages(arena, (long)addr__ign, page_cnt, node_id, true);
> +	addr = arena_kaddr_to_uaddr(arena, addr__arena__nullable);
> +	return (void *)arena_alloc_pages(arena, addr, page_cnt, node_id, true);
>  }
>  
>  void *bpf_arena_alloc_pages_non_sleepable(void *p__map, void *addr__ign, u32 page_cnt,

[Severity: High]
Does this code miss address translation when the verifier dynamically replaces
the kfunc with its non-sleepable or sleepable variants?

When a BPF program calls bpf_arena_alloc_pages() from within a non-sleepable
context, the verifier replaces the call with bpf_arena_alloc_pages_non_sleepable().

Since the BPF JIT adds kern_vm_start to the pointer argument based on the
__arena__nullable annotation on the main kfunc, won't bpf_arena_alloc_pages_non_sleepable()
receive a kernel address in addr__ign?

Because bpf_arena_alloc_pages_non_sleepable() just casts addr__ign without calling
arena_kaddr_to_uaddr(), it appears this will pass a kernel address directly to
arena_alloc_pages(), potentially causing arena memory corruption or silent
allocation failures.

[ ... ]
> @@ -1082,14 +1092,20 @@ void *bpf_arena_alloc_pages_sleepable(void *p__map, void *addr__ign, u32 page_cn
>  	return (void *)arena_alloc_pages(arena, (long)addr__ign, page_cnt, node_id, true);
>  }
>  
> -__bpf_kfunc void bpf_arena_free_pages(void *p__map, void *ptr__ign, u32 page_cnt)
> +/*
> + * A valid arena address can have zero low 32 bits, so ptr must be rebased
> + * unconditionally instead of being treated as nullable.
> + */
> +__bpf_kfunc void bpf_arena_free_pages(void *p__map, void *ptr__arena, u32 page_cnt)
>  {
>  	struct bpf_map *map = p__map;
>  	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	long ptr;
>  
> -	if (map->map_type != BPF_MAP_TYPE_ARENA || !page_cnt || !ptr__ign)
> +	if (map->map_type != BPF_MAP_TYPE_ARENA || !page_cnt)
>  		return;
> -	arena_free_pages(arena, (long)ptr__ign, page_cnt, true);
> +	ptr = arena_kaddr_to_uaddr(arena, ptr__arena);
> +	arena_free_pages(arena, ptr, page_cnt, true);
>  }
>  
>  void bpf_arena_free_pages_non_sleepable(void *p__map, void *ptr__ign, u32 page_cnt)

[Severity: High]
Does this have the same issue with the non-sleepable variant of free?

When bpf_arena_free_pages_non_sleepable() is substituted by the verifier, it will
receive a JIT-rebased kernel address but will pass ptr__ign directly to
arena_free_pages() without address translation.

Will this cause the wrong pages inside the arena to be freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11