INTERNAL REVIEW: btfgen v4

.github/actions/patch-kernel/action.yml

@@ -0,0 +1,8 @@
+name: 'patch kernel tree'
+description: 'patch kernel'
+runs:
+  using: "composite"
+  steps:
+    - name: apply temporary patches
+      shell: bash
+      run: ${GITHUB_ACTION_PATH}/patch_kernel.sh

.github/actions/patch-kernel/diffs/0001-bpf-Check-the-other-end-of-slot_type-for-STACK_SPILL.diff

@@ -0,0 +1,142 @@
+From 74eff69d03921c199ab66f905226b59eba412e6e Mon Sep 17 00:00:00 2001
+From: Martin KaFai Lau <[email protected]>
+Date: Tue, 21 Sep 2021 17:49:34 -0700
+Subject: [PATCH 1/2] bpf: Check the other end of slot_type for STACK_SPILL
+
+Every 8 bytes of the stack is tracked by a bpf_stack_state.
+Within each bpf_stack_state, there is a 'u8 slot_type[8]' to track
+the type of each byte.  Verifier tests slot_type[0] == STACK_SPILL
+to decide if the spilled reg state is saved.  Verifier currently only
+saves the reg state if the whole 8 bytes are spilled to the stack,
+so checking the slot_type[7] is the same as checking slot_type[0].
+
+The later patch will allow verifier to save the bounded scalar
+reg also for <8 bytes spill.  There is a llvm patch [1] to ensure
+the <8 bytes spill will be 8-byte aligned,  so checking
+slot_type[7] instead of slot_type[0] is required.
+
+While at it, this patch refactors the slot_type[0] == STACK_SPILL
+test into a new function is_spilled_reg() and change the
+slot_type[0] check to slot_type[7] check in there also.
+
+[1] https://reviews.llvm.org/D109073
+
+Signed-off-by: Martin KaFai Lau <[email protected]>
+Signed-off-by: Alexei Starovoitov <[email protected]>
+Link: https://lore.kernel.org/bpf/[email protected]
+(cherry picked from commit 27113c59b6d0a587b29ae72d4ff3f832f58b0651)
+---
+ kernel/bpf/verifier.c | 30 +++++++++++++++++++-----------
+ 1 file changed, 19 insertions(+), 11 deletions(-)
+
+diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
+index e76b55917905..2ad2a12c5482 100644
+--- a/kernel/bpf/verifier.c
++++ b/kernel/bpf/verifier.c
+@@ -612,6 +612,14 @@ static const char *kernel_type_name(const struct btf* btf, u32 id)
+ 	return btf_name_by_offset(btf, btf_type_by_id(btf, id)->name_off);
+ }
+
++/* The reg state of a pointer or a bounded scalar was saved when
++ * it was spilled to the stack.
++ */
++static bool is_spilled_reg(const struct bpf_stack_state *stack)
++{
++	return stack->slot_type[BPF_REG_SIZE - 1] == STACK_SPILL;
++}
++
+ static void print_verifier_state(struct bpf_verifier_env *env,
+ 				 const struct bpf_func_state *state)
+ {
+@@ -717,7 +725,7 @@ static void print_verifier_state(struct bpf_verifier_env *env,
+ 			continue;
+ 		verbose(env, " fp%d", (-i - 1) * BPF_REG_SIZE);
+ 		print_liveness(env, state->stack[i].spilled_ptr.live);
+-		if (state->stack[i].slot_type[0] == STACK_SPILL) {
++		if (is_spilled_reg(&state->stack[i])) {
+ 			reg = &state->stack[i].spilled_ptr;
+ 			t = reg->type;
+ 			verbose(env, "=%s", reg_type_str[t]);
+@@ -2373,7 +2381,7 @@ static void mark_all_scalars_precise(struct bpf_verifier_env *env,
+ 				reg->precise = true;
+ 			}
+ 			for (j = 0; j < func->allocated_stack / BPF_REG_SIZE; j++) {
+-				if (func->stack[j].slot_type[0] != STACK_SPILL)
++				if (!is_spilled_reg(&func->stack[j]))
+ 					continue;
+ 				reg = &func->stack[j].spilled_ptr;
+ 				if (reg->type != SCALAR_VALUE)
+@@ -2415,7 +2423,7 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int regno,
+ 	}
+
+ 	while (spi >= 0) {
+-		if (func->stack[spi].slot_type[0] != STACK_SPILL) {
++		if (!is_spilled_reg(&func->stack[spi])) {
+ 			stack_mask = 0;
+ 			break;
+ 		}
+@@ -2514,7 +2522,7 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int regno,
+ 				return 0;
+ 			}
+
+-			if (func->stack[i].slot_type[0] != STACK_SPILL) {
++			if (!is_spilled_reg(&func->stack[i])) {
+ 				stack_mask &= ~(1ull << i);
+ 				continue;
+ 			}
+@@ -2713,7 +2721,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
+ 		/* regular write of data into stack destroys any spilled ptr */
+ 		state->stack[spi].spilled_ptr.type = NOT_INIT;
+ 		/* Mark slots as STACK_MISC if they belonged to spilled ptr. */
+-		if (state->stack[spi].slot_type[0] == STACK_SPILL)
++		if (is_spilled_reg(&state->stack[spi]))
+ 			for (i = 0; i < BPF_REG_SIZE; i++)
+ 				state->stack[spi].slot_type[i] = STACK_MISC;
+
+@@ -2923,7 +2931,7 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
+ 	stype = reg_state->stack[spi].slot_type;
+ 	reg = &reg_state->stack[spi].spilled_ptr;
+
+-	if (stype[0] == STACK_SPILL) {
++	if (is_spilled_reg(&reg_state->stack[spi])) {
+ 		if (size != BPF_REG_SIZE) {
+ 			if (reg->type != SCALAR_VALUE) {
+ 				verbose_linfo(env, env->insn_idx, "; ");
+@@ -4514,11 +4522,11 @@ static int check_stack_range_initialized(
+ 			goto mark;
+ 		}
+
+-		if (state->stack[spi].slot_type[0] == STACK_SPILL &&
++		if (is_spilled_reg(&state->stack[spi]) &&
+ 		    state->stack[spi].spilled_ptr.type == PTR_TO_BTF_ID)
+ 			goto mark;
+
+-		if (state->stack[spi].slot_type[0] == STACK_SPILL &&
++		if (is_spilled_reg(&state->stack[spi]) &&
+ 		    (state->stack[spi].spilled_ptr.type == SCALAR_VALUE ||
+ 		     env->allow_ptr_leaks)) {
+ 			if (clobber) {
+@@ -10356,9 +10364,9 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
+ 			 * return false to continue verification of this path
+ 			 */
+ 			return false;
+-		if (i % BPF_REG_SIZE)
++		if (i % BPF_REG_SIZE != BPF_REG_SIZE - 1)
+ 			continue;
+-		if (old->stack[spi].slot_type[0] != STACK_SPILL)
++		if (!is_spilled_reg(&old->stack[spi]))
+ 			continue;
+ 		if (!regsafe(env, &old->stack[spi].spilled_ptr,
+ 			     &cur->stack[spi].spilled_ptr, idmap))
+@@ -10565,7 +10573,7 @@ static int propagate_precision(struct bpf_verifier_env *env,
+ 	}
+
+ 	for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
+-		if (state->stack[i].slot_type[0] != STACK_SPILL)
++		if (!is_spilled_reg(&state->stack[i]))
+ 			continue;
+ 		state_reg = &state->stack[i].spilled_ptr;
+ 		if (state_reg->type != SCALAR_VALUE ||
+-- 
+2.30.2
+

.github/actions/patch-kernel/diffs/0002-bpf-Support-8-byte-scalar-spill-and-refill.diff

@@ -0,0 +1,214 @@
+From f4ef97a9a88a95e1ed2df3aed0fe637a449c1f2d Mon Sep 17 00:00:00 2001
+From: Martin KaFai Lau <[email protected]>
+Date: Tue, 21 Sep 2021 17:49:41 -0700
+Subject: [PATCH 2/2] bpf: Support <8-byte scalar spill and refill
+
+The verifier currently does not save the reg state when
+spilling <8byte bounded scalar to the stack.  The bpf program
+will be incorrectly rejected when this scalar is refilled to
+the reg and then used to offset into a packet header.
+The later patch has a simplified bpf prog from a real use case
+to demonstrate this case.  The current work around is
+to reparse the packet again such that this offset scalar
+is close to where the packet data will be accessed to
+avoid the spill.  Thus, the header is parsed twice.
+
+The llvm patch [1] will align the <8bytes spill to
+the 8-byte stack address.  This can simplify the verifier
+support by avoiding to store multiple reg states for
+each 8 byte stack slot.
+
+This patch changes the verifier to save the reg state when
+spilling <8bytes scalar to the stack.  This reg state saving
+is limited to spill aligned to the 8-byte stack address.
+The current refill logic has already called coerce_reg_to_size(),
+so coerce_reg_to_size() is not called on state->stack[spi].spilled_ptr
+during spill.
+
+When refilling in check_stack_read_fixed_off(),  it checks
+the refill size is the same as the number of bytes marked with
+STACK_SPILL before restoring the reg state.  When restoring
+the reg state to state->regs[dst_regno], it needs
+to avoid the state->regs[dst_regno].subreg_def being
+over written because it has been marked by the check_reg_arg()
+earlier [check_mem_access() is called after check_reg_arg() in
+do_check()].  Reordering check_mem_access() and check_reg_arg()
+will need a lot of changes in test_verifier's tests because
+of the difference in verifier's error message.  Thus, the
+patch here is to save the state->regs[dst_regno].subreg_def
+first in check_stack_read_fixed_off().
+
+There are cases that the verifier needs to scrub the spilled slot
+from STACK_SPILL to STACK_MISC.  After this patch the spill is not always
+in 8 bytes now, so it can no longer assume the other 7 bytes are always
+marked as STACK_SPILL.  In particular, the scrub needs to avoid marking
+an uninitialized byte from STACK_INVALID to STACK_MISC.  Otherwise, the
+verifier will incorrectly accept bpf program reading uninitialized bytes
+from the stack.  A new helper scrub_spilled_slot() is created for this
+purpose.
+
+[1]: https://reviews.llvm.org/D109073
+
+Signed-off-by: Martin KaFai Lau <[email protected]>
+Signed-off-by: Alexei Starovoitov <[email protected]>
+Link: https://lore.kernel.org/bpf/[email protected]
+(cherry picked from commit 354e8f1970f821d4952458f77b1ab6c3eb24d530)
+---
+ kernel/bpf/verifier.c | 67 +++++++++++++++++++++++++++++++++----------
+ 1 file changed, 52 insertions(+), 15 deletions(-)
+
+diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
+index 2ad2a12c5482..7a8351604f67 100644
+--- a/kernel/bpf/verifier.c
++++ b/kernel/bpf/verifier.c
+@@ -620,6 +620,12 @@ static bool is_spilled_reg(const struct bpf_stack_state *stack)
+ 	return stack->slot_type[BPF_REG_SIZE - 1] == STACK_SPILL;
+ }
+
++static void scrub_spilled_slot(u8 *stype)
++{
++	if (*stype != STACK_INVALID)
++		*stype = STACK_MISC;
++}
++
+ static void print_verifier_state(struct bpf_verifier_env *env,
+ 				 const struct bpf_func_state *state)
+ {
+@@ -2634,15 +2640,21 @@ static bool __is_pointer_value(bool allow_ptr_leaks,
+ }
+
+ static void save_register_state(struct bpf_func_state *state,
+-				int spi, struct bpf_reg_state *reg)
++				int spi, struct bpf_reg_state *reg,
++				int size)
+ {
+ 	int i;
+
+ 	state->stack[spi].spilled_ptr = *reg;
+-	state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
++	if (size == BPF_REG_SIZE)
++		state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
+
+-	for (i = 0; i < BPF_REG_SIZE; i++)
+-		state->stack[spi].slot_type[i] = STACK_SPILL;
++	for (i = BPF_REG_SIZE; i > BPF_REG_SIZE - size; i--)
++		state->stack[spi].slot_type[i - 1] = STACK_SPILL;
++
++	/* size < 8 bytes spill */
++	for (; i; i--)
++		scrub_spilled_slot(&state->stack[spi].slot_type[i - 1]);
+ }
+
+ /* check_stack_{read,write}_fixed_off functions track spill/fill of registers,
+@@ -2689,7 +2701,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
+ 			env->insn_aux_data[insn_idx].sanitize_stack_spill = true;
+ 	}
+
+-	if (reg && size == BPF_REG_SIZE && register_is_bounded(reg) &&
++	if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) &&
+ 	    !register_is_null(reg) && env->bpf_capable) {
+ 		if (dst_reg != BPF_REG_FP) {
+ 			/* The backtracking logic can only recognize explicit
+@@ -2702,7 +2714,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
+ 			if (err)
+ 				return err;
+ 		}
+-		save_register_state(state, spi, reg);
++		save_register_state(state, spi, reg, size);
+ 	} else if (reg && is_spillable_regtype(reg->type)) {
+ 		/* register containing pointer is being spilled into stack */
+ 		if (size != BPF_REG_SIZE) {
+@@ -2714,7 +2726,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
+ 			verbose(env, "cannot spill pointers to stack into stack frame of the caller\n");
+ 			return -EINVAL;
+ 		}
+-		save_register_state(state, spi, reg);
++		save_register_state(state, spi, reg, size);
+ 	} else {
+ 		u8 type = STACK_MISC;
+
+@@ -2723,7 +2735,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
+ 		/* Mark slots as STACK_MISC if they belonged to spilled ptr. */
+ 		if (is_spilled_reg(&state->stack[spi]))
+ 			for (i = 0; i < BPF_REG_SIZE; i++)
+-				state->stack[spi].slot_type[i] = STACK_MISC;
++				scrub_spilled_slot(&state->stack[spi].slot_type[i]);
+
+ 		/* only mark the slot as written if all 8 bytes were written
+ 		 * otherwise read propagation may incorrectly stop too soon
+@@ -2926,23 +2938,50 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
+ 	struct bpf_func_state *state = vstate->frame[vstate->curframe];
+ 	int i, slot = -off - 1, spi = slot / BPF_REG_SIZE;
+ 	struct bpf_reg_state *reg;
+-	u8 *stype;
++	u8 *stype, type;
+
+ 	stype = reg_state->stack[spi].slot_type;
+ 	reg = &reg_state->stack[spi].spilled_ptr;
+
+ 	if (is_spilled_reg(&reg_state->stack[spi])) {
+ 		if (size != BPF_REG_SIZE) {
++			u8 scalar_size = 0;
++
+ 			if (reg->type != SCALAR_VALUE) {
+ 				verbose_linfo(env, env->insn_idx, "; ");
+ 				verbose(env, "invalid size of register fill\n");
+ 				return -EACCES;
+ 			}
+-			if (dst_regno >= 0) {
++
++			mark_reg_read(env, reg, reg->parent, REG_LIVE_READ64);
++			if (dst_regno < 0)
++				return 0;
++
++			for (i = BPF_REG_SIZE; i > 0 && stype[i - 1] == STACK_SPILL; i--)
++				scalar_size++;
++
++			if (!(off % BPF_REG_SIZE) && size == scalar_size) {
++				/* The earlier check_reg_arg() has decided the
++				 * subreg_def for this insn.  Save it first.
++				 */
++				s32 subreg_def = state->regs[dst_regno].subreg_def;
++
++				state->regs[dst_regno] = *reg;
++				state->regs[dst_regno].subreg_def = subreg_def;
++			} else {
++				for (i = 0; i < size; i++) {
++					type = stype[(slot - i) % BPF_REG_SIZE];
++					if (type == STACK_SPILL)
++						continue;
++					if (type == STACK_MISC)
++						continue;
++					verbose(env, "invalid read from stack off %d+%d size %d\n",
++						off, i, size);
++					return -EACCES;
++				}
+ 				mark_reg_unknown(env, state->regs, dst_regno);
+-				state->regs[dst_regno].live |= REG_LIVE_WRITTEN;
+ 			}
+-			mark_reg_read(env, reg, reg->parent, REG_LIVE_READ64);
++			state->regs[dst_regno].live |= REG_LIVE_WRITTEN;
+ 			return 0;
+ 		}
+ 		for (i = 1; i < BPF_REG_SIZE; i++) {
+@@ -2973,8 +3012,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
+ 		}
+ 		mark_reg_read(env, reg, reg->parent, REG_LIVE_READ64);
+ 	} else {
+-		u8 type;
+-
+ 		for (i = 0; i < size; i++) {
+ 			type = stype[(slot - i) % BPF_REG_SIZE];
+ 			if (type == STACK_MISC)
+@@ -4532,7 +4569,7 @@ static int check_stack_range_initialized(
+ 			if (clobber) {
+ 				__mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
+ 				for (j = 0; j < BPF_REG_SIZE; j++)
+-					state->stack[spi].slot_type[j] = STACK_MISC;
++					scrub_spilled_slot(&state->stack[spi].slot_type[j]);
+ 			}
+ 			goto mark;
+ 		}
+-- 
+2.30.2
+