From 41b40f0eac90c8cf413a130e276060f53968a045 Mon Sep 17 00:00:00 2001 From: Arthur Fabre Date: Thu, 27 Aug 2026 10:29:08 +0200 Subject: [PATCH] Emit new indirect guards when parents have different starts If a block has two parents, that each make a LoadIndirect with a different start, we wouldn't always emit a new guard if the block did a LoadIndirect itself: if the start was greater, we'd consider it covered under the parents' guards. But as of commit ce7ee68ade2 "Validate variable packet offsets, support negative offsets", we store the packet pointer plus the start offset in a register. If the guards have different starts, this register will therefore have a different value. We can't naively use it in the child block. Emit new guards (to load a new value in the register) when the start is different. For the C backend: move the asm volatile constraint just after calculating indirect, so clang can't assume it's value and reuse it between guards. Switch indirect from being a input, to an input / output so clang can't know it's value. --- c.go | 6 +++--- cbpfc.go | 10 ++++++---- cbpfc_test.go | 9 +++++++-- insn_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 9 deletions(-) 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()