elf: Fix crash in late dlmopen failure when auditing (bug 26076)

  If _dl_map_object_deps fails, the initial module that has been
loaded by _dl_map_objects needs to be freed using _dl_close_worker.
This produces an empty namespace midway through _dl_close_worker.
The code did not expected and deferefenced a NULL pointer.

---
 elf/Makefile   | 16 +++++++++++++++-
 elf/dl-close.c |  2 +-
 2 files changed, 16 insertions(+), 2 deletions(-)
  

On 6/4/20 9:20 AM, Florian Weimer via Libc-alpha wrote:
> If _dl_map_object_deps fails, the initial module that has been
> loaded by _dl_map_objects needs to be freed using _dl_close_worker.
> This produces an empty namespace midway through _dl_close_worker.
> The code did not expected and deferefenced a NULL pointer.

The problem is that do_audit's value is wrong after unloading.

I think we need to clean this up. See below.
 
> ---
>  elf/Makefile   | 16 +++++++++++++++-
>  elf/dl-close.c |  2 +-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 6fe1df90bb..648d6d3d11 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -223,7 +223,7 @@ tests += $(tests-execstack-$(have-z-execstack))
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-leaks1-mem.out \
>  		 $(objpfx)tst-leaks1-static-mem.out $(objpfx)noload-mem.out \
> -		 $(objpfx)tst-ldconfig-X.out
> +		 $(objpfx)tst-ldconfig-X.out $(objpfx)tst-auditlatefail.out

OK.

>  endif
>  tlsmod17a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
>  tlsmod18a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
> @@ -1502,6 +1502,20 @@ $(objpfx)tst-auditmany.out: $(objpfx)tst-auditmanymod1.so \
>  tst-auditmany-ENV = \
>    LD_AUDIT=tst-auditmanymod1.so:tst-auditmanymod2.so:tst-auditmanymod3.so:tst-auditmanymod4.so:tst-auditmanymod5.so:tst-auditmanymod6.so:tst-auditmanymod7.so:tst-auditmanymod8.so:tst-auditmanymod9.so
>  
> +# Check that a late failure in loading an audit module does not result
> +# in a segmentation fault (bug 26076).  Reuse parts of the
> +# tst-auditmany test for this.  They tst-auditmany program just exits
> +# with zero, after loading has failed.  Check that the expected audit
> +# module load error message appears on standard error, and that output
> +# from the other (successfully loaded) audit module occurred.
> +$(objpfx)tst-auditlatefail.out: $(objpfx)tst-auditmanymod1.so \
> +  $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-auditmany Makefile
> +	$(test-wrapper) $(rtld-prefix) \
> +	  --audit tst-auditmanymod1.so:tst-dlopenfailmod1.so \
> +	  $(objpfx)tst-auditmany > $@ 2>&1 && \
> +	grep -q "object 'tst-dlopenfailmod1.so' cannot be loaded as audit interface: cannot open shared object file; ignored." $@ && \
> +	grep -q "^info: 1, la_objopen" $@; $(evaluate-test)

OK. All you need to do is map in an audit module that works, and one
that then fails late after being loaded.

> +
>  LDFLAGS-tst-audit14 = -Wl,--audit=tst-auditlogmod-1.so
>  $(objpfx)tst-auditlogmod-1.so: $(libsupport)
>  $(objpfx)tst-audit14.out: $(objpfx)tst-auditlogmod-1.so
> diff --git a/elf/dl-close.c b/elf/dl-close.c
> index 73b2817bbf..896e59e42e 100644
> --- a/elf/dl-close.c
> +++ b/elf/dl-close.c
> @@ -782,7 +782,7 @@ _dl_close_worker (struct link_map *map, bool force)
>      {
>        struct link_map *head = ns->_ns_loaded;
>        /* Do not call the functions for any auditing object.  */
> -      if (head->l_auditing == 0)
> +      if (head != NULL && head->l_auditing == 0)

Please recompute do_audit or refactor to remove do_audit and check
the appropriate values.

>  	{
>  	  struct audit_ifaces *afct = GLRO(dl_audit);
>  	  for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>
  

* Carlos O'Donell:

> Please recompute do_audit or refactor to remove do_audit and check
> the appropriate values.

I would like to give up this patch.

The core issue is that

  /* Mark the DSO as being used for auditing.  */
  dlmargs.map->l_auditing = 1;

in elf/rtld.c:load_audit_module is just too late to mark a namespace as
being used for auditing.  This is what the do_audit flag

  bool do_audit = GLRO(dl_naudit) > 0 && !ns->_ns_loaded->l_auditing;

is supposed to check.  The flag should be per-namespace, and set
immediately when the namespace ID is created for the audit module.  This
will make the reporting of audit events to other auditors more
consistent, too.

Szabolcs' namespace rework will conflict with a refactoring here, and
this change does not block the rseq work in anyway.

Thanks,
Florian
  

On 6/4/20 12:19 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> Please recompute do_audit or refactor to remove do_audit and check
>> the appropriate values.
> 
> I would like to give up this patch.

Your call. Thanks for filling the bug.

> The core issue is that
> 
>   /* Mark the DSO as being used for auditing.  */
>   dlmargs.map->l_auditing = 1;
> 
> in elf/rtld.c:load_audit_module is just too late to mark a namespace as
> being used for auditing.  This is what the do_audit flag

Late marking of a namespace as being used for auditing is OK?

>   bool do_audit = GLRO(dl_naudit) > 0 && !ns->_ns_loaded->l_auditing;
> 
> is supposed to check.
Right, so I assume you argue that if l_auditing had been set early as a
consequence of the fact that we are calling load_audit_module() and so
know we should never audit, that should work.

Let me counter with the following:

The same scenario can be created *without* audit modules.

Consider:
* We load libfoo.so into a new namespace.
* l_auditing is always 0.
* GLRO(dl_naudit) is > 0.

If we failed to load libfoo.so, then we would also crash since _ns_loaded
would be NULL because we unloaded everything from the namespace.

This use case has nothing to do with l_auditing.

> The flag should be per-namespace, and set
> immediately when the namespace ID is created for the audit module.  This
> will make the reporting of audit events to other auditors more
> consistent, too.

Yes, the flag should be per-namespace.

Yes, the it should be set when LM_ID_NEWLM + __RTLD_AUDIT is used.

However, I think the non-audit use case *requires* your original fix.

Given this I would accept your original fix with an adjusted comment.

Thoughts?
 
> Szabolcs' namespace rework will conflict with a refactoring here, and
> this change does not block the rseq work in anyway.