Skip to content

Commit 9220ffb

Browse files
jservjserv
committed
Fix critical memory issues
This adds proper NULL checks for all critical malloc/calloc/mpool_alloc calls that previously relied solely on assert(), which compiles out in production builds with -DNDEBUG. It also resolves critical memory safety issues found that could allow guest code to corrupt host memory or cause crashes. - Fix potential integer underflow in memcpy length calculation - Add validation that args[0] length exceeds 7 before subtraction - Add bounds checking in memset_handler to validate dest and count - Add bounds checking in memcpy_handler for both src and dest - Prevent guest code from writing to arbitrary host memory addresses - Raise traps (STORE_MISALIGNED/LOAD_MISALIGNED) on violations - Reading arbitrary host memory via out-of-bounds memcpy source - Writing arbitrary host memory via out-of-bounds memcpy/memset dest - Causing host crashes via integer overflow in size calculations
1 parent 1a06240 commit 9220ffb

File tree

7 files changed

+202
-58
lines changed

7 files changed

+202
-58
lines changed

‎src/cache.c‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,17 @@ void *cache_put(cache_t *cache, uint32_t key, void *value)
285285
}
286286

287287
cache_entry_t *new_entry = calloc(1, sizeof(cache_entry_t));
288+
if (unlikely(!new_entry)) {
289+
/* Allocation failed - restore replaced entry if exists */
290+
if (replaced) {
291+
replaced->alive = true;
292+
list_del_init(&replaced->list);
293+
list_add(&replaced->list, &cache->list);
294+
cache->size++;
295+
cache->ghost_list_size--;
296+
}
297+
return NULL;
298+
}
288299
assert(new_entry);
289300

290301
INIT_LIST_HEAD(&new_entry->list);

‎src/emulate.c‎

Lines changed: 96 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,8 @@ HASH_FUNC_IMPL(map_hash, BLOCK_MAP_CAPACITY_BITS, 1 << BLOCK_MAP_CAPACITY_BITS)
273273
static block_t *block_alloc(riscv_t *rv)
274274
{
275275
block_t *block = mpool_alloc(rv->block_mp);
276+
if (unlikely(!block))
277+
return NULL;
276278
assert(block);
277279
block->n_insn = 0;
278280
#if RV32_HAS(JIT)
@@ -619,13 +621,15 @@ FORCE_INLINE bool insn_is_indirect_branch(uint8_t opcode)
619621
}
620622
}
621623

622-
static void block_translate(riscv_t *rv, block_t *block)
624+
static bool block_translate(riscv_t *rv, block_t *block)
623625
{
624626
retranslate:
625627
block->pc_start = block->pc_end = rv->PC;
626628

627629
rv_insn_t *prev_ir = NULL;
628630
rv_insn_t *ir = mpool_calloc(rv->block_ir_mp);
631+
if (unlikely(!ir))
632+
return false;
629633
block->ir_head = ir;
630634

631635
/* translate the basic block */
@@ -665,6 +669,8 @@ static void block_translate(riscv_t *rv, block_t *block)
665669
if (insn_is_branch(ir->opcode)) {
666670
if (insn_is_indirect_branch(ir->opcode)) {
667671
ir->branch_table = calloc(1, sizeof(branch_history_table_t));
672+
if (unlikely(!ir->branch_table))
673+
return false;
668674
assert(ir->branch_table);
669675
memset(ir->branch_table->PC, -1,
670676
sizeof(uint32_t) * HISTORY_SIZE);
@@ -673,36 +679,44 @@ static void block_translate(riscv_t *rv, block_t *block)
673679
}
674680

675681
ir = mpool_calloc(rv->block_ir_mp);
682+
if (unlikely(!ir))
683+
return false;
676684
}
677685

678686
assert(prev_ir);
679687
block->ir_tail = prev_ir;
680688
block->ir_tail->next = NULL;
689+
return true;
681690
}
682691

