[2/4] elf: Move note processing after l_phdr is updated [BZ #26831]
diff mbox series

Message ID 7b008fd34f802456db3731a043ff56683b569ff7.1604393169.git.szabolcs.nagy@arm.com
State New, archived
Headers show
Series
  • aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) [BZ #26831]
Related show

Commit Message

Szabolcs Nagy Nov. 3, 2020, 10:26 a.m. UTC
Program headers are processed in two pass: after the first pass
load segments are mmapped so in the second pass target specific
note processing logic can access the notes.

The second pass is moved later so various link_map fields are
set up that may be useful for note processing such as l_phdr.
---
 elf/dl-load.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Florian Weimer Nov. 3, 2020, 10:38 a.m. UTC | #1
* Szabolcs Nagy:

> Program headers are processed in two pass: after the first pass
> load segments are mmapped so in the second pass target specific
> note processing logic can access the notes.
>
> The second pass is moved later so various link_map fields are
> set up that may be useful for note processing such as l_phdr.
> ---
>  elf/dl-load.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index ceaab7f18e..673cf960a0 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1259,21 +1259,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  				  maplength, has_holes, loader);
>      if (__glibc_unlikely (errstring != NULL))
>        goto call_lose;
> -
> -    /* Process program headers again after load segments are mapped in
> -       case processing requires accessing those segments.  Scan program
> -       headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> -       exits.  */
> -    for (ph = &phdr[l->l_phnum]; ph != phdr; --ph)
> -      switch (ph[-1].p_type)
> -	{
> -	case PT_NOTE:
> -	  _dl_process_pt_note (l, fd, &ph[-1]);
> -	  break;
> -	case PT_GNU_PROPERTY:
> -	  _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> -	  break;
> -	}
>    }
>  
>    if (l->l_ld == 0)
> @@ -1481,6 +1466,21 @@ cannot enable executable stack as shared object requires");
>      /* Assign the next available module ID.  */
>      l->l_tls_modid = _dl_next_tls_modid ();
>  
> +  /* Process program headers again after load segments are mapped in
> +     case processing requires accessing those segments.  Scan program
> +     headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> +     exits.  */
> +  for (ph = &l->l_phdr[l->l_phnum]; ph != l->l_phdr; --ph)
> +    switch (ph[-1].p_type)
> +      {
> +      case PT_NOTE:
> +	_dl_process_pt_note (l, fd, &ph[-1]);
> +	break;
> +      case PT_GNU_PROPERTY:
> +	_dl_process_pt_gnu_property (l, fd, &ph[-1]);
> +	break;
> +      }
> +
>  #ifdef DL_AFTER_LOAD
>    DL_AFTER_LOAD (l);
>  #endif

Is this still compatible with the CET requirements?

I hope it is because the CET magic happens in _dl_open_check, so after
the the code in elf/dl-load.c has run.

