bpf: Stop caching subprog index in the bpf_pseudo_func insn

commit 3990ed4c426652fcd469f8c9dc08156294b36c28 upstream.

This patch is to fix an out-of-bound access issue when jit-ing the
bpf_pseudo_func insn (i.e. ld_imm64 with src_reg == BPF_PSEUDO_FUNC)

In jit_subprog(), it currently reuses the subprog index cached in
insn[1].imm.  This subprog index is an index into a few array related
to subprogs.  For example, in jit_subprog(), it is an index to the newly
allocated 'struct bpf_prog **func' array.

The subprog index was cached in insn[1].imm after add_subprog().  However,
this could become outdated (and too big in this case) if some subprogs
are completely removed during dead code elimination (in
adjust_subprog_starts_after_remove).  The cached index in insn[1].imm
is not updated accordingly and causing out-of-bound issue in the later
jit_subprog().

Unlike bpf_pseudo_'func' insn, the current bpf_pseudo_'call' insn
is handling the DCE properly by calling find_subprog(insn->imm) to
figure out the index instead of caching the subprog index.
The existing bpf_adj_branches() will adjust the insn->imm
whenever insn is added or removed.

Instead of having two ways handling subprog index,
this patch is to make bpf_pseudo_func works more like
bpf_pseudo_call.

First change is to stop caching the subprog index result
in insn[1].imm after add_subprog().  The verification
process will use find_subprog(insn->imm) to figure
out the subprog index.

Second change is in bpf_adj_branches() and have it to
adjust the insn->imm for the bpf_pseudo_func insn also
whenever insn is added or removed.

Third change is in jit_subprog().  Like the bpf_pseudo_call handling,
bpf_pseudo_func temporarily stores the find_subprog() result
in insn->off.  It is fine because the prog's insn has been finalized
at this point.  insn->off will be reset back to 0 later to avoid
confusing the userspace prog dump tool.

Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper")
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20211106014014.651018-1-kafai@fb.com
Cc: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bb766a0b9b51e4eaffe1a151235019f1ac10aa3e..818cd594e9229d09ac654cc4d29afa0f98538514 100644 (file)
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -533,6 +533,12 @@ bpf_ctx_record_field_size(struct bpf_insn_access_aux *aux, u32 size)
        aux->ctx_field_size = size;
 }
 
+static inline bool bpf_pseudo_func(const struct bpf_insn *insn)
+{
+       return insn->code == (BPF_LD | BPF_IMM | BPF_DW) &&
+              insn->src_reg == BPF_PSEUDO_FUNC;
+}
+
 struct bpf_prog_ops {
        int (*test_run)(struct bpf_prog *prog, const union bpf_attr *kattr,
                        union bpf_attr __user *uattr);

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 48eb9c329da605fd5eeb16c96fda6aa2625b1fae..15946c11524e7013d709e6b67d8136a283ef1019 100644 (file)
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -389,6 +389,13 @@ static int bpf_adj_branches(struct bpf_prog *prog, u32 pos, s32 end_old,
                        i = end_new;
                        insn = prog->insnsi + end_old;
                }
+               if (bpf_pseudo_func(insn)) {
+                       ret = bpf_adj_delta_to_imm(insn, pos, end_old,
+                                                  end_new, i, probe_pass);
+                       if (ret)
+                               return ret;
+                       continue;
+               }
                code = insn->code;
                if ((BPF_CLASS(code) != BPF_JMP &&
                     BPF_CLASS(code) != BPF_JMP32) ||

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b72ed3ae00d847c1bb3abd5f66e34bd8adf04e30..346d36c905a90a0a483a3cf573525478f73db13d 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -240,12 +240,6 @@ static bool bpf_pseudo_kfunc_call(const struct bpf_insn *insn)
               insn->src_reg == BPF_PSEUDO_KFUNC_CALL;
 }
 
-static bool bpf_pseudo_func(const struct bpf_insn *insn)
-{
-       return insn->code == (BPF_LD | BPF_IMM | BPF_DW) &&
-              insn->src_reg == BPF_PSEUDO_FUNC;
-}
-
 struct bpf_call_arg_meta {
        struct bpf_map *map_ptr;
        bool raw_mode;
@@ -1788,16 +1782,10 @@ static int add_subprog_and_kfunc(struct bpf_verifier_env *env)
                        return -EPERM;
                }
 
-               if (bpf_pseudo_func(insn)) {
+               if (bpf_pseudo_func(insn) || bpf_pseudo_call(insn))
                        ret = add_subprog(env, i + insn->imm + 1);
-                       if (ret >= 0)
-                               /* remember subprog */
-                               insn[1].imm = ret;
-               } else if (bpf_pseudo_call(insn)) {
-                       ret = add_subprog(env, i + insn->imm + 1);
-               } else {
+               else
                        ret = add_kfunc_call(env, insn->imm);
-               }
 
                if (ret < 0)
                        return ret;
@@ -9222,7 +9210,8 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
 
        if (insn->src_reg == BPF_PSEUDO_FUNC) {
                struct bpf_prog_aux *aux = env->prog->aux;
-               u32 subprogno = insn[1].imm;
+               u32 subprogno = find_subprog(env,
+                                            env->insn_idx + insn->imm + 1);
 
                if (!aux->func_info) {
                        verbose(env, "missing btf func_info\n");
@@ -12392,14 +12381,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
                return 0;
 
        for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
-               if (bpf_pseudo_func(insn)) {
-                       env->insn_aux_data[i].call_imm = insn->imm;
-                       /* subprog is encoded in insn[1].imm */
+               if (!bpf_pseudo_func(insn) && !bpf_pseudo_call(insn))
                        continue;
-               }
 
-               if (!bpf_pseudo_call(insn))
-                       continue;
                /* Upon error here we cannot fall back to interpreter but
                 * need a hard reject of the program. Thus -EFAULT is
                 * propagated in any case.
@@ -12420,6 +12404,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)
                env->insn_aux_data[i].call_imm = insn->imm;
                /* point imm to __bpf_call_base+1 from JITs point of view */
                insn->imm = 1;
+               if (bpf_pseudo_func(insn))
+                       /* jit (e.g. x86_64) may emit fewer instructions
+                        * if it learns a u32 imm is the same as a u64 imm.
+                        * Force a non zero here.
+                        */
+                       insn[1].imm = 1;
        }
 
        err = bpf_prog_alloc_jited_linfo(prog);
@@ -12503,7 +12493,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
                insn = func[i]->insnsi;
                for (j = 0; j < func[i]->len; j++, insn++) {
                        if (bpf_pseudo_func(insn)) {
-                               subprog = insn[1].imm;
+                               subprog = insn->off;
                                insn[0].imm = (u32)(long)func[subprog]->bpf_func;
                                insn[1].imm = ((u64)(long)func[subprog]->bpf_func) >> 32;
                                continue;
@@ -12555,7 +12545,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
        for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
                if (bpf_pseudo_func(insn)) {
                        insn[0].imm = env->insn_aux_data[i].call_imm;
-                       insn[1].imm = find_subprog(env, i + insn[0].imm + 1);
+                       insn[1].imm = insn->off;
+                       insn->off = 0;
                        continue;
                }
                if (!bpf_pseudo_call(insn))