| Message ID | 52555E49.4050506@linaro.org |
|---|---|
| State | Accepted |
| Commit | 321e26847188300173a5dc0ca42c2ff7b9bf7a78 |
| Headers | show |
On Wed, 9 Oct 2013, Will Newton wrote: > + /* Check for overflow. */ > + if (bytes > SIZE_MAX - alignment - MINSIZE) At this point no upper bound is established on the value of 'alignment', so the test may pass when 'alignment' is so large that right-hand side overflows. (also, when 'alignment' is larger than SIZE_MAX/2+1, _int_memalign enters an infinite loop) Alexander
On 9 October 2013 15:15, Alexander Monakov <amonakov@ispras.ru> wrote: > On Wed, 9 Oct 2013, Will Newton wrote: >> + /* Check for overflow. */ >> + if (bytes > SIZE_MAX - alignment - MINSIZE) > > At this point no upper bound is established on the value of 'alignment', so > the test may pass when 'alignment' is so large that right-hand side > overflows. Thanks for noticing this. If it's ok I'd like to deal with that issue as a separate patch though.
On 10/09/2013 09:46 AM, Will Newton wrote: > > A large value of bytes passed to memalign_check can cause an integer > overflow in _int_memalign and heap corruption. This issue can be > exposed by running tst-memalign with MALLOC_CHECK_=3. > > ChangeLog: > > 2013-10-09 Will Newton <will.newton@linaro.org> > > * malloc/hooks.c (memalign_check): Ensure the value of bytes > passed to _int_memalign does not overflow. > --- > malloc/hooks.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/malloc/hooks.c b/malloc/hooks.c > index 8c25846..3f663bb 100644 > --- a/malloc/hooks.c > +++ b/malloc/hooks.c > @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller) > if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL); > if (alignment < MINSIZE) alignment = MINSIZE; > > - if (bytes+1 == 0) { > - __set_errno (ENOMEM); > - return NULL; > - } > + /* Check for overflow. */ > + if (bytes > SIZE_MAX - alignment - MINSIZE) > + { > + __set_errno (ENOMEM); > + return 0; > + } > + > (void)mutex_lock(&main_arena.mutex); > mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) : > NULL; > This is better than `bytes+1' so it should go in immediately to fix the test regression under _MALLOC_CHECK=3. We still need to deal with `alignment' having no upper bound. Cheers, Carlos.
On 9 October 2013 19:36, Carlos O'Donell <carlos@redhat.com> wrote: Hi Carlos, > On 10/09/2013 09:46 AM, Will Newton wrote: >> >> A large value of bytes passed to memalign_check can cause an integer >> overflow in _int_memalign and heap corruption. This issue can be >> exposed by running tst-memalign with MALLOC_CHECK_=3. >> >> ChangeLog: >> >> 2013-10-09 Will Newton <will.newton@linaro.org> >> >> * malloc/hooks.c (memalign_check): Ensure the value of bytes >> passed to _int_memalign does not overflow. >> --- >> malloc/hooks.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/malloc/hooks.c b/malloc/hooks.c >> index 8c25846..3f663bb 100644 >> --- a/malloc/hooks.c >> +++ b/malloc/hooks.c >> @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller) >> if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL); >> if (alignment < MINSIZE) alignment = MINSIZE; >> >> - if (bytes+1 == 0) { >> - __set_errno (ENOMEM); >> - return NULL; >> - } >> + /* Check for overflow. */ >> + if (bytes > SIZE_MAX - alignment - MINSIZE) >> + { >> + __set_errno (ENOMEM); >> + return 0; >> + } >> + >> (void)mutex_lock(&main_arena.mutex); >> mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) : >> NULL; >> > > This is better than `bytes+1' so it should go in immediately to fix > the test regression under _MALLOC_CHECK=3. I took that as an OK and applied the patch.
On 10/10/2013 09:56 AM, Will Newton wrote: > On 9 October 2013 19:36, Carlos O'Donell <carlos@redhat.com> wrote: > > Hi Carlos, > >> On 10/09/2013 09:46 AM, Will Newton wrote: >>> >>> A large value of bytes passed to memalign_check can cause an integer >>> overflow in _int_memalign and heap corruption. This issue can be >>> exposed by running tst-memalign with MALLOC_CHECK_=3. >>> >>> ChangeLog: >>> >>> 2013-10-09 Will Newton <will.newton@linaro.org> >>> >>> * malloc/hooks.c (memalign_check): Ensure the value of bytes >>> passed to _int_memalign does not overflow. >>> --- >>> malloc/hooks.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/malloc/hooks.c b/malloc/hooks.c >>> index 8c25846..3f663bb 100644 >>> --- a/malloc/hooks.c >>> +++ b/malloc/hooks.c >>> @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller) >>> if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL); >>> if (alignment < MINSIZE) alignment = MINSIZE; >>> >>> - if (bytes+1 == 0) { >>> - __set_errno (ENOMEM); >>> - return NULL; >>> - } >>> + /* Check for overflow. */ >>> + if (bytes > SIZE_MAX - alignment - MINSIZE) >>> + { >>> + __set_errno (ENOMEM); >>> + return 0; >>> + } >>> + >>> (void)mutex_lock(&main_arena.mutex); >>> mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) : >>> NULL; >>> >> >> This is better than `bytes+1' so it should go in immediately to fix >> the test regression under _MALLOC_CHECK=3. > > I took that as an OK and applied the patch. It was. Sorry for not being sufficiently explicit. Cheers, Carlos.
diff --git a/malloc/hooks.c b/malloc/hooks.c index 8c25846..3f663bb 100644 --- a/malloc/hooks.c +++ b/malloc/hooks.c @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller) if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL); if (alignment < MINSIZE) alignment = MINSIZE; - if (bytes+1 == 0) { - __set_errno (ENOMEM); - return NULL; - } + /* Check for overflow. */ + if (bytes > SIZE_MAX - alignment - MINSIZE) + { + __set_errno (ENOMEM); + return 0; + } + (void)mutex_lock(&main_arena.mutex); mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) : NULL;