Thanks,
Florian
H.J. Lu Nov. 3, 2020, 12:36 p.m. UTC | #2
On Tue, Nov 3, 2020 at 2:38 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Szabolcs Nagy:
>
> > Program headers are processed in two pass: after the first pass
> > load segments are mmapped so in the second pass target specific
> > note processing logic can access the notes.
> >
> > The second pass is moved later so various link_map fields are
> > set up that may be useful for note processing such as l_phdr.
> > ---
> >  elf/dl-load.c | 30 +++++++++++++++---------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > index ceaab7f18e..673cf960a0 100644
> > --- a/elf/dl-load.c
> > +++ b/elf/dl-load.c
> > @@ -1259,21 +1259,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> >                                 maplength, has_holes, loader);
> >      if (__glibc_unlikely (errstring != NULL))
> >        goto call_lose;
> > -
> > -    /* Process program headers again after load segments are mapped in
> > -       case processing requires accessing those segments.  Scan program
> > -       headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > -       exits.  */
> > -    for (ph = &phdr[l->l_phnum]; ph != phdr; --ph)
> > -      switch (ph[-1].p_type)
> > -     {
> > -     case PT_NOTE:
> > -       _dl_process_pt_note (l, fd, &ph[-1]);
> > -       break;
> > -     case PT_GNU_PROPERTY:
> > -       _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > -       break;
> > -     }
> >    }
> >
> >    if (l->l_ld == 0)
> > @@ -1481,6 +1466,21 @@ cannot enable executable stack as shared object requires");
> >      /* Assign the next available module ID.  */
> >      l->l_tls_modid = _dl_next_tls_modid ();
> >
> > +  /* Process program headers again after load segments are mapped in
> > +     case processing requires accessing those segments.  Scan program
> > +     headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > +     exits.  */
> > +  for (ph = &l->l_phdr[l->l_phnum]; ph != l->l_phdr; --ph)
> > +    switch (ph[-1].p_type)
> > +      {
> > +      case PT_NOTE:
> > +     _dl_process_pt_note (l, fd, &ph[-1]);
> > +     break;
> > +      case PT_GNU_PROPERTY:
> > +     _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > +     break;
> > +      }
> > +
> >  #ifdef DL_AFTER_LOAD
> >    DL_AFTER_LOAD (l);
> >  #endif
>
> Is this still compatible with the CET requirements?
>
> I hope it is because the CET magic happens in _dl_open_check, so after
> the the code in elf/dl-load.c has run.
>
>

_dl_process_pt_note and _dl_process_pt_gnu_property may call
_dl_signal_error.  Are we prepared to clean more things up when it
happens?  I am investigating:

https://sourceware.org/bugzilla/show_bug.cgi?id=26825

I don't think cleanup of _dl_process_pt_gnu_property failure is done
properly.
Szabolcs Nagy Nov. 3, 2020, 3:04 p.m. UTC | #3
The 11/03/2020 04:36, H.J. Lu wrote:
> On Tue, Nov 3, 2020 at 2:38 AM Florian Weimer <fweimer@redhat.com> wrote:
> > * Szabolcs Nagy:
> >
> > > Program headers are processed in two pass: after the first pass
> > > load segments are mmapped so in the second pass target specific
> > > note processing logic can access the notes.
> > >
> > > The second pass is moved later so various link_map fields are
> > > set up that may be useful for note processing such as l_phdr.
> > > ---
> > >  elf/dl-load.c | 30 +++++++++++++++---------------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > > index ceaab7f18e..673cf960a0 100644
> > > --- a/elf/dl-load.c
> > > +++ b/elf/dl-load.c
> > > @@ -1259,21 +1259,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> > >                                 maplength, has_holes, loader);
> > >      if (__glibc_unlikely (errstring != NULL))
> > >        goto call_lose;
> > > -
> > > -    /* Process program headers again after load segments are mapped in
> > > -       case processing requires accessing those segments.  Scan program
> > > -       headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > > -       exits.  */
> > > -    for (ph = &phdr[l->l_phnum]; ph != phdr; --ph)
> > > -      switch (ph[-1].p_type)
> > > -     {
> > > -     case PT_NOTE:
> > > -       _dl_process_pt_note (l, fd, &ph[-1]);
> > > -       break;
> > > -     case PT_GNU_PROPERTY:
> > > -       _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > > -       break;
> > > -     }
> > >    }
> > >
> > >    if (l->l_ld == 0)
> > > @@ -1481,6 +1466,21 @@ cannot enable executable stack as shared object requires");
> > >      /* Assign the next available module ID.  */
> > >      l->l_tls_modid = _dl_next_tls_modid ();
> > >
> > > +  /* Process program headers again after load segments are mapped in
> > > +     case processing requires accessing those segments.  Scan program
> > > +     headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > > +     exits.  */
> > > +  for (ph = &l->l_phdr[l->l_phnum]; ph != l->l_phdr; --ph)
> > > +    switch (ph[-1].p_type)
> > > +      {
> > > +      case PT_NOTE:
> > > +     _dl_process_pt_note (l, fd, &ph[-1]);
> > > +     break;
> > > +      case PT_GNU_PROPERTY:
> > > +     _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > > +     break;
> > > +      }
> > > +
> > >  #ifdef DL_AFTER_LOAD
> > >    DL_AFTER_LOAD (l);
> > >  #endif
> >
> > Is this still compatible with the CET requirements?
> >
> > I hope it is because the CET magic happens in _dl_open_check, so after
> > the the code in elf/dl-load.c has run.

