Re: [PATCH RFC] binfmt_elf: fully allocate bss pages

[Pedro Falcato]

On Fri, Sep 15, 2023 at 4:54 AM Thomas Weißschuh <linux@xxxxxxxxxxxxxx> wrote:
>
> When allocating the pages for bss the start address needs to be rounded
> down instead of up.
> Otherwise the start of the bss segment may be unmapped.
>
> The was reported to happen on Aarch64:
>
> Memory allocated by set_brk():
> Before: start=0x420000 end=0x420000
> After:  start=0x41f000 end=0x420000
>
> The triggering binary looks like this:
>
>     Elf file type is EXEC (Executable file)
>     Entry point 0x400144
>     There are 4 program headers, starting at offset 64
>
>     Program Headers:
>       Type           Offset             VirtAddr           PhysAddr
>                      FileSiz            MemSiz              Flags  Align
>       LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
>                      0x0000000000000178 0x0000000000000178  R E    0x10000
>       LOAD           0x000000000000ffe8 0x000000000041ffe8 0x000000000041ffe8
>                      0x0000000000000000 0x0000000000000008  RW     0x10000
>       NOTE           0x0000000000000120 0x0000000000400120 0x0000000000400120
>                      0x0000000000000024 0x0000000000000024  R      0x4
>       GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
>                      0x0000000000000000 0x0000000000000000  RW     0x10
>
>      Section to Segment mapping:
>       Segment Sections...
>        00     .note.gnu.build-id .text .eh_frame
>        01     .bss
>        02     .note.gnu.build-id
>        03
>
> Reported-by: Sebastian Ott <sebott@xxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/5d49767a-fbdc-fbe7-5fb2-d99ece3168cb@xxxxxxxxxx/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> ---
>
> I'm not really familiar with the ELF loading process, so putting this
> out as RFC.
>
> A example binary compiled with aarch64-linux-gnu-gcc 13.2.0 is available
> at https://test.t-8ch.de/binfmt-bss-repro.bin
> ---
>  fs/binfmt_elf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 7b3d2d491407..4008a57d388b 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -112,7 +112,7 @@ static struct linux_binfmt elf_format = {
>
>  static int set_brk(unsigned long start, unsigned long end, int prot)
>  {
> -       start = ELF_PAGEALIGN(start);
> +       start = ELF_PAGESTART(start);
>         end = ELF_PAGEALIGN(end);
>         if (end > start) {
>                 /*

I don't see how this change can be correct. set_brk takes the start of
.bss as the start, so doing ELF_PAGESTART(start) will give you what
may very well be another ELF segment. In the common case, you'd map an
anonymous page on top of someone's .data, which will misload the ELF.

The current logic looks OK to me (gosh this code would ideally take a
good refactoring...). I still can't quite tell how padzero() (in the
original report) is -EFAULTing though.

-- 
Pedro