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

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

[Florian Weimer]

This does not fix any bugs as such, but makes it more obvious
if _dl_debug_initialize is called with invalid arguments
(which would otherwise cause the function to clobber unrelated
data).

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 elf/dl-debug.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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)));
+      r = &GL(dl_ns)[ns]._ns_debug;
+    }
 
   if (r->r_map == NULL || ldbase != 0)
     {

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

[Carlos O'Donell]

On 6/23/21 8:42 AM, Florian Weimer via Libc-alpha wrote:
> This does not fix any bugs as such, but makes it more obvious
> if _dl_debug_initialize is called with invalid arguments
> (which would otherwise cause the function to clobber unrelated
> data).
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.

I know I'm expanding the scope here to include _dl_map_object,
but it's another place where we have a similar check, and so
I'm just thinking that for consistency we should make both
robust in the same way.

I'm not asking you to fix what appears to be a problem in
dl_open_worker that we appear to do *no* validation of nsid
which is user controlled as input to dlopen :>
 
> ---
>  elf/dl-debug.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> 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?

> +      r = &GL(dl_ns)[ns]._ns_debug;
> +    }
>  
>    if (r->r_map == NULL || ldbase != 0)
>      {
> 


-- 
Cheers,
Carlos.

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

[Florian Weimer]

* 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.

Thanks,
Florian

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

[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? :-)

-- 
Cheers,
Carlos.

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

[Andreas Schwab]

On Jun 23 2021, Florian Weimer via Libc-alpha wrote:

> @@ -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 asserts can be combined.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Florian Weimer]

* 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