683692
#if RV32_HAS(MOP_FUSION)
684-
#define COMBINE_MEM_OPS(RW) \
685-
next_ir = ir->next; \
686-
count = 1; \
687-
while (1) { \
688-
if (next_ir->opcode != IIF(RW)(rv_insn_lw, rv_insn_sw)) \
689-
break; \
690-
count++; \
691-
if (!next_ir->next) \
692-
break; \
693-
next_ir = next_ir->next; \
694-
} \
695-
if (count > 1) { \
696-
ir->opcode = IIF(RW)(rv_insn_fuse4, rv_insn_fuse3); \
697-
ir->fuse = malloc(count * sizeof(opcode_fuse_t)); \
698-
assert(ir->fuse); \
699-
ir->imm2 = count; \
700-
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t)); \
701-
ir->impl = dispatch_table[ir->opcode]; \
702-
next_ir = ir->next; \
703-
for (int j = 1; j < count; j++, next_ir = next_ir->next) \
704-
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t)); \
705-
remove_next_nth_ir(rv, ir, block, count - 1); \
693+
#define COMBINE_MEM_OPS(RW) \
694+
next_ir = ir->next; \
695+
count = 1; \
696+
while (1) { \
697+
if (next_ir->opcode != IIF(RW)(rv_insn_lw, rv_insn_sw)) \
698+
break; \
699+
count++; \
700+
if (!next_ir->next) \
701+
break; \
702+
next_ir = next_ir->next; \
703+
} \
704+
if (count > 1) { \
705+
ir->opcode = IIF(RW)(rv_insn_fuse4, rv_insn_fuse3); \
706+
ir->fuse = malloc(count * sizeof(opcode_fuse_t)); \
707+
if (unlikely(!ir->fuse)) { \
708+
ir->opcode = IIF(RW)(rv_insn_lw, rv_insn_sw); \
709+
count = 1; /* Degrade to non-fused operation */ \
710+
} else { \
711+
assert(ir->fuse); \
712+
ir->imm2 = count; \
713+
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t)); \
714+
ir->impl = dispatch_table[ir->opcode]; \
715+
next_ir = ir->next; \
716+
for (int j = 1; j < count; j++, next_ir = next_ir->next) \
717+
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t)); \
718+
remove_next_nth_ir(rv, ir, block, count - 1); \
719+
} \
706720
}
707721

708722
static inline void remove_next_nth_ir(const riscv_t *rv,
@@ -762,16 +776,20 @@ static void match_pattern(riscv_t *rv, block_t *block)
762776
next_ir = next_ir->next;
763777
}
764778
if (count > 1) {
765-
ir->opcode = rv_insn_fuse1;
766779
ir->fuse = malloc(count * sizeof(opcode_fuse_t));
767-
assert(ir->fuse);
768-
ir->imm2 = count;
769-
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t));
770-
ir->impl = dispatch_table[ir->opcode];
771-
next_ir = ir->next;
772-
for (int j = 1; j < count; j++, next_ir = next_ir->next)
773-
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t));
774-
remove_next_nth_ir(rv, ir, block, count - 1);
780+
if (likely(ir->fuse)) {
781+
ir->opcode = rv_insn_fuse1;
782+
assert(ir->fuse);
783+
ir->imm2 = count;
784+
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t));
785+
ir->impl = dispatch_table[ir->opcode];
786+
next_ir = ir->next;
787+
for (int j = 1; j < count; j++, next_ir = next_ir->next)
788+
memcpy(ir->fuse + j, next_ir,
789+
sizeof(opcode_fuse_t));
790+
remove_next_nth_ir(rv, ir, block, count - 1);
791+
}
792+
/* If malloc failed, degrade gracefully to non-fused ops */
775793
}
776794
break;
777795
}
@@ -803,15 +821,18 @@ static void match_pattern(riscv_t *rv, block_t *block)
803821
}
804822
if (count > 1) {
805823
ir->fuse = malloc(count * sizeof(opcode_fuse_t));
806-
assert(ir->fuse);
807-
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t));
808-
ir->opcode = rv_insn_fuse5;
809-
ir->imm2 = count;
810-
ir->impl = dispatch_table[ir->opcode];
811-
next_ir = ir->next;
812-
for (int j = 1; j < count; j++, next_ir = next_ir->next)
813-
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t));
814-
remove_next_nth_ir(rv, ir, block, count - 1);
824+
if (likely(ir->fuse)) {
825+
assert(ir->fuse);
826+
memcpy(ir->fuse, ir, sizeof(opcode_fuse_t));
827+
ir->opcode = rv_insn_fuse5;
828+
ir->imm2 = count;
829+
ir->impl = dispatch_table[ir->opcode];
830+
next_ir = ir->next;
831+
for (int j = 1; j < count; j++, next_ir = next_ir->next)
832+
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t));
833+
remove_next_nth_ir(rv, ir, block, count - 1);
834+
}
835+
/* If malloc failed, degrade gracefully to non-fused ops */
815836
}
816837
break;
817838
}
@@ -881,8 +902,11 @@ static block_t *block_find_or_translate(riscv_t *rv)
881902
#endif
882903
/* allocate a new block */
883904
next_blk = block_alloc(rv);
905+
if (unlikely(!next_blk))
906+
return NULL;
884907

