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

[Carlos O'Donell]

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.


-- 
Cheers,
Carlos.