On Wed, Dec 13, 2023 at 10:14 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2023-12-13 at 10:06 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > > }
> > > > }
> > > > } else {
> > > > + /* if main BPF program has associated BTF info, validate that
> > > > + * it's matching expected signature, and otherwise mark BTF
> > > > + * info for main program as unreliable
> > > > + */
> > > > + if (env->prog->aux->func_info_aux) {
> > > > + ret = btf_prepare_func_args(env, 0);
> > > > + if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > > > + env->prog->aux->func_info_aux[0].unreliable = true;
> > > > + }
> > >
> > > Nit: should this return if ret == -EFAULT?
> > >
> > >
> >
> > no, why? I think the old behavior also didn't fail in this case
>
> I think it did, here is an excerpt from the current patch:
Ah, sorry, you meant exit if -EFAULT is returned, not on any error.
Yes, that was old behavior, but I don't think those conditions can
ever happen because if func_info_aux is non-null, then we successfully
passed check_btf_func_early() and check_btf_func() checks, which will
fail early if those conditions are not satisfied.
So instead of a scary-looking check, I figured it's simpler to just
mark BTF unreliable and move on with the rest of the logic. The whole
idea of this check is to do basically optional BTF check, but
otherwise ignore BTF if it doesn't match our expectation.
>
> - ret = btf_check_subprog_arg_match(env, subprog, regs);
> - if (ret == -EFAULT)
> - /* unlikely verifier bug. abort.
> - * ret == 0 and ret < 0 are sadly acceptable for
> - * main() function due to backward compatibility.
> - * Like socket filter program may be written as:
> - * int bpf_prog(struct pt_regs *ctx)
> - * and never dereference that ctx in the program.
> - * 'struct pt_regs' is a type mismatch for socket
> - * filter that should be using 'struct __sk_buff'.
> - */
> - goto out;
