bpf: Fix use-after-free in fmod_ret check

Fix the following issue:
[  436.749342] BUG: KASAN: use-after-free in bpf_trampoline_put+0x39/0x2a0
[  436.749995] Write of size 4 at addr ffff8881ef38b8a0 by task kworker/3:5/2243
[  436.750712]
[  436.752677] Workqueue: events bpf_prog_free_deferred
[  436.753183] Call Trace:
[  436.756483]  bpf_trampoline_put+0x39/0x2a0
[  436.756904]  bpf_prog_free_deferred+0x16d/0x3d0
[  436.757377]  process_one_work+0x94a/0x15b0
[  436.761969]
[  436.762130] Allocated by task 2529:
[  436.763323]  bpf_trampoline_lookup+0x136/0x540
[  436.763776]  bpf_check+0x2872/0xa0a8
[  436.764144]  bpf_prog_load+0xb6f/0x1350
[  436.764539]  __do_sys_bpf+0x16d7/0x3720
[  436.765825]
[  436.765988] Freed by task 2529:
[  436.767084]  kfree+0xc6/0x280
[  436.767397]  bpf_trampoline_put+0x1fd/0x2a0
[  436.767826]  bpf_check+0x6832/0xa0a8
[  436.768197]  bpf_prog_load+0xb6f/0x1350
[  436.768594]  __do_sys_bpf+0x16d7/0x3720

prog->aux->trampoline = tr should be set only when prog is valid.
Otherwise prog freeing will try to put trampoline via prog->aux->trampoline,
but it may not point to a valid trampoline.

Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: KP Singh <kpsingh@google.com>
Link: https://lore.kernel.org/bpf/20200529043839.15824-2-alexei.starovoitov@gmail.com

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8d7ee40e2748441bd0036f9fc0528e37255c8d27..bfea3f9a972fddecc12248f2b47046986c1a5332 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10428,22 +10428,13 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 }
 #define SECURITY_PREFIX "security_"
 
-static int check_attach_modify_return(struct bpf_verifier_env *env)
+static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
 {
-       struct bpf_prog *prog = env->prog;
-       unsigned long addr = (unsigned long) prog->aux->trampoline->func.addr;
-
-       /* This is expected to be cleaned up in the future with the KRSI effort
-        * introducing the LSM_HOOK macro for cleaning up lsm_hooks.h.
-        */
        if (within_error_injection_list(addr) ||
            !strncmp(SECURITY_PREFIX, prog->aux->attach_func_name,
                     sizeof(SECURITY_PREFIX) - 1))
                return 0;
 
-       verbose(env, "fmod_ret attach_btf_id %u (%s) is not modifiable\n",
-               prog->aux->attach_btf_id, prog->aux->attach_func_name);
-
        return -EINVAL;
 }
 
@@ -10654,11 +10645,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
                                goto out;
                        }
                }
+
+               if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
+                       ret = check_attach_modify_return(prog, addr);
+                       if (ret)
+                               verbose(env, "%s() is not modifiable\n",
+                                       prog->aux->attach_func_name);
+               }
+
+               if (ret)
+                       goto out;
                tr->func.addr = (void *)addr;
                prog->aux->trampoline = tr;
-
-               if (prog->expected_attach_type == BPF_MODIFY_RETURN)
-                       ret = check_attach_modify_return(env);
 out:
                mutex_unlock(&tr->mutex);
                if (ret)