i believe the note processing and later cet magic
are not affected by this code move.

but i did not test this with cet.

> 
> _dl_process_pt_note and _dl_process_pt_gnu_property may call
> _dl_signal_error.  Are we prepared to clean more things up when it
> happens?  I am investigating:

yeah, this is difficult to reason about.

it seems to me that after _dl_map_object returns there
may be _dl_map_object_deps which can fail in a way that
all of dlopen has to be rolled back, so if i move things
around in _dl_map_object that should not introduce new
issues.

but it is not clear to me how robust the dlopen code is
against arbitrary failure in dl_open_worker.

> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=26825
> 
> I don't think cleanup of _dl_process_pt_gnu_property failure is done
> properly.
> 
> -- 
> H.J.

--
H.J. Lu Nov. 3, 2020, 3:27 p.m. UTC | #4
On Tue, Nov 3, 2020 at 7:04 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 11/03/2020 04:36, H.J. Lu wrote:
> > On Tue, Nov 3, 2020 at 2:38 AM Florian Weimer <fweimer@redhat.com> wrote:
> > > * Szabolcs Nagy:
> > >
> > > > Program headers are processed in two pass: after the first pass
> > > > load segments are mmapped so in the second pass target specific
> > > > note processing logic can access the notes.
> > > >
> > > > The second pass is moved later so various link_map fields are
> > > > set up that may be useful for note processing such as l_phdr.
> > > > ---
> > > >  elf/dl-load.c | 30 +++++++++++++++---------------
> > > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > > > index ceaab7f18e..673cf960a0 100644
> > > > --- a/elf/dl-load.c
> > > > +++ b/elf/dl-load.c
> > > > @@ -1259,21 +1259,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> > > >                                 maplength, has_holes, loader);
> > > >      if (__glibc_unlikely (errstring != NULL))
> > > >        goto call_lose;
> > > > -
> > > > -    /* Process program headers again after load segments are mapped in
> > > > -       case processing requires accessing those segments.  Scan program
> > > > -       headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > > > -       exits.  */
> > > > -    for (ph = &phdr[l->l_phnum]; ph != phdr; --ph)
> > > > -      switch (ph[-1].p_type)
> > > > -     {
> > > > -     case PT_NOTE:
> > > > -       _dl_process_pt_note (l, fd, &ph[-1]);
> > > > -       break;
> > > > -     case PT_GNU_PROPERTY:
> > > > -       _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > > > -       break;
> > > > -     }
> > > >    }
> > > >
> > > >    if (l->l_ld == 0)
> > > > @@ -1481,6 +1466,21 @@ cannot enable executable stack as shared object requires");
> > > >      /* Assign the next available module ID.  */
> > > >      l->l_tls_modid = _dl_next_tls_modid ();
> > > >
> > > > +  /* Process program headers again after load segments are mapped in
> > > > +     case processing requires accessing those segments.  Scan program
> > > > +     headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
> > > > +     exits.  */
> > > > +  for (ph = &l->l_phdr[l->l_phnum]; ph != l->l_phdr; --ph)
> > > > +    switch (ph[-1].p_type)
> > > > +      {
> > > > +      case PT_NOTE:
> > > > +     _dl_process_pt_note (l, fd, &ph[-1]);
> > > > +     break;
> > > > +      case PT_GNU_PROPERTY:
> > > > +     _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> > > > +     break;
> > > > +      }
> > > > +
> > > >  #ifdef DL_AFTER_LOAD
> > > >    DL_AFTER_LOAD (l);
> > > >  #endif
> > >
> > > Is this still compatible with the CET requirements?
> > >
> > > I hope it is because the CET magic happens in _dl_open_check, so after
> > > the the code in elf/dl-load.c has run.
>
> i believe the note processing and later cet magic
> are not affected by this code move.
>
> but i did not test this with cet.
>
> >
> > _dl_process_pt_note and _dl_process_pt_gnu_property may call
> > _dl_signal_error.  Are we prepared to clean more things up when it
> > happens?  I am investigating:
>
> yeah, this is difficult to reason about.
>
> it seems to me that after _dl_map_object returns there
> may be _dl_map_object_deps which can fail in a way that
> all of dlopen has to be rolled back, so if i move things
> around in _dl_map_object that should not introduce new
> issues.

