On Mon, Mar 27, 2023 at 6:47 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx>. > > Reported-by: James Hilliard <james.hilliard1@xxxxxxxxx> > Link: https://lore.kernel.org/bpf/CADvTj4o7ZWUikKwNTwFq0O_AaX+46t_+Ca9gvWMYdWdRtTGeHQ@xxxxxxxxxxxxxx/ Tested-by: James Hilliard <james.hilliard1@xxxxxxxxx> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- > 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 >
