2023-05-12 02:56:51

by Fangrui Song

[permalink] [raw]
Subject: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE

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
[2]: https://reviews.llvm.org/D150022
---
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/proc/vmcore.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

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);
--
2.40.1.606.ga4b1b128d6-goog



2023-05-12 18:35:18

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE

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

--
Kees Cook

2023-05-12 19:51:05

by Fangrui Song

[permalink] [raw]
Subject: Re: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE

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

2023-05-16 21:44:50

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE

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

--
Kees Cook