Re: [PATCH] elf: Support multiple PT_GNU_RELRO segments

* Adhemerval Zanella:

> diff --git a/elf/tst-relro-multi-notes.S b/elf/tst-relro-multi-notes.S
> new file mode 100644
> index 00000000000..c66ad8e0baf
> --- /dev/null
> +++ b/elf/tst-relro-multi-notes.S
> @@ -0,0 +1,46 @@

> +/* Assembly is used because compilers emit SHT_PROGBITS for
> +   __attribute__ ((section)) definitions and gas might warns when its
> +   name-based heuristic retypes .note.* sections; and  note sections
> +   must not have the SHF_WRITE flag for the same reason.  */

typo: might warn[]

> diff --git a/elf/tst-relro-symbols.py b/elf/tst-relro-symbols.py
> index ffbe9958fed..7633a8dfb84 100644
> --- a/elf/tst-relro-symbols.py
> +++ b/elf/tst-relro-symbols.py
> @@ -32,25 +32,31 @@ sys.path.append(os.path.join(

These changes could go in first.

> -def find_relro(path: str, img: glibcelf.Image) -> (int, int):
> -    """Discover the address range of the PT_GNU_RELRO segment."""
> +def find_relro(path: str, img: glibcelf.Image) -> list:
> +    """Discover the address ranges of the PT_GNU_RELRO segments."""
> +    regions = []
>      for phdr in img.phdrs():
>          if phdr.p_type == glibcelf.Pt.PT_GNU_RELRO:
>              # The computation is not entirely accurate because
>              # _dl_protect_relro in elf/dl-reloc.c rounds both the
>              # start end and downwards using the run-time page size.
> +            regions.append((phdr.p_vaddr, phdr.p_vaddr + phdr.p_memsz))

That's:

    regions = [(phdr.p_vaddr, phdr.p_vaddr + phdr.p_memsz)
               for phdr in img.phdrs()]

> diff --git a/scripts/tst-elf-edit.py b/scripts/tst-elf-edit.py
> index 07fa7e90f55..4c5e73e2f0e 100644
> --- a/scripts/tst-elf-edit.py
> +++ b/scripts/tst-elf-edit.py
> @@ -47,7 +47,11 @@ ET_EXEC=2
>  ET_DYN=3
>  
>  PT_LOAD=1
> +PT_NOTE=4
>  PT_TLS=7
> +PT_GNU_RELRO=0x6474e552
> +
> +PF_W=2
>  
>  def elf_types_fmts(e_ident):
>      endian = '<' if e_ident[EI_DATA] == ELFDATA2LSB else '>'
> @@ -156,6 +160,34 @@ def elf_edit_maximize_tls_size(phdr, elfclass):
>      else:
>          phdr.p_memsz = 1 << 63
>  
> +def elf_edit_note_to_relro(f, e_ident, ehdr, expected):
> +    phdrs = []
> +    for i in range(0, ehdr.e_phnum):
> +        phdr = Elf_Phdr(e_ident)
> +        f.seek(ehdr.e_phoff + i * phdr.len)
> +        phdr.read(f)
> +        phdrs.append(phdr)
> +
> +    wr_loads = [(p.p_vaddr, p.p_vaddr + p.p_memsz) for p in phdrs
> +                if p.p_type == PT_LOAD and (p.p_flags & PF_W) != 0]
> +
> +    converted = 0
> +    for i, phdr in enumerate(phdrs):
> +        if phdr.p_type != PT_NOTE:
> +            continue
> +        if not any(lo <= phdr.p_vaddr < hi for lo, hi in wr_loads):
> +            continue
> +        phdr.p_type = PT_GNU_RELRO
> +        # Match the alignment the linker uses for PT_GNU_RELRO.
> +        phdr.p_align = 1
> +        f.seek(ehdr.e_phoff + i * phdr.len)
> +        phdr.write(f)
> +        converted += 1

I think it would slightly less hackish if we looked at the note
*contents* to trigger the conversion.  As in, the note itself could be
a marker that indicates that it should be turned into PT_GNU_RELRO.
I think this is more robust if the toolchain generates its own notes.

> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 305ca6e0df2..0d5f2a2c96d 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h

> @@ -1037,6 +1038,24 @@ void _dl_relocate_object_no_relro (struct link_map *map,
>  /* Protect PT_GNU_RELRO area.  */
>  extern void _dl_protect_relro (struct link_map *map) attribute_hidden;
>  
> +struct dl_relro_range
> +{
> +  ElfW(Addr) start;
> +  ElfW(Addr) end;
> +};
> +
> +/* Compute the range for the PT_GNU_RELRO segment PH of map L.  */
> +static inline struct dl_relro_range
> +_dl_relro_range (const struct link_map *l, const ElfW(Phdr) *ph)
> +{
> +  return (struct dl_relro_range)
> +    {
> +      .start = ALIGN_DOWN (l->l_addr + ph->p_vaddr, GLRO(dl_pagesize)),
> +      .end = ALIGN_DOWN (l->l_addr + ph->p_vaddr + ph->p_memsz,
> +			 GLRO(dl_pagesize)),
> +    };
> +}

I think we should finally add a comment why it's using ALIGN_DOWN
twice.  It's even possible that with multi-RELRO, this becomes
downright buggy on systems with multiple page sizes.