diff --git a/crates/volamos-core/src/dosfile.rs b/crates/volamos-core/src/dosfile.rs index a733504..04bec04 100644 --- a/crates/volamos-core/src/dosfile.rs +++ b/crates/volamos-core/src/dosfile.rs @@ -1359,6 +1359,11 @@ fn alloc_dos_object_handler(ctx: &mut HandlerContext<'_, C>) -> Result<( for i in 0..struct_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } + // After the zeroing, never before -- those writes heal shadow + // bytes, so poisoning first would flag this handler's own + // initialisation. `false` because the block is zeroed, hence + // initialised. See `crate::execmem::poison_allocation_edges`. + crate::execmem::poison_allocation_edges(ctx, addr, false); ctx.cpu.set_data_register(DataRegister(0), addr); Ok(()) } @@ -1390,6 +1395,8 @@ fn free_dos_object_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() }); } + // Before the free, while the extent is still queryable. + crate::execmem::poison_freed_block(ctx, addr); ctx.heap .free(addr) .map_err(|e| DispatchError::HandlerFailed { diff --git a/crates/volamos-core/src/execlist.rs b/crates/volamos-core/src/execlist.rs index 67ecc62..3ec0d60 100644 --- a/crates/volamos-core/src/execlist.rs +++ b/crates/volamos-core/src/execlist.rs @@ -506,6 +506,11 @@ fn create_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() } }; 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 + // setup. `false` because the block is initialised by the time we + // get here. See `crate::execmem::poison_allocation_edges`. + crate::execmem::poison_allocation_edges(ctx, port, false); ctx.cpu.set_data_register(DataRegister(0), port); Ok(()) } @@ -540,6 +545,7 @@ fn delete_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() if port == 0 { return Ok(()); } + crate::execmem::poison_freed_block(ctx, port); ctx.heap .free(port) .map_err(|e| DispatchError::HandlerFailed { @@ -565,6 +571,13 @@ fn create_io_request_handler(ctx: &mut HandlerContext<'_, C>) -> Result< let port = ctx.cpu.address_register(AddressRegister(0)); let size = ctx.cpu.data_register(DataRegister(0)); let addr = create_io_request(ctx.heap, ctx.mem, port, size).unwrap_or(0); + if addr != 0 { + // Poisoned here rather than inside `create_io_request`, which + // takes `heap`/`mem` separately and so has no `HandlerContext` + // to reach the shadow map through. After its zeroing and field + // writes, per `poison_allocation_edges`' ordering rule. + crate::execmem::poison_allocation_edges(ctx, addr, false); + } ctx.cpu.set_data_register(DataRegister(0), addr); Ok(()) } @@ -595,6 +608,7 @@ fn delete_io_request_handler(ctx: &mut HandlerContext<'_, C>) -> Result< if addr == 0 { return Ok(()); } + crate::execmem::poison_freed_block(ctx, addr); ctx.heap .free(addr) .map_err(|e| DispatchError::HandlerFailed { diff --git a/crates/volamos-core/src/execmem.rs b/crates/volamos-core/src/execmem.rs index 920ffef..0030d58 100644 --- a/crates/volamos-core/src/execmem.rs +++ b/crates/volamos-core/src/execmem.rs @@ -204,6 +204,17 @@ fn apply_dirty_fill(ctx: &mut HandlerContext<'_, C>, addr: u32, len: u32 /// at `addr` (issue #65). A no-op unless both a shadow map is installed /// (`--sanitize`) and the allocation actually exists in the heap. /// +/// `pub(crate)` because `AllocMem`/`AllocVec`/`AllocPooled` are not the +/// only handlers that carve guest-visible blocks out of +/// [`crate::guestmem::GuestHeap`]: `utility.library`'s +/// `AllocateTagItems`, `dos.library`'s `AllocDosObject` and +/// `exec.library`'s `CreateIORequest`/`CreateMsgPort` all do too, and +/// every one of them hands the guest a block it can overrun. Since they +/// allocate from the same heap, redzone *space* is already being +/// reserved for them whenever `--sanitize` is on -- only the shadow +/// marking was missing, which is why sharing this one helper is the +/// whole fix rather than a per-module reimplementation. +/// /// Marks the leading and trailing redzones and the alignment slack /// unaddressable, so any guest access that runs off either end of the /// block -- including into the padding between the size it asked for @@ -224,7 +235,7 @@ fn apply_dirty_fill(ctx: &mut HandlerContext<'_, C>, addr: u32, len: u32 /// default -- see `crate::sanitize`). Pass `false` when the handler has /// already zeroed the block for `MEMF_CLEAR`, since those bytes are /// then genuinely initialized. -fn poison_allocation_edges( +pub(crate) fn poison_allocation_edges( ctx: &mut HandlerContext<'_, C>, addr: u32, mark_data_uninit: bool, @@ -262,7 +273,7 @@ fn poison_allocation_edges( /// the other half is `crate::guestmem`'s free quarantine holding the /// address out of circulation, so a subsequent allocation doesn't /// immediately re-mark these same bytes valid and hide the bug. -fn poison_freed_block(ctx: &mut HandlerContext<'_, C>, addr: u32) { +pub(crate) fn poison_freed_block(ctx: &mut HandlerContext<'_, C>, addr: u32) { let Some(extent) = ctx.heap.extent_of_live_alloc(addr) else { return; }; diff --git a/crates/volamos-core/src/utility.rs b/crates/volamos-core/src/utility.rs index 194d7d9..7698dfd 100644 --- a/crates/volamos-core/src/utility.rs +++ b/crates/volamos-core/src/utility.rs @@ -279,6 +279,13 @@ fn allocate_tag_items_handler( for i in 0..byte_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } + // After the zeroing, never before -- the writes above heal + // shadow bytes, so poisoning first would make this handler + // report a violation against its own initialisation. Same + // ordering rule as `crate::execmem`'s `MEMF_CLEAR` path; + // see `poison_allocation_edges`' doc. `false` because the + // block *is* initialised: it was just zeroed. + crate::execmem::poison_allocation_edges(ctx, addr, false); ctx.cpu.set_data_register(DataRegister(0), addr); } Err(_) => { @@ -325,6 +332,8 @@ fn free_tag_items_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), } let byte_size = count.saturating_mul(TAG_ITEM_SIZE); + // Before the free, while the extent is still queryable. + crate::execmem::poison_freed_block(ctx, list); ctx.heap .free(list) .map_err(|e| DispatchError::HandlerFailed { @@ -958,6 +967,91 @@ mod tests { 0x2040 | (n << 9) } + /// `move.l Dsrc,Ddst` -- MOVE.L with both operands in data-register + /// direct mode (`0010 ddd 000 000 sss`), for stashing an address + /// across a library call that clobbers D0. + fn move_d_to_d(dst: u16, src: u16) -> u16 { + 0x2000 | (dst << 9) | src + } + + #[test] + fn freed_tag_items_block_is_poisoned_for_use_after_free() { + // The other half: `FreeTagItems` now marks the block freed, so + // a guest still holding the pointer is caught reading it. + let mut words = Vec::new(); + words.push(move_imm_to_d(0)); + words.push(0); + words.push(3); + words.extend_from_slice(&jsr_disp16_a6(-66)); // AllocateTagItems -> D0 + words.push(move_d0_to_a(0)); // A0 = the array + words.push(move_d_to_d(2, 0)); // stash the address in D2 + words.extend_from_slice(&jsr_disp16_a6(-78)); // FreeTagItems(A0) + words.push(move_d_to_d(0, 2)); // D0 = the now-freed address + words.push(RTS); + + let mut rt = program(&words); + rt.memory_mut().enable_sanitizer(); + rt.enable_heap_sanitizer(); + 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, "expected the freed block's address back in D0"); + + let shadow = rt.memory().shadow().expect("sanitizer enabled above"); + assert_eq!( + shadow.state(addr), + crate::sanitize::ShadowState::Unaddressable, + "a freed TagItem array should read as unaddressable" + ); + assert_eq!( + shadow.poison_reason(addr), + Some(crate::sanitize::PoisonReason::Freed), + "and specifically as freed, not as a redzone" + ); + } + + #[test] + fn allocate_tag_items_block_gets_redzones_under_the_sanitizer() { + // `AllocateTagItems` carves from the same `GuestHeap` as + // `AllocMem`, so redzone *space* was already being reserved for + // it whenever the sanitizer was on -- only the shadow marking + // was missing, which meant a guest overrunning a TagItem array + // volamos handed it went unreported. This asserts the guard + // bytes either side of the block are now actually poisoned. + let mut words = Vec::new(); + words.push(move_imm_to_d(0)); // D0 = numTags + words.push(0); + words.push(3); + words.extend_from_slice(&jsr_disp16_a6(-66)); // AllocateTagItems + words.push(RTS); + + let mut rt = program(&words); + rt.memory_mut().enable_sanitizer(); + rt.enable_heap_sanitizer(); + 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); + + let shadow = rt.memory().shadow().expect("sanitizer enabled above"); + let size = 3 * TAG_ITEM_SIZE; + assert_eq!( + shadow.state(addr.wrapping_sub(1)), + crate::sanitize::ShadowState::Unaddressable, + "the byte before the block should be a poisoned redzone" + ); + assert_eq!( + shadow.state(addr + size), + crate::sanitize::ShadowState::Unaddressable, + "the byte just past the block should be a poisoned redzone" + ); + assert_eq!( + shadow.state(addr), + crate::sanitize::ShadowState::Valid, + "the block itself was zeroed by the handler, so it is initialised" + ); + } + #[test] fn allocate_tag_items_returns_a_zeroed_block() { let mut words = Vec::new();