On Tue, Oct 22, 2024 at 1:27 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Oct 22, 2024 at 10:40 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 22, 2024 at 12:36 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> > > > On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> > > >> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> > > >>> From: Hou Tao <houtao1@xxxxxxxxxx>
> > > >>>
> > > >>> If a corresponding link type doesn't invoke BPF_LINK_TYPE(), accessing
> > > >>> bpf_link_type_strs[link->type] may result in out-of-bound access.
> > > >>>
> > > >>> To prevent such missed invocations in the future, the following static
> > > >>> assertion seems feasible:
> > > >>>
> > > >>> BUILD_BUG_ON(ARRAY_SIZE(bpf_link_type_strs) != __MAX_BPF_LINK_TYPE)
> > > >>>
> > > >>> However, this doesn't work well. The reason is that the invocation of
> > > >>> BPF_LINK_TYPE() for one link type is optional due to its CONFIG_XXX
> > > >>> dependency and the elements in bpf_link_type_strs[] will be sparse. For
> > > >>> example, if CONFIG_NET is disabled, the size of bpf_link_type_strs will
> > > >>> be BPF_LINK_TYPE_UPROBE_MULTI + 1.
> > > >>>
> > > >>> Therefore, in addition to the static assertion, remove all CONFIG_XXX
> > > >>> conditions for the invocation of BPF_LINK_TYPE(). If these CONFIG_XXX
> > > >>> conditions become necessary later, the fix may need to be revised (e.g.,
> > > >>> to check the validity of link_type in bpf_link_show_fdinfo()).
> > > >>>
> > > >>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> > > >>> ---
> > > >>> include/linux/bpf_types.h | 6 ------
> > > >>> kernel/bpf/syscall.c | 2 ++
> > > >>> 2 files changed, 2 insertions(+), 6 deletions(-)
> > > >>>
> > > >>> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > > >>> index fa78f49d4a9a..6b7eabe9a115 100644
> > > >>> --- a/include/linux/bpf_types.h
> > > >>> +++ b/include/linux/bpf_types.h
> > > >>> @@ -136,21 +136,15 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_ARENA, arena_map_ops)
> > > >>>
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_TRACING, tracing)
> > > >>> -#ifdef CONFIG_CGROUP_BPF
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_CGROUP, cgroup)
> > > >>> -#endif
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
> > > >>> -#ifdef CONFIG_NET
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_XDP, xdp)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_NETFILTER, netfilter)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_TCX, tcx)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_NETKIT, netkit)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_SOCKMAP, sockmap)
> > > >>> -#endif
> > > >>> -#ifdef CONFIG_PERF_EVENTS
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
> > > >>> -#endif
> > > > I'm not sure what's the implication here, but I'd avoid doing that.
> > > > But see below.
> > >
I'll just elaborate a bit why I wouldn't remove #ifdef guards. This
BPF_LINK_TYPE() macro magic can be used to define some extra data
structures that are specific to link type. E.g., some sort of
bpf_<type>_link_lops references or something along those lines. Having
BPF_LINK_TYPE() definition when the kernel actually doesn't implement
that link will be PITA in that case, generating references to
non-existent data structures.
> > > OK.
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_KPROBE_MULTI, kprobe_multi)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_STRUCT_OPS, struct_ops)
> > > >>> BPF_LINK_TYPE(BPF_LINK_TYPE_UPROBE_MULTI, uprobe_multi)
> > > >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > >>> index 8cfa7183d2ef..9f335c379b05 100644
> > > >>> --- a/kernel/bpf/syscall.c
> > > >>> +++ b/kernel/bpf/syscall.c
> > > >>> @@ -3071,6 +3071,8 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
> > > >>> const struct bpf_prog *prog = link->prog;
> > > >>> char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
> > > >>>
> > > >>> + BUILD_BUG_ON(ARRAY_SIZE(bpf_link_type_strs) != __MAX_BPF_LINK_TYPE);
> > > > If this is useless, why are you adding it?
> > >
> > > It will work after removing these CONFIG_XXX dependencies for
> > > BPF_LINK_TYPE() invocations.
> > > >
> > > > Let's instead do a NULL check inside bpf_link_show_fdinfo() to handle
> > > > sparsity. And to avoid out-of-bounds, just add
> > > >
> > > > [__MAX_BPF_LINK_TYPE] = NULL,
> > > >
> > > > into the definition of bpf_link_type_strs
> > >
> > > Instead of outputting a null string for a link_type which didn't invoke
> > > BPF_LINK_TYPE, is outputting the numerical value of link->type more
> > > reasonable as shown below ?
> >
> > In correctly configured kernel this should never happen. So we can
> > have WARN() there for the NULL case and just return an error or
Actually, it seems like this is a void-returning function, so yeah,
instead of returning an error we can just emit an integer value. But
we should definitely have a WARN_ONCE().
> > something.
>
> I don't understand why this patch is needed.
> Is it solving a theoretical problem ?
>
> Something like the kernel managed to create a link
> with link->type == BPF_LINK_TYPE_CGROUP,
> but CONFIG_CGROUP_BPF was not defined somehow ?
>
It's just too easy to forget to add
BPF_LINK_TYPE(BPF_LINK_TYPE_<newlinktype>, ...) into
include/linux/bpf_types.h when adding a new type of BPF link. So Hou
is following up with changes that will make it easier to spot these
omissions in the future.
> There is no out-of-bounds or access to empty
> bpf_link_type_strs[link->type] as far as I can tell.
>
> What am I missing?
