On Mon, Jun 17, 2024 at 8:34 AM Barret Rhoden <brho@xxxxxxxxxx> wrote: > > On 6/15/24 14:19, Alexei Starovoitov wrote: > > From: Alexei Starovoitov <ast@xxxxxxxxxx> > > > > 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@xxxxxxxxx> > > Closes: https://lore.kernel.org/bpf/Zmuw29IhgyPNKnIM@xxxxxxxxxxxxxxxx/ > > Fixes: 317460317a02 ("bpf: Introduce bpf_arena.") > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx> > > --- > > 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()"? yep. > > 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. good point. will change. pw-bot: cr
