netdev - Re: [PATCH bpf v2] bpf: fix potential race in tail call compatibility check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Oct 2021 16:28:19 +0200
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf v2] bpf: fix potential race in tail call
 compatibility check

> Lorenzo noticed that the code testing for program type compatibility of
> tail call maps is potentially racy in that two threads could encounter a
> map with an unset type simultaneously and both return true even though they
> are inserting incompatible programs.
> 
> The race window is quite small, but artificially enlarging it by adding a
> usleep_range() inside the check in bpf_prog_array_compatible() makes it
> trivial to trigger from userspace with a program that does, essentially:
> 
>         map_fd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, 4, 4, 2, 0);
>         pid = fork();
>         if (pid) {
>                 key = 0;
>                 value = xdp_fd;
>         } else {
>                 key = 1;
>                 value = tc_fd;
>         }
>         err = bpf_map_update_elem(map_fd, &key, &value, 0);
> 
> While the race window is small, it has potentially serious ramifications in
> that triggering it would allow a BPF program to tail call to a program of a
> different type. So let's get rid of it by protecting the update with a
> spinlock. The commit in the Fixes tag is the last commit that touches the
> code in question.
> 
> v2:
> - Use a spinlock instead of an atomic variable and cmpxchg() (Alexei)
> 
> Fixes: 3324b584b6f6 ("ebpf: misc core cleanup")
> Reported-by: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
>  include/linux/bpf.h   |  1 +
>  kernel/bpf/arraymap.c |  1 +
>  kernel/bpf/core.c     | 14 ++++++++++----
>  kernel/bpf/syscall.c  |  2 ++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 020a7d5bf470..98d906176d89 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -929,6 +929,7 @@ struct bpf_array_aux {
>  	 * stored in the map to make sure that all callers and callees have
>  	 * the same prog type and JITed flag.
>  	 */
> +	spinlock_t type_check_lock;

I was wondering if we can use a mutex instead of a spinlock here since it is
run from a syscall AFAIU. The only downside is mutex_lock is run inside
aux->used_maps_mutex critical section. Am I missing something?

Regards,
Lorenzo

>  	enum bpf_prog_type type;
>  	bool jited;
>  	/* Programs with direct jumps into programs part of this array. */
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index cebd4fb06d19..da9b1e96cadc 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -1072,6 +1072,7 @@ static struct bpf_map *prog_array_map_alloc(union bpf_attr *attr)
>  	INIT_WORK(&aux->work, prog_array_map_clear_deferred);
>  	INIT_LIST_HEAD(&aux->poke_progs);
>  	mutex_init(&aux->poke_mutex);
> +	spin_lock_init(&aux->type_check_lock);
>  
>  	map = array_map_alloc(attr);
>  	if (IS_ERR(map)) {
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index c1e7eb3f1876..9439c839d279 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1823,20 +1823,26 @@ static unsigned int __bpf_prog_ret0_warn(const void *ctx,
>  bool bpf_prog_array_compatible(struct bpf_array *array,
>  			       const struct bpf_prog *fp)
>  {
> +	bool ret;
> +
>  	if (fp->kprobe_override)
>  		return false;
>  
> +	spin_lock(&array->aux->type_check_lock);
> +
>  	if (!array->aux->type) {
>  		/* There's no owner yet where we could check for
>  		 * compatibility.
>  		 */
>  		array->aux->type  = fp->type;
>  		array->aux->jited = fp->jited;
> -		return true;
> +		ret = true;
> +	} else {
> +		ret = array->aux->type  == fp->type &&
> +		      array->aux->jited == fp->jited;
>  	}
> -
> -	return array->aux->type  == fp->type &&
> -	       array->aux->jited == fp->jited;
> +	spin_unlock(&array->aux->type_check_lock);
> +	return ret;
>  }
>  
>  static int bpf_check_tail_call(const struct bpf_prog *fp)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4e50c0bfdb7d..955011c7df29 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -543,8 +543,10 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
>  
>  	if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY) {
>  		array = container_of(map, struct bpf_array, map);
> +		spin_lock(&array->aux->type_check_lock);
>  		type  = array->aux->type;
>  		jited = array->aux->jited;
> +		spin_unlock(&array->aux->type_check_lock);
>  	}
>  
>  	seq_printf(m,
> -- 
> 2.33.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)