[bpf-next,2/2] libbpf: Fix double-free when linker processes empty sections

Commit Message

Eduard Zingerman March 28, 2023, 12:47 a.m. UTC

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 <[e-mail]>.

Reported-by: James Hilliard <[e-mail]>
Link: https://lore.kernel.org/bpf/[e-mail]/
Signed-off-by: Eduard Zingerman <[e-mail]>
---
 tools/lib/bpf/linker.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

James Hilliard March 28, 2023, 2:18 a.m. UTC | #1

On Mon, Mar 27, 2023 at 6:47 PM Eduard Zingerman <[e-mail]> 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 <[e-mail]>.
>
> Reported-by: James Hilliard <[e-mail]>
> Link: https://lore.kernel.org/bpf/[e-mail]/
Tested-by: James Hilliard <[e-mail]>
> Signed-off-by: Eduard Zingerman <[e-mail]>
> ---
>  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
>

Patch

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;