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

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 9/16/21 8:48 PM, H.J. Lu wrote:
> On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote:
>>> * H. J. Lu:
>>>
>>>> There is nothing wrong with read-only dynamic segment.
>>>
>>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION.
>>> ELF requires that DT_STRTAB is present.  DT_STRTAB needs relocation.
>>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in
>>> a read-ony LOAD segment for a valid ELF file.
>>
>> I agree strongly with this position.
>>
>> Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid
>> other security issues e.g. hardening not loosening the restrictions).
>>
>> In theory the vDSO is invalid.
>>
>> In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime
>> rather than statically at compile time for the target.

Actually it isn't.  The dynamic section in the vdso is already relocated 
by the kernel when it's mapped in.  DL_RO_DYN_SECTION DSOs are not 
relocated because of which any references to pointers written in the 
.dynamic section need to be relocated.

> 
> This patch makes DL_RO_DYN_SECTION semi-dynamic.  We can also simply
> remove DL_RO_DYN_SECTION.
> 
> > From dc8d0fbc17024696cfa9e1ed8df2ecee4bad4696 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Thu, 16 Sep 2021 08:15:29 -0700
> Subject: [PATCH] ld.so: Change DL_RO_DYN_SECTION to semi-dynamic
> 
> ---
>  elf/dl-load.c              |  1 +
>  elf/dl-reloc-static-pie.c  | 10 ++++++++++
>  elf/get-dynamic-info.h     |  4 +---
>  elf/rtld.c                 |  2 ++
>  include/link.h             |  1 +
>  sysdeps/generic/ldsodefs.h | 12 +++++++-----
>  6 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 650e4edc35..292e921292 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1149,6 +1149,7 @@ _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));
> +	      l->l_ld_readonly = (ph->p_flags & PF_W) == 0;

One concern Florian had was that the read-only decision should be made 
based on the LOAD segment flags and not the DYNAMIC segment flags.  It 
works for vdso since those permissions are in sync as far as write is 
concerned (RX and R for LOAD and DYNAMIC respectively) but if we want to 
write to .dynamic when LOAD is RW (but DYNAMIC isn't for some reason) 
then we should look for the LOAD segment that overlaps with this.

>  	    }
>  	  break;
>  
> diff --git a/elf/dl-reloc-static-pie.c b/elf/dl-reloc-static-pie.c
> index d5bd2f31e9..d5fbeb137c 100644
> --- a/elf/dl-reloc-static-pie.c
> +++ b/elf/dl-reloc-static-pie.c
> @@ -40,6 +40,16 @@ _dl_relocate_static_pie (void)
>  
>    /* Read our own dynamic section and fill in the info array.  */
>    main_map->l_ld = ((void *) main_map->l_addr + elf_machine_dynamic ());
> +
> +  const ElfW(Phdr) *ph, *phdr = GL(dl_phdr);
> +  size_t phnum = GL(dl_phnum);
> +  for (ph = phdr; ph < &phdr[phnum]; ++ph)
> +    if (ph->p_type == PT_DYNAMIC)
> +      {
> +	main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
> +	break;
> +      }
> +
>    elf_get_dynamic_info (main_map, NULL);

Same question here.

>  
>  # ifdef ELF_MACHINE_BEFORE_RTLD_RELOC
> diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
> index d8ec32377d..02dc281ec5 100644
> --- a/elf/get-dynamic-info.h
> +++ b/elf/get-dynamic-info.h
> @@ -71,9 +71,8 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>  
>  #define DL_RO_DYN_TEMP_CNT	8
>  
> -#ifndef DL_RO_DYN_SECTION
>    /* Don't adjust .dynamic unnecessarily.  */
> -  if (l->l_addr != 0)
> +  if (l->l_addr != 0 && !l->l_ld_readonly)

This still has the concern that applications will incorrectly assume 
that these entries are relocated.  If we want to continue to load DSOs 
with read-only DYNAMIC, shouldn't we just allocate memory (like we do 
for setup_vdso, but dynamically per DSO) and write the relocations there 
instead?

>      {
>        ElfW(Addr) l_addr = l->l_addr;
>        int cnt = 0;
> @@ -109,7 +108,6 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>  # undef ADJUST_DYN_INFO
>        assert (cnt <= DL_RO_DYN_TEMP_CNT);
>      }
> -#endif
>    if (info[DT_PLTREL] != NULL)
>      {
>  #if ELF_MACHINE_NO_RELA
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 878e6480f4..c7818586a2 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -463,6 +463,7 @@ _dl_start_final (void *arg, struct dl_start_final_info *info)
>  #ifndef DONT_USE_BOOTSTRAP_MAP
>    GL(dl_rtld_map).l_addr = info->l.l_addr;
>    GL(dl_rtld_map).l_ld = info->l.l_ld;
> +  GL(dl_rtld_map).l_ld_readonly = info->l.l_ld_readonly;
>    memcpy (GL(dl_rtld_map).l_info, info->l.l_info,
>  	  sizeof GL(dl_rtld_map).l_info);
>    GL(dl_rtld_map).l_mach = info->l.l_mach;
> @@ -1468,6 +1469,7 @@ dl_main (const ElfW(Phdr) *phdr,
>  	/* This tells us where to find the dynamic section,
>  	   which tells us everything we need to do.  */
>  	main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr;
> +	main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
>  	break;
>        case PT_INTERP:
>  	/* This "interpreter segment" was used by the program loader to
> diff --git a/include/link.h b/include/link.h
> index 4af16cb596..963a9f0147 100644
> --- a/include/link.h
> +++ b/include/link.h
> @@ -205,6 +205,7 @@ struct link_map
>      unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be
>  				       freed, ie. not allocated with
>  				       the dummy malloc in ld.so.  */
> +    unsigned int l_ld_readonly;	/* Nonzero if dynamic section is readonly.  */
>  
>      /* NODELETE status of the map.  Only valid for maps of type
>         lt_loaded.  Lazy binding sets l_nodelete_active directly,
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index fd67871f4b..3710d7ea79 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -69,17 +69,19 @@ __BEGIN_DECLS
>     `ElfW(TYPE)' is used in place of `Elf32_TYPE' or `Elf64_TYPE'.  */
>  #define ELFW(type)	_ElfW (ELF, __ELF_NATIVE_CLASS, type)
>  
> +#ifndef DL_RO_DYN_SECTION
> +# define DL_RO_DYN_SECTION 0
> +#endif
> +
>  /* All references to the value of l_info[DT_PLTGOT],
>    l_info[DT_STRTAB], l_info[DT_SYMTAB], l_info[DT_RELA],
>    l_info[DT_REL], l_info[DT_JMPREL], and l_info[VERSYMIDX (DT_VERSYM)]
>    have to be accessed via the D_PTR macro.  The macro is needed since for
>    most architectures the entry is already relocated - but for some not
>    and we need to relocate at access time.  */
> -#ifdef DL_RO_DYN_SECTION
> -# define D_PTR(map, i) ((map)->i->d_un.d_ptr + (map)->l_addr)
> -#else
> -# define D_PTR(map, i) (map)->i->d_un.d_ptr
> -#endif
> +#define D_PTR(map, i) \
> +  ((map)->i->d_un.d_ptr \
> +   + (DL_RO_DYN_SECTION || (map)->l_ld_readonly ? (map)->l_addr : 0))

OK for vDSO since it doesn't get marked as readonly in setup_vdso.

>  
>  /* Result of the lookup functions and how to retrieve the base address.  */
>  typedef struct link_map *lookup_t;
> -- 
> 2.31.1