* [PATCH bpf-next 1/2] selftests/bpf: Test if bpftool linker handles empty sections
2023-03-28 0:47 [PATCH bpf-next 0/2] Fix double-free when linker processes empty sections Eduard Zingerman
@ 2023-03-28 0:47 ` Eduard Zingerman
2023-03-28 3:04 ` Andrii Nakryiko
2023-03-28 0:47 ` [PATCH bpf-next 2/2] libbpf: Fix double-free when linker processes " Eduard Zingerman
2023-03-28 3:10 ` [PATCH bpf-next 0/2] " patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2023-03-28 0:47 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yhs, james.hilliard1,
Eduard Zingerman
Adds two empty functions to linked_funcs[12].c. The functions are
annotated as "naked" and go to a separate section. This section ends
up having size 0. bpftool linker merges content for sections with
identical names. This tests if it can handle empty sections.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/progs/linked_funcs1.c | 3 +++
tools/testing/selftests/bpf/progs/linked_funcs2.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c
index c4b49ceea967..029bb5022ba2 100644
--- a/tools/testing/selftests/bpf/progs/linked_funcs1.c
+++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c
@@ -86,4 +86,7 @@ int BPF_PROG(handler1, struct pt_regs *regs, long id)
return 0;
}
+SEC(".empty_section")
+__naked void empty_function1(void) {}
+
char LICENSE[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/linked_funcs2.c b/tools/testing/selftests/bpf/progs/linked_funcs2.c
index 013ff0645f0c..4547c8dfc689 100644
--- a/tools/testing/selftests/bpf/progs/linked_funcs2.c
+++ b/tools/testing/selftests/bpf/progs/linked_funcs2.c
@@ -86,4 +86,7 @@ int BPF_PROG(handler2, struct pt_regs *regs, long id)
return 0;
}
+SEC(".empty_section")
+__naked void empty_function2(void) {}
+
char LICENSE[] SEC("license") = "GPL";
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 1/2] selftests/bpf: Test if bpftool linker handles empty sections
2023-03-28 0:47 ` [PATCH bpf-next 1/2] selftests/bpf: Test if bpftool linker handles " Eduard Zingerman
@ 2023-03-28 3:04 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2023-03-28 3:04 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yhs,
james.hilliard1
On Mon, Mar 27, 2023 at 5:47 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Adds two empty functions to linked_funcs[12].c. The functions are
> annotated as "naked" and go to a separate section. This section ends
> up having size 0. bpftool linker merges content for sections with
> identical names. This tests if it can handle empty sections.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/testing/selftests/bpf/progs/linked_funcs1.c | 3 +++
> tools/testing/selftests/bpf/progs/linked_funcs2.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c
> index c4b49ceea967..029bb5022ba2 100644
> --- a/tools/testing/selftests/bpf/progs/linked_funcs1.c
> +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c
> @@ -86,4 +86,7 @@ int BPF_PROG(handler1, struct pt_regs *regs, long id)
> return 0;
> }
>
> +SEC(".empty_section")
> +__naked void empty_function1(void) {}
> +
> char LICENSE[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs2.c b/tools/testing/selftests/bpf/progs/linked_funcs2.c
> index 013ff0645f0c..4547c8dfc689 100644
> --- a/tools/testing/selftests/bpf/progs/linked_funcs2.c
> +++ b/tools/testing/selftests/bpf/progs/linked_funcs2.c
> @@ -86,4 +86,7 @@ int BPF_PROG(handler2, struct pt_regs *regs, long id)
> return 0;
> }
>
> +SEC(".empty_section")
> +__naked void empty_function2(void) {}
These empty section functions make this whole BPF object file invalid
from libbpf's standpoint. It didn't feel worth it to add this
confusion just to test this edge case in realloc() handling. So I
dropped this patch and only applied libbpf fix. Pushed to bpf-next,
thanks!
> +
> char LICENSE[] SEC("license") = "GPL";
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] libbpf: Fix double-free when linker processes empty sections
2023-03-28 0:47 [PATCH bpf-next 0/2] Fix double-free when linker processes empty sections Eduard Zingerman
2023-03-28 0:47 ` [PATCH bpf-next 1/2] selftests/bpf: Test if bpftool linker handles " Eduard Zingerman
@ 2023-03-28 0:47 ` Eduard Zingerman
2023-03-28 2:18 ` James Hilliard
2023-03-28 3:10 ` [PATCH bpf-next 0/2] " patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2023-03-28 0:47 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yhs, james.hilliard1,
Eduard Zingerman
Double-free error in bpf_linker__free() was reported by James Hilliard.
The error is caused by miss-use of realloc() in extend_sec().
The error occurs when two files with empty sections of the same name
are linked:
- when first file is processed:
- extend_sec() calls realloc(dst->raw_data, dst_align_sz)
with dst->raw_data == NULL and dst_align_sz == 0;
- dst->raw_data is set to a special pointer to a memory block of
size zero;
- when second file is processed:
- extend_sec() calls realloc(dst->raw_data, dst_align_sz)
with dst->raw_data == <special pointer> and dst_align_sz == 0;
- realloc() "frees" dst->raw_data special pointer and returns NULL;
- extend_sec() exits with -ENOMEM, and the old dst->raw_data value
is preserved (it is now invalid);
- eventually, bpf_linker__free() attempts to free dst->raw_data again.
This patch fixes the bug by avoiding -ENOMEM exit for dst_align_sz == 0.
The fix was suggested by Andrii Nakryiko <andrii.nakryiko@gmail.com>.
Reported-by: James Hilliard <james.hilliard1@gmail.com>
Link: https://lore.kernel.org/bpf/CADvTj4o7ZWUikKwNTwFq0O_AaX+46t_+Ca9gvWMYdWdRtTGeHQ@mail.gmail.com/
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/lib/bpf/linker.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index d7069780984a..5ced96d99f8c 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -1115,7 +1115,19 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
if (src->shdr->sh_type != SHT_NOBITS) {
tmp = realloc(dst->raw_data, dst_final_sz);
- if (!tmp)
+ /* If dst_align_sz == 0, realloc() behaves in a special way:
+ * 1. When dst->raw_data is NULL it returns:
+ * "either NULL or a pointer suitable to be passed to free()" [1].
+ * 2. When dst->raw_data is not-NULL it frees dst->raw_data and returns NULL,
+ * thus invalidating any "pointer suitable to be passed to free()" obtained
+ * at step (1).
+ *
+ * The dst_align_sz > 0 check avoids error exit after (2), otherwise
+ * dst->raw_data would be freed again in bpf_linker__free().
+ *
+ * [1] man 3 realloc
+ */
+ if (!tmp && dst_align_sz > 0)
return -ENOMEM;
dst->raw_data = tmp;
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] libbpf: Fix double-free when linker processes empty sections
2023-03-28 0:47 ` [PATCH bpf-next 2/2] libbpf: Fix double-free when linker processes " Eduard Zingerman
@ 2023-03-28 2:18 ` James Hilliard
0 siblings, 0 replies; 6+ messages in thread
From: James Hilliard @ 2023-03-28 2:18 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yhs
On Mon, Mar 27, 2023 at 6:47 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Double-free error in bpf_linker__free() was reported by James Hilliard.
> The error is caused by miss-use of realloc() in extend_sec().
> The error occurs when two files with empty sections of the same name
> are linked:
> - when first file is processed:
> - extend_sec() calls realloc(dst->raw_data, dst_align_sz)
> with dst->raw_data == NULL and dst_align_sz == 0;
> - dst->raw_data is set to a special pointer to a memory block of
> size zero;
> - when second file is processed:
> - extend_sec() calls realloc(dst->raw_data, dst_align_sz)
> with dst->raw_data == <special pointer> and dst_align_sz == 0;
> - realloc() "frees" dst->raw_data special pointer and returns NULL;
> - extend_sec() exits with -ENOMEM, and the old dst->raw_data value
> is preserved (it is now invalid);
> - eventually, bpf_linker__free() attempts to free dst->raw_data again.
>
> This patch fixes the bug by avoiding -ENOMEM exit for dst_align_sz == 0.
> The fix was suggested by Andrii Nakryiko <andrii.nakryiko@gmail.com>.
>
> Reported-by: James Hilliard <james.hilliard1@gmail.com>
> Link: https://lore.kernel.org/bpf/CADvTj4o7ZWUikKwNTwFq0O_AaX+46t_+Ca9gvWMYdWdRtTGeHQ@mail.gmail.com/
Tested-by: James Hilliard <james.hilliard1@gmail.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/lib/bpf/linker.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index d7069780984a..5ced96d99f8c 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -1115,7 +1115,19 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
>
> if (src->shdr->sh_type != SHT_NOBITS) {
> tmp = realloc(dst->raw_data, dst_final_sz);
> - if (!tmp)
> + /* If dst_align_sz == 0, realloc() behaves in a special way:
> + * 1. When dst->raw_data is NULL it returns:
> + * "either NULL or a pointer suitable to be passed to free()" [1].
> + * 2. When dst->raw_data is not-NULL it frees dst->raw_data and returns NULL,
> + * thus invalidating any "pointer suitable to be passed to free()" obtained
> + * at step (1).
> + *
> + * The dst_align_sz > 0 check avoids error exit after (2), otherwise
> + * dst->raw_data would be freed again in bpf_linker__free().
> + *
> + * [1] man 3 realloc
> + */
> + if (!tmp && dst_align_sz > 0)
> return -ENOMEM;
> dst->raw_data = tmp;
>
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 0/2] Fix double-free when linker processes empty sections
2023-03-28 0:47 [PATCH bpf-next 0/2] Fix double-free when linker processes empty sections Eduard Zingerman
2023-03-28 0:47 ` [PATCH bpf-next 1/2] selftests/bpf: Test if bpftool linker handles " Eduard Zingerman
2023-03-28 0:47 ` [PATCH bpf-next 2/2] libbpf: Fix double-free when linker processes " Eduard Zingerman
@ 2023-03-28 3:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-28 3:10 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yhs,
james.hilliard1
Hello:
This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Tue, 28 Mar 2023 03:47:36 +0300 you wrote:
> Fixes double-free error in linker.c:bpf_linker__free() caused by
> realloc(..., 0) call in linker.c:extend_sec() (such a call "frees"
> memory every second time :). The error is triggered when object files
> with empty sections of the same name are processed by linker.
>
> - The first patch extends progs/linked_funcs[12].c to trigger the
> error upon tests compilation;
> - The second patch contains detailed description of the error, fix and
> appropriate attributions.
>
> [...]
Here is the summary with links:
- [bpf-next,1/2] selftests/bpf: Test if bpftool linker handles empty sections
(no matching commit)
- [bpf-next,2/2] libbpf: Fix double-free when linker processes empty sections
https://git.kernel.org/bpf/bpf-next/c/d08ab82f59d5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread