Re: [PATCH v8 1/4] elf: Properly align all PT_LOAD segments [BZ #28676]

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 18/01/2022 18:57, H.J. Lu wrote:
> Linker may set p_align of a PT_LOAD segment larger than p_align of the
> first PT_LOAD segment to satisfy a section alignment:
> 
> Elf file type is DYN (Shared object file)
> Entry point 0x0
> There are 10 program headers, starting at offset 64
> 
> Program Headers:
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags  Align
>   LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
>                  0x0000000000000834 0x0000000000000834  R E    0x1000
>   LOAD           0x0000000000000e00 0x0000000000001e00 0x0000000000001e00
>                  0x0000000000000230 0x0000000000000230  RW     0x1000
>   LOAD           0x0000000000400000 0x0000000000400000 0x0000000000400000
>                  0x0000000000000004 0x0000000000000008  RW     0x400000
> ...
> 
>  Section to Segment mapping:
>   Segment Sections...
>    00     .note.gnu.property .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .plt.got .text .fini .rodata .eh_frame_hdr .eh_frame
>    01     .init_array .fini_array .data.rel.ro .dynamic .got .got.plt
>    02     .data .bss
> 
> We should align the first PT_LOAD segment to the maximum p_align of all
> PT_LOAD segments, similar to the kernel commit:
> 
> commit ce81bb256a224259ab686742a6284930cbe4f1fa
> Author: Chris Kennelly <ckennelly@google.com>
> Date:   Thu Oct 15 20:12:32 2020 -0700
> 
>     fs/binfmt_elf: use PT_LOAD p_align values for suitable start address

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-load.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 12744caf91..5b0ff41ee1 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1101,6 +1101,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      size_t nloadcmds = 0;
>      bool has_holes = false;
>      bool empty_dynamic = false;
> +    ElfW(Addr) p_align_max = 0;
>  
>      /* The struct is initialized to zero so this is not necessary:
>      l->l_ld = 0;
> @@ -1146,7 +1147,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	  c->mapend = ALIGN_UP (ph->p_vaddr + ph->p_filesz, GLRO(dl_pagesize));
>  	  c->dataend = ph->p_vaddr + ph->p_filesz;
>  	  c->allocend = ph->p_vaddr + ph->p_memsz;
> -	  c->mapalign = ph->p_align;
> +	  /* Remember the maximum p_align.  */
> +	  if (powerof2 (ph->p_align) && ph->p_align > p_align_max)
> +	    p_align_max = ph->p_align;
>  	  c->mapoff = ALIGN_DOWN (ph->p_offset, GLRO(dl_pagesize));
>  
>  	  /* Determine whether there is a gap between the last segment
> @@ -1221,6 +1224,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	goto lose;
>        }
>  
> +    /* Align all PT_LOAD segments to the maximum p_align.  */
> +    for (size_t i = 0; i < nloadcmds; i++)
> +      loadcmds[i].mapalign = p_align_max;
> +
>      /* dlopen of an executable is not valid because it is not possible
>         to perform proper relocations, handle static TLS, or run the
>         ELF constructors.  For PIE, the check needs the dynamic