From 9dcbf460ff692c049e4d327afcbdbc39ba4e8c65 Mon Sep 17 00:00:00 2001 From: Miquel Sabaté Solà Date: Wed, 3 Sep 2025 19:08:05 +0200 Subject: Validate that memory access is done via variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The address sanitizer is now able to detect whenever in an instruction a memory access is done without using variables. This is now detected for all instructions except for branching, which falls outside of this scope. Moreover, simple arithmetics is allowed and bounds are checked for simple cases. That being said, more involved bound checks should be done with other tools (e.g. emulators). Signed-off-by: Miquel Sabaté Solà --- lib/xixanta/src/assembler.rs | 136 ++++++++++++++++++++++++++++++++++++--- lib/xixanta/src/node.rs | 30 +++++++++ scripts/test-e2e.sh | 6 ++ tests/bare_accesses.s | 37 +++++++++++ tests/expected/bare_accesses.txt | 4 ++ 5 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 tests/bare_accesses.s create mode 100644 tests/expected/bare_accesses.txt diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs index f63e785..09f0c9d 100644 --- a/lib/xixanta/src/assembler.rs +++ b/lib/xixanta/src/assembler.rs @@ -1,5 +1,8 @@ use crate::mapping::{get_mapping_configuration, Mapping}; -use crate::node::{CommentType, ControlType, EchoKind, NodeType, OperationType, PNode, PString}; +use crate::node::{ + is_asan_friendly_name, CommentType, ControlType, EchoKind, NodeType, OperationType, PNode, + PString, +}; use crate::object::{Bundle, Context, Object, ObjectType}; use crate::opcodes::{AddressingMode, INSTRUCTIONS}; use crate::parser::Parser; @@ -856,9 +859,10 @@ impl<'a> Assembler<'a> { } // If it doesn't have the proper prefix, skip as well. - if !name.starts_with("zp_") && !name.starts_with("m_") && !name.starts_with("wr_") { + if !is_asan_friendly_name(name) { continue; } + let actual_name = name.split("::").last().unwrap_or(""); // Build up the memory range object for this bundle. let val = bundle.bundle.value() as usize; @@ -902,7 +906,7 @@ impl<'a> Assembler<'a> { // Increase the counters for memory usage on either RAM slot and // check for bounds. - if name.starts_with("zp_") || name.starts_with("m_") { + if actual_name.starts_with("zp_") || actual_name.starts_with("m_") { memory.total_internal_ram += bundle.asan_reserve as usize; if memory.total_internal_ram > 0x800 { @@ -913,7 +917,7 @@ impl<'a> Assembler<'a> { source: self.sources[0].clone(), }); } - } else if name.starts_with("wr_") { + } else if actual_name.starts_with("wr_") { memory.total_working_ram += bundle.asan_reserve as usize; if memory.total_working_ram > 0x2000 { @@ -2318,7 +2322,8 @@ impl<'a> Assembler<'a> { }); } - let val = self.evaluate_node(left.left.as_ref().unwrap())?; + let evaluated_node = left.left.as_ref().unwrap(); + let val = self.evaluate_node(evaluated_node)?; if val.size != 1 { return Err(Error { message: "address can only be one byte long on indirect Y addressing" @@ -2328,6 +2333,7 @@ impl<'a> Assembler<'a> { global: false, }); } + self.asan_check_arm(evaluated_node, &val)?; return Ok((AddressingMode::IndirectY, val)); } Err(Error { @@ -2340,7 +2346,8 @@ impl<'a> Assembler<'a> { None => match left.right.as_ref() { Some(right) => { if right.value.value.trim().to_lowercase() == "x" { - let val = self.evaluate_node(left.left.as_ref().unwrap())?; + let evaluated_node = left.left.as_ref().unwrap(); + let val = self.evaluate_node(evaluated_node)?; if val.size != 1 { return Err(Error { message: @@ -2351,6 +2358,7 @@ impl<'a> Assembler<'a> { global: false, }); } + self.asan_check_arm(evaluated_node, &val)?; return Ok((AddressingMode::IndirectX, val)); } Err(Error { @@ -2361,7 +2369,8 @@ impl<'a> Assembler<'a> { }) } None => { - let val = self.evaluate_node(left.left.as_ref().unwrap())?; + let evaluated_node = left.left.as_ref().unwrap(); + let val = self.evaluate_node(evaluated_node)?; if val.size != 2 { return Err(Error { message: "expecting a full 16-bit address".to_string(), @@ -2370,6 +2379,7 @@ impl<'a> Assembler<'a> { global: false, }); } + self.asan_check_arm(evaluated_node, &val)?; Ok((AddressingMode::Indirect, val)) } }, @@ -2398,6 +2408,8 @@ impl<'a> Assembler<'a> { let right = node.right.as_ref().unwrap(); match right.value.value.to_lowercase().trim() { "x" => { + self.asan_check_arm(left, &val)?; + // If the size == 2 but we can fit it on a single byte (i.e. // because the second byte is just 0x00), then just "compress" // this instruction. Note that this is only valid if the value @@ -2429,6 +2441,8 @@ impl<'a> Assembler<'a> { } } "y" => { + self.asan_check_arm(left, &val)?; + // Same optimization as with the "x" case. if val.size == 1 || (val.resolved && val.bytes[1] == 0x00) { // Similar to the case on "x" indexing. @@ -2471,10 +2485,13 @@ impl<'a> Assembler<'a> { Some(LiteralMode::Hexadecimal) => { // As for checking the most significant byte, it's the same // optimization as with absolute to zeropage indexed addressing. - if base.is_branch() - || val.size == 1 + if base.is_branch() { + val.size = 1; + Ok((AddressingMode::RelativeOrZeropage, val)) + } else if val.size == 1 || (val.bytes[1] == 0x00 && !matches!(base.value.value.as_str(), "jmp" | "jsr")) { + self.asan_check_arm(left_arm, &val)?; val.size = 1; Ok((AddressingMode::RelativeOrZeropage, val)) } else { @@ -2508,6 +2525,107 @@ impl<'a> Assembler<'a> { } } + // Check the `node` considering is an instructions' "left" arm. More than + // that, it assumes that it's a memory access. Hence, only call this + // function if the addressing mode allows for it. Moreover, a `value` is + // also supplied to perform further checks on the given memory access. + // + // Note that it will only run if the address sanitizer is enabled, and it + // will check that any access only occurs as a variable or a simple pointer + // arithmetic. Also note that this function assumes that it's not being + // called by `jmp` or `jsr` kind of instructions on the absolute addressing + // mode, as they operate on another level. + fn asan_check_arm(&mut self, node: &PNode, value: &Bundle) -> Result<(), Error> { + if !self.asan_enabled { + return Ok(()); + } + + match node.node_type { + NodeType::Value => { + let name = &node.value.value; + if !is_asan_friendly_name(name) { + // If it's referencing an actual ObjectType::Address, then + // let it be (e.g. 'lda palettes, x'; where 'palettes' is a + // legitimate name even if not 'is_asan_friendly_name'). + if let Ok(var) = self.context.get_variable(&node.value, &self.mappings) { + if matches!(var.object_type, ObjectType::Address) { + return Ok(()); + } + } + + // Everything has been exhausted, this is actually not a + // good name access. + self.warnings.push(Error { + line: node.value.line, + message: format!( + "accessing a memory region without a proper name ('{name}')" + ), + source: self.source_for(node), + global: false, + }); + } + Ok(()) + } + NodeType::Operation(OperationType::Add) | NodeType::Operation(OperationType::Sub) => { + // Get the name of the variable involved. + let left_name = &node.left.as_ref().unwrap().value.value; + let right_name = &node.left.as_ref().unwrap().value.value; + let name = if is_asan_friendly_name(left_name) { + node.left.as_ref().unwrap() + } else if is_asan_friendly_name(right_name) { + node.right.as_ref().unwrap() + } else { + self.warnings.push(Error { + line: node.value.line, + message: "accessing a memory region without a proper name".to_string(), + source: self.source_for(node), + global: false, + }); + return Ok(()); + }; + + // Fetch the variable. If it doesn't exist or its value hasn't + // been resolved yet, we will ignore this check. + match self.context.get_variable(&name.value, &self.mappings) { + Ok(object) => { + // If it has a resolved value, then check that the + // pointer arithmetics are within reserved bounds. + if object.bundle.resolved { + let original_value = object.bundle.value() as usize; + let given_value = value.value() as usize; + let end = original_value + object.asan_reserve as usize; + + if given_value < original_value || given_value >= end { + let range = MemoryRange { + name: name.value.value.clone(), + range: (original_value..end), + }; + return Err(Error { + line: node.value.line, + message: format!("out of bounds memory access for {range}"), + source: self.source_for(node), + global: false, + }); + } + } + Ok(()) + } + Err(_) => Ok(()), + } + } + _ => { + self.warnings.push(Error { + line: node.value.line, + message: "accessing a memory region without using a variable".to_string(), + source: self.source_for(node), + global: false, + }); + + Ok(()) + } + } + } + fn to_relative_address(&self, node: &PNode, bundle: &mut Bundle) -> Result<(), Error> { if !bundle.resolved { return Ok(()); diff --git a/lib/xixanta/src/node.rs b/lib/xixanta/src/node.rs index 7cfc107..465dde2 100644 --- a/lib/xixanta/src/node.rs +++ b/lib/xixanta/src/node.rs @@ -1,5 +1,13 @@ use std::fmt; +/// Returns true if this String follows the naming +/// conventions as expected by the address sanitizer. +pub fn is_asan_friendly_name(string: &str) -> bool { + let name = string.split("::").last().unwrap_or(""); + + name.starts_with("zp_") || name.starts_with("wr_") || name.starts_with("m_") +} + /// A Positioned String. That is, a String which also has information on the /// line number and the column range. #[derive(Debug, Default, Clone, PartialEq)] @@ -456,3 +464,25 @@ impl PNode { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn is_asan_friendly_name_test() { + let tests = vec![ + ("zp_name", true), + ("name", false), + ("Scope::zp_name", true), + ("Scope::Inner::zp_good", true), + ("Scope::Inner::bad", false), + ]; + + for (name, expect) in tests { + let string = String::from(name); + + assert_eq!(is_asan_friendly_name(&string), expect); + } + } +} diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 9da3a31..ab7312b 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -52,9 +52,15 @@ exit_code=$((exit_code + $?)) echo "test: custom => unused.nes" ./target/debug/nasm -c empty --asan -o tests/out/unused.nes tests/unused.s 2>tests/out/unused-warning.txt diff tests/out/unused-warning.txt tests/expected/unused-warning.txt +exit_code=$((exit_code + $?)) diff tests/out/unused.nes tests/expected/unused.nes exit_code=$((exit_code + $?)) +echo "test: custom => bare_accesses.nes" +./target/debug/nasm -c empty --asan tests/bare_accesses.s 2>tests/out/bare_accesses.txt +diff tests/out/bare_accesses.txt tests/expected/bare_accesses.txt +exit_code=$((exit_code + $?)) + ## # code.nes diff --git a/tests/bare_accesses.s b/tests/bare_accesses.s new file mode 100644 index 0000000..42a6409 --- /dev/null +++ b/tests/bare_accesses.s @@ -0,0 +1,37 @@ +.segment "HEADER" + .byte 'N', 'E', 'S', $1A + .byte $02 + .byte $01 + .byte $00 + .byte $00 + +.segment "CODE" + +.scope Scope + zp_valid = $02 + + .scope Inner + zp_valid_too = $03 + .endscope +.endscope + +zp_used = $00 ; asan:reserve $02 +whatever = $01 + +lda zp_used +lda $00 +lda whatever +lda Scope::zp_valid +lda Scope::Inner::zp_valid_too +lda zp_used + 1 +lda zp_used + 2 +lda zp_used - 1 + +@something: + beq @something + +ldx #0 +lda palettes, x + +palettes: + .byte $0F diff --git a/tests/expected/bare_accesses.txt b/tests/expected/bare_accesses.txt new file mode 100644 index 0000000..4c14629 --- /dev/null +++ b/tests/expected/bare_accesses.txt @@ -0,0 +1,4 @@ +warning: accessing a memory region without using a variable (bare_accesses.s: line 22) +warning: accessing a memory region without a proper name ('whatever') (bare_accesses.s: line 23) +error: out of bounds memory access for 'zp_used' ($00-$01) (bare_accesses.s: line 27) +error: out of bounds memory access for 'zp_used' ($00-$01) (bare_accesses.s: line 28) -- cgit v1.2.3