On Wed, May 14, 2025 at 4:35 AM Paul Chaignon <paul.chaignon@xxxxxxxxx> wrote:
>
> Throughout the verifier's logic, there are multiple checks for
> inconsistent states that should never happen and would indicate a
> verifier bug. These bugs are typically logged in the verifier logs and
> sometimes preceded by a WARN_ONCE.
>
> This patch reworks these checks to consistently emit a verifier log AND
> a warning when CONFIG_DEBUG_KERNEL is enabled. The consistent use of
> WARN_ONCE should help fuzzers (ex. syzkaller) expose any situation
> where they are actually able to reach one of those buggy verifier
> states.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@xxxxxxxxx>
> ---
> Changes in v2:
> - Introduce a new BPF_WARN_ONCE macro, with WARN_ONCE conditioned on
> CONFIG_DEBUG_KERNEL, as per reviews.
> - Use the new helper function for verifier bugs missed in v1,
> particularly around backtracking.
>
[...]
> @@ -4277,14 +4274,15 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
> * should be literally next instruction in
> * caller program
> */
> - WARN_ONCE(idx + 1 != subseq_idx, "verifier backtracking bug");
> + if (unlikely(idx + 1 != subseq_idx))
> + verifier_bug(env, "extra insn from subprog");
maybe let's add verifier_buf_if(cond, env, fmt, args...) that would do
if (unlikely(...)) inside? it can also return the result of cond if we
want to do something like
if (verifier_bug_if(<condition>, env, "we are doomed"))
return -EFAULT;
> /* r1-r5 are invalidated after subprog call,
> * so for global func call it shouldn't be set
> * anymore
> */
> if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
> - verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> - WARN_ONCE(1, "verifier backtracking bug");
> + verifier_bug(env, "scratch reg set: regs %x\n",
> + bt_reg_mask(bt));
> return -EFAULT;
but please don't go overboard with verifier_buf_if() for cases like
this, I think this should use plain verifier_bug() as you did, even if
it *can* be expressed with verifier_buf_if() check
> }
> /* global subprog always sets R0 */
> @@ -4298,16 +4296,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
> * the current frame should be zero by now
> */
> if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS) {
> - verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> - WARN_ONCE(1, "verifier backtracking bug");
> + verifier_bug(env, "unexpected precise regs %x\n",
> + bt_reg_mask(bt));
> return -EFAULT;
> }
> /* we are now tracking register spills correctly,
> * so any instance of leftover slots is a bug
> */
> if (bt_stack_mask(bt) != 0) {
> - verbose(env, "BUG stack slots %llx\n", bt_stack_mask(bt));
> - WARN_ONCE(1, "verifier backtracking bug (subprog leftover stack slots)");
> + verifier_bug(env, "subprog leftover stack slots %llx\n",
> + bt_stack_mask(bt));
> return -EFAULT;
> }
> /* propagate r1-r5 to the caller */
[...]
