Re: [PATCH v6 bpf-next] bpf: Populate mmap-able array map memory lazily

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Integer overflow in `array_map_mmap` bounds check allows mapping arbitrary kernel memory, leading to arbitrary kernel code execution.
--

--- Patch [1]: [PATCH v6 bpf-next] bpf: Populate mmap-able array map memory lazily ---
commit ef1e2903b40b525ff2c8448b624a44742a4bc1a4
Author: Song Liu <[email protected]>

bpf: Populate mmap-able array map memory lazily

This patch replaces the eager remap_vmalloc_range() in array_map_mmap()
with lazy, on-demand page faulting via array_map_mmap_fault(). This makes
mmap() O(1) by only setting up the VMA bounds and inserting pages during
actual accesses.

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 34865701f7f75..a6e44428a6c52 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -608,17 +608,37 @@ static int array_map_check_btf(struct bpf_map *map,
>  static int array_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
>  {
>  	struct bpf_array *array = container_of(map, struct bpf_array, map);
> -	pgoff_t pgoff = PAGE_ALIGN(sizeof(*array)) >> PAGE_SHIFT;
>  
>  	if (!(map->map_flags & BPF_F_MMAPABLE))
>  		return -EINVAL;
>  
> -	if (vma->vm_pgoff * PAGE_SIZE + (vma->vm_end - vma->vm_start) >
> +	/* use u64 math so the offset cannot overflow on 32-bit archs */
> +	if ((u64)vma->vm_pgoff * PAGE_SIZE + (vma->vm_end - vma->vm_start) >
>  	    PAGE_ALIGN((u64)array->map.max_entries * array->elem_size))
>  		return -EINVAL;

[Severity: Critical]
Can this arithmetic wrap around 64 bits to a small value, bypassing the safety
check in array_map_mmap()?

If the user-provided vma->vm_pgoff is carefully crafted, the 64-bit addition
could wrap around to a small value. This overflow was previously harmless
because the eagerly mapped remap_vmalloc_range() strictly guarded against
shift overflows.

> -	return remap_vmalloc_range(vma, array_map_vmalloc_addr(array),
> -				   vma->vm_pgoff + pgoff);
> +	/* pages are faulted in on demand by array_map_mmap_fault() */
> +	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);
> +
> +	return 0;
> +}
> +
> +static vm_fault_t array_map_mmap_fault(struct bpf_map *map,
> +				       struct vm_fault *vmf)
> +{
> +	struct bpf_array *array = container_of(map, struct bpf_array, map);
> +	struct page *page;
> +
> +	page = vmalloc_to_page(array->value + ((u64)vmf->pgoff << PAGE_SHIFT));

[Severity: Critical]
Does this shift lack overflow checking, allowing backwards pointer arithmetic
in array_map_mmap_fault()?

If a wrapped pgoff bypasses the bounds check in array_map_mmap(), shifting the
user-controlled pgoff here without validation can produce a large negative
offset. Since array->value is explicitly aligned to start on the second page
of the map's vmalloc allocation, a negative offset could walk backward to the
first page, inadvertently exposing the struct bpf_array metadata to userspace.

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