[v2] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

  ld.so on x86_64 (and possibly for all other architectures that don't
have a read-only dynamic section by default but I haven't checked on
all of them), when invoked with a DSO that has a read-only .dynamic
section, crashes when trying to update gotplt, symtab, etc. entries
with the load address.  This crash happens even during verification,
i.e. when it is invoked with --verify or with LD_TRACE_LOADED_OBJECTS.

This is seen with vdso64.so from the Linux kernel on x86_64 for
example.

Handle this case more gracefully by bailing out early when a read-only
PT_DYNAMIC segment is encountered and l_addr is non-zero, indicating
the need for fixing up entries in .dynamic.  A test case has also been
added to verify that BZ #28340 has been fixed.
---
Changes from v1:
- Assume that all programs with a .dynamic section need a fixup if
  l_addr != 0.  Drop elf_dynamic_info_needs_fixup()  to that effect.
- Rename tst-ro-dynamic.map to tst-ro-dynamic-mod.map for consistency.

 elf/Makefile               | 11 +++++++++--
 elf/dl-load.c              | 14 ++++++++++++++
 elf/rtld.c                 | 12 ++++++++++++
 elf/tst-ro-dynamic-mod.c   |  1 +
 elf/tst-ro-dynamic-mod.map | 13 +++++++++++++
 elf/tst-ro-dynamic.sh      | 36 ++++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 elf/tst-ro-dynamic-mod.c
 create mode 100644 elf/tst-ro-dynamic-mod.map
 create mode 100644 elf/tst-ro-dynamic.sh
  

* Siddhesh Poyarekar:

> @@ -1149,6 +1152,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  		 such a segment to avoid a crash later.  */
>  	      l->l_ld = (void *) ph->p_vaddr;
>  	      l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
> +#ifndef DL_RO_DYN_SECTION
> +	      readonly_dynamic = (ph->p_flags & PF_W) == 0;
> +#endif
>  	    }
>  	  break;

As far as I can tell, this does not check whether the DYNAMIC segment is
actually covered by a read-write LOAD segment.  I wonder how much value
this imperfect check has.

Would it make more sense to fix the kernel linker script that generates
those bogus vDSO reference files?

Thanks,
Florian
  

On 9/15/21 4:10 PM, Florian Weimer wrote:
> As far as I can tell, this does not check whether the DYNAMIC segment is
> actually covered by a read-write LOAD segment.  I wonder how much value
> this imperfect check has.

I reckon there's less value in trying to make this corner case work; I 
can't think of a reason for someone (outside of the VDSO use case) to do 
this on purpose.  Read-only DYNAMIC segments in ET_DYN objects shouldn't 
be a supported use case.

> Would it make more sense to fix the kernel linker script that generates
> those bogus vDSO reference files?

The vdso is not meant to be relocated, which is why all segments in it 
are read-only.  setup_vdso only pretends to relocate it in the interest 
of internal consistency in ld.so.

Siddhesh
  

* Siddhesh Poyarekar:

> On 9/15/21 4:10 PM, Florian Weimer wrote:
>> As far as I can tell, this does not check whether the DYNAMIC segment is
>> actually covered by a read-write LOAD segment.  I wonder how much value
>> this imperfect check has.
>
> I reckon there's less value in trying to make this corner case work; I
> can't think of a reason for someone (outside of the VDSO use case) to
> do this on purpose.  Read-only DYNAMIC segments in ET_DYN objects
> shouldn't be a supported use case.

Sure, but the corresponding LOAD segment is read-only as well, that's
what actually matters.

>> Would it make more sense to fix the kernel linker script that generates
>> those bogus vDSO reference files?
>
> The vdso is not meant to be relocated, which is why all segments in it
> are read-only.  setup_vdso only pretends to relocate it in the
> interest of internal consistency in ld.so.

And they do not want multiple LOAD segments because it can fit all on
the same page.  We would end up with an RWX segment if we made this
single LOAD segment writable, which is probably even more confusing.

So I guess the status quo is probably best.

Maybe someone else can comment on the additional detection.  In the
past, we said that crashes on corrupt ELF files weren't something we
cared about, but I increasingly see th value of good diagnostics for
common special cases.

(See also the discussion about DSOs without entry points.)

Thanks,
Florian
  

On 9/15/21 5:48 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar:
> 
>> On 9/15/21 4:10 PM, Florian Weimer wrote:
>>> As far as I can tell, this does not check whether the DYNAMIC segment is
>>> actually covered by a read-write LOAD segment.  I wonder how much value
>>> this imperfect check has.
>>
>> I reckon there's less value in trying to make this corner case work; I
>> can't think of a reason for someone (outside of the VDSO use case) to
>> do this on purpose.  Read-only DYNAMIC segments in ET_DYN objects
>> shouldn't be a supported use case.
> 
> Sure, but the corresponding LOAD segment is read-only as well, that's
> what actually matters.

I don't understand.  Why isn't the fact that the DYNAMIC segment is 
read-only sufficient to bail out with at error?  Or to rephrase, what 
valid use case would we be supporting by checking flags on the LOAD 
segment containing the .dynamic section to check for write permission?

I know that the crash wouldn't happen if the LOAD was read-write, but I 
don't know if the additional check covers a valid use case.

> And they do not want multiple LOAD segments because it can fit all on
> the same page.  We would end up with an RWX segment if we made this
> single LOAD segment writable, which is probably even more confusing.

Right.

> So I guess the status quo is probably best.

I don't know about that :)

> Maybe someone else can comment on the additional detection.  In the
> past, we said that crashes on corrupt ELF files weren't something we
> cared about, but I increasingly see th value of good diagnostics for
> common special cases.

This seems like a situation where we can add an inexpensive check and 
not crash.

> (See also the discussion about DSOs without entry points.)

I need to collect my thoughts about that.  I had run into it last year 
and I need to see if I had written something down then.  I'd definitely 
like to know if there is a historical perspective to setting .text as 
the entry point by default.

Siddhesh