Re: [PATCH] elf: Support multiple PT_GNU_RELRO segments

On 19/08/26 18:22, Florian Weimer wrote:
> * 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[]

Ack.

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

Without multiple PT_GNU_RELRO support, I think it makes more sense to trigger
a failure if tst-relro-ldso or tst-relro-libc could not place the required
data symbols in the first RELRO segment.

So I don't think this change should be applied independently.

> 
>> -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()]

I think you meant:

regions = [(phdr.p_vaddr, phdr.p_vaddr + phdr.p_memsz)
           for phdr in img.phdrs()
           if phdr.p_type == glibcelf.Pt.PT_GNU_RELRO]

I will change to this.

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

It makes sense, I change to check for an specific marking.

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

Right, I read your thread again [1] and I think it would be better to keep rounding
down when PT_GNU_RELRO segment begins a PT_LOAD segment. A segment that does not
begin a PT_LOAD segment might instead preceded by live writable data, which must
not lose write access, so its start is rounded up and any partially covered page
is left writable.

[1] https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html