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)
v3:
- Put lock and the members it protects into an embedded 'owner' struct (Daniel)

Fixes: 3324b584b6f6 ("ebpf: misc core cleanup")
Reported-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20211026110019.363464-1-toke@redhat.com

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 020a7d5bf4701afbf1e504476ecd41323bd1fa6a..3db6f6c95489e742d07ca9b46fb98ee2d0a6302b 100644 (file)
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -929,8 +929,11 @@ struct bpf_array_aux {
         * stored in the map to make sure that all callers and callees have
         * the same prog type and JITed flag.
         */
-       enum bpf_prog_type type;
-       bool jited;
+       struct {
+               spinlock_t lock;
+               enum bpf_prog_type type;
+               bool jited;
+       } owner;
        /* Programs with direct jumps into programs part of this array. */
        struct list_head poke_progs;
        struct bpf_map *map;

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index cebd4fb06d190a561205b4c5ecf6aeeaffca70fd..447def5405444249983f00fcd5997f7dff7e8ed4 100644 (file)
--- 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->owner.lock);
 
        map = array_map_alloc(attr);
        if (IS_ERR(map)) {

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index c1e7eb3f18760e63fb6f2f44f77aaa91d87d43a2..6e3ae90ad107a23dd10fd8382168f92e549d83b1 100644 (file)
--- 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;
 
-       if (!array->aux->type) {
+       spin_lock(&array->aux->owner.lock);
+
+       if (!array->aux->owner.type) {
                /* There's no owner yet where we could check for
                 * compatibility.
                 */
-               array->aux->type  = fp->type;
-               array->aux->jited = fp->jited;
-               return true;
+               array->aux->owner.type  = fp->type;
+               array->aux->owner.jited = fp->jited;
+               ret = true;
+       } else {
+               ret = array->aux->owner.type  == fp->type &&
+                     array->aux->owner.jited == fp->jited;
        }
-
-       return array->aux->type  == fp->type &&
-              array->aux->jited == fp->jited;
+       spin_unlock(&array->aux->owner.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 9dab49d3f394ded85d974159c6784c4618c81aa7..1cad6979a0d0f4bec405fb4654d80ecd03d9dc1a 100644 (file)
--- 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);
-               type  = array->aux->type;
-               jited = array->aux->jited;
+               spin_lock(&array->aux->owner.lock);
+               type  = array->aux->owner.type;
+               jited = array->aux->owner.jited;
+               spin_unlock(&array->aux->owner.lock);
        }
 
        seq_printf(m,