| Message ID | 20230512022528.3430327-1-maskray@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | coredump, vmcore: Set p_align to 4 for PT_NOTE | expand |
On Fri, May 12, 2023 at 02:25:28AM +0000, Fangrui Song wrote: > Tools like readelf/llvm-readelf use p_align to parse a PT_NOTE program > header as an array of 4-byte entries or 8-byte entries. Currently, there > are workarounds[1] in place for Linux to treat p_align==0 as 4. However, > it would be more appropriate to set the correct alignment so that tools > do not have to rely on guesswork. FreeBSD coredumps set p_align to 4 as > well. > > [1]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=82ed9683ec099d8205dc499ac84febc975235af6 The interesting bit from here is: /* NB: Some note sections may have alignment value of 0 or 1. gABI specifies that notes should be aligned to 4 bytes in 32-bit objects and to 8 bytes in 64-bit objects. As a Linux extension, we also support 4 byte alignment in 64-bit objects. If section alignment is less than 4, we treate alignment as 4 bytes. */ if (align < 4) align = 4; else if (align != 4 && align != 8) { warn (_("Corrupt note: alignment %ld, expecting 4 or 8\n"), (long) align); return FALSE; } Should Linux use 8 for 64-bit processes to avoid the other special case? (And do we need to make some changes to make sure we are actually aligned?) -Kees
On 2023-05-12, Kees Cook wrote: >On Fri, May 12, 2023 at 02:25:28AM +0000, Fangrui Song wrote: >> Tools like readelf/llvm-readelf use p_align to parse a PT_NOTE program >> header as an array of 4-byte entries or 8-byte entries. Currently, there >> are workarounds[1] in place for Linux to treat p_align==0 as 4. However, >> it would be more appropriate to set the correct alignment so that tools >> do not have to rely on guesswork. FreeBSD coredumps set p_align to 4 as >> well. >> >> [1]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=82ed9683ec099d8205dc499ac84febc975235af6 > >The interesting bit from here is: > > /* NB: Some note sections may have alignment value of 0 or 1. gABI > specifies that notes should be aligned to 4 bytes in 32-bit > objects and to 8 bytes in 64-bit objects. As a Linux extension, > we also support 4 byte alignment in 64-bit objects. If section > alignment is less than 4, we treate alignment as 4 bytes. */ > if (align < 4) > align = 4; > else if (align != 4 && align != 8) > { > warn (_("Corrupt note: alignment %ld, expecting 4 or 8\n"), > (long) align); > return FALSE; > } > >Should Linux use 8 for 64-bit processes to avoid the other special case? > >(And do we need to make some changes to make sure we are actually >aligned?) > >-Kees 64-bit objects should use 8-byte entries and naturally the 8-byte alignment. Unfortunately, many systems including Solaris, *BSD, and Linux use 4-byte entries for SHT_NOTE/PT_NOTE, and changing this will create a large compatibility problem (see tcmalloc that I recently updated[1]) Linux introduced 8-byte alignment note sections (.note.gnu.property) a while ago, so the ecosystem has to deal with notes of mixed alignments. The resolution is to use the note alignment to decide whether it should be parsed as 4-byte entries or 8-byte entries. I think that just setting `p_align = 4` on the kernel side should be good enough:) [1]: https://github.com/google/tcmalloc/commit/c33cb2d8935002f8ba942028a1f0871d075345a1
On Fri, 12 May 2023 02:25:28 +0000, Fangrui Song wrote: > Tools like readelf/llvm-readelf use p_align to parse a PT_NOTE program > header as an array of 4-byte entries or 8-byte entries. Currently, there > are workarounds[1] in place for Linux to treat p_align==0 as 4. However, > it would be more appropriate to set the correct alignment so that tools > do not have to rely on guesswork. FreeBSD coredumps set p_align to 4 as > well. > > [...] Applied to for-next/execve, thanks! [1/1] coredump, vmcore: Set p_align to 4 for PT_NOTE https://git.kernel.org/kees/c/60592fb6b67c
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 1033fbdfdbec..44b4c42ab8e8 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1517,7 +1517,7 @@ static void fill_elf_note_phdr(struct elf_phdr *phdr, int sz, loff_t offset) phdr->p_filesz = sz; phdr->p_memsz = 0; phdr->p_flags = 0; - phdr->p_align = 0; + phdr->p_align = 4; } static void fill_note(struct memelfnote *note, const char *name, int type, diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 05a1471d5283..d76ad3d4f676 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -1269,7 +1269,7 @@ static inline void fill_elf_note_phdr(struct elf_phdr *phdr, int sz, loff_t offs phdr->p_filesz = sz; phdr->p_memsz = 0; phdr->p_flags = 0; - phdr->p_align = 0; + phdr->p_align = 4; return; } diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c index 03f5963914a1..cb80a7703d58 100644 --- a/fs/proc/vmcore.c +++ b/fs/proc/vmcore.c @@ -877,7 +877,7 @@ static int __init merge_note_headers_elf64(char *elfptr, size_t *elfsz, phdr.p_offset = roundup(note_off, PAGE_SIZE); phdr.p_vaddr = phdr.p_paddr = 0; phdr.p_filesz = phdr.p_memsz = phdr_sz; - phdr.p_align = 0; + phdr.p_align = 4; /* Add merged PT_NOTE program header*/ tmp = elfptr + sizeof(Elf64_Ehdr); @@ -1068,7 +1068,7 @@ static int __init merge_note_headers_elf32(char *elfptr, size_t *elfsz, phdr.p_offset = roundup(note_off, PAGE_SIZE); phdr.p_vaddr = phdr.p_paddr = 0; phdr.p_filesz = phdr.p_memsz = phdr_sz; - phdr.p_align = 0; + phdr.p_align = 4; /* Add merged PT_NOTE program header*/ tmp = elfptr + sizeof(Elf32_Ehdr);