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

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Jan 18, 2022 at 9:49 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 03/01/2022 20:04, 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
> > ---
> >  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 ddc4295ef5..109bed3fb5 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 (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
>
> Kernel also skips non-power of two alignment as invalid, should we do the same
> to consider the max alignment?

Fixed.

> > @@ -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

Thanks.


-- 
H.J.