I haven't investigated it in detail.  But there are

1314   if (l->l_phdr == NULL)
1315     {
1316       /* The program header is not contained in any of the segments.
1317          We have to allocate memory ourself and copy it over from out
1318          temporary place.  */
1319       ElfW(Phdr) *newp = (ElfW(Phdr) *) malloc (header->e_phnum
1320                                                 * sizeof (ElfW(Phdr)));
1321       if (newp == NULL)
1322         {
1323           errstring = N_("cannot allocate memory for program header");
1324           goto call_lose_errno;
1325         }
1326
1327       l->l_phdr = memcpy (newp, phdr,
1328                           (header->e_phnum * sizeof (ElfW(Phdr))));
1329       l->l_phdr_allocated = 1;
1330     }

When _dl_process_pt_gnu_property is moved after it, will l_phdr be
free on _dl_signal_error?

> but it is not clear to me how robust the dlopen code is
> against arbitrary failure in dl_open_worker.

I think we are mostly OK, except for some corner cases.   Delay
_dl_process_pt_gnu_property may introduce more corner cases.

> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=26825
> >
> > I don't think cleanup of _dl_process_pt_gnu_property failure is done
> > properly.
> >
> > --
> > H.J.
>
> --

Patch
diff mbox series

diff --git a/elf/dl-load.c b/elf/dl-load.c
index ceaab7f18e..673cf960a0 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1259,21 +1259,6 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 				  maplength, has_holes, loader);
     if (__glibc_unlikely (errstring != NULL))
       goto call_lose;
-
-    /* Process program headers again after load segments are mapped in
-       case processing requires accessing those segments.  Scan program
-       headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
-       exits.  */
-    for (ph = &phdr[l->l_phnum]; ph != phdr; --ph)
-      switch (ph[-1].p_type)
-	{
-	case PT_NOTE:
-	  _dl_process_pt_note (l, fd, &ph[-1]);
-	  break;
-	case PT_GNU_PROPERTY:
-	  _dl_process_pt_gnu_property (l, fd, &ph[-1]);
-	  break;
-	}
   }
 
   if (l->l_ld == 0)
@@ -1481,6 +1466,21 @@  cannot enable executable stack as shared object requires");
     /* Assign the next available module ID.  */
     l->l_tls_modid = _dl_next_tls_modid ();
 
+  /* Process program headers again after load segments are mapped in
+     case processing requires accessing those segments.  Scan program
+     headers backward so that PT_NOTE can be skipped if PT_GNU_PROPERTY
+     exits.  */
+  for (ph = &l->l_phdr[l->l_phnum]; ph != l->l_phdr; --ph)
+    switch (ph[-1].p_type)
+      {
+      case PT_NOTE:
+	_dl_process_pt_note (l, fd, &ph[-1]);
+	break;
+      case PT_GNU_PROPERTY:
+	_dl_process_pt_gnu_property (l, fd, &ph[-1]);
+	break;
+      }
+
 #ifdef DL_AFTER_LOAD
   DL_AFTER_LOAD (l);
 #endif