From: Qiliang Yuan bpf_convert_ctx_accesses() can patch one insn two times. The first patch puts a barrier ('nospec') in front of the insn. The second patch replaces the insn, for example a ctx access. bpf_patch_insn_data() moves the insn immediately, and the second call gets its new index. The patch list only queues a patch, and no insn moves before the commit. Thus a queued insn has no index, and bpf_patch_list_add() accepts only one patch for each insn. Let a second bpf_patch_list_add() for the same insn replace the last insn of the queued patch. That is where the first patch puts the insn. The result is one patch. Here insn i reads skb->ifindex: queued patch of insn i after the second bpf_patch_list_add() nospec nospec r0 = *(u32 *)(r1 + 40) r0 = *(u64 *)(r1 + 16) if r0 == 0 goto +1 r0 = *(u32 *)(r0 + 224) - The pass writes the second patch as if it was the only patch of insn i. But in the list its insns are behind the barrier. Thus a backward jump or call that leaves the patch gets longer by the number of insns in front: 'gotol -5' becomes 'gotol -6'. A forward jump, as the 'if r0 == 0 goto +1' above, does not change. - In this pass only the 'gotol' from an 'exit' back to the first epilogue is such a backward jump. No prog has it behind a barrier. A prog with an epilogue needs CAP_PERFMON, and then the verifier adds no barrier. The list adjusts the jump all the same, because bpf_patch_list_add() must not depend on what its callers queue. - The second patch changes the length of the queued patch. Thus the insns in front of the last insn must not jump behind the patch. A barrier does not jump. For such a jump bpf_patch_list_add() returns -EFAULT with a WARN. - A second patch of 1 insn goes to the list. It is not written in place, because the commit takes the insn from the list. Signed-off-by: Qiliang Yuan Signed-off-by: Alexei Starovoitov --- kernel/bpf/core.c | 64 +++++++++++++++++++++++++++++++++++++++++++-- kernel/bpf/fixups.c | 9 ++++--- 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index e96de7a2c786..633728a5f0ea 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -602,6 +602,46 @@ static int insn_set_rel(struct bpf_insn *insn, s64 rel, bool is_imm) return 0; } +/* Whether one of the 'len' insns of a patch in front of the last one jumps past it. */ +static bool patch_list_jumps_past(const struct bpf_insn *insns, u32 len) +{ + bool is_imm; + s64 rel; + u32 k; + + for (k = 0; k + 1 < len; k++) + if (insn_get_rel(&insns[k], &rel, &is_imm) && (s64)k + rel + 1 >= len) + return true; + return false; +} + +/* + * The 'len' insns of a patch were moved down by 'by' insns inside it. Keep + * what they referred to in front of the patch, the same way as + * bpf_adj_moved_insn() does: a call to the start of the patch, the old start + * of a subprog, stays there, a jump to itself doesn't. + */ +static int patch_list_move_down(struct bpf_insn *insns, u32 len, u32 by) +{ + bool is_imm, call; + s64 rel, tgt; + u32 k; + int err; + + for (k = 0; k < len; k++) { + if (!insn_get_rel(&insns[k], &rel, &is_imm)) + continue; + call = bpf_pseudo_func(&insns[k]) || bpf_pseudo_call(&insns[k]); + tgt = (s64)k + rel + 1; + if (tgt > 0 || (tgt == 0 && !call)) + continue; + err = insn_set_rel(&insns[k], rel - by, is_imm); + if (err) + return err; + } + return 0; +} + /* * insn is number 'idx' of the 'len' insns that replace insn 'off' of the old * image (an insn that isn't patched is its own patch of length 1). It lands @@ -634,7 +674,10 @@ static int patch_list_adj_insn(struct bpf_insn *insn, const u32 *new_off, * Queue the replacement of insn 'off' of 'prog' by 'patch[0..len)'. Patches * are queued in increasing order of 'off' and applied together, so a pass * keeps addressing insns by their index in 'prog'. Jumps in the patch that - * leave it are relative to the patch placed at 'off'. + * leave it are relative to the patch placed at 'off'. Patching 'off' again + * replaces the last insn of its patch, where the insn being patched went, and + * the insns in front of it must not jump past it. Jumps of the new insns that + * leave the patch are relative to the first of them, as for a first patch. * * Return the copy of the last insn of the patch, which stays valid until the * next call. @@ -645,9 +688,26 @@ struct bpf_insn *__bpf_patch_list_add(struct bpf_patch_list *pl, const struct bpf_prog *prog, u32 off, const struct bpf_insn *patch, u32 len) { - struct bpf_insn_patch *p; + struct bpf_insn_patch *p = pl->cnt ? &pl->patches[pl->cnt - 1] : NULL; int err; + if (p && off == p->off) { + if (WARN_ON_ONCE(patch_list_jumps_past(pl->insns + p->start, p->len))) + return ERR_PTR(-EFAULT); + err = patch_list_reserve_insns(pl, len - 1); + if (err) + return ERR_PTR(err); + /* the last patch is at the end of pl->insns */ + memcpy(pl->insns + p->start + p->len - 1, patch, len * sizeof(*patch)); + err = patch_list_move_down(pl->insns + p->start + p->len - 1, len, + p->len - 1); + if (err) + return ERR_PTR(err); + p->len += len - 1; + pl->insn_cnt += len - 1; + return &pl->insns[p->start + p->len - 1]; + } + err = patch_list_reserve_patch(pl) ?: patch_list_reserve_insns(pl, len); if (err) return ERR_PTR(err); diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c index 6fe47e400bc2..7f8d51617b2e 100644 --- a/kernel/bpf/fixups.c +++ b/kernel/bpf/fixups.c @@ -350,28 +350,31 @@ struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off, /* * Queue the replacement of insn 'off' by 'patch[0..len)' until * bpf_patch_list_commit(), see __bpf_patch_list_add(). Replacing one insn by - * one moves nothing and is done right away. + * one moves nothing and is done right away in the program, unless 'off' has a + * queued patch: the commit takes the insn from there. */ struct bpf_insn *bpf_patch_list_add(struct bpf_verifier_env *env, u32 off, const struct bpf_insn *patch, u32 len) { struct bpf_patch_list *pl = &env->patch_list; + struct bpf_insn_patch *p = pl->cnt ? &pl->patches[pl->cnt - 1] : NULL; if (bpf_rewrite_must_abort()) return ERR_PTR(-EINTR); if (verifier_bug_if(!len || off >= env->prog->len || - (pl->cnt && off <= pl->patches[pl->cnt - 1].off), + (p && (off < p->off || (off == p->off && !p->len))), env, "insn %u patched out of order", off)) return ERR_PTR(-EFAULT); - if (len == 1) { + if (len == 1 && (!p || off != p->off)) { env->prog->insnsi[off] = *patch; adjust_insn_aux_data(env, env->prog, off, 1, NULL, BPF_PATCH_KEEP_TARGET); return &env->prog->insnsi[off]; } + /* -ERANGE takes a second patch with a 16-bit jump back out of it: no pass has one */ return __bpf_patch_list_add(pl, env->prog, off, patch, len); } -- 2.55.0