[v2,bpf] bpf: Fix remap of arena.

Commit Message

Alexei Starovoitov June 17, 2024, 5:18 p.m. UTC

From: Alexei Starovoitov <[e-mail]>

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 <[e-mail]>
Closes: https://lore.kernel.org/bpf/[e-mail]/
Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 kernel/bpf/arena.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Barret Rhoden June 17, 2024, 5:52 p.m. UTC | #1

On 6/17/24 13:18, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
> Signed-off-by: Alexei Starovoitov <[e-mail]>

Reviewed-by: Barret Rhoden <[e-mail]>

Pengfei Xu June 18, 2024, 2:02 a.m. UTC | #2

The issue cannot be reproduced in the new patch below.

Best Regards,
Thanks!

On 2024-06-17 at 10:18:12 -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> ---
>  kernel/bpf/arena.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 583ee4fe48ef..fe35b45cf6f8 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -212,6 +212,7 @@ static u64 arena_map_mem_usage(const struct bpf_map *map)
>  struct vma_list {
>  	struct vm_area_struct *vma;
>  	struct list_head head;
> +	atomic_t mmap_count;
>  };
>  
>  static int remember_vma(struct bpf_arena *arena, struct vm_area_struct *vma)
> @@ -221,20 +222,32 @@ static int remember_vma(struct bpf_arena *arena, struct vm_area_struct *vma)
>  	vml = kmalloc(sizeof(*vml), GFP_KERNEL);
>  	if (!vml)
>  		return -ENOMEM;
> +	atomic_set(&vml->mmap_count, 1);
>  	vma->vm_private_data = vml;
>  	vml->vma = vma;
>  	list_add(&vml->head, &arena->vma_list);
>  	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);
> +	struct vma_list *vml = vma->vm_private_data;
> +
> +	atomic_inc(&vml->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;
> +	struct vma_list *vml = vma->vm_private_data;
>  
> +	if (!atomic_dec_and_test(&vml->mmap_count))
> +		return;
>  	guard(mutex)(&arena->lock);
> -	vml = vma->vm_private_data;
> +	/* update link list under lock */
>  	list_del(&vml->head);
>  	vma->vm_private_data = NULL;
>  	kfree(vml);
> @@ -287,6 +300,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,
>  };
> -- 
> 2.43.0
>

kernel test robot June 18, 2024, 4:43 a.m. UTC | #3

Hi Alexei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexei-Starovoitov/bpf-Fix-remap-of-arena/20240618-012054
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
patch link:    https://lore.kernel.org/r/20240617171812.76634-1-alexei.starovoitov%40gmail.com
patch subject: [PATCH v2 bpf] bpf: Fix remap of arena.
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240618/[e-mail]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240618/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

   kernel/bpf/arena.c: In function 'arena_vm_open':
>> kernel/bpf/arena.c:235:27: warning: unused variable 'arena' [-Wunused-variable]
     235 |         struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
         |                           ^~~~~


vim +/arena +235 kernel/bpf/arena.c

   231	
   232	static void arena_vm_open(struct vm_area_struct *vma)
   233	{
   234		struct bpf_map *map = vma->vm_file->private_data;
 > 235		struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
   236		struct vma_list *vml = vma->vm_private_data;
   237	
   238		atomic_inc(&vml->mmap_count);
   239	}
   240

Alexei Starovoitov June 18, 2024, 2:49 p.m. UTC | #4

On Mon, Jun 17, 2024 at 9:43 PM kernel test robot <[e-mail]> wrote:
>
> Hi Alexei,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on bpf/master]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Alexei-Starovoitov/bpf-Fix-remap-of-arena/20240618-012054
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
> patch link:    https://lore.kernel.org/r/20240617171812.76634-1-alexei.starovoitov%40gmail.com
> patch subject: [PATCH v2 bpf] bpf: Fix remap of arena.
> config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240618/[e-mail]/config)
> compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240618/[e-mail]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[e-mail]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>
> All warnings (new ones prefixed by >>):
>
>    kernel/bpf/arena.c: In function 'arena_vm_open':
> >> kernel/bpf/arena.c:235:27: warning: unused variable 'arena' [-Wunused-variable]
>      235 |         struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
>          |                           ^~~~~

Daniel or Andrii,
could you please remove this line while applying?

Daniel Borkmann June 18, 2024, 3:28 p.m. UTC | #5

On 6/18/24 4:49 PM, Alexei Starovoitov wrote:
> On Mon, Jun 17, 2024 at 9:43 PM kernel test robot <[e-mail]> wrote:
>>
>> Hi Alexei,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on bpf/master]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Alexei-Starovoitov/bpf-Fix-remap-of-arena/20240618-012054
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
>> patch link:    https://lore.kernel.org/r/20240617171812.76634-1-alexei.starovoitov%40gmail.com
>> patch subject: [PATCH v2 bpf] bpf: Fix remap of arena.
>> config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240618/[e-mail]/config)
>> compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240618/[e-mail]/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <[e-mail]>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>
>> All warnings (new ones prefixed by >>):
>>
>>     kernel/bpf/arena.c: In function 'arena_vm_open':
>>>> kernel/bpf/arena.c:235:27: warning: unused variable 'arena' [-Wunused-variable]
>>       235 |         struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
>>           |                           ^~~~~
> 
> Daniel or Andrii,
> could you please remove this line while applying?

Ok, done, both lines actually.

[e-mail] June 18, 2024, 3:30 p.m. UTC | #6

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <[e-mail]>:

On Mon, 17 Jun 2024 10:18:12 -0700 you wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> 
> [...]

Here is the summary with links:
  - [v2,bpf] bpf: Fix remap of arena.
    https://git.kernel.org/bpf/bpf/c/b90d77e5fd78

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index 583ee4fe48ef..fe35b45cf6f8 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -212,6 +212,7 @@  static u64 arena_map_mem_usage(const struct bpf_map *map)
 struct vma_list {
 	struct vm_area_struct *vma;
 	struct list_head head;
+	atomic_t mmap_count;
 };
 
 static int remember_vma(struct bpf_arena *arena, struct vm_area_struct *vma)
@@ -221,20 +222,32 @@  static int remember_vma(struct bpf_arena *arena, struct vm_area_struct *vma)
 	vml = kmalloc(sizeof(*vml), GFP_KERNEL);
 	if (!vml)
 		return -ENOMEM;
+	atomic_set(&vml->mmap_count, 1);
 	vma->vm_private_data = vml;
 	vml->vma = vma;
 	list_add(&vml->head, &arena->vma_list);
 	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);
+	struct vma_list *vml = vma->vm_private_data;
+
+	atomic_inc(&vml->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;
+	struct vma_list *vml = vma->vm_private_data;
 
+	if (!atomic_dec_and_test(&vml->mmap_count))
+		return;
 	guard(mutex)(&arena->lock);
-	vml = vma->vm_private_data;
+	/* update link list under lock */
 	list_del(&vml->head);
 	vma->vm_private_data = NULL;
 	kfree(vml);
@@ -287,6 +300,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,
 };