From e21a6c36587e0644ca8cc49dea723eac937e4b9f Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Mon, 21 Feb 2022 14:28:55 +0100 Subject: [PATCH 1/7] Make stack-painting faster! --- src/canary.rs | 115 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 10 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index 69a95723..251e08c8 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -1,8 +1,8 @@ use std::time::Instant; -use crate::TIMEOUT; -use crate::{Elf, TargetInfo}; -use probe_rs::{MemoryInterface, Session}; +use probe_rs::{Core, MemoryInterface, Session}; + +use crate::{registers::PC, Elf, TargetInfo, TIMEOUT}; const CANARY_VALUE: u8 = 0xAA; @@ -45,6 +45,7 @@ pub(crate) struct Canary { } impl Canary { + /// Decide if and where to place the stack canary. pub(crate) fn install( sess: &mut Session, target_info: &TargetInfo, @@ -54,8 +55,6 @@ impl Canary { let mut core = sess.core(0)?; core.reset_and_halt(TIMEOUT)?; - // Decide if and where to place the stack canary. - let stack_info = match &target_info.stack_info { Some(range) => range, None => { @@ -69,7 +68,9 @@ impl Canary { return Ok(None); } - let stack_available = stack_info.range.end() - stack_info.range.start() - 1; + let stack_start = *stack_info.range.start(); + let stack_end = *stack_info.range.end() - 1; + let stack_available = stack_end - stack_start; let size = if measure_stack { // When measuring stack consumption, we have to color the whole stack. @@ -97,10 +98,8 @@ impl Canary { size_kb ); } - let address = *stack_info.range.start(); - let canary = vec![CANARY_VALUE; size]; let start = Instant::now(); - core.write_8(address, &canary)?; + paint_stack(&mut core, stack_start, stack_start + size as u32)?; let seconds = start.elapsed().as_secs_f64(); log::trace!( "setting up canary took {:.3}s ({:.2} KiB/s)", @@ -109,7 +108,7 @@ impl Canary { ); Ok(Some(Canary { - address, + address: stack_start, size, stack_available, data_below_stack: stack_info.data_below_stack, @@ -186,3 +185,99 @@ impl Canary { } } } + +/// Write [`CANARY_VALUE`] to the stack. +fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs::Error> { + // make sure stack_end is properly aligned + if end % 4 != 0 { + end += end % 4; + } + + // construct subroutine + let subroutine = subroutine(start + SUBROUTINE_LENGTH, end); + + // does the subroutine fit inside the stack? + let stack_size = (end - start) as usize; + assert!( + subroutine.len() < stack_size, + "subroutine doesn't fit inside stack" + ); + + // write subroutine to RAM + core.write_8(start, &subroutine)?; + + // store current PC and set PC to beginning of subroutine + let previous_pc = core.read_core_reg(PC)?; + core.write_core_reg(PC, start)?; + + // execute the subroutine and wait for it to finish + core.run()?; + core.wait_for_core_halted(TIMEOUT).unwrap(); + + // overwrite subroutine + core.write_8(start, &vec![CANARY_VALUE; subroutine.len()])?; + + // reset PC to where it was before + core.write_core_reg(PC, previous_pc)?; + + Ok(()) +} + +/// The length of the subroutine. +/// +/// This is checked on runtime and needs to be updated when changing the subroutine. +const SUBROUTINE_LENGTH: u32 = 28; + +/// Create a subroutine to paint [`CANARY_VALUE`] from `start` till `end`. +/// +/// Both `start` and `end` need to be 4-byte-aligned. +// +// Roughly corresponds to following assembly: +// +// 00000108 : +// 108: 4803 ldr r0, [pc, #12] ; (118 ) +// 10a: 4904 ldr r1, [pc, #16] ; (11c ) +// 10c: 4a04 ldr r2, [pc, #16] ; (120 ) +// +// 0000010e : +// 10e: 4281 cmp r1, r0 +// 110: d001 beq.n 116 +// 112: c004 stmia r0!, {r2} +// 114: e7fb b.n 10e +// +// 00000116 : +// 116: be00 bkpt 0x0000 +// 118: 20000100 .word 0x20000100 ; start +// 11c: 20000200 .word 0x20000200 ; end +// 120: aaaaaaaa .word 0xaaaaaaaa ; pattern +fn subroutine(start: u32, end: u32) -> Vec { + assert!(start < end, "start needs to be smaller than end address"); + assert_eq!(start % 4, 0, "`start` needs to be 4-byte-aligned"); + assert_eq!(end % 4, 0, "`end` needs to be 4-byte-aligned"); + + // convert start and end address to bytes + let start_bytes = start.to_le_bytes(); + let end_bytes = end.to_le_bytes(); + + let mut subroutine = vec![ + 0x03, 0x48, // ldr + 0x04, 0x49, // ldr + 0x04, 0x4a, // ldr + 0x81, 0x42, // cmp + 0x01, 0xD0, // beq.n + 0x04, 0xC0, // stmia + 0xFB, 0xE7, // b.n + 0x00, 0xBE, // bkpt + ]; + + // add the start and end address + subroutine.extend(start_bytes); + subroutine.extend(end_bytes); + + // add the pattern to be painted + subroutine.extend([CANARY_VALUE; 4]); + + assert_eq!(subroutine.len() as u32, SUBROUTINE_LENGTH); + + subroutine +} From 73040b3de918e3c4ee0cbec12dbc3139b9290ca5 Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Mon, 21 Feb 2022 14:48:19 +0100 Subject: [PATCH 2/7] Avoid allocating vector for subroutine --- src/canary.rs | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index 251e08c8..79963910 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -193,18 +193,16 @@ fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs end += end % 4; } - // construct subroutine - let subroutine = subroutine(start + SUBROUTINE_LENGTH, end); - // does the subroutine fit inside the stack? let stack_size = (end - start) as usize; assert!( - subroutine.len() < stack_size, + SUBROUTINE_LENGTH < stack_size, "subroutine doesn't fit inside stack" ); // write subroutine to RAM - core.write_8(start, &subroutine)?; + // NOTE: add `SUBROUTINE_LENGTH` to `start`, to avoid the subroutine overwriting itself + core.write_8(start, &subroutine(start + SUBROUTINE_LENGTH as u32, end))?; // store current PC and set PC to beginning of subroutine let previous_pc = core.read_core_reg(PC)?; @@ -215,7 +213,7 @@ fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs core.wait_for_core_halted(TIMEOUT).unwrap(); // overwrite subroutine - core.write_8(start, &vec![CANARY_VALUE; subroutine.len()])?; + core.write_8(start, &[CANARY_VALUE; SUBROUTINE_LENGTH])?; // reset PC to where it was before core.write_core_reg(PC, previous_pc)?; @@ -224,9 +222,7 @@ fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs } /// The length of the subroutine. -/// -/// This is checked on runtime and needs to be updated when changing the subroutine. -const SUBROUTINE_LENGTH: u32 = 28; +const SUBROUTINE_LENGTH: usize = 28; /// Create a subroutine to paint [`CANARY_VALUE`] from `start` till `end`. /// @@ -250,16 +246,17 @@ const SUBROUTINE_LENGTH: u32 = 28; // 118: 20000100 .word 0x20000100 ; start // 11c: 20000200 .word 0x20000200 ; end // 120: aaaaaaaa .word 0xaaaaaaaa ; pattern -fn subroutine(start: u32, end: u32) -> Vec { +fn subroutine(start: u32, end: u32) -> [u8; SUBROUTINE_LENGTH] { assert!(start < end, "start needs to be smaller than end address"); assert_eq!(start % 4, 0, "`start` needs to be 4-byte-aligned"); assert_eq!(end % 4, 0, "`end` needs to be 4-byte-aligned"); // convert start and end address to bytes - let start_bytes = start.to_le_bytes(); - let end_bytes = end.to_le_bytes(); + let [s1, s2, s3, s4] = start.to_le_bytes(); + let [e1, e2, e3, e4] = end.to_le_bytes(); - let mut subroutine = vec![ + const CV: u8 = CANARY_VALUE; + [ 0x03, 0x48, // ldr 0x04, 0x49, // ldr 0x04, 0x4a, // ldr @@ -268,16 +265,8 @@ fn subroutine(start: u32, end: u32) -> Vec { 0x04, 0xC0, // stmia 0xFB, 0xE7, // b.n 0x00, 0xBE, // bkpt - ]; - - // add the start and end address - subroutine.extend(start_bytes); - subroutine.extend(end_bytes); - - // add the pattern to be painted - subroutine.extend([CANARY_VALUE; 4]); - - assert_eq!(subroutine.len() as u32, SUBROUTINE_LENGTH); - - subroutine + s1, s2, s3, s4, // start address + e1, e2, e3, e4, // end address + CV, CV, CV, CV, // canary value + ] } From 6c1aa1dbd37d80083b18caa7abf59f615fc58f80 Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Tue, 22 Feb 2022 10:56:48 +0100 Subject: [PATCH 3/7] Assert `start < end` earlier --- src/canary.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/canary.rs b/src/canary.rs index 79963910..fc349ac1 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -188,6 +188,8 @@ impl Canary { /// Write [`CANARY_VALUE`] to the stack. fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs::Error> { + assert!(start < end, "start needs to be smaller than end address"); + // make sure stack_end is properly aligned if end % 4 != 0 { end += end % 4; @@ -247,7 +249,6 @@ const SUBROUTINE_LENGTH: usize = 28; // 11c: 20000200 .word 0x20000200 ; end // 120: aaaaaaaa .word 0xaaaaaaaa ; pattern fn subroutine(start: u32, end: u32) -> [u8; SUBROUTINE_LENGTH] { - assert!(start < end, "start needs to be smaller than end address"); assert_eq!(start % 4, 0, "`start` needs to be 4-byte-aligned"); assert_eq!(end % 4, 0, "`end` needs to be 4-byte-aligned"); From 73f8111edf983a91e9517001d0c9f947744058b2 Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Tue, 22 Feb 2022 10:59:25 +0100 Subject: [PATCH 4/7] Extend comments inside `fn subroutine` --- src/canary.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index fc349ac1..ccbbd1b0 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -258,16 +258,19 @@ fn subroutine(start: u32, end: u32) -> [u8; SUBROUTINE_LENGTH] { const CV: u8 = CANARY_VALUE; [ - 0x03, 0x48, // ldr - 0x04, 0x49, // ldr - 0x04, 0x4a, // ldr - 0x81, 0x42, // cmp - 0x01, 0xD0, // beq.n - 0x04, 0xC0, // stmia - 0xFB, 0xE7, // b.n - 0x00, 0xBE, // bkpt - s1, s2, s3, s4, // start address - e1, e2, e3, e4, // end address - CV, CV, CV, CV, // canary value + 0x03, 0x48, // ldr r0, [pc, #12] + 0x04, 0x49, // ldr r1, [pc, #16] + 0x04, 0x4a, // ldr r2, [pc, #16] + // + 0x81, 0x42, // cmp r1, r0 + 0x01, 0xD0, // beq.n 116 + 0x04, 0xC0, // stmia r0!, {r2} + 0xFB, 0xE7, // b.n 10e + // + 0x00, 0xBE, // bkpt 0x0000 + // + s1, s2, s3, s4, // .word ; start address + e1, e2, e3, e4, // .word ; end address + CV, CV, CV, CV, // .word ; canary value ] } From cea80d34371bc66e266717355110226d66aae0f8 Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Tue, 22 Feb 2022 11:49:04 +0100 Subject: [PATCH 5/7] Minor fixes --- src/canary.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index ccbbd1b0..6757adee 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -56,7 +56,7 @@ impl Canary { core.reset_and_halt(TIMEOUT)?; let stack_info = match &target_info.stack_info { - Some(range) => range, + Some(stack_info) => stack_info, None => { log::debug!("couldn't find valid stack range, not placing stack canary"); return Ok(None); @@ -212,7 +212,7 @@ fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs // execute the subroutine and wait for it to finish core.run()?; - core.wait_for_core_halted(TIMEOUT).unwrap(); + core.wait_for_core_halted(TIMEOUT)?; // overwrite subroutine core.write_8(start, &[CANARY_VALUE; SUBROUTINE_LENGTH])?; From 4331ea1d344b3887c2555364520a19f16878df1a Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Tue, 22 Feb 2022 11:49:28 +0100 Subject: [PATCH 6/7] Solve alignment issues --- src/canary.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index 6757adee..a2445b79 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -69,7 +69,8 @@ impl Canary { } let stack_start = *stack_info.range.start(); - let stack_end = *stack_info.range.end() - 1; + // add 1 to end up with the initial stack pointer, which is always 4-byte-aligned + let stack_end = *stack_info.range.end() + 1; let stack_available = stack_end - stack_start; let size = if measure_stack { @@ -187,13 +188,12 @@ impl Canary { } /// Write [`CANARY_VALUE`] to the stack. -fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs::Error> { +/// +/// Both `start` and `end` need to be 4-byte-aligned. +fn paint_stack(core: &mut Core, start: u32, end: u32) -> Result<(), probe_rs::Error> { assert!(start < end, "start needs to be smaller than end address"); - - // make sure stack_end is properly aligned - if end % 4 != 0 { - end += end % 4; - } + assert_eq!(start % 4, 0, "`start` needs to be 4-byte-aligned"); + assert_eq!(end % 4, 0, "`end` needs to be 4-byte-aligned"); // does the subroutine fit inside the stack? let stack_size = (end - start) as usize; @@ -227,8 +227,6 @@ fn paint_stack(core: &mut Core, start: u32, mut end: u32) -> Result<(), probe_rs const SUBROUTINE_LENGTH: usize = 28; /// Create a subroutine to paint [`CANARY_VALUE`] from `start` till `end`. -/// -/// Both `start` and `end` need to be 4-byte-aligned. // // Roughly corresponds to following assembly: // @@ -249,9 +247,6 @@ const SUBROUTINE_LENGTH: usize = 28; // 11c: 20000200 .word 0x20000200 ; end // 120: aaaaaaaa .word 0xaaaaaaaa ; pattern fn subroutine(start: u32, end: u32) -> [u8; SUBROUTINE_LENGTH] { - assert_eq!(start % 4, 0, "`start` needs to be 4-byte-aligned"); - assert_eq!(end % 4, 0, "`end` needs to be 4-byte-aligned"); - // convert start and end address to bytes let [s1, s2, s3, s4] = start.to_le_bytes(); let [e1, e2, e3, e4] = end.to_le_bytes(); From 67c15760a1eeb13f006ec371562917c36c16dfa5 Mon Sep 17 00:00:00 2001 From: Johann Hemmann Date: Wed, 23 Feb 2022 09:56:39 +0100 Subject: [PATCH 7/7] Stack end 4 bytes below initial SP, not 1 --- src/canary.rs | 4 +--- src/target_info.rs | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/canary.rs b/src/canary.rs index a2445b79..e1a600b9 100644 --- a/src/canary.rs +++ b/src/canary.rs @@ -69,9 +69,7 @@ impl Canary { } let stack_start = *stack_info.range.start(); - // add 1 to end up with the initial stack pointer, which is always 4-byte-aligned - let stack_end = *stack_info.range.end() + 1; - let stack_available = stack_end - stack_start; + let stack_available = *stack_info.range.end() - stack_start; let size = if measure_stack { // When measuring stack consumption, we have to color the whole stack. diff --git a/src/target_info.rs b/src/target_info.rs index eac743e2..24971cf6 100644 --- a/src/target_info.rs +++ b/src/target_info.rs @@ -73,8 +73,8 @@ fn extract_stack_info(elf: &Elf, ram_range: &Range) -> Option { let initial_stack_pointer = elf.vector_table.initial_stack_pointer; - // SP points one past the end of the stack. - let mut stack_range = ram_range.start..=initial_stack_pointer - 1; + // SP points one word (4-byte) past the end of the stack. + let mut stack_range = ram_range.start..=initial_stack_pointer - 4; for section in elf.sections() { let size: u32 = section.size().try_into().expect("expected 32-bit ELF");