| Message ID | 87y2b04us6.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | elf: Assert range of ns argument in _dl_debug_initialize | expand |
| Context | Check | Description |
|---|---|---|
| dj/TryBot-apply_patch | success | Patch applied to master at the time it was sent |
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) > { >
* 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
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? :-)
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.
* 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
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) {