885-
block_translate(rv, next_blk);
908+
if (unlikely(!block_translate(rv, next_blk)))
909+
return NULL;
886910

887911
#if RV32_HAS(JIT) && RV32_HAS(SYSTEM)
888912
/*
@@ -1129,6 +1153,10 @@ void rv_step(void *arg)
11291153
else if (!block->compiled && block->n_invoke >= THRESHOLD) {
11301154
block->compiled = true;
11311155
queue_entry_t *entry = malloc(sizeof(queue_entry_t));
1156+
if (unlikely(!entry)) {
1157+
/* Malloc failed - degrade to tier-1 JIT, don't queue for T2C */
1158+
continue;
1159+
}
11321160
entry->block = block;
11331161
pthread_mutex_lock(&rv->wait_queue_lock);
11341162
list_add(&entry->list, &rv->wait_queue);
@@ -1378,16 +1406,38 @@ void ecall_handler(riscv_t *rv)
13781406
void memset_handler(riscv_t *rv)
13791407
{
13801408
memory_t *m = PRIV(rv)->mem;
1381-
memset((char *) m->mem_base + rv->X[rv_reg_a0], rv->X[rv_reg_a1],
1382-
rv->X[rv_reg_a2]);
1409+
uint32_t dest = rv->X[rv_reg_a0];
1410+
uint32_t value = rv->X[rv_reg_a1];
1411+
uint32_t count = rv->X[rv_reg_a2];
1412+
1413+
/* Bounds checking to prevent buffer overflow */
1414+
if (dest >= m->mem_size || count > m->mem_size - dest) {
1415+
SET_CAUSE_AND_TVAL_THEN_TRAP(rv, STORE_MISALIGNED, dest);
1416+
return;
1417+
}
1418+
1419+
memset((char *) m->mem_base + dest, value, count);
13831420
rv->PC = rv->X[rv_reg_ra] & ~1U;
13841421
}
13851422

13861423
void memcpy_handler(riscv_t *rv)
13871424
{
13881425
memory_t *m = PRIV(rv)->mem;
1389-
memcpy((char *) m->mem_base + rv->X[rv_reg_a0],
1390-
(char *) m->mem_base + rv->X[rv_reg_a1], rv->X[rv_reg_a2]);
1426+
uint32_t dest = rv->X[rv_reg_a0];
1427+
uint32_t src = rv->X[rv_reg_a1];
1428+
uint32_t count = rv->X[rv_reg_a2];
1429+
1430+
/* Bounds checking to prevent buffer overflow */
1431+
if (dest >= m->mem_size || count > m->mem_size - dest) {
1432+
SET_CAUSE_AND_TVAL_THEN_TRAP(rv, STORE_MISALIGNED, dest);
1433+
return;
1434+
}
1435+
if (src >= m->mem_size || count > m->mem_size - src) {
1436+
SET_CAUSE_AND_TVAL_THEN_TRAP(rv, LOAD_MISALIGNED, src);
1437+
return;
1438+
}
1439+
1440+
memcpy((char *) m->mem_base + dest, (char *) m->mem_base + src, count);
13911441
rv->PC = rv->X[rv_reg_ra] & ~1U;
13921442
}
13931443

