diff --git a/c.go b/c.go index 4d28de4..badc777 100644 --- a/c.go +++ b/c.go @@ -215,11 +215,11 @@ func insnToC(insn instruction, blk *block) ([]string, error) { return []string{ // Sign extend RegX to 64bits. fmt.Sprintf("indirect = (uint8_t *) (((int64_t) (int32_t) x) + %d);", i.start), + // Prevent clang from sharing indirect between bounds checks: + // the verifier needs to see the full dance every time. + fmt.Sprintf(`asm volatile("" : "+r" (indirect));`), fmt.Sprintf("if ((uint64_t)indirect >= %d) return false;", i.maxStartOffset()), fmt.Sprintf("indirect = data + (uint64_t)indirect;"), - // Prevent clang from calculating indirect + delta() directly from the packet start when RegX is constant: - // only indirect has the correct bounds check. - fmt.Sprintf(`asm volatile("" : : "r" (indirect));`), fmt.Sprintf("if (indirect + %d > data_end) return false;", i.length()), }, nil diff --git a/cbpfc.go b/cbpfc.go index f00d3fb..89631af 100644 --- a/cbpfc.go +++ b/cbpfc.go @@ -242,11 +242,13 @@ func (a packetGuardIndirect) restrict(o packetGuard) packetGuard { return packetGuardIndirect{} } - n := a - - if b.start > a.start { - n.start = b.start + // If the start is different, we need a new guard: we can't know which of the two + // offsets have been stored for the RegX + start check. + if a.start != b.start { + return packetGuardIndirect{} } + + n := a if b.end < a.end { n.end = b.end } diff --git a/cbpfc_test.go b/cbpfc_test.go index cdff1b7..fd8d5a2 100644 --- a/cbpfc_test.go +++ b/cbpfc_test.go @@ -1410,7 +1410,7 @@ func TestIndirectGuardParentsNotOK(t *testing.T) { addIndirectPacketGuards(blocks, compileOpts{}) matchBlock(t, blocks[0], join( - []instruction{{Instruction: packetGuardIndirect{start: 9, end: 14}}}, + []instruction{{Instruction: packetGuardIndirect{start: 10, end: 14}}}, insns[:2], ), nil) matchBlock(t, blocks[1], join( @@ -1421,7 +1421,12 @@ func TestIndirectGuardParentsNotOK(t *testing.T) { []instruction{{Instruction: packetGuardIndirect{start: 9, end: 15}}}, insns[4:5], ), nil) - matchBlock(t, blocks[3], insns[5:], nil) + // block 3 is reached from block 1 (start 8) and block 2 (start 9). The two + // anchors disagree, so it can't reuse either parent's guard and emits its own. + matchBlock(t, blocks[3], join( + []instruction{{Instruction: packetGuardIndirect{start: 9, end: 10}}}, + insns[5:], + ), nil) } func join(insns ...[]instruction) []instruction { diff --git a/insn_test.go b/insn_test.go index 651717a..fe3cb17 100644 --- a/insn_test.go +++ b/insn_test.go @@ -347,6 +347,49 @@ func TestLoadIndirectGuardOverflow(t *testing.T) { }, nil, noMatch) } +// A block with an indirect load that is reachable from two parents with +// indirect guards that have different starts. +func TestLoadIndirectGuardStartMismatch(t *testing.T) { + t.Parallel() + + filter := []bpf.Instruction{ + // block 0: variable RegX (0 for the test packets), branch on packet[0]. + /* 0 */ bpf.LoadAbsolute{Off: 0, Size: 4}, + /* 1 */ bpf.TAX{}, // RegX = packet[0:4] = 0 + /* 2 */ bpf.LoadAbsolute{Off: 4, Size: 1}, + /* 3 */ bpf.JumpIf{Cond: bpf.JumpEqual, Val: 1, SkipTrue: 0, SkipFalse: 2}, // block 1 or block 2 + + // block 1: indirect guard start 8. + /* 4 */ bpf.LoadIndirect{Off: 8, Size: 4}, // guard [8:12] + /* 5 */ bpf.Jump{Skip: 1}, // jump to block 3 + + // block 2: indirect guard start 13. + /* 6 */ bpf.LoadIndirect{Off: 13, Size: 2}, // guard [13:15] + // fall through to block 3 + + // block 3: indirect load at Off 9 - reached from both parents. + /* 7 */ bpf.LoadIndirect{Off: 9, Size: 1}, + /* 8 */ bpf.JumpIf{Cond: bpf.JumpEqual, Val: 0x42, SkipTrue: 1}, + /* 9 */ bpf.RetConstant{Val: 0}, + /* 10 */ bpf.RetConstant{Val: 1}, + } + + // packet[0:4] = 0 -> RegX == 0, so LoadIndirect offsets are absolute packet offsets. + // packet[9] = 0x42 is the byte block 3 must read to match. + // The other guarded bytes are 0 so a wrong read yields the wrong (noMatch) result. + packet := func(selector byte) []byte { + in := make([]byte, 16) + in[4] = selector // selects block 1 (==1) or block 2 (!=1) + in[9] = 0x42 + return in + } + + // Path through block 1 (guard start 8). + checkBackends(t, filter, packet(1), match) + // Path through block 2 (guard start 13). + checkBackends(t, filter, packet(0), match) +} + // Indirect load with an offset packet pointer. func TestLoadIndirectPacketStartMaxOffset(t *testing.T) { t.Parallel()