From 7ab237b025cbb4c25d345604da32e894379c1721 Mon Sep 17 00:00:00 2001 From: Marcin Bukat Date: Thu, 9 Jan 2014 21:37:07 +0100 Subject: [PATCH] buflib: Add crc field protecting buflib cookie integrity This should catch the case of buffer misuse which results in corrupted cookie of next allocation. The check is performed on move_block() so it may be a bit late. There is buflib_check_valid() provided which checks the integrity of all cookies for given context. On DEBUG build with --sdl-thread this check is carried out for core_ctx on every context switch to catch problems earlier. Change-Id: I999d4576084592394e3dbd3bdf0f32935ff5f601 Reviewed-on: http://gerrit.rockbox.org/711 Reviewed-by: Thomas Martitz --- apps/plugin.h | 2 +- firmware/buflib.c | 72 +++++++++++++++++++++---- firmware/common/crc32.c | 4 +- firmware/core_alloc.c | 7 +++ firmware/include/buflib.h | 6 +++ firmware/include/core_alloc.h | 3 ++ firmware/include/crc32.h | 4 +- firmware/target/hosted/sdl/thread-sdl.c | 4 ++ firmware/thread.c | 6 +++ 9 files changed, 95 insertions(+), 13 deletions(-) diff --git a/apps/plugin.h b/apps/plugin.h index 8a0d0562ff..1d1e9ee26e 100644 --- a/apps/plugin.h +++ b/apps/plugin.h @@ -457,7 +457,7 @@ struct plugin_api { int numberlen IF_CNFN_NUM_(, int *num)); bool (*file_exists)(const char *file); char* (*strip_extension)(char* buffer, int buffer_size, const char *filename); - unsigned (*crc_32)(const void *src, unsigned len, unsigned crc32); + uint32_t (*crc_32)(const void *src, uint32_t len, uint32_t crc32); int (*filetype_get_attr)(const char* file); diff --git a/firmware/buflib.c b/firmware/buflib.c index db09d3efc9..0a87a4c4d8 100644 --- a/firmware/buflib.c +++ b/firmware/buflib.c @@ -31,6 +31,8 @@ #include "buflib.h" #include "string-extra.h" /* strlcpy() */ #include "debug.h" +#include "panic.h" +#include "crc32.h" #include "system.h" /* for ALIGN_*() */ /* The main goal of this design is fast fetching of the pointer for a handle. @@ -54,13 +56,14 @@ * * Example: * |<- alloc block #1 ->|<- unalloc block ->|<- alloc block #2 ->|<-handle table->| - * |L|H|C|cccc|L2|XXXXXX|-L|YYYYYYYYYYYYYYYY|L|H|C|cc|L2|XXXXXXXXXXXXX|AAA| + * |L|H|C|cccc|L2|crc|XXXXXX|-L|YYYYYYYYYYYYYYYY|L|H|C|cc|L2|crc|XXXXXXXXXXXXX|AAA| * * L - length marker (negative if block unallocated) * H - handle table enry pointer * C - pointer to struct buflib_callbacks * c - variable sized string identifier * L2 - second length marker for string identifier + * crc - crc32 protecting buflib cookie integrity * X - actual payload * Y - unallocated space * @@ -224,8 +227,17 @@ static bool move_block(struct buflib_context* ctx, union buflib_data* block, int shift) { char* new_start; - union buflib_data *new_block, *tmp = block[1].handle; + union buflib_data *new_block, *tmp = block[1].handle, *crc_slot; struct buflib_callbacks *ops = block[2].ops; + crc_slot = (union buflib_data*)tmp->alloc - 1; + int cookie_size = (crc_slot - block)*sizeof(union buflib_data); + uint32_t crc = crc_32((void *)block, cookie_size, 0xffffffff); + + /* check for cookie validity */ + if (crc != crc_slot->crc) + panicf("buflib cookie corrupted, crc: 0x%08x, expected: 0x%08x", + (unsigned int)crc, (unsigned int)crc_slot->crc); + if (!IS_MOVABLE(block)) return false; @@ -299,6 +311,7 @@ buflib_compact(struct buflib_context *ctx) ret = true; /* Move was successful. The memory at block is now free */ block->val = -len; + /* add its length to shift */ shift += -len; /* Reduce the size of the hole accordingly @@ -474,9 +487,9 @@ buflib_alloc_ex(struct buflib_context *ctx, size_t size, const char *name, size += name_len; size = (size + sizeof(union buflib_data) - 1) / sizeof(union buflib_data) - /* add 4 objects for alloc len, pointer to handle table entry and - * name length, and the ops pointer */ - + 4; + /* add 5 objects for alloc len, pointer to handle table entry and + * name length, the ops pointer and crc */ + + 5; handle_alloc: handle = handle_alloc(ctx); if (!handle) @@ -555,14 +568,22 @@ buffer_alloc: /* Set up the allocated block, by marking the size allocated, and storing * a pointer to the handle. */ - union buflib_data *name_len_slot; + union buflib_data *name_len_slot, *crc_slot; block->val = size; block[1].handle = handle; block[2].ops = ops; strcpy(block[3].name, name); name_len_slot = (union buflib_data*)B_ALIGN_UP(block[3].name + name_len); name_len_slot->val = 1 + name_len/sizeof(union buflib_data); - handle->alloc = (char*)(name_len_slot + 1); + crc_slot = (union buflib_data*)(name_len_slot + 1); + crc_slot->crc = crc_32((void *)block, + (crc_slot - block)*sizeof(union buflib_data), + 0xffffffff); + handle->alloc = (char*)(crc_slot + 1); + + BDEBUGF("buflib_alloc_ex: size=%d handle=%p clb=%p crc=0x%0x name=\"%s\"\n", + (unsigned int)size, (void *)handle, (void *)ops, + (unsigned int)crc_slot->crc, block[3].name); block += size; /* alloc_end must be kept current if we're taking the last block. */ @@ -778,6 +799,8 @@ buflib_alloc_maximum(struct buflib_context* ctx, const char* name, size_t *size, bool buflib_shrink(struct buflib_context* ctx, int handle, void* new_start, size_t new_size) { + union buflib_data *crc_slot; + int cookie_size; char* oldstart = buflib_get_data(ctx, handle); char* newstart = new_start; char* newend = newstart + new_size; @@ -823,6 +846,11 @@ buflib_shrink(struct buflib_context* ctx, int handle, void* new_start, size_t ne block = new_block; } + /* update crc of the cookie */ + crc_slot = (union buflib_data*)new_block[1].handle->alloc - 1; + cookie_size = (crc_slot - new_block)*sizeof(union buflib_data); + crc_slot->crc = crc_32((void *)new_block, cookie_size, 0xffffffff); + /* Now deal with size changes that create free blocks after the allocation */ if (old_next_block != new_next_block) { @@ -847,12 +875,38 @@ const char* buflib_get_name(struct buflib_context *ctx, int handle) union buflib_data *data = ALIGN_DOWN(buflib_get_data(ctx, handle), sizeof (*data)); if (!data) return NULL; - size_t len = data[-1].val; + size_t len = data[-2].val; if (len <= 1) return NULL; - return data[-len].name; + return data[-len-1].name; } +#ifdef DEBUG +void buflib_check_valid(struct buflib_context *ctx) +{ + union buflib_data *crc_slot; + int cookie_size; + uint32_t crc; + + for(union buflib_data* this = ctx->buf_start; + this < ctx->alloc_end; + this += abs(this->val)) + { + if (this->val < 0) + continue; + + crc_slot = (union buflib_data*) + ((union buflib_data*)this[1].handle)->alloc - 1; + cookie_size = (crc_slot - this)*sizeof(union buflib_data); + crc = crc_32((void *)this, cookie_size, 0xffffffff); + + if (crc != crc_slot->crc) + panicf("buflib check crc: 0x%08x, expected: 0x%08x", + (unsigned int)crc, (unsigned int)crc_slot->crc); + } +} +#endif + #ifdef BUFLIB_DEBUG_BLOCKS void buflib_print_allocs(struct buflib_context *ctx, void (*print)(int, const char*)) diff --git a/firmware/common/crc32.c b/firmware/common/crc32.c index 1cd0ca0bd5..c8c70e415c 100644 --- a/firmware/common/crc32.c +++ b/firmware/common/crc32.c @@ -25,7 +25,7 @@ /* Tool function to calculate a CRC32 across some buffer */ /* third argument is either 0xFFFFFFFF to start or value from last piece */ -unsigned crc_32(const void *src, unsigned len, unsigned crc32) +uint32_t crc_32(const void *src, uint32_t len, uint32_t crc32) { const unsigned char *buf = (const unsigned char *)src; @@ -39,7 +39,7 @@ unsigned crc_32(const void *src, unsigned len, unsigned crc32) }; unsigned char byte; - unsigned t; + uint32_t t; while (len--) { diff --git a/firmware/core_alloc.c b/firmware/core_alloc.c index aa662fbee5..e9f9795917 100644 --- a/firmware/core_alloc.c +++ b/firmware/core_alloc.c @@ -96,3 +96,10 @@ void core_print_block_at(int block_num, char* buf, size_t bufsize) { buflib_print_block_at(&core_ctx, block_num, buf, bufsize); } + +#ifdef DEBUG +void core_check_valid(void) +{ + buflib_check_valid(&core_ctx); +} +#endif diff --git a/firmware/include/buflib.h b/firmware/include/buflib.h index 0b26c04bcd..171ab5bcd7 100644 --- a/firmware/include/buflib.h +++ b/firmware/include/buflib.h @@ -40,6 +40,7 @@ union buflib_data struct buflib_callbacks* ops; char* alloc; union buflib_data *handle; + uint32_t crc; }; struct buflib_context @@ -346,4 +347,9 @@ int buflib_get_num_blocks(struct buflib_context *ctx); */ void buflib_print_block_at(struct buflib_context *ctx, int block_num, char* buf, size_t bufsize); + +/** + * Check integrity of given buflib context + */ +void buflib_check_valid(struct buflib_context *ctx); #endif diff --git a/firmware/include/core_alloc.h b/firmware/include/core_alloc.h index a100b7cc6c..095cb5da11 100644 --- a/firmware/include/core_alloc.h +++ b/firmware/include/core_alloc.h @@ -17,6 +17,9 @@ bool core_shrink(int handle, void* new_start, size_t new_size); int core_free(int handle); size_t core_available(void); size_t core_allocatable(void); +#ifdef DEBUG +void core_check_valid(void); +#endif /* DO NOT ADD wrappers for buflib_buffer_out/in. They do not call * the move callbacks and are therefore unsafe in the core */ diff --git a/firmware/include/crc32.h b/firmware/include/crc32.h index 034c3984ab..8e1f868988 100644 --- a/firmware/include/crc32.h +++ b/firmware/include/crc32.h @@ -18,10 +18,12 @@ * KIND, either express or implied. * ****************************************************************************/ +#include + #ifndef _CRC32_H #define _CRC32_H -unsigned crc_32(const void *src, unsigned len, unsigned crc32); +uint32_t crc_32(const void *src, uint32_t len, uint32_t crc32); #endif diff --git a/firmware/target/hosted/sdl/thread-sdl.c b/firmware/target/hosted/sdl/thread-sdl.c index eaffa86aee..fbc26c8a9f 100644 --- a/firmware/target/hosted/sdl/thread-sdl.c +++ b/firmware/target/hosted/sdl/thread-sdl.c @@ -32,6 +32,7 @@ #include "kernel.h" #include "thread.h" #include "debug.h" +#include "core_alloc.h" /* Define this as 1 to show informational messages that are not errors. */ #define THREAD_SDL_DEBUGF_ENABLED 0 @@ -382,6 +383,9 @@ void switch_thread(void) } /* STATE_SLEEPING: */ } +#ifdef DEBUG + core_check_valid(); +#endif cores[CURRENT_CORE].running = current; if (threads_status != THREADS_RUN) diff --git a/firmware/thread.c b/firmware/thread.c index ce9252ccc6..b687144f4f 100644 --- a/firmware/thread.c +++ b/firmware/thread.c @@ -39,6 +39,7 @@ #ifdef RB_PROFILE #include #endif +#include "core_alloc.h" #include "gcc_extensions.h" /**************************************************************************** @@ -1161,6 +1162,11 @@ void switch_thread(void) * to this call. */ store_context(&thread->context); +#ifdef DEBUG + /* Check core_ctx buflib integrity */ + core_check_valid(); +#endif + /* Check if the current thread stack is overflown */ if (UNLIKELY(thread->stack[0] != DEADBEEF) && thread->stack_size > 0) thread_stkov(thread);