Re: [PATCH bpf] bpf: Fix remap of arena.

[Barret Rhoden <brho@google.com>]

On 6/15/24 14:19, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The bpf arena logic didn't account for mremap operation. Add a refcnt for
> multiple mmap events to prevent use-after-free in arena_vm_close.
> 
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Closes: https://lore.kernel.org/bpf/Zmuw29IhgyPNKnIM@xpf.sh.intel.com/
> Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>   kernel/bpf/arena.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 583ee4fe48ef..f31fcaf7ee8e 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -48,6 +48,7 @@ struct bpf_arena {
>   	struct maple_tree mt;
>   	struct list_head vma_list;
>   	struct mutex lock;
> +	atomic_t mmap_count;
>   };
>   
>   u64 bpf_arena_get_kern_vm_start(struct bpf_arena *arena)
> @@ -227,12 +228,22 @@ static int remember_vma(struct bpf_arena *arena, struct vm_area_struct *vma)
>   	return 0;
>   }
>   
> +static void arena_vm_open(struct vm_area_struct *vma)
> +{
> +	struct bpf_map *map = vma->vm_file->private_data;
> +	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +
> +	atomic_inc(&arena->mmap_count);
> +}
> +
>   static void arena_vm_close(struct vm_area_struct *vma)
>   {
>   	struct bpf_map *map = vma->vm_file->private_data;
>   	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
>   	struct vma_list *vml;
>   
> +	if (!atomic_dec_and_test(&arena->mmap_count))
> +		return;
>   	guard(mutex)(&arena->lock);
>   	vml = vma->vm_private_data;
>   	list_del(&vml->head);
> @@ -287,6 +298,7 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>   }
>   
>   static const struct vm_operations_struct arena_vm_ops = {
> +	.open		= arena_vm_open,
>   	.close		= arena_vm_close,
>   	.fault          = arena_vm_fault,
>   };
> @@ -361,6 +373,7 @@ static int arena_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
>   	 */
>   	vm_flags_set(vma, VM_DONTEXPAND);
>   	vma->vm_ops = &arena_vm_ops;
> +	atomic_set(&arena->mmap_count, 1);

i'm not sure, but i have the feeling that this refcnt should be on the 
struct vma_list or something.

what happens if two different processes mmap the same arena?  will the 
second one come in and set the mmap_count = 1, clobbering whatever the 
first process had already done?

what are the rules for a vma's vm_ops?  something like: "there will be a 
close() for the initial mmap and for every open()"?

if that's what it's doing, then this initial refcnt = 1 corresponds to 
the remember_vma() call.  in which case, vm_ops->open ought to lookup 
the remembered vma (struct vma_list) and do the incref there.

barret