| Message ID | 52569CED.3060700@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Oct 10, 2013 at 01:26:21PM +0100, Will Newton wrote: > + > + /* Test to expose integer overflow in malloc internals from BZ #16038. */ > + p = memalign (-1, pagesize); > + > + save = errno; > + > + if (p != NULL) > + merror ("memalign (-1, pagesize) succeeded."); > + > + if (p == NULL && save != EINVAL) > + merror ("memalign (-1, -pagesize) errno is not set correctly"); > + Why are you switching between pagesize and -pagesize?
On 11 October 2013 14:33, Ondřej Bílka <neleai@seznam.cz> wrote: > On Thu, Oct 10, 2013 at 01:26:21PM +0100, Will Newton wrote: >> + >> + /* Test to expose integer overflow in malloc internals from BZ #16038. */ >> + p = memalign (-1, pagesize); >> + >> + save = errno; >> + >> + if (p != NULL) >> + merror ("memalign (-1, pagesize) succeeded."); >> + >> + if (p == NULL && save != EINVAL) >> + merror ("memalign (-1, -pagesize) errno is not set correctly"); >> + > Why are you switching between pagesize and -pagesize? Thanks for spotting the typo, fixed.
On 10 October 2013 13:26, Will Newton <will.newton@linaro.org> wrote: > > A very large alignment argument passed to mealign/posix_memalign > causes _int_memalign to enter an infinite loop. Limit the maximum > alignment value to the maximum representable power of two to > prevent this from happening. > > Changelog: > > 2013-10-10 Will Newton <will.newton@linaro.org> > > [BZ #16038] > * malloc/hooks.c (memalign_check): Limit alignment to the > maximum representable power of two. > * malloc/malloc.c (__libc_memalign): Likewise. > * malloc/tst-memalign.c (do_test): Add test for very > large alignment values. > * malloc/tst-posix_memalign.c (do_test): Likewise. > --- > malloc/hooks.c | 8 ++++++++ > malloc/malloc.c | 8 ++++++++ > malloc/tst-memalign.c | 15 +++++++++++++++ > malloc/tst-posix_memalign.c | 10 ++++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/malloc/hooks.c b/malloc/hooks.c > index 3f663bb..1dbe93f 100644 > --- a/malloc/hooks.c > +++ b/malloc/hooks.c > @@ -361,6 +361,14 @@ 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 the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a > + power of 2 and will cause overflow in the check below. */ > + if (alignment > SIZE_MAX / 2 + 1) > + { > + __set_errno (EINVAL); > + return 0; > + } > + > /* Check for overflow. */ > if (bytes > SIZE_MAX - alignment - MINSIZE) > { > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 2938234..f1949f0 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -3026,6 +3026,14 @@ __libc_memalign(size_t alignment, size_t bytes) > /* Otherwise, ensure that it is at least a minimum chunk size */ > if (alignment < MINSIZE) alignment = MINSIZE; > > + /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a > + power of 2 and will cause overflow in the check below. */ > + if (alignment > SIZE_MAX / 2 + 1) > + { > + __set_errno (EINVAL); > + return 0; > + } > + > /* Check for overflow. */ > if (bytes > SIZE_MAX - alignment - MINSIZE) > { > diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c > index 1c59752..b8eec9e 100644 > --- a/malloc/tst-memalign.c > +++ b/malloc/tst-memalign.c > @@ -70,6 +70,21 @@ do_test (void) > > free (p); > > + errno = 0; > + > + /* Test to expose integer overflow in malloc internals from BZ #16038. */ > + p = memalign (-1, pagesize); > + > + save = errno; > + > + if (p != NULL) > + merror ("memalign (-1, pagesize) succeeded."); > + > + if (p == NULL && save != EINVAL) > + merror ("memalign (-1, -pagesize) errno is not set correctly"); > + > + free (p); > + > /* A zero-sized allocation should succeed with glibc, returning a > non-NULL value. */ > p = memalign (sizeof (void *), 0); > diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c > index 27c0dd2..7f34e37 100644 > --- a/malloc/tst-posix_memalign.c > +++ b/malloc/tst-posix_memalign.c > @@ -65,6 +65,16 @@ do_test (void) > > p = NULL; > > + /* Test to expose integer overflow in malloc internals from BZ #16038. */ > + ret = posix_memalign (&p, -1, pagesize); > + > + if (ret != EINVAL) > + merror ("posix_memalign (&p, -1, pagesize) succeeded."); > + > + free (p); > + > + p = NULL; > + > /* A zero-sized allocation should succeed with glibc, returning zero > and setting p to a non-NULL value. */ > ret = posix_memalign (&p, sizeof (void *), 0); > -- > 1.8.1.4 > Ping?
On Thu, Oct 17, 2013 at 08:42:42AM +0100, Will Newton wrote: > On 10 October 2013 13:26, Will Newton <will.newton@linaro.org> wrote: > > > > A very large alignment argument passed to mealign/posix_memalign > > causes _int_memalign to enter an infinite loop. Limit the maximum > > alignment value to the maximum representable power of two to > > prevent this from happening. > > > > Ping? > looks ok.
On 17 October 2013 00:42, Will Newton <will.newton@linaro.org> wrote: > On 10 October 2013 13:26, Will Newton <will.newton@linaro.org> wrote: >> >> A very large alignment argument passed to mealign/posix_memalign >> causes _int_memalign to enter an infinite loop. Limit the maximum >> alignment value to the maximum representable power of two to >> prevent this from happening. >> >> Changelog: >> >> 2013-10-10 Will Newton <will.newton@linaro.org> >> >> [BZ #16038] >> * malloc/hooks.c (memalign_check): Limit alignment to the >> maximum representable power of two. >> * malloc/malloc.c (__libc_memalign): Likewise. >> * malloc/tst-memalign.c (do_test): Add test for very >> large alignment values. >> * malloc/tst-posix_memalign.c (do_test): Likewise. >> --- >> malloc/hooks.c | 8 ++++++++ >> malloc/malloc.c | 8 ++++++++ >> malloc/tst-memalign.c | 15 +++++++++++++++ >> malloc/tst-posix_memalign.c | 10 ++++++++++ >> 4 files changed, 41 insertions(+) >> >> diff --git a/malloc/hooks.c b/malloc/hooks.c >> index 3f663bb..1dbe93f 100644 >> --- a/malloc/hooks.c >> +++ b/malloc/hooks.c >> @@ -361,6 +361,14 @@ 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 the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a >> + power of 2 and will cause overflow in the check below. */ >> + if (alignment > SIZE_MAX / 2 + 1) >> + { >> + __set_errno (EINVAL); >> + return 0; >> + } >> + >> /* Check for overflow. */ >> if (bytes > SIZE_MAX - alignment - MINSIZE) >> { >> diff --git a/malloc/malloc.c b/malloc/malloc.c >> index 2938234..f1949f0 100644 >> --- a/malloc/malloc.c >> +++ b/malloc/malloc.c >> @@ -3026,6 +3026,14 @@ __libc_memalign(size_t alignment, size_t bytes) >> /* Otherwise, ensure that it is at least a minimum chunk size */ >> if (alignment < MINSIZE) alignment = MINSIZE; >> >> + /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a >> + power of 2 and will cause overflow in the check below. */ >> + if (alignment > SIZE_MAX / 2 + 1) >> + { >> + __set_errno (EINVAL); >> + return 0; >> + } >> + >> /* Check for overflow. */ >> if (bytes > SIZE_MAX - alignment - MINSIZE) >> { >> diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c >> index 1c59752..b8eec9e 100644 >> --- a/malloc/tst-memalign.c >> +++ b/malloc/tst-memalign.c >> @@ -70,6 +70,21 @@ do_test (void) >> >> free (p); >> >> + errno = 0; >> + >> + /* Test to expose integer overflow in malloc internals from BZ #16038. */ >> + p = memalign (-1, pagesize); >> + >> + save = errno; >> + >> + if (p != NULL) >> + merror ("memalign (-1, pagesize) succeeded."); >> + >> + if (p == NULL && save != EINVAL) >> + merror ("memalign (-1, -pagesize) errno is not set correctly"); >> + >> + free (p); >> + >> /* A zero-sized allocation should succeed with glibc, returning a >> non-NULL value. */ >> p = memalign (sizeof (void *), 0); >> diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c >> index 27c0dd2..7f34e37 100644 >> --- a/malloc/tst-posix_memalign.c >> +++ b/malloc/tst-posix_memalign.c >> @@ -65,6 +65,16 @@ do_test (void) >> >> p = NULL; >> >> + /* Test to expose integer overflow in malloc internals from BZ #16038. */ >> + ret = posix_memalign (&p, -1, pagesize); >> + >> + if (ret != EINVAL) >> + merror ("posix_memalign (&p, -1, pagesize) succeeded."); >> + >> + free (p); >> + >> + p = NULL; >> + >> /* A zero-sized allocation should succeed with glibc, returning zero >> and setting p to a non-NULL value. */ >> ret = posix_memalign (&p, sizeof (void *), 0); >> -- >> 1.8.1.4 >> > > Ping? Ping? Any further comments on this or should I go ahead and commit? Thanks,
On Thu, Oct 10, 2013 at 5:26 AM, Will Newton <will.newton@linaro.org> wrote: > > A very large alignment argument passed to mealign/posix_memalign > causes _int_memalign to enter an infinite loop. Limit the maximum > alignment value to the maximum representable power of two to > prevent this from happening. > > Changelog: > > 2013-10-10 Will Newton <will.newton@linaro.org> > > [BZ #16038] > * malloc/hooks.c (memalign_check): Limit alignment to the > maximum representable power of two. > * malloc/malloc.c (__libc_memalign): Likewise. > * malloc/tst-memalign.c (do_test): Add test for very > large alignment values. > * malloc/tst-posix_memalign.c (do_test): Likewise. I think the patch looks good. Carlos, what do you think? Ryan
On 10/10/2013 08:26 AM, Will Newton wrote: > A very large alignment argument passed to mealign/posix_memalign > causes _int_memalign to enter an infinite loop. Limit the maximum > alignment value to the maximum representable power of two to > prevent this from happening. OK to checkin. This looks good to me, adds tests with comments, and detects an invalid alignment which is required given the linux kernel man pages documentation. We still lack updated manual info for memalign and posix_memalign where we should be covering what is returned as valid errno values for these functions e.g. EINVAL and ENOMEM (that is to say that manual/memory.texi doesn't say that memalign and posix_memalign return any useful errors). > Changelog: > > 2013-10-10 Will Newton <will.newton@linaro.org> > > [BZ #16038] > * malloc/hooks.c (memalign_check): Limit alignment to the > maximum representable power of two. > * malloc/malloc.c (__libc_memalign): Likewise. > * malloc/tst-memalign.c (do_test): Add test for very > large alignment values. > * malloc/tst-posix_memalign.c (do_test): Likewise. > --- > malloc/hooks.c | 8 ++++++++ > malloc/malloc.c | 8 ++++++++ > malloc/tst-memalign.c | 15 +++++++++++++++ > malloc/tst-posix_memalign.c | 10 ++++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/malloc/hooks.c b/malloc/hooks.c > index 3f663bb..1dbe93f 100644 > --- a/malloc/hooks.c > +++ b/malloc/hooks.c > @@ -361,6 +361,14 @@ 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 the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a > + power of 2 and will cause overflow in the check below. */ > + if (alignment > SIZE_MAX / 2 + 1) > + { > + __set_errno (EINVAL); > + return 0; > + } > + OK. > /* Check for overflow. */ > if (bytes > SIZE_MAX - alignment - MINSIZE) > { > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 2938234..f1949f0 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -3026,6 +3026,14 @@ __libc_memalign(size_t alignment, size_t bytes) > /* Otherwise, ensure that it is at least a minimum chunk size */ > if (alignment < MINSIZE) alignment = MINSIZE; > > + /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a > + power of 2 and will cause overflow in the check below. */ > + if (alignment > SIZE_MAX / 2 + 1) > + { > + __set_errno (EINVAL); > + return 0; > + } > + OK. > /* Check for overflow. */ > if (bytes > SIZE_MAX - alignment - MINSIZE) > { > diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c > index 1c59752..b8eec9e 100644 > --- a/malloc/tst-memalign.c > +++ b/malloc/tst-memalign.c > @@ -70,6 +70,21 @@ do_test (void) > > free (p); > > + errno = 0; > + > + /* Test to expose integer overflow in malloc internals from BZ #16038. */ > + p = memalign (-1, pagesize); > + > + save = errno; > + > + if (p != NULL) > + merror ("memalign (-1, pagesize) succeeded."); > + > + if (p == NULL && save != EINVAL) > + merror ("memalign (-1, -pagesize) errno is not set correctly"); As Ondrej pointed out that should be "pagesize". > + > + free (p); > + OK. > /* A zero-sized allocation should succeed with glibc, returning a > non-NULL value. */ > p = memalign (sizeof (void *), 0); > diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c > index 27c0dd2..7f34e37 100644 > --- a/malloc/tst-posix_memalign.c > +++ b/malloc/tst-posix_memalign.c > @@ -65,6 +65,16 @@ do_test (void) > > p = NULL; > > + /* Test to expose integer overflow in malloc internals from BZ #16038. */ > + ret = posix_memalign (&p, -1, pagesize); > + > + if (ret != EINVAL) > + merror ("posix_memalign (&p, -1, pagesize) succeeded."); > + > + free (p); > + > + p = NULL; > + OK. > /* A zero-sized allocation should succeed with glibc, returning zero > and setting p to a non-NULL value. */ > ret = posix_memalign (&p, sizeof (void *), 0); > Cheers, Carlos.
On 30 October 2013 11:54, Carlos O'Donell <carlos@redhat.com> wrote: > On 10/10/2013 08:26 AM, Will Newton wrote: >> A very large alignment argument passed to mealign/posix_memalign >> causes _int_memalign to enter an infinite loop. Limit the maximum >> alignment value to the maximum representable power of two to >> prevent this from happening. > > OK to checkin. Thanks, applied. > This looks good to me, adds tests with comments, and detects an > invalid alignment which is required given the linux kernel man pages > documentation. > > We still lack updated manual info for memalign and posix_memalign > where we should be covering what is returned as valid errno values > for these functions e.g. EINVAL and ENOMEM (that is to say that > manual/memory.texi doesn't say that memalign and posix_memalign > return any useful errors). I've added that to my todo list.
diff --git a/malloc/hooks.c b/malloc/hooks.c index 3f663bb..1dbe93f 100644 --- a/malloc/hooks.c +++ b/malloc/hooks.c @@ -361,6 +361,14 @@ 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 the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a + power of 2 and will cause overflow in the check below. */ + if (alignment > SIZE_MAX / 2 + 1) + { + __set_errno (EINVAL); + return 0; + } + /* Check for overflow. */ if (bytes > SIZE_MAX - alignment - MINSIZE) { diff --git a/malloc/malloc.c b/malloc/malloc.c index 2938234..f1949f0 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3026,6 +3026,14 @@ __libc_memalign(size_t alignment, size_t bytes) /* Otherwise, ensure that it is at least a minimum chunk size */ if (alignment < MINSIZE) alignment = MINSIZE; + /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a + power of 2 and will cause overflow in the check below. */ + if (alignment > SIZE_MAX / 2 + 1) + { + __set_errno (EINVAL); + return 0; + } + /* Check for overflow. */ if (bytes > SIZE_MAX - alignment - MINSIZE) { diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c index 1c59752..b8eec9e 100644 --- a/malloc/tst-memalign.c +++ b/malloc/tst-memalign.c @@ -70,6 +70,21 @@ do_test (void) free (p); + errno = 0; + + /* Test to expose integer overflow in malloc internals from BZ #16038. */ + p = memalign (-1, pagesize); + + save = errno; + + if (p != NULL) + merror ("memalign (-1, pagesize) succeeded."); + + if (p == NULL && save != EINVAL) + merror ("memalign (-1, -pagesize) errno is not set correctly"); + + free (p); + /* A zero-sized allocation should succeed with glibc, returning a non-NULL value. */ p = memalign (sizeof (void *), 0); diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c index 27c0dd2..7f34e37 100644 --- a/malloc/tst-posix_memalign.c +++ b/malloc/tst-posix_memalign.c @@ -65,6 +65,16 @@ do_test (void) p = NULL; + /* Test to expose integer overflow in malloc internals from BZ #16038. */ + ret = posix_memalign (&p, -1, pagesize); + + if (ret != EINVAL) + merror ("posix_memalign (&p, -1, pagesize) succeeded."); + + free (p); + + p = NULL; + /* A zero-sized allocation should succeed with glibc, returning zero and setting p to a non-NULL value. */ ret = posix_memalign (&p, sizeof (void *), 0);