Re: [PATCH bpf-next v5 08/11] bpf: Reject register-pair returns when the subprog BTF is unreliable

> btf_check_subprog_call() can decide, at a call site, that this BTF is not
> to be trusted and mark the subprogram unreliable, which happens when
> compiler optimizations remove arguments from a static function or when a
> mismatched type is passed to a global one. Verification carries on, but
> the prototype the return convention was read from is one the verifier has
> already declared not to describe the compiled code.
>
> Rather than keep tracking R2 on the strength of a discarded signature,
> reject a return value larger than 8 bytes as soon as the prototype it was
> derived from becomes unreliable. Add subprog_ret_pair_unreliable() and
> test it in check_func_call() on the path to a static subprogram, which is
> where the flag can be observed while the call still proceeds. For a static
> callee this only triggers on a genuine argument mismatch, since the
> previous patch stopped btf_validate_return_type() from marking a local
> function unreliable for returning an aggregate.
>
> No check is needed anywhere else:
>  - a global subprogram is already rejected by the existing "Caller passes
>    invalid args into func#N" path, because btf_check_subprog_call()
>    returns an error both when it marks the BTF unreliable and on every
>    later call;
>  - the main program does not use the convention at all: its return value
>    is the program's exit code, read out of R0, so nothing looks at R2
>    there;

Does this enumeration cover all call sites? btf_check_subprog_call() has
two callers: check_func_call() and push_callback_call() in verifier.c.

push_callback_call() also swallows every non-EFAULT error:

    err = btf_check_subprog_call(env, subprog, caller->regs);
    if (err == -EFAULT)
            return err;

so a callback subprogram's BTF can be marked unreliable while the call
still proceeds, exactly the situation described above for the static
check_func_call() path. The two bullets given (global subprogram, main
program) do not cover it.

It interacts with the sticky nature of the flag: a subprogram marked
unreliable by a callback call site will subsequently be rejected by the
new check at an unrelated direct call site whose own arguments were fine,
since btf_check_subprog_call() returns -EINVAL for every later call once
the flag is set (the early return in btf_check_subprog_call()).


> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b23be0fa95af..0ffb3bed1649 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -441,6 +441,23 @@ static void bpf_compute_subprog_ret_regs(struct bpf_verifier_env *env)
>  	}
>  }
>
> +/*
> + * A >8 byte BPF return changes the calling convention to R0:R2, and the
> + * verifier derives that convention from the subprogram's BTF prototype
> + * alone. Once that prototype is marked unreliable it is known not to
> + * describe the compiled code, so the convention read from it cannot be
> + * trusted either: reject the call rather than keep tracking R2 on the
> + * strength of a signature the verifier has already discarded.
> + */
> +static bool subprog_ret_pair_unreliable(struct bpf_verifier_env *env, int subprog)
> +{
> +	struct bpf_prog_aux *aux = env->prog->aux;
> +
> +	return bpf_ret_reg_pair(env, subprog) &&
> +	       aux->func_info_aux &&
> +	       aux->func_info_aux[subprog].unreliable;
> +}
> +
[ ... ]
> @@ -9527,6 +9544,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		return 0;
>  	}
>
> +	if (subprog_ret_pair_unreliable(env, subprog)) {
> +		verbose(env, "Func#%d ('%s') returns >8 bytes, which requires reliable BTF\n",
> +			subprog, subprog_name(env, subprog));
> +		return -EINVAL;
> +	}
> +

Can this gate reject valid programs? For a static callee, func_info_aux[]
.unreliable is set for reasons that have nothing to do with an argument
mismatch.

The path is: do_check_insn() calls check_func_call(), which calls
btf_check_subprog_call(), which calls btf_check_func_arg_match().

btf_check_func_arg_match() in kernel/bpf/verifier.c starts with:

    ret = btf_prepare_func_args(env, subprog);
    if (ret) {
            if (bpf_in_stack_arg_cnt(sub) > 0) ...
            return ret;
    }

Then btf_check_subprog_call() does unconditionally on any non-zero err:

    if (err)
            prog->aux->func_info_aux[subprog].unreliable = true;

btf_prepare_func_args() returns -EINVAL for a non-global subprogram in the
ordinary case of an untagged pointer argument. In the argument loop
(kernel/bpf/btf.c), a pointer arg that is not the prog ctx type, not a
dynptr, and carries no __arg_trusted/__arg_untrusted/__arg_arena tag skips
the generic-user-data-pointer branch and falls through to:

  skip_pointer:
        if (tags) { ... return -EINVAL; }
        if (btf_type_is_int(t) || btf_is_any_enum(t)) { ... }
        if (!is_global)
                return -EINVAL;

A by-value struct argument takes the same exit. This is expected,
documented behaviour for static callees since btf_prepare_func_args() even
comments "not fatal for static funcs" at kernel/bpf/btf.c, because a
static subprogram is verified inline so the verifier never needs a usable
BTF argument model for it.

Concrete example that verifies fine at the parent commit but is now
rejected:

    struct pair { __u64 a, b; };

    static __noinline struct pair make_pair(__u64 *p)
    {
            struct pair r = { *p, 1 };
            return r;
    }

    SEC("tc") int prog(struct __sk_buff *ctx)
    {
            __u64 v = 1;
            struct pair p = make_pair(&v);
            return p.a + p.b;
    }

This program is rejected: "Func#1 ('make_pair') returns >8 bytes, which
requires reliable BTF".

The flow is: bpf_compute_subprog_ret_regs() sees struct pair, size 16,
and sets ret_reg_pair = true. btf_validate_return_type() accepts it via
the local_func path. The argument loop then returns -EINVAL for __u64 *p,
setting unreliable = true. The new check fires.

The rejection is also unnecessary for soundness on this path: a static
callee is verified inline, so prepare_func_exit() copies the callee's
actual tracked R0 and R2 into the caller. R2 tracking does not rest on the
BTF prototype the way it does for a global callee.

The commit message premise appears incorrect: "For a static callee this
only triggers on a genuine argument mismatch, since the previous patch
stopped btf_validate_return_type() from marking a local function
unreliable for returning an aggregate." Relaxing btf_validate_return_type()
removed only one of two independent -EINVAL exits inside
btf_prepare_func_args(); the argument-conversion exit at kernel/bpf/btf.c
is untouched and is the common one. The new comment in verifier.c encodes
the same premise: "Once that prototype is marked unreliable it is known
not to describe the compiled code" - but the flag also means merely "the
verifier cannot express these argument types".

Should the gate be restricted to cases where the return convention itself
is untrustworthy, rather than whenever any argument cannot be expressed in
BTF?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31740414277