[PATCH 1/4] x86: Fix ptrdiff_t for x86_64
sbrk() assumes ptrdiff_t is large enough to enlarge/shrink the heap by LONG_MIN/LONG_MAX. So, use the long type, also to match the rest of the Linux ecosystem.
Signed-off-by: Richard Weinberger richard@nod.at --- arch/x86/include/asm/posix_types.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h index dbcea7f47f..e1ed9bcabc 100644 --- a/arch/x86/include/asm/posix_types.h +++ b/arch/x86/include/asm/posix_types.h @@ -20,11 +20,12 @@ typedef unsigned short __kernel_gid_t; #if defined(__x86_64__) typedef unsigned long __kernel_size_t; typedef long __kernel_ssize_t; +typedef long __kernel_ptrdiff_t; #else typedef unsigned int __kernel_size_t; typedef int __kernel_ssize_t; -#endif typedef int __kernel_ptrdiff_t; +#endif typedef long __kernel_time_t; typedef long __kernel_suseconds_t; typedef long __kernel_clock_t;
req is of type size_t, casting it to long opens the door for an integer overflow. Values between LONG_MAX - (SIZE_SZ + MALLOC_ALIGN_MASK) - 1 and LONG_MAX cause and overflow such that request2size() returns MINSIZE.
Fix by removing the cast. The origin of the cast is unclear, it's in u-boot and ppcboot since ever and predates the CVS history. Doug Lea's original dlmalloc implementation also doesn't have it.
Signed-off-by: Richard Weinberger richard@nod.at --- common/dlmalloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 62e8557daa..44b06e38b2 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -386,8 +386,8 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ /* pad request bytes into a usable size */
#define request2size(req) \ - (((long)((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) < \ - (long)(MINSIZE + MALLOC_ALIGN_MASK)) ? MINSIZE : \ + ((((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) < \ + (MINSIZE + MALLOC_ALIGN_MASK)) ? MINSIZE : \ (((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) & ~(MALLOC_ALIGN_MASK)))
/* Check if m has acceptable alignment */
On Fri, 2 Aug 2024 at 04:08, Richard Weinberger richard@nod.at wrote:
req is of type size_t, casting it to long opens the door for an integer overflow. Values between LONG_MAX - (SIZE_SZ + MALLOC_ALIGN_MASK) - 1 and LONG_MAX cause and overflow such that request2size() returns MINSIZE.
Fix by removing the cast. The origin of the cast is unclear, it's in u-boot and ppcboot since ever and predates the CVS history. Doug Lea's original dlmalloc implementation also doesn't have it.
Signed-off-by: Richard Weinberger richard@nod.at
common/dlmalloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 62e8557daa..44b06e38b2 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -386,8 +386,8 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ /* pad request bytes into a usable size */
#define request2size(req) \
- (((long)((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) < \
- (long)(MINSIZE + MALLOC_ALIGN_MASK)) ? MINSIZE : \
- ((((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) < \
- (MINSIZE + MALLOC_ALIGN_MASK)) ? MINSIZE : \ (((req) + (SIZE_SZ + MALLOC_ALIGN_MASK)) & ~(MALLOC_ALIGN_MASK)))
/* Check if m has acceptable alignment */
2.35.3
Make sure that the new break is within mem_malloc_start and mem_malloc_end before making progress. ulong new = old + increment; can overflow for extremely large increment values and memset() can get wrongly called.
Signed-off-by: Richard Weinberger richard@nod.at --- common/dlmalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 44b06e38b2..c8d1da1cb1 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -581,6 +581,9 @@ void *sbrk(ptrdiff_t increment) ulong old = mem_malloc_brk; ulong new = old + increment;
+ if ((new < mem_malloc_start) || (new > mem_malloc_end)) + return (void *)MORECORE_FAILURE; + /* * if we are giving memory back make sure we clear it out since * we set MORECORE_CLEARS to 1 @@ -588,9 +591,6 @@ void *sbrk(ptrdiff_t increment) if (increment < 0) memset((void *)new, 0, -increment);
- if ((new < mem_malloc_start) || (new > mem_malloc_end)) - return (void *)MORECORE_FAILURE; - mem_malloc_brk = new;
return (void *)old;
On Fri, 2 Aug 2024 at 04:08, Richard Weinberger richard@nod.at wrote:
Make sure that the new break is within mem_malloc_start and mem_malloc_end before making progress. ulong new = old + increment; can overflow for extremely large increment values and memset() can get wrongly called.
Signed-off-by: Richard Weinberger richard@nod.at
common/dlmalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Should we update dlmalloc to the new version?
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 44b06e38b2..c8d1da1cb1 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -581,6 +581,9 @@ void *sbrk(ptrdiff_t increment) ulong old = mem_malloc_brk; ulong new = old + increment;
if ((new < mem_malloc_start) || (new > mem_malloc_end))return (void *)MORECORE_FAILURE;/* * if we are giving memory back make sure we clear it out since * we set MORECORE_CLEARS to 1@@ -588,9 +591,6 @@ void *sbrk(ptrdiff_t increment) if (increment < 0) memset((void *)new, 0, -increment);
if ((new < mem_malloc_start) || (new > mem_malloc_end))return (void *)MORECORE_FAILURE;mem_malloc_brk = new; return (void *)old;-- 2.35.3
On Tue, Aug 06, 2024 at 03:50:41PM -0600, Simon Glass wrote:
On Fri, 2 Aug 2024 at 04:08, Richard Weinberger richard@nod.at wrote:
Make sure that the new break is within mem_malloc_start and mem_malloc_end before making progress. ulong new = old + increment; can overflow for extremely large increment values and memset() can get wrongly called.
Signed-off-by: Richard Weinberger richard@nod.at
common/dlmalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Should we update dlmalloc to the new version?
A worthy but non-trivial goal, I think.
Since U-Boot does not support memory overcommit we can enforce that the allocation size is within the malloc area. This is a simple and efficient hardening measure to mitigate further integer overflows in dlmalloc.
Signed-off-by: Richard Weinberger richard@nod.at --- common/dlmalloc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index c8d1da1cb1..d264fc031a 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -1274,7 +1274,8 @@ Void_t* mALLOc_impl(bytes) size_t bytes; return NULL; }
- if ((long)bytes < 0) return NULL; + if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0) + return NULL;
nb = request2size(bytes); /* padded request size; */
@@ -1687,7 +1688,8 @@ Void_t* rEALLOc_impl(oldmem, bytes) Void_t* oldmem; size_t bytes; } #endif
- if ((long)bytes < 0) return NULL; + if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0) + return NULL;
/* realloc of null is supposed to be same as malloc */ if (oldmem == NULL) return mALLOc_impl(bytes); @@ -1907,7 +1909,8 @@ Void_t* mEMALIGn_impl(alignment, bytes) size_t alignment; size_t bytes; mchunkptr remainder; /* spare room at end to split off */ long remainder_size; /* its size */
- if ((long)bytes < 0) return NULL; + if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0) + return NULL;
#if CONFIG_IS_ENABLED(SYS_MALLOC_F) if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
Hi Richard,
On Fri, 2 Aug 2024 at 04:08, Richard Weinberger richard@nod.at wrote:
Since U-Boot does not support memory overcommit we can enforce that the allocation size is within the malloc area. This is a simple and efficient hardening measure to mitigate further integer overflows in dlmalloc.
Signed-off-by: Richard Weinberger richard@nod.at
common/dlmalloc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index c8d1da1cb1..d264fc031a 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -1274,7 +1274,8 @@ Void_t* mALLOc_impl(bytes) size_t bytes; return NULL; }
- if ((long)bytes < 0) return NULL;
if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0)
return NULL;nb = request2size(bytes); /* padded request size; */
@@ -1687,7 +1688,8 @@ Void_t* rEALLOc_impl(oldmem, bytes) Void_t* oldmem; size_t bytes; } #endif
- if ((long)bytes < 0) return NULL;
if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0)
return NULL;/* realloc of null is supposed to be same as malloc */ if (oldmem == NULL) return mALLOc_impl(bytes);
@@ -1907,7 +1909,8 @@ Void_t* mEMALIGn_impl(alignment, bytes) size_t alignment; size_t bytes; mchunkptr remainder; /* spare room at end to split off */ long remainder_size; /* its size */
- if ((long)bytes < 0) return NULL;
- if (bytes > CONFIG_SYS_MALLOC_LEN || (long)bytes < 0)
return NULL;#if CONFIG_IS_ENABLED(SYS_MALLOC_F) if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) { -- 2.35.3
Reviewed-by: Simon Glass sjg@chromium.org
I wonder if we can get away without the memalign() one since it is calling malloc() always? There is still the request2size() though.
Regards, Simon
On Fri, 2 Aug 2024 at 04:08, Richard Weinberger richard@nod.at wrote:
sbrk() assumes ptrdiff_t is large enough to enlarge/shrink the heap by LONG_MIN/LONG_MAX. So, use the long type, also to match the rest of the Linux ecosystem.
Signed-off-by: Richard Weinberger richard@nod.at
arch/x86/include/asm/posix_types.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h index dbcea7f47f..e1ed9bcabc 100644 --- a/arch/x86/include/asm/posix_types.h +++ b/arch/x86/include/asm/posix_types.h @@ -20,11 +20,12 @@ typedef unsigned short __kernel_gid_t; #if defined(__x86_64__) typedef unsigned long __kernel_size_t; typedef long __kernel_ssize_t; +typedef long __kernel_ptrdiff_t; #else typedef unsigned int __kernel_size_t; typedef int __kernel_ssize_t; -#endif typedef int __kernel_ptrdiff_t; +#endif typedef long __kernel_time_t; typedef long __kernel_suseconds_t; typedef long __kernel_clock_t; -- 2.35.3
participants (3)
-
Richard Weinberger -
Simon Glass -
Tom Rini