‎src/io.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ memory_t *memory_new(uint32_t size)
2323
return NULL;
2424

2525
memory_t *mem = malloc(sizeof(memory_t));
26+
if (!mem)
27+
return NULL;
2628
assert(mem);
2729
#if HAVE_MMAP
2830
data_memory_base = mmap(NULL, size, PROT_READ | PROT_WRITE,

‎src/jit.c‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2330,7 +2330,10 @@ void jit_translate(riscv_t *rv, block_t *block)
23302330
struct jit_state *jit_state_init(size_t size)
23312331
{
23322332
struct jit_state *state = malloc(sizeof(struct jit_state));
2333+
if (!state)
2334+
return NULL;
23332335
assert(state);
2336+
23342337
state->offset = 0;
23352338
state->size = size;
23362339
state->buf = mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC,
@@ -2340,13 +2343,32 @@ struct jit_state *jit_state_init(size_t size)
23402343
#endif
23412344
,
23422345
-1, 0);
2343-
state->n_blocks = 0;
2346+
if (state->buf == MAP_FAILED) {
2347+
free(state);
2348+
return NULL;
2349+
}
23442350
assert(state->buf != MAP_FAILED);
2351+
2352+
state->n_blocks = 0;
23452353
set_reset(&state->set);
23462354
reset_reg();
23472355
prepare_translate(state);
2356+
23482357
state->offset_map = calloc(MAX_BLOCKS, sizeof(struct offset_map));
2358+
if (!state->offset_map) {
2359+
munmap(state->buf, state->size);
2360+
free(state);
2361+
return NULL;
2362+
}
2363+
23492364
state->jumps = calloc(MAX_JUMPS, sizeof(struct jump));
2365+
if (!state->jumps) {
2366+
free(state->offset_map);
2367+
munmap(state->buf, state->size);
2368+
free(state);
2369+
return NULL;
2370+
}
2371+
23502372
return state;
23512373
}
23522374

‎src/main.c‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -177,20 +177,29 @@ static bool parse_args(int argc, char **args)
177177
assert(getcwd(cwd_path, PATH_MAX));
178178

179179
char rel_path[PATH_MAX] = {0};
180-
memcpy(rel_path, args[0], strlen(args[0]) - 7 /* strlen("rv32emu") */);
180+
size_t args0_len = strlen(args[0]);
181+
/* Ensure args[0] is long enough before subtracting */
182+
if (args0_len > 7) { /* strlen("rv32emu") */
183+
size_t copy_len = args0_len - 7;
184+
if (copy_len >= PATH_MAX)
185+
copy_len = PATH_MAX - 1;
186+
memcpy(rel_path, args[0], copy_len);
187+
rel_path[copy_len] = '\0';
188+
}
181189

182190
char *prog_basename = basename(opt_prog_name);
183-
prof_out_file = malloc(strlen(cwd_path) + 1 + strlen(rel_path) +
184-
strlen(prog_basename) + 5 + 1);
191+
size_t total_len = strlen(cwd_path) + 1 + strlen(rel_path) +
192+
strlen(prog_basename) + 5 + 1;
193+
prof_out_file = malloc(total_len);
185194
assert(prof_out_file);
186195

187-
sprintf(prof_out_file, "%s/%s%s.prof", cwd_path, rel_path,
188-
prog_basename);
196+
snprintf(prof_out_file, total_len, "%s/%s%s.prof", cwd_path, rel_path,
197+
prog_basename);
189198
}
190199
return true;
191200
}
192201

193-
static void dump_test_signature(const char *prog_name)
202+
static void dump_test_signature(const char UNUSED *prog_name)
194203
{
195204
elf_t *elf = elf_new();
196205
assert(elf && elf_open(elf, prog_name));

0 commit comments

Comments
 (0)