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 ce7ee68 "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.

Author: arthurfabre

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
 

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
 	}

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 {

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()