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

[Carlos O'Donell <carlos@redhat.com>]

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.