diff mbox series

[bpf,v2,2/7] bpf: Add assertion for the size of bpf_link_type_strs[]

Message ID 20241021014004.1647816-3-houtao@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Misc fixes for bpf | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 213 this patch: 213
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/build_clang success Errors and warnings before: 272 this patch: 272
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6964 this patch: 6964
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 fail Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-14 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-13 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 fail Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-30 fail Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 fail Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-37 fail Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 fail Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 fail Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Hou Tao Oct. 21, 2024, 1:39 a.m. UTC 
From: Hou Tao <houtao1@huawei.com>

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@huawei.com>
---
 include/linux/bpf_types.h | 6 ------
 kernel/bpf/syscall.c      | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

Comments

Jiri Olsa Oct. 21, 2024, 8:18 a.m. UTC | #1 
On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> 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@huawei.com>
> ---
>  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
>  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);

I wonder it'd be simpler to just kill BPF_LINK_TYPE completely
and add link names directly to bpf_link_type_strs array..
it seems it's the only purpose of the BPF_LINK_TYPE macro

jirka
Andrii Nakryiko Oct. 21, 2024, 11:02 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> > From: Hou Tao <houtao1@huawei.com>
> >
> > 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@huawei.com>
> > ---
> >  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.

> >  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?

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

pw-bot: cr

>
> I wonder it'd be simpler to just kill BPF_LINK_TYPE completely
> and add link names directly to bpf_link_type_strs array..
> it seems it's the only purpose of the BPF_LINK_TYPE macro
>

This seems like a bit too short-sighted approach, let's not go there just yet.

> jirka
Hou Tao Oct. 22, 2024, 7:35 a.m. UTC | #3 
Hi,

On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> 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@huawei.com>
>>> ---
>>>  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.

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 ?

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2873302faf39..9a02cd914ed8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3073,14 +3073,15 @@ static void bpf_link_show_fdinfo(struct seq_file
*m, struct file *filp)
        const struct bpf_link *link = filp->private_data;
        const struct bpf_prog *prog = link->prog;
        char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
+       enum bpf_link_type type;

+       if (type < ARRAY_SIZE(bpf_link_type_strs) &&
bpf_link_type_strs[type])
+               seq_printf(m, "link_type:\t%s\n", bpf_link_type_strs[type]);
+       else
+               seq_printf(m, "link_type:\t[%d]\n", type);
+
+       seq_printf(m, "link_id:\t%u\n", link->id);

-       seq_printf(m,
-                  "link_type:\t%s\n"
-                  "link_id:\t%u\n",
-                  bpf_link_type_strs[link->type],
-                  link->id);

>
> pw-bot: cr
>
>> I wonder it'd be simpler to just kill BPF_LINK_TYPE completely
>> and add link names directly to bpf_link_type_strs array..
>> it seems it's the only purpose of the BPF_LINK_TYPE macro
>>
> This seems like a bit too short-sighted approach, let's not go there just yet.
>
>> jirka
Andrii Nakryiko Oct. 22, 2024, 5:40 p.m. UTC | #4 
On Tue, Oct 22, 2024 at 12:36 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> > On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> >>> From: Hou Tao <houtao1@huawei.com>
> >>>
> >>> 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@huawei.com>
> >>> ---
> >>>  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.
>
> 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
something.

>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 2873302faf39..9a02cd914ed8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3073,14 +3073,15 @@ static void bpf_link_show_fdinfo(struct seq_file
> *m, struct file *filp)
>         const struct bpf_link *link = filp->private_data;
>         const struct bpf_prog *prog = link->prog;
>         char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
> +       enum bpf_link_type type;
>
> +       if (type < ARRAY_SIZE(bpf_link_type_strs) &&
> bpf_link_type_strs[type])
> +               seq_printf(m, "link_type:\t%s\n", bpf_link_type_strs[type]);
> +       else
> +               seq_printf(m, "link_type:\t[%d]\n", type);
> +
> +       seq_printf(m, "link_id:\t%u\n", link->id);
>
> -       seq_printf(m,
> -                  "link_type:\t%s\n"
> -                  "link_id:\t%u\n",
> -                  bpf_link_type_strs[link->type],
> -                  link->id);
>
> >
> > pw-bot: cr
> >
> >> I wonder it'd be simpler to just kill BPF_LINK_TYPE completely
> >> and add link names directly to bpf_link_type_strs array..
> >> it seems it's the only purpose of the BPF_LINK_TYPE macro
> >>
> > This seems like a bit too short-sighted approach, let's not go there just yet.
> >
> >> jirka
>
Alexei Starovoitov Oct. 22, 2024, 8:26 p.m. UTC | #5 
On Tue, Oct 22, 2024 at 10:40 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 12:36 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >
> > Hi,
> >
> > On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> > > On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> > >>> From: Hou Tao <houtao1@huawei.com>
> > >>>
> > >>> 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@huawei.com>
> > >>> ---
> > >>>  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.
> >
> > 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
> 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 ?

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?
Andrii Nakryiko Oct. 22, 2024, 8:41 p.m. UTC | #6 
On Tue, Oct 22, 2024 at 1:27 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 10:40 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2024 at 12:36 AM Hou Tao <houtao@huaweicloud.com> wrote:
> > >
> > > Hi,
> > >
> > > On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> > > > On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> > > >>> From: Hou Tao <houtao1@huawei.com>
> > > >>>
> > > >>> 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@huawei.com>
> > > >>> ---
> > > >>>  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?
diff mbox series

Patch

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
 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);
+
 	seq_printf(m,
 		   "link_type:\t%s\n"
 		   "link_id:\t%u\n",