| Message ID | 521327C7.3020501@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 20 August 2013 09:24, Will Newton <will.newton@linaro.org> wrote: > > A large bytes parameter to pvalloc could cause an integer overflow > and corrupt allocator internals. Check the overflow does not occur > before continuing with the allocation. > > ChangeLog: > > 2013-08-12 Will Newton <will.newton@linaro.org> > > [BZ #15855] > * malloc/malloc.c (__libc_pvalloc): Check the value of bytes > does not overflow. > --- > malloc/malloc.c | 4 ++++ > 1 file changed, 4 insertions(+) Does anybody have any comments on this patch series? Thanks,
On 20 August 2013 09:24, Will Newton <will.newton@linaro.org> wrote: > > A large bytes parameter to pvalloc could cause an integer overflow > and corrupt allocator internals. Check the overflow does not occur > before continuing with the allocation. > > ChangeLog: > > 2013-08-12 Will Newton <will.newton@linaro.org> > > [BZ #15855] > * malloc/malloc.c (__libc_pvalloc): Check the value of bytes > does not overflow. > --- > malloc/malloc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > Changes in v2: > - Add BZ number Ping?
On Tue, Aug 20, 2013 at 09:24:39AM +0100, Will Newton wrote: > > A large bytes parameter to pvalloc could cause an integer overflow > and corrupt allocator internals. Check the overflow does not occur > before continuing with the allocation. > > ChangeLog: > > 2013-08-12 Will Newton <will.newton@linaro.org> > > [BZ #15855] > * malloc/malloc.c (__libc_pvalloc): Check the value of bytes > does not overflow. > --- > malloc/malloc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > Changes in v2: > - Add BZ number > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index be472b2..7468758 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes) > size_t page_mask = GLRO(dl_pagesize) - 1; > size_t rounded_bytes = (bytes + page_mask) & ~(page_mask); > > + /* Check for overflow. */ > + if (bytes + 2*pagesz + MINSIZE < bytes) > + return 0; > + It might be safer to use SIZE_MAX instead of relying on overflow behaviour of the processor: if (SIZE_MAX - 2 * pagesz - MINSIZE < bytes) return 0; > void *(*hook) (size_t, size_t, const void *) = > force_reg (__memalign_hook); > if (__builtin_expect (hook != NULL, 0)) > -- > 1.8.1.4 > Siddhesh
On Tue, Sep 10, 2013 at 08:10:44AM +0530, Siddhesh Poyarekar wrote: > > diff --git a/malloc/malloc.c b/malloc/malloc.c > > index be472b2..7468758 100644 > > --- a/malloc/malloc.c > > +++ b/malloc/malloc.c > > @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes) > > size_t page_mask = GLRO(dl_pagesize) - 1; > > size_t rounded_bytes = (bytes + page_mask) & ~(page_mask); > > > > + /* Check for overflow. */ > > + if (bytes + 2*pagesz + MINSIZE < bytes) > > + return 0; > > + > > It might be safer to use SIZE_MAX instead of relying on overflow > behaviour of the processor: This code does not "rely on overflow behavior of the processor". Unsigned arithmetic cannot overflow. It's performed modulo one plus the maximum value of the type. In general, expressions of the form "a+b+c < a" are not safe to check for overflow; since even if a+b<a, a+b+c>=a is possible. However, since b+c (i.e. 2*pagesz+MINSIZE) cannot overflow in the above expression, the check is safe. > if (SIZE_MAX - 2 * pagesz - MINSIZE < bytes) > return 0; I do prefer this form, but you got it backwards in the process of Yoda-fying it. It should be: if (bytes > SIZE_MAX - 2 * pagesz - MINSIZE) and the "value being tested" should be on the left-hand side of a comparison (i.e. non-Yoda-fied). By the way, I think the code is failing to set errno, too. Rich
On Tue, Sep 10, 2013 at 12:46:00AM -0400, Rich Felker wrote: > This code does not "rely on overflow behavior of the processor". > Unsigned arithmetic cannot overflow. It's performed modulo one plus > the maximum value of the type. ACK. Thanks. Siddhesh
diff --git a/malloc/malloc.c b/malloc/malloc.c index be472b2..7468758 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes) size_t page_mask = GLRO(dl_pagesize) - 1; size_t rounded_bytes = (bytes + page_mask) & ~(page_mask); + /* Check for overflow. */ + if (bytes + 2*pagesz + MINSIZE < bytes) + return 0; + void *(*hook) (size_t, size_t, const void *) = force_reg (__memalign_hook); if (__builtin_expect (hook != NULL, 0))