diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 8355b585cd18..746025df82c8 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -697,8 +697,11 @@ struct bpf_idmap { }; struct bpf_idset { - u32 count; - u32 ids[BPF_ID_MAP_SIZE]; + u32 num_ids; + struct { + u32 id; + u32 cnt; + } entries[BPF_ID_MAP_SIZE]; }; /* see verifier.c:compute_scc_callchain() */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6b62b6d57175..da03bbbc1620 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5518,6 +5518,12 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, */ s32 subreg_def = state->regs[dst_regno].subreg_def; + if (env->bpf_capable && size == 4 && spill_size == 4 && + get_reg_width(reg) <= 32) + /* Ensure stack slot has an ID to build a relation + * with the destination register on fill. + */ + assign_scalar_id_before_mov(env, reg); copy_register_state(&state->regs[dst_regno], reg); state->regs[dst_regno].subreg_def = subreg_def; @@ -5563,6 +5569,11 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, } } else if (dst_regno >= 0) { /* restore register state from stack */ + if (env->bpf_capable) + /* Ensure stack slot has an ID to build a relation + * with the destination register on fill. + */ + assign_scalar_id_before_mov(env, reg); copy_register_state(&state->regs[dst_regno], reg); /* mark reg as written since spilled pointer state likely * has its liveness marks cleared by is_state_visited() @@ -8984,15 +8995,24 @@ static bool regs_exact(const struct bpf_reg_state *rold, const struct bpf_reg_state *rcur, struct bpf_idmap *idmap); +/* + * Check if scalar registers are exact for the purpose of not widening. + * More lenient than regs_exact() + */ +static bool scalars_exact_for_widen(const struct bpf_reg_state *rold, + const struct bpf_reg_state *rcur) +{ + return !memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)); +} + static void maybe_widen_reg(struct bpf_verifier_env *env, - struct bpf_reg_state *rold, struct bpf_reg_state *rcur, - struct bpf_idmap *idmap) + struct bpf_reg_state *rold, struct bpf_reg_state *rcur) { if (rold->type != SCALAR_VALUE) return; if (rold->type != rcur->type) return; - if (rold->precise || rcur->precise || regs_exact(rold, rcur, idmap)) + if (rold->precise || rcur->precise || scalars_exact_for_widen(rold, rcur)) return; __mark_reg_unknown(env, rcur); } @@ -9004,7 +9024,6 @@ static int widen_imprecise_scalars(struct bpf_verifier_env *env, struct bpf_func_state *fold, *fcur; int i, fr, num_slots; - reset_idmap_scratch(env); for (fr = old->curframe; fr >= 0; fr--) { fold = old->frame[fr]; fcur = cur->frame[fr]; @@ -9012,8 +9031,7 @@ static int widen_imprecise_scalars(struct bpf_verifier_env *env, for (i = 0; i < MAX_BPF_REG; i++) maybe_widen_reg(env, &fold->regs[i], - &fcur->regs[i], - &env->idmap_scratch); + &fcur->regs[i]); num_slots = min(fold->allocated_stack / BPF_REG_SIZE, fcur->allocated_stack / BPF_REG_SIZE); @@ -9024,8 +9042,7 @@ static int widen_imprecise_scalars(struct bpf_verifier_env *env, maybe_widen_reg(env, &fold->stack[i].spilled_ptr, - &fcur->stack[i].spilled_ptr, - &env->idmap_scratch); + &fcur->stack[i].spilled_ptr); } } return 0; @@ -19370,13 +19387,29 @@ static bool check_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap) return false; } -/* Similar to check_ids(), but allocate a unique temporary ID - * for 'old_id' or 'cur_id' of zero. - * This makes pairs like '0 vs unique ID', 'unique ID vs 0' valid. +/* + * Compare scalar register IDs for state equivalence. + * + * When old_id == 0, the old register is independent - not linked to any + * other register. Any linking in the current state only adds constraints, + * making it more restrictive. Since the old state didn't rely on any ID + * relationships for this register, it's always safe to accept cur regardless + * of its ID. Hence, return true immediately. + * + * When old_id != 0 but cur_id == 0, we need to ensure that different + * independent registers in cur don't incorrectly satisfy the ID matching + * requirements of linked registers in old. + * + * Example: if old has r6.id=X and r7.id=X (linked), but cur has r6.id=0 + * and r7.id=0 (both independent), without temp IDs both would map old_id=X + * to cur_id=0 and pass. With temp IDs: r6 maps X->temp1, r7 tries to map + * X->temp2, but X is already mapped to temp1, so the check fails correctly. */ static bool check_scalar_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap) { - old_id = old_id ? old_id : ++idmap->tmp_id_gen; + if (!old_id) + return true; + cur_id = cur_id ? cur_id : ++idmap->tmp_id_gen; return check_ids(old_id, cur_id, idmap); @@ -19450,6 +19483,72 @@ static void clean_verifier_state(struct bpf_verifier_env *env, * doesn't meant that the states are DONE. The verifier has to compare * the callsites */ + +/* Find id in idset and increment its count, or add new entry */ +static void idset_cnt_inc(struct bpf_idset *idset, u32 id) +{ + u32 i; + + for (i = 0; i < idset->num_ids; i++) { + if (idset->entries[i].id == id) { + idset->entries[i].cnt++; + return; + } + } + /* New id */ + if (idset->num_ids < BPF_ID_MAP_SIZE) { + idset->entries[idset->num_ids].id = id; + idset->entries[idset->num_ids].cnt = 1; + idset->num_ids++; + } +} + +/* Find id in idset and return its count, or 0 if not found */ +static u32 idset_cnt_get(struct bpf_idset *idset, u32 id) +{ + u32 i; + + for (i = 0; i < idset->num_ids; i++) { + if (idset->entries[i].id == id) + return idset->entries[i].cnt; + } + return 0; +} + +/* + * Clear singular scalar ids in a state. + * A register with a non-zero id is called singular if no other register shares + * the same base id. Such registers can be treated as independent (id=0). + */ +static void clear_singular_ids(struct bpf_verifier_env *env, + struct bpf_verifier_state *st) +{ + struct bpf_idset *idset = &env->idset_scratch; + struct bpf_func_state *func; + struct bpf_reg_state *reg; + + idset->num_ids = 0; + + bpf_for_each_reg_in_vstate(st, func, reg, ({ + if (reg->type != SCALAR_VALUE) + continue; + if (!reg->id) + continue; + idset_cnt_inc(idset, reg->id & ~BPF_ADD_CONST); + })); + + bpf_for_each_reg_in_vstate(st, func, reg, ({ + if (reg->type != SCALAR_VALUE) + continue; + if (!reg->id) + continue; + if (idset_cnt_get(idset, reg->id & ~BPF_ADD_CONST) == 1) { + reg->id = 0; + reg->off = 0; + } + })); +} + static void clean_live_states(struct bpf_verifier_env *env, int insn, struct bpf_verifier_state *cur) { @@ -19535,11 +19634,21 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold, } if (!rold->precise && exact == NOT_EXACT) return true; - if ((rold->id & BPF_ADD_CONST) != (rcur->id & BPF_ADD_CONST)) - return false; - if ((rold->id & BPF_ADD_CONST) && (rold->off != rcur->off)) - return false; - /* Why check_ids() for scalar registers? + /* + * Linked register tracking uses rold->id to detect relationships. + * When rold->id == 0, the register is independent and any linking + * in rcur only adds constraints. When rold->id != 0, we must verify + * id mapping and (for BPF_ADD_CONST) offset consistency. + * + * +------------------+-----------+------------------+---------------+ + * | | rold->id | rold + ADD_CONST | rold->id == 0 | + * |------------------+-----------+------------------+---------------| + * | rcur->id | range,ids | false | range | + * | rcur + ADD_CONST | false | range,ids,off | range | + * | rcur->id == 0 | range,ids | false | range | + * +------------------+-----------+------------------+---------------+ + * + * Why check_ids() for scalar registers? * * Consider the following BPF code: * 1: r6 = ... unbound scalar, ID=a ... @@ -19563,9 +19672,22 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold, * --- * Also verify that new value satisfies old value range knowledge. */ - return range_within(rold, rcur) && - tnum_in(rold->var_off, rcur->var_off) && - check_scalar_ids(rold->id, rcur->id, idmap); + + /* ADD_CONST mismatch: different linking semantics */ + if ((rold->id & BPF_ADD_CONST) && !(rcur->id & BPF_ADD_CONST)) + return false; + + if (rold->id && !(rold->id & BPF_ADD_CONST) && (rcur->id & BPF_ADD_CONST)) + return false; + + /* Both have offset linkage: offsets must match */ + if ((rold->id & BPF_ADD_CONST) && rold->off != rcur->off) + return false; + + if (!check_scalar_ids(rold->id, rcur->id, idmap)) + return false; + + return range_within(rold, rcur) && tnum_in(rold->var_off, rcur->var_off); case PTR_TO_MAP_KEY: case PTR_TO_MAP_VALUE: case PTR_TO_MEM: @@ -20448,6 +20570,8 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx) if (env->bpf_capable) mark_all_scalars_imprecise(env, cur); + clear_singular_ids(env, cur); + /* add new state to the head of linked list */ new = &new_sl->state; err = copy_verifier_state(new, cur); diff --git a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c index c0ce690ddb68..3072fee9a448 100644 --- a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c +++ b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c @@ -715,6 +715,51 @@ __naked void ignore_unique_scalar_ids_old(void) : __clobber_all); } +/* Check that two registers with 0 scalar IDs in a verified state can be mapped + * to the same scalar ID in current state. + */ +SEC("socket") +__success __log_level(2) +/* The states should be equivalent on reaching insn 12. + */ +__msg("12: safe") +__msg("processed 17 insns") +__flag(BPF_F_TEST_STATE_FREQ) +__naked void two_nil_old_ids_one_cur_id(void) +{ + asm volatile ( + /* Give unique scalar IDs to r{6,7} */ + "call %[bpf_ktime_get_ns];" + "r0 &= 0xff;" + "r6 = r0;" + "r6 *= 1;" + "call %[bpf_ktime_get_ns];" + "r0 &= 0xff;" + "r7 = r0;" + "r7 *= 1;" + "r0 = 0;" + /* Maybe make r{6,7} IDs identical */ + "if r6 > r7 goto l0_%=;" + "goto l1_%=;" +"l0_%=:" + "r6 = r7;" +"l1_%=:" + /* Mark r{6,7} precise. + * Get here in two states: + * - first: r6{.id=0}, r7{.id=0} (cached state) + * - second: r6{.id=A}, r7{.id=A} + * Verifier considers such states equivalent. + * Thus "exit;" would be verified only once. + */ + "r2 = r10;" + "r2 += r6;" + "r2 += r7;" + "exit;" + : + : __imm(bpf_ktime_get_ns) + : __clobber_all); +} + /* Check that two different scalar IDs in a verified state can't be * mapped to the same scalar ID in current state. */ @@ -723,9 +768,9 @@ __success __log_level(2) /* The exit instruction should be reachable from two states, * use two matches and "processed .. insns" to ensure this. */ -__msg("13: (95) exit") -__msg("13: (95) exit") -__msg("processed 18 insns") +__msg("15: (95) exit") +__msg("15: (95) exit") +__msg("processed 20 insns") __flag(BPF_F_TEST_STATE_FREQ) __naked void two_old_ids_one_cur_id(void) { @@ -734,9 +779,11 @@ __naked void two_old_ids_one_cur_id(void) "call %[bpf_ktime_get_ns];" "r0 &= 0xff;" "r6 = r0;" + "r8 = r0;" "call %[bpf_ktime_get_ns];" "r0 &= 0xff;" "r7 = r0;" + "r9 = r0;" "r0 = 0;" /* Maybe make r{6,7} IDs identical */ "if r6 > r7 goto l0_%=;"