| Message ID | 7b008fd34f802456db3731a043ff56683b569ff7.1604393169.git.szabolcs.nagy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) [BZ #26831] | expand |
* 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
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.
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. --
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. > > --
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