diff mbox series

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

Message ID 20220103230433.1907240-2-hjl.tools@gmail.com
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers show
Series Properly align all PT_LOAD segments with tests | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

H.J. Lu Jan. 3, 2022, 11:04 p.m. UTC
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(-)

Comments

Adhemerval Zanella Jan. 18, 2022, 5:48 p.m. UTC | #1
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?

> @@ -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
H.J. Lu Jan. 18, 2022, 9:46 p.m. UTC | #2
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.
diff mbox series

Patch

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