Re: [PATCH v4 0/6] binfmt_elf: Support segments with 0 filesz and misaligned starts

From: Eric W. Biederman
Date: Fri Sep 29 2023 - 11:40:04 EST

Pedro Falcato <pedro.falcato@xxxxxxxxx> writes:

> On Fri, Sep 29, 2023 at 4:24 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> This is the continuation of the work Eric started for handling
>> "p_memsz > p_filesz" in arbitrary segments (rather than just the last,
>> BSS, segment). I've added the suggested changes:
>>
>> - drop unused "elf_bss" variable
>> - refactor load_elf_interp() to use elf_load()
>> - refactor load_elf_library() to use elf_load()
>> - report padzero() errors when PROT_WRITE is present
>> - drop vm_brk()
>>
>> Thanks!
>>
>> -Kees
>>
>> v4:
>> - refactor load_elf_library() too
>> - don't refactor padzero(), just test in the only remaining caller
>> - drop now-unused vm_brk()
>> v3: https://lore.kernel.org/all/20230927033634.make.602-kees@xxxxxxxxxx
>> v2: https://lore.kernel.org/lkml/87sf71f123.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> v1: https://lore.kernel.org/lkml/87jzsemmsd.fsf_-_@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>>
>> Eric W. Biederman (1):
>> binfmt_elf: Support segments with 0 filesz and misaligned starts
>>
>> Kees Cook (5):
>> binfmt_elf: elf_bss no longer used by load_elf_binary()
>> binfmt_elf: Use elf_load() for interpreter
>> binfmt_elf: Use elf_load() for library
>> binfmt_elf: Only report padzero() errors when PROT_WRITE
>> mm: Remove unused vm_brk()
>>
>> fs/binfmt_elf.c | 214 ++++++++++++++++-----------------------------
>> include/linux/mm.h | 3 +-
>> mm/mmap.c | 6 --
>> mm/nommu.c | 5 --
>> 4 files changed, 76 insertions(+), 152 deletions(-)
>
> Sorry for taking so long to take a look at this.
> While I didn't test PPC64 (I don't own PPC64 hardware, and I wasn't
> the original reporter), I did manage to craft a reduced test case of:
>
> a.out:
>
> Program Headers:
> Type Offset VirtAddr PhysAddr
> FileSiz MemSiz Flags Align
> PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040
> 0x00000000000001f8 0x00000000000001f8 R 0x8
> INTERP 0x0000000000000238 0x0000000000000238 0x0000000000000238
> 0x0000000000000020 0x0000000000000020 R 0x1
> [Requesting program interpreter: /home/pfalcato/musl/lib/libc.so]
> LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
> 0x0000000000000428 0x0000000000000428 R 0x1000
> LOAD 0x0000000000001000 0x0000000000001000 0x0000000000001000
> 0x00000000000000cd 0x00000000000000cd R E 0x1000
> LOAD 0x0000000000002000 0x0000000000002000 0x0000000000002000
> 0x0000000000000084 0x0000000000000084 R 0x1000
> LOAD 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50
> 0x00000000000001c8 0x00000000000001c8 RW 0x1000
> DYNAMIC 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50
> 0x0000000000000180 0x0000000000000180 RW 0x8
> GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
> 0x0000000000000000 0x0000000000000000 RW 0x10
> GNU_RELRO 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50
> 0x00000000000001b0 0x00000000000001b0 R 0x1
>
> /home/pfalcato/musl/lib/libc.so:
> Program Headers:
> Type Offset VirtAddr PhysAddr
> FileSiz MemSiz Flags Align
> PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040
> 0x0000000000000230 0x0000000000000230 R 0x8
> LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
> 0x0000000000049d9c 0x0000000000049d9c R 0x1000
> LOAD 0x0000000000049da0 0x000000000004ada0 0x000000000004ada0
> 0x0000000000057d30 0x0000000000057d30 R E 0x1000
> LOAD 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0
> 0x00000000000005f0 0x00000000000015f0 RW 0x1000
> LOAD 0x00000000000a20c0 0x00000000000a60c0 0x00000000000a60c0
> 0x0000000000000428 0x0000000000002f80 RW 0x1000
> DYNAMIC 0x00000000000a1f38 0x00000000000a3f38 0x00000000000a3f38
> 0x0000000000000110 0x0000000000000110 RW 0x8
> GNU_RELRO 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0
> 0x00000000000005f0 0x0000000000002530 R 0x1
> GNU_EH_FRAME 0x0000000000049d10 0x0000000000049d10 0x0000000000049d10
> 0x0000000000000024 0x0000000000000024 R 0x4
> GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
> 0x0000000000000000 0x0000000000000000 RW 0x0
> NOTE 0x0000000000000270 0x0000000000000270 0x0000000000000270
> 0x0000000000000018 0x0000000000000018 R 0x4
>
> Section to Segment mapping:
> Segment Sections...
> 00
> 01 .note.gnu.build-id .dynsym .gnu.hash .hash .dynstr .rela.dyn
> .rela.plt .rodata .eh_frame_hdr .eh_frame
> 02 .text .plt
> 03 .data.rel.ro .dynamic .got .toc
> 04 .data .got.plt .bss
> 05 .dynamic
> 06 .data.rel.ro .dynamic .got .toc
> 07 .eh_frame_hdr
> 08
> 09 .note.gnu.build-id
>
>
> So on that end, you can take my
>
> Tested-by: Pedro Falcato <pedro.falcato@xxxxxxxxx>
>
> Although this still doesn't address the other bug I found
> (https://github.com/heatd/elf-bug-questionmark), where segments can
> accidentally overwrite cleared BSS if we end up in a situation where
> e.g we have the following segments:
>
> Program Headers:
> Type Offset VirtAddr PhysAddr
> FileSiz MemSiz Flags Align
> LOAD 0x0000000000001000 0x0000000000400000 0x0000000000400000
> 0x0000000000000045 0x0000000000000045 R E 0x1000
> LOAD 0x0000000000002000 0x0000000000401000 0x0000000000401000
> 0x000000000000008c 0x000000000000008c R 0x1000
> LOAD 0x0000000000000000 0x0000000000402000 0x0000000000402000
> 0x0000000000000000 0x0000000000000801 RW 0x1000
> LOAD 0x0000000000002801 0x0000000000402801 0x0000000000402801
> 0x0000000000000007 0x0000000000000007 RW 0x1000
> NOTE 0x0000000000002068 0x0000000000401068 0x0000000000401068
> 0x0000000000000024 0x0000000000000024 0x4
>
> Section to Segment mapping:
> Segment Sections...
> 00 .text
> 01 .rodata .note.gnu.property .note.gnu.build-id
> 02 .bss
> 03 .data
> 04 .note.gnu.build-id
>
> since the mmap of .data will end up happening over .bss.

This is simply invalid userspace, doubly so with the alignment set to
0x1000.

The best the kernel can do is have a pre-pass through the elf program
headers (before the point of no-return) and if they describe overlapping
segments or out of order segments, cause execve syscall to fail.

Eric