Re: [PATCH bpf-next v3 0/8] libbpf, selftests/bpf: Support cross-endian usage

[Tony Ambardar <tony.ambardar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

On Wed, Aug 28, 2024 at 04:11:50AM -0700, Tony Ambardar wrote:
> Hello all,

[snip]

> Changelog:
> ---------
> v2 -> v3: (feedback from Andrii)
>  - improve some log and commit message formatting
>  - restructure BTF.ext endianness safety checks and byte-swapping
>  - use BTF.ext info record definitions for swapping, require BTF v1
>  - follow BTF API implementation more closely for BTF.ext
>  - explicitly reject loading non-native endianness program into kernel
>  - simplify linker output byte-order setting
>  - drop redundant safety checks during linking
>  - simplify endianness macro and improve blob setup code for light skel
>  - no unexpected test failures after cross-compiling x86_64 -> s390x

Sadly, shortly after posting v3 I hit a strange new issue in CI testing.

Existing code in bpf_object__elf_finish() doesn't zero Ehdr references
after freeing the related ELF data, allowing use of stale endian data
which can be reallocated and overwritten, leading to rare, confusing CI
errors like:

  test_tailcall_count:PASS:open fentry_obj file 0 nsec
  test_tailcall_count:PASS:find fentry prog 0 nsec
  test_tailcall_count:PASS:set_attach_target subprog_tail 0 nsec
  libbpf: object 'tailcall_bpf2bp' is not native endianness
  test_tailcall_count:FAIL:load fentry_obj unexpected error: -4003 (errno 4003)
  #333/13  tailcalls/tailcall_bpf2bpf_fentry:FAIL

I have a minor patch to fix this but will wait for feedback on v3 before
posting it together with any further requested changes in a v4. Apologies
for the extra churn, and I'll attach the pending patch for reference.

Thanks,
Tony


[-- Attachment #2: elf-use-after-free.patch --]
[-- Type: text/x-diff, Size: 1614 bytes --]

--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -694,6 +694,8 @@ struct bpf_object {
 	/* Information when doing ELF related work. Only valid if efile.elf is not NULL */
 	struct elf_state efile;
 
+	unsigned char byteorder;
+
 	struct btf *btf;
 	struct btf_ext *btf_ext;
 
@@ -1521,6 +1523,7 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
 
 	elf_end(obj->efile.elf);
 	obj->efile.elf = NULL;
+	obj->efile.ehdr = NULL;
 	obj->efile.symbols = NULL;
 	obj->efile.arena_data = NULL;
 
@@ -1586,6 +1589,18 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 		goto errout;
 	}
 
+	/* Validate ELF object endianness... */
+	if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB &&
+	    ehdr->e_ident[EI_DATA] != ELFDATA2MSB) {
+		err = -LIBBPF_ERRNO__ENDIAN;
+		pr_warn("elf: '%s' has unknown byte order\n", obj->path);
+		goto errout;
+	}
+	/* and preserve outside lifetime of bpf_object_open() */
+	obj->byteorder = ehdr->e_ident[EI_DATA];
+
+
+
 	if (elf_getshdrstrndx(elf, &obj->efile.shstrndx)) {
 		pr_warn("elf: failed to get section names section index for %s: %s\n",
 			obj->path, elf_errmsg(-1));
@@ -1617,9 +1632,9 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 static bool is_native_endianness(struct bpf_object *obj)
 {
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-	return obj->efile.ehdr->e_ident[EI_DATA] == ELFDATA2LSB;
+	return obj->byteorder == ELFDATA2LSB;
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-	return obj->efile.ehdr->e_ident[EI_DATA] == ELFDATA2MSB;
+	return obj->byteorder == ELFDATA2MSB;
 #else
 # error "Unrecognized __BYTE_ORDER__"
 #endif