alloc: fix potential oob in rbrealloc

Previous memcpy of bytes is unsafe if new object is larger then old one. Typically realloc retrieves the old size internally and does not need this information passed as argument but sof heap implementation does not store the exact size of the allocated memory buffer. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
iganakov · Apr 8, 2020 · e306d12 · e306d12
1 parent d8e3f63
commit e306d12
Show file tree

Hide file tree

Showing 5 changed files with 13 additions and 8 deletions.
diff --git a/src/audio/buffer.c b/src/audio/buffer.c
@@ -99,7 +99,8 @@ int buffer_set_size(struct comp_buffer *buffer, uint32_t size)
 	if (size == buffer->stream.size)
 		return 0;
 
-	new_ptr = rbrealloc(buffer->stream.addr, 0, buffer->caps, size);
+	new_ptr = rbrealloc(buffer->stream.addr, 0, buffer->caps, size,
+			    buffer->stream.size);
 
 	/* we couldn't allocate bigger chunk */
 	if (!new_ptr && size > buffer->stream.size) {

diff --git a/src/include/sof/lib/alloc.h b/src/include/sof/lib/alloc.h
@@ -126,20 +126,22 @@ void *rrealloc(void *ptr, enum mem_zone zone, uint32_t flags, uint32_t caps,
  * @param flags Flags, see SOF_MEM_FLAG_...
  * @param caps Capabilities, see SOF_MEM_CAPS_...
  * @param bytes New size in bytes.
+ * @param old_bytes Old size in bytes.
  * @param alignment Alignment in bytes.
  * @return Pointer to the resized memory of NULL if failed.
  */
 void *rbrealloc_align(void *ptr, uint32_t flags, uint32_t caps, size_t bytes,
-		      uint32_t alignment);
+		      size_t old_bytes, uint32_t alignment);
 
 /**
  * Similar to rballoc_align(), returns resized buffer aligned to
  * PLATFORM_DCACHE_ALIGN.
  */
 static inline void *rbrealloc(void *ptr, uint32_t flags, uint32_t caps,
-			      size_t bytes)
+			      size_t bytes, size_t old_bytes)
 {
-	return rbrealloc_align(ptr, flags, caps, bytes, PLATFORM_DCACHE_ALIGN);
+	return rbrealloc_align(ptr, flags, caps, bytes, old_bytes,
+			       PLATFORM_DCACHE_ALIGN);
 }
 
 /**

diff --git a/src/lib/alloc.c b/src/lib/alloc.c
@@ -941,11 +941,12 @@ void *rrealloc(void *ptr, enum mem_zone zone, uint32_t flags, uint32_t caps,
 }
 
 void *rbrealloc_align(void *ptr, uint32_t flags, uint32_t caps, size_t bytes,
-		      uint32_t alignment)
+		      size_t old_bytes, uint32_t alignment)
 {
 	struct mm *memmap = memmap_get();
 	void *new_ptr = NULL;
 	uint32_t lock_flags;
+	size_t copy_bytes = MIN(bytes, old_bytes);
 
 	if (!bytes)
 		return new_ptr;
@@ -955,7 +956,7 @@ void *rbrealloc_align(void *ptr, uint32_t flags, uint32_t caps, size_t bytes,
 	new_ptr = _balloc_unlocked(flags, caps, bytes, alignment);
 
 	if (new_ptr && ptr)
-		memcpy_s(new_ptr, bytes, ptr, bytes);
+		memcpy_s(new_ptr, copy_bytes, ptr, copy_bytes);
 
 	if (new_ptr)
 		_rfree_unlocked(ptr);

diff --git a/test/cmocka/src/common_mocks.c b/test/cmocka/src/common_mocks.c
@@ -38,10 +38,11 @@ void WEAK *rzalloc(enum mem_zone zone, uint32_t flags, uint32_t caps,
 }
 
 void WEAK *rbrealloc_align(void *ptr, uint32_t flags, uint32_t caps,
-			   size_t bytes, uint32_t alignment)
+			   size_t bytes, size_t old_bytes, uint32_t alignment)
 {
 	(void)flags;
 	(void)caps;
+	(void)old_bytes;
 
 	return realloc(ptr, bytes);
 }

diff --git a/tools/testbench/alloc.c b/tools/testbench/alloc.c
@@ -45,7 +45,7 @@ void *rrealloc(void *ptr, enum mem_zone zone, uint32_t flags, uint32_t caps,
 }
 
 void *rbrealloc_align(void *ptr, uint32_t flags, uint32_t caps, size_t bytes,
-		      uint32_t alignment)
+		      size_t old_bytes, uint32_t alignment)
 {
 	return realloc(ptr, bytes);
 }