diff --git a/crates/volamos-core/src/dosfile.rs b/crates/volamos-core/src/dosfile.rs index 39d0640..cbacf3f 100644 --- a/crates/volamos-core/src/dosfile.rs +++ b/crates/volamos-core/src/dosfile.rs @@ -1369,6 +1369,7 @@ fn alloc_dos_object_handler(ctx: &mut HandlerContext<'_, C>) -> Result<( "AllocDosObject(type={object_type}): guest heap allocation failed: {e}" ), })?; + ctx.mem.clear_fresh_block(addr, struct_size); for i in 0..struct_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/execlist.rs b/crates/volamos-core/src/execlist.rs index 3ec0d60..98ec33b 100644 --- a/crates/volamos-core/src/execlist.rs +++ b/crates/volamos-core/src/execlist.rs @@ -505,6 +505,7 @@ fn create_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() return Ok(()); } }; + ctx.mem.clear_fresh_block(port, MSGPORT_SIZE); init_msg_port_fields(ctx.mem, port, ctx.current_task); // After the field initialisation, never before: those writes heal // shadow bytes, so poisoning first would flag this handler's own @@ -591,6 +592,7 @@ fn create_io_request( size: u32, ) -> Option { let addr = heap.alloc(size).ok()?; + mem.clear_fresh_block(addr, size); for i in 0..size { mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/execmem.rs b/crates/volamos-core/src/execmem.rs index 0030d58..1fe9772 100644 --- a/crates/volamos-core/src/execmem.rs +++ b/crates/volamos-core/src/execmem.rs @@ -309,6 +309,7 @@ fn alloc_mem_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // knows the unrounded number. match ctx.heap.alloc_with_requested(rounded, byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -421,6 +422,7 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp let total = user_rounded + ALLOCVEC_HEADER_SIZE; match ctx.heap.alloc(total) { Ok(block) => { + ctx.mem.clear_fresh_block(block, total); // Header: total block size (u32) followed by 4 bytes of // reserved padding (see the module docs). ctx.mem.write_u32(block, total); @@ -445,12 +447,22 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // included -- the header is ours, not the guest's, but it // sits inside the same heap block, so the redzones bracket // both). Done after the header write and the MEMF_CLEAR - // loop above, per `poison_allocation_edges`' doc. The data - // bytes are left un-marked rather than marked uninit: the - // block address the heap knows is `block`, not `user_ptr`, - // so an uninit range starting at `block` would wrongly - // cover the header this handler just wrote. + // loop above, per `poison_allocation_edges`' doc. `false` + // because the data range cannot be described from `block`: + // that is what the heap knows, so an uninit range starting + // there would cover the header this handler just wrote. poison_allocation_edges(ctx, block, false); + // Which leaves the guest's own range marked valid by the + // call above -- right for the header, but it loses the + // uninitialized state of the part the guest owns. Mark that + // from `user_ptr`, the one address that excludes the header. + if let Some(shadow) = ctx.mem.shadow_mut() { + if cleared { + shadow.mark_valid(user_ptr, user_rounded); + } else { + shadow.mark_uninit(user_ptr, user_rounded); + } + } ctx.cpu.set_data_register(DataRegister(0), user_ptr); } Err(_) => { @@ -532,6 +544,7 @@ fn create_pool_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di match ctx.heap.alloc(POOL_HEADER_SIZE) { Ok(pool) => { + ctx.mem.clear_fresh_block(pool, POOL_HEADER_SIZE); ctx.mem.write_u32(pool, requirements); ctx.cpu.set_data_register(DataRegister(0), pool); } @@ -590,6 +603,7 @@ fn alloc_pooled_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), D let rounded = round_up_8(byte_size); match ctx.heap.alloc_with_requested(rounded, byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -969,6 +983,64 @@ mod tests { ); } + #[test] + fn a_recycled_alloc_vec_block_is_no_longer_a_freed_block() { + // Issue #95: FreeVec poisons the block, and the heap hands the + // same addresses back out once the free quarantine releases + // them. If the allocation path leaves that poison in place, + // both the handler's own AllocVec header write and every write + // the guest makes into its brand-new buffer are reported as + // use-after-free -- and at one violation per byte the run slows + // to a crawl. + let mut words = Vec::new(); + words.push(move_imm_to_d(0)); // D0 = byteSize + words.push(0); + words.push(64); + words.push(move_imm_to_d(1)); // D1 = 0, no MEMF_CLEAR + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-684)); // AllocVec + words.push(0x2240); // movea.l d0,a1 (A1 = the block to free) + words.extend_from_slice(&jsr_disp16_a6(-690)); // FreeVec + words.push(move_imm_to_d(0)); // same size again + words.push(0); + words.push(64); + words.push(move_imm_to_d(1)); + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-684)); // AllocVec + words.push(0x2040); // movea.l d0,a0 + words.push(0x10bc); // move.b #1,(a0) -- the guest's first write + words.push(0x0001); + words.push(RTS); + + let mut rt = program(&words); + rt.memory_mut().enable_sanitizer(); + rt.enable_heap_sanitizer(); + // No quarantine, so the second AllocVec is handed the block the + // FreeVec just poisoned -- the whole point of the test. With the + // default budget a block this small is held back instead, and + // the run never recycles anything. + rt.heap_mut().set_quarantine_budget(0); + let mut out = Vec::new(); + let code = rt.run(&mut out, None).expect("run should succeed"); + let addr = code as u32; + + assert_ne!(addr, 0, "both allocations must succeed for this to test anything"); + let shadow = rt.memory().shadow().expect("sanitizer was enabled above"); + assert_eq!( + shadow.violation_count(), + 0, + "writing into a freshly allocated block must not be reported, \ + whatever that memory was used for before" + ); + assert_ne!( + shadow.state(addr), + crate::sanitize::ShadowState::Unaddressable, + "the recycled block must not still be marked as freed" + ); + } + #[test] fn alloc_mem_with_memf_clear_zeroes_and_alloc_free_round_trips() { let mut words = Vec::new(); diff --git a/crates/volamos-core/src/memory.rs b/crates/volamos-core/src/memory.rs index 392aa79..2817e7d 100644 --- a/crates/volamos-core/src/memory.rs +++ b/crates/volamos-core/src/memory.rs @@ -50,6 +50,21 @@ pub trait AddressSpace { /// Writes a single byte. Silently ignored if `addr` is out of range. fn write_u8(&mut self, addr: u32, value: u8); + /// Clears any sanitizer poison over `[addr, addr + len)`, a block the + /// guest heap has just handed out. + /// + /// Call this as soon as an allocation succeeds, *before* the handler + /// writes a header or zeroes the block: the heap recycles addresses, + /// so those bytes may still carry the `PoisonReason::Freed` marking + /// of the previous allocation, and writing over them does not clear + /// it -- the shadow map heals `Uninit` bytes but keeps + /// `Unaddressable` ones. Without this, the handler's own + /// initialization is reported as a use-after-free, and so is every + /// write the guest then makes. + /// + /// Default: nothing to do, for address spaces with no shadow map. + fn clear_fresh_block(&mut self, _addr: u32, _len: u32) {} + /// Reads a big-endian 16-bit value. Any byte that falls out of range /// reads as `0`. fn read_u16(&self, addr: u32) -> u16 { @@ -224,6 +239,12 @@ impl AddressSpace for FlatMemory { self.bytes.get(addr as usize).copied().unwrap_or(0) } + fn clear_fresh_block(&mut self, addr: u32, len: u32) { + if let Some(shadow) = &mut self.shadow { + shadow.mark_valid(addr, len); + } + } + fn write_u8(&mut self, addr: u32, value: u8) { if let Some(shadow) = &mut self.shadow { shadow.check_write(addr, 1); diff --git a/crates/volamos-core/src/utility.rs b/crates/volamos-core/src/utility.rs index 7698dfd..fe179a6 100644 --- a/crates/volamos-core/src/utility.rs +++ b/crates/volamos-core/src/utility.rs @@ -276,6 +276,7 @@ fn allocate_tag_items_handler( let byte_size = num_tags.saturating_mul(TAG_ITEM_SIZE); match ctx.heap.alloc(byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, byte_size); for i in 0..byte_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); }