Re: [PATCH 03/13] elf: dont pass fd to _dl_process_pt_xx

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 03/30/2023 17:46, Adhemerval Zanella Netto wrote:
> On 30/03/23 13:08, stsp wrote:
> > 
> > 29.03.2023 22:10, Adhemerval Zanella Netto пишет:
> >>
> >> On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
> >>> It is not used in these functions.
> >>> rtld.c:rtld_setup_main_map() does the same.
> >>>
> >>> The test-suite was run on x86_64/64 and showed no regressions.
> >>>
> >>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> >>> ---
> >>>   elf/dl-load.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/elf/dl-load.c b/elf/dl-load.c
> >>> index fcb39a78d4..ab8b648687 100644
> >>> --- a/elf/dl-load.c
> >>> +++ b/elf/dl-load.c
> >>> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
> >>>       switch (ph[-1].p_type)
> >>>         {
> >>>         case PT_NOTE:
> >>> -    _dl_process_pt_note (l, fd, &ph[-1]);
> >>> +    _dl_process_pt_note (l, -1, &ph[-1]);
> >>>       break;
> >>>         case PT_GNU_PROPERTY:
> >>> -    _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> >>> +    _dl_process_pt_gnu_property (l, -1, &ph[-1]);
> >>>       break;
> >>>         }
> >>>   
> >>
> >> It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
> >> if the called where rtld code during statup code or dlopen.  But you are
> >> right that it is not used.
> >>
> >> However this does not accomplish anything, a better refactor would to just
> >> remove the argument altogether.  It at least would simplify the interface
> >> and allow slight better code generation.
> > 
> > I tried to do that, but there is also that
> > _dl_process_gnu_property() called from
> > _dl_process_pt_gnu_property().
> > For aarch64 it has this:
> >       unsigned int feature_1 = *(unsigned int *) data;
> >       if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> >         _dl_bti_protect (l, fd);
> > 
> > What should I do here to remove that fd?
> > 
> 
> In fact aarch64 _dl_bti_protect requires to know whether the map was done by 
> the kernel or not.
> 
> Szabolcs, shouldn't the code:
> 
> 1257   for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
> 1258     switch (ph[-1].p_type)
> 1259       {
> 1260       case PT_NOTE:
> 1261         _dl_process_pt_note (main_map, -1, &ph[-1]);
> 1262         break;
> 1263       case PT_GNU_PROPERTY:
> 1264         _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
> 1265         break;
> 1266       }
> 
> Take in consideration whether the main_map was allocated by the kernel or
> by loader to pass the correct value on 'fd'?  

if the exe is loaded by ld.so then we already took care of bti
(and other gnu properties) at load time.

here we could skip processing properties again in that case but
it does not hurt on aarch64 (it will try to mprotect with
PROT_BTI again and ignore any failures).

however the fd must be passed down in elf/dl-load.c otherwise
bti protection is silently dropped (when systemd filters out
mprotect with PROT_EXEC, this is being fixed, but there are old
systems configured like that).