Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Martin KaFai Lau <kafai@fb.com>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc
Date: Wed, 2 Mar 2022 15:39:35 -0800	[thread overview]
Message-ID: <CAADnVQ+VpaBy4bcKBwmhyGT70LU+EhvE_o3KOba4NzE7QA=K-A@mail.gmail.com> (raw)
In-Reply-To: <20220302232952.2p7nn5bzqaflftev@apollo.legion>

On Wed, Mar 2, 2022 at 3:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote:
> > On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > fwiw I like patches 1-3.
> > > > I think extra check here for release func is justified on its own.
> > > > Converting it into:
> > > >   fixed_off_ok = false;
> > > >   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > > >           fixed_off_ok = true;
> > > > obfuscates the check to me.
> > >
> > > I was talking of putting this inside check_func_arg_reg_off. I think we should
> > > do the same check for BPF helpers as well (rn only one supports releasing
> > > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to
> > > check_func_arg_reg_off to indicate we are checking for release func (helper or
> > > kfunc have same rules here) would allow putting this check inside it.
> >
> > Hmm. check_func_arg() is called before we call
> > is_release_function(func_id).
> > Are you proposing to call it before and pass
> > another boolean into check_func_arg() or store the flag in meta?
>
> We save meta.func_id before calling check_func_arg. Inside it we can do:
> err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
>
> I actually tried open coding it for BPF helpers, and it was more complicated. If
> we delay this check until is_release_function call after check_func_arg, we need
> to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID.
> If we put it inside check_reg_type or check_func_arg, you need to call
> is_release_function anyway there.
>
> Compared to these two options, doing it in check_func_arg_reg_off looks better
> to me, but ymmv.

I see. Yeah. You're right.
check_func_arg_reg_off(env, reg, regno, arg_type,
  is_release_function(meta->func_id))
is indeed cleaner.