malloc: Fix for infinite loop in memalign/posix_memalign.

Message ID 52569CED.3060700@linaro.org
State Accepted
Headers show

Commit Message

Will Newton Oct. 10, 2013, 12:26 p.m.
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(+)

Comments

Ondřej Bílka Oct. 11, 2013, 1:33 p.m. | #1
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?
Will Newton Oct. 11, 2013, 1:46 p.m. | #2
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.
Will Newton Oct. 17, 2013, 7:42 a.m. | #3
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?
Ondřej Bílka Oct. 21, 2013, 7:07 a.m. | #4
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.
Will Newton Oct. 29, 2013, 10:59 p.m. | #5
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,
Ryan S. Arnold Oct. 30, 2013, 12:43 a.m. | #6
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
Carlos O'Donell Oct. 30, 2013, 6:54 p.m. | #7
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.
Will Newton Oct. 30, 2013, 9:50 p.m. | #8
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.

Patch

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);