Re: [PATCH] elf: Assert range of ns argument in _dl_debug_initialize

[Florian Weimer <fweimer@redhat.com>]

* Carlos O'Donell:

> On 6/27/21 6:51 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>>> diff --git a/elf/dl-debug.c b/elf/dl-debug.c
>>>> index 2cd5f09753..85b087455e 100644
>>>> --- a/elf/dl-debug.c
>>>> +++ b/elf/dl-debug.c
>>>> @@ -16,6 +16,8 @@
>>>>     License along with the GNU C Library; if not, see
>>>>     <https://www.gnu.org/licenses/>.  */
>>>>  
>>>> +#include <array_length.h>
>>>> +#include <assert.h>
>>>>  #include <ldsodefs.h>
>>>>  
>>>>  
>>>> @@ -49,7 +51,11 @@ _dl_debug_initialize (ElfW(Addr) ldbase, Lmid_t ns)
>>>>    if (ns == LM_ID_BASE)
>>>>      r = &_r_debug;
>>>>    else
>>>> -    r = &GL(dl_ns)[ns]._ns_debug;
>>>> +    {
>>>> +      assert (ns >= 0);
>>>> +      assert (ns < array_length (GL (dl_ns)));
>>>
>>> The check in _dl_map_object is:
>>>   assert (nsid >= 0);
>>>   assert (nsid < GL(dl_nns));
>>>
>>> Should we be consistent one way or the other?
>> 
>> I wasn't sure if _dl_debug_initialize can be called with a
>> not-yet-allocated (or already-deallocated) namespace ID.  _dl_map_object
>> is somewhat higher-level, so it's not surprising that it expects an
>> active ID.  An out-of-bounds array access is clearly invalid, though.
>
> Assert on the tighter bound and we'll see? :-)

Do we do things this way?

I'm mainly interested in catching the LM_ID_NEWLM case, to be honest.

Thanks,
Florian