| From foo@baz Mon Feb 29 14:33:50 PST 2016 |
| From: Daniel Borkmann <daniel@iogearbox.net> |
| Date: Wed, 10 Feb 2016 16:47:11 +0100 |
| Subject: bpf: fix branch offset adjustment on backjumps after patching ctx expansion |
| |
| From: Daniel Borkmann <daniel@iogearbox.net> |
| |
| [ Upstream commit a1b14d27ed0965838350f1377ff97c93ee383492 ] |
| |
| When ctx access is used, the kernel often needs to expand/rewrite |
| instructions, so after that patching, branch offsets have to be |
| adjusted for both forward and backward jumps in the new eBPF program, |
| but for backward jumps it fails to account the delta. Meaning, for |
| example, if the expansion happens exactly on the insn that sits at |
| the jump target, it doesn't fix up the back jump offset. |
| |
| Analysis on what the check in adjust_branches() is currently doing: |
| |
| /* adjust offset of jmps if necessary */ |
| if (i < pos && i + insn->off + 1 > pos) |
| insn->off += delta; |
| else if (i > pos && i + insn->off + 1 < pos) |
| insn->off -= delta; |
| |
| First condition (forward jumps): |
| |
| Before: After: |
| |
| insns[0] insns[0] |
| insns[1] <--- i/insn insns[1] <--- i/insn |
| insns[2] <--- pos insns[P] <--- pos |
| insns[3] insns[P] `------| delta |
| insns[4] <--- target_X insns[P] `-----| |
| insns[5] insns[3] |
| insns[4] <--- target_X |
| insns[5] |
| |
| First case is if we cross pos-boundary and the jump instruction was |
| before pos. This is handeled correctly. I.e. if i == pos, then this |
| would mean our jump that we currently check was the patchlet itself |
| that we just injected. Since such patchlets are self-contained and |
| have no awareness of any insns before or after the patched one, the |
| delta is correctly not adjusted. Also, for the second condition in |
| case of i + insn->off + 1 == pos, means we jump to that newly patched |
| instruction, so no offset adjustment are needed. That part is correct. |
| |
| Second condition (backward jumps): |
| |
| Before: After: |
| |
| insns[0] insns[0] |
| insns[1] <--- target_X insns[1] <--- target_X |
| insns[2] <--- pos <-- target_Y insns[P] <--- pos <-- target_Y |
| insns[3] insns[P] `------| delta |
| insns[4] <--- i/insn insns[P] `-----| |
| insns[5] insns[3] |
| insns[4] <--- i/insn |
| insns[5] |
| |
| Second interesting case is where we cross pos-boundary and the jump |
| instruction was after pos. Backward jump with i == pos would be |
| impossible and pose a bug somewhere in the patchlet, so the first |
| condition checking i > pos is okay only by itself. However, i + |
| insn->off + 1 < pos does not always work as intended to trigger the |
| adjustment. It works when jump targets would be far off where the |
| delta wouldn't matter. But, for example, where the fixed insn->off |
| before pointed to pos (target_Y), it now points to pos + delta, so |
| that additional room needs to be taken into account for the check. |
| This means that i) both tests here need to be adjusted into pos + delta, |
| and ii) for the second condition, the test needs to be <= as pos |
| itself can be a target in the backjump, too. |
| |
| Fixes: 9bac3d6d548e ("bpf: allow extended BPF programs access skb fields") |
| Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/bpf/verifier.c | 2 +- |
| 1 file changed, 1 insertion(+), 1 deletion(-) |
| |
| --- a/kernel/bpf/verifier.c |
| +++ b/kernel/bpf/verifier.c |
| @@ -2082,7 +2082,7 @@ static void adjust_branches(struct bpf_p |
| /* adjust offset of jmps if necessary */ |
| if (i < pos && i + insn->off + 1 > pos) |
| insn->off += delta; |
| - else if (i > pos && i + insn->off + 1 < pos) |
| + else if (i > pos + delta && i + insn->off + 1 <= pos + delta) |
| insn->off -= delta; |
| } |
| } |