Skip to content

Commit 5337ac4

Browse files
Alexei Starovoitovborkmann
Alexei Starovoitov
authored andcommitted
bpf: Fix the corner case with may_goto and jump to the 1st insn.
When the following program is processed by the verifier: L1: may_goto L2 goto L1 L2: w0 = 0 exit the may_goto insn is first converted to: L1: r11 = *(u64 *)(r10 -8) if r11 == 0x0 goto L2 r11 -= 1 *(u64 *)(r10 -8) = r11 goto L1 L2: w0 = 0 exit then later as the last step the verifier inserts: *(u64 *)(r10 -8) = BPF_MAX_LOOPS as the first insn of the program to initialize loop count. When the first insn happens to be a branch target of some jmp the bpf_patch_insn_data() logic will produce: L1: *(u64 *)(r10 -8) = BPF_MAX_LOOPS r11 = *(u64 *)(r10 -8) if r11 == 0x0 goto L2 r11 -= 1 *(u64 *)(r10 -8) = r11 goto L1 L2: w0 = 0 exit because instruction patching adjusts all jmps and calls, but for this particular corner case it's incorrect and the L1 label should be one instruction down, like: *(u64 *)(r10 -8) = BPF_MAX_LOOPS L1: r11 = *(u64 *)(r10 -8) if r11 == 0x0 goto L2 r11 -= 1 *(u64 *)(r10 -8) = r11 goto L1 L2: w0 = 0 exit and that's what this patch is fixing. After bpf_patch_insn_data() call adjust_jmp_off() to adjust all jmps that point to newly insert BPF_ST insn to point to insn after. Note that bpf_patch_insn_data() cannot easily be changed to accommodate this logic, since jumps that point before or after a sequence of patched instructions have to be adjusted with the full length of the patch. Conceptually it's somewhat similar to "insert" of instructions between other instructions with weird semantics. Like "insert" before 1st insn would require adjustment of CALL insns to point to newly inserted 1st insn, but not an adjustment JMP insns that point to 1st, yet still adjusting JMP insns that cross over 1st insn (point to insn before or insn after), hence use simple adjust_jmp_off() logic to fix this corner case. Ideally bpf_patch_insn_data() would have an auxiliary info to say where 'the start of newly inserted patch is', but it would be too complex for backport. Fixes: 011832b ("bpf: Introduce may_goto instruction") Reported-by: Zac Ecob <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]> Signed-off-by: Daniel Borkmann <[email protected]> Acked-by: Eduard Zingerman <[email protected]> Closes: https://lore.kernel.org/bpf/CAADnVQJ_WWx8w4b=6Gc2EpzAjgv+6A0ridnMz2TvS2egj4r3Gw@mail.gmail.com/ Link: https://lore.kernel.org/bpf/[email protected]
1 parent 66b5867 commit 5337ac4

File tree

1 file changed

+50
-0
lines changed

1 file changed

+50
-0
lines changed

kernel/bpf/verifier.c

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12721,6 +12721,16 @@ static bool signed_add32_overflows(s32 a, s32 b)
1272112721
return res < a;
1272212722
}
1272312723

12724+
static bool signed_add16_overflows(s16 a, s16 b)
12725+
{
12726+
/* Do the add in u16, where overflow is well-defined */
12727+
s16 res = (s16)((u16)a + (u16)b);
12728+
12729+
if (b < 0)
12730+
return res > a;
12731+
return res < a;
12732+
}
12733+
1272412734
static bool signed_sub_overflows(s64 a, s64 b)
1272512735
{
1272612736
/* Do the sub in u64, where overflow is well-defined */
@@ -18732,6 +18742,39 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
1873218742
return new_prog;
1873318743
}
1873418744

18745+
/*
18746+
* For all jmp insns in a given 'prog' that point to 'tgt_idx' insn adjust the
18747+
* jump offset by 'delta'.
18748+
*/
18749+
static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
18750+
{
18751+
struct bpf_insn *insn = prog->insnsi;
18752+
u32 insn_cnt = prog->len, i;
18753+
18754+
for (i = 0; i < insn_cnt; i++, insn++) {
18755+
u8 code = insn->code;
18756+
18757+
if ((BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) ||
18758+
BPF_OP(code) == BPF_CALL || BPF_OP(code) == BPF_EXIT)
18759+
continue;
18760+
18761+
if (insn->code == (BPF_JMP32 | BPF_JA)) {
18762+
if (i + 1 + insn->imm != tgt_idx)
18763+
continue;
18764+
if (signed_add32_overflows(insn->imm, delta))
18765+
return -ERANGE;
18766+
insn->imm += delta;
18767+
} else {
18768+
if (i + 1 + insn->off != tgt_idx)
18769+
continue;
18770+
if (signed_add16_overflows(insn->imm, delta))
18771+
return -ERANGE;
18772+
insn->off += delta;
18773+
}
18774+
}
18775+
return 0;
18776+
}
18777+
1873518778
static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
1873618779
u32 off, u32 cnt)
1873718780
{
@@ -20548,6 +20591,13 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
2054820591
if (!new_prog)
2054920592
return -ENOMEM;
2055020593
env->prog = prog = new_prog;
20594+
/*
20595+
* If may_goto is a first insn of a prog there could be a jmp
20596+
* insn that points to it, hence adjust all such jmps to point
20597+
* to insn after BPF_ST that inits may_goto count.
20598+
* Adjustment will succeed because bpf_patch_insn_data() didn't fail.
20599+
*/
20600+
WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
2055120601
}
2055220602

2055320603
/* Since poke tab is now finalized, publish aux to tracker. */

0 commit comments

Comments
 (0)