Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Nov 2022 12:27:34 -0800
From:   Fangrui Song <maskray@google.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     Pedro Falcato <pedro.falcato@gmail.com>,
        Rich Felker <dalias@libc.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Eric Biederman <ebiederm@xmission.com>, sam@gentoo.org,
        linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] binfmt_elf: Allow .bss in any interp PT_LOAD
Message-ID: <20221111202734.m5gk6vr4e5zd25lk@google.com>
References: <20221111061315.gonna.703-kees@kernel.org>
 <20221111074234.xm5a6ota7ppdsto5@google.com>
 <202211111211.93ED8B4B@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <202211111211.93ED8B4B@keescook>
Precedence: bulk

On 2022-11-11, Kees Cook wrote:
>On Thu, Nov 10, 2022 at 11:42:34PM -0800, Fangrui Song wrote:
>> (+ sam@gentoo.org from Pedro Falcato's patch)
>>
>> On 2022-11-10, Kees Cook wrote:
>> > Traditionally, only the final PT_LOAD for load_elf_interp() supported
>> > having p_memsz > p_filesz. Recently, lld's construction of musl's
>> > libc.so on PowerPC64 started having two PT_LOAD program headers with
>> > p_memsz > p_filesz.
>> >
>> > As the least invasive change possible, check for p_memsz > p_filesz for
>> > each PT_LOAD in load_elf_interp.
>> >
>> > Reported-by: Rich Felker <dalias@libc.org>
>> > Link: https://maskray.me/blog/2022-11-05-lld-musl-powerpc64
>> > Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> > Cc: Fangrui Song <maskray@google.com>
>> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> > Cc: Eric Biederman <ebiederm@xmission.com>
>> > Cc: linux-fsdevel@vger.kernel.org
>> > Cc: linux-mm@kvack.org
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > ---
>> > v2: I realized we need to retain the final padding call.
>> > v1: https://lore.kernel.org/linux-hardening/20221111055747.never.202-kees@kernel.org/
>> > ---
>> > fs/binfmt_elf.c | 18 ++++++++++++++----
>> > 1 file changed, 14 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> > index 528e2ac8931f..0a24bbbef1d6 100644
>> > --- a/fs/binfmt_elf.c
>> > +++ b/fs/binfmt_elf.c
>> > @@ -673,15 +673,25 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>> > 				last_bss = k;
>> > 				bss_prot = elf_prot;
>> > 			}
>> > +
>> > +			/*
>> > +			 * Clear any p_memsz > p_filesz area up to the end
>> > +			 * of the page to wipe anything left over from the
>> > +			 * loaded file contents.
>> > +			 */
>> > +			if (last_bss > elf_bss && padzero(elf_bss))
>>
>> Missing {
>>
>> But after fixing this, I get a musl ld.so error.
>>
>> > +				error = -EFAULT;
>> > +				goto out;
>> > +			}
>> > 		}
>> > 	}
>> >
>> > 	/*
>> > -	 * Now fill out the bss section: first pad the last page from
>> > -	 * the file up to the page boundary, and zero it from elf_bss
>> > -	 * up to the end of the page.
>> > +	 * Finally, pad the last page from the file up to the page boundary,
>> > +	 * and zero it from elf_bss up to the end of the page, if this did
>> > +	 * not already happen with the last PT_LOAD.
>> > 	 */
>> > -	if (padzero(elf_bss)) {
>> > +	if (last_bss == elf_bss && padzero(elf_bss)) {
>> > 		error = -EFAULT;
>> > 		goto out;
>> > 	}
>> > --
>> > 2.34.1
>> >
>>
>> I added a new section to https://maskray.me/blog/2022-11-05-lld-musl-powerpc64
>> Copying here:
>>
>> To test that the kernel ELF loader can handle more RW `PT_LOAD` program headers, we can create an executable with more RW `PT_LOAD` program headers with `p_filesz < p_memsz`.
>> We can place a read-only section after `.bss` followed by a `SHT_NOBITS` `SHF_ALLOC|SHF_WRITE` section. The read-only section will form a read-only `PT_LOAD` while the RW section will form a RW `PT_LOAD`.
>>
>> ```text
>> #--- a.c
>> #include <assert.h>
>> #include <stdio.h>
>>
>> extern const char toc[];
>> char nobits0[0] __attribute__((section(".nobits0")));
>> char nobits1[0] __attribute__((section(".nobits1")));
>>
>> int main(void) {
>>   assert(toc[4096-1] == 0);
>>   for (int i = 0; i < 1024; i++)
>>     assert(nobits0[i] == 0);
>>   nobits0[0] = nobits0[1024-1] = 1;
>>   for (int i = 0; i < 4096; i++)
>>     assert(nobits1[i] == 0);
>>   nobits1[0] = nobits1[4096-1] = 1;
>>
>>   puts("hello");
>> }
>>
>> #--- toc.s
>> .section .toc,"aw",@nobits
>> .globl toc
>> toc:
>> .space 4096
>>
>> .section .ro0,"a"; .byte 255
>> .section .nobits0,"aw",@nobits; .space 1024
>> .section .ro1,"a"; .byte 255
>> .section .nobits1,"aw",@nobits; .space 4096
>>
>> #--- a.lds
>> SECTIONS { .ro0 : {} .nobits0 : {} .ro1 : {} .nobits1 : {} } INSERT AFTER .bss;
>> ```
>>
>> ```sh
>> split-file a.txt a
>> path/to/musl-gcc -Wl,--dynamic-linker=/lib/libc.so a/a.c a/a.lds -o toy
>> ```
>>
>> split-file is a utility in llvm-project.
>
>Where is a.txt? Also, it'd be nice to have this without needing the
>musl-gcc.

Sorry for the unclear description. I rewrite it.
(`char nobits0[0] __attribute__((section(".nobits0")));` is not effective. It's SHT_PROGBITS and makes the output section SHT_PROGBITS.
The new example addresses the deficiency.)


Create some files. If you have split-file (a [test utility](https://llvm.org/docs/TestingGuide.html#extra-files) from llvm-project), you may place the following content into `a.txt`.

```text
#--- a.c
#include <assert.h>
#include <stdio.h>

extern const char toc[];
extern char nobits0[], nobits1[];

int main(void) {
   assert(toc[4096-1] == 0);
   for (int i = 0; i < 1024; i++) {
     assert(nobits0[i] == 0);
     nobits0[i] = 1;
   }
   for (int i = 0; i < 8192; i++) {
     assert(nobits1[i] == 0);
     nobits1[i] = 1;
   }

   puts("hello");
}

#--- toc.s
.globl toc, nobits0, nobits1

.section .toc,"aw",@nobits; toc: .space 4096

.section .ro0,"a"; .byte 255
.section .nobits0,"aw",@nobits; nobits0: .space 1024
.section .ro1,"a"; .byte 255
.section .nobits1,"aw",@nobits; nobits1: .space 8192

#--- a.lds
SECTIONS { .ro0 : {} .nobits0 : {} .ro1 : {} .nobits1 : {} } INSERT AFTER .bss;
```

Then run:
```sh
split-file a.txt a
path/to/musl-gcc -Wl,--dynamic-linker=/lib/libc.so a/a.c a/a.lds -o toy
```

Note: when a `SHT_NOBITS` section is followed by another section, the `SHT_NOBITS` section behaves as if it occupies the file offset range. This is because ld.lld does not implement a file size optimization.


For this simple example, using glibc based gcc works as well (musl provides __assert_fail and puts referenced by the executable):

gcc -Wl,--dynamic-linker=/lib/libc.so a/a.c a/a.lds -o toy