From a6995372d5674a37b5b7b8513fb008fa33bf2120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 16 Apr 2025 18:40:10 +0000 Subject: [PATCH 01/27] Patches SBPF dependency in Cargo.toml --- Cargo.lock | 5 +- Cargo.toml | 2 +- program-runtime/src/serialization.rs | 5 +- programs/bpf_loader/src/lib.rs | 4 +- programs/bpf_loader/src/syscalls/cpi.rs | 91 ++++++++-------- programs/bpf_loader/src/syscalls/mem_ops.rs | 114 ++++++++++---------- programs/sbf/Cargo.lock | 5 +- programs/sbf/Cargo.toml | 2 +- programs/sbf/benches/bpf_loader.rs | 15 +-- svm/examples/Cargo.lock | 5 +- transaction-context/Cargo.toml | 1 + transaction-context/src/lib.rs | 56 ++++++---- 12 files changed, 161 insertions(+), 144 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9805614930efd8..3adf119a46f4c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10269,9 +10269,9 @@ checksum = "61f1bc1357b8188d9c4a3af3fc55276e56987265eb7ad073ae6f8180ee54cecf" [[package]] name = "solana-sbpf" -version = "0.11.2" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "642335ab08889cd963790faeaa7824e320a5ada4b6eb93ebfc842fa03ddef425" +checksum = "3c7a3d3cff34df928b804917bf111d3ede779af406703580cd7ed8fb239f5acf" dependencies = [ "byteorder", "combine 3.8.1", @@ -11304,6 +11304,7 @@ dependencies = [ "solana-instructions-sysvar", "solana-pubkey", "solana-rent", + "solana-sbpf", "solana-sdk-ids", "solana-signature", "solana-system-interface", diff --git a/Cargo.toml b/Cargo.toml index 9af129df9d4a5c..600f2590edf0df 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -499,7 +499,7 @@ solana-rpc-client-types = { path = "rpc-client-types", version = "=3.0.0" } solana-runtime = { path = "runtime", version = "=3.0.0" } solana-runtime-transaction = { path = "runtime-transaction", version = "=3.0.0" } solana-sanitize = "2.2.1" -solana-sbpf = "=0.11.2" +solana-sbpf = "=0.12.0" solana-sdk-ids = "2.2.1" solana-secp256k1-program = "2.2.3" solana-secp256k1-recover = "2.2.1" diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index e890f3cad22ca4..b2f358c225a9bd 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -108,7 +108,8 @@ impl Serializer { MemoryRegion::new_readonly(account.get_data(), self.vaddr) }; if writable && shared { - new_region.cow_callback_payload = account.get_index_in_transaction() as u32; + new_region.access_violation_handler_payload = + Some(account.get_index_in_transaction()); } self.vaddr += new_region.len; self.regions.push(new_region); @@ -1328,7 +1329,7 @@ mod tests { let mut mem = AlignedMemory::zero_filled(len); for region in regions { let host_slice = unsafe { - slice::from_raw_parts(region.host_addr.get() as *const u8, region.len as usize) + slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) }; mem.as_slice_mut()[(region.vm_addr - MM_INPUT_START) as usize..][..region.len as usize] .copy_from_slice(host_slice) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index df4c9297ba1c1a..477c2a83295972 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -349,11 +349,11 @@ fn create_memory_mapping<'a, 'b, C: ContextObject>( .chain(additional_regions) .collect(); - Ok(MemoryMapping::new_with_cow( + Ok(MemoryMapping::new_with_access_violation_handler( regions, config, sbpf_version, - transaction_context.account_data_write_access_handler(), + transaction_context.access_violation_handler(), )?) } diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index e54a8b6a8849dd..3be10bd9941299 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -313,7 +313,7 @@ impl<'a> CallerAccount<'a> { fn realloc_region( &self, - memory_mapping: &'a MemoryMapping<'_>, + memory_mapping: &'a mut MemoryMapping<'_>, is_loader_deprecated: bool, ) -> Result, Error> { account_realloc_region( @@ -1212,26 +1212,21 @@ fn update_caller_account_perms( let mut new_region = if writable && !shared { MemoryRegion::new_writable( unsafe { - std::slice::from_raw_parts_mut( - region.host_addr.get() as *mut u8, - region.len as usize, - ) + std::slice::from_raw_parts_mut(region.host_addr as *mut u8, region.len as usize) }, region.vm_addr, ) } else { MemoryRegion::new_readonly( unsafe { - std::slice::from_raw_parts( - region.host_addr.get() as *const u8, - region.len as usize, - ) + std::slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) }, region.vm_addr, ) }; if writable && shared { - new_region.cow_callback_payload = callee_account.get_index_in_transaction() as u32; + new_region.access_violation_handler_payload = + Some(callee_account.get_index_in_transaction()); } memory_mapping.replace_region(region_index, new_region)?; } @@ -1245,20 +1240,14 @@ fn update_caller_account_perms( let new_region = if callee_account.can_data_be_changed().is_ok() { MemoryRegion::new_writable( unsafe { - std::slice::from_raw_parts_mut( - region.host_addr.get() as *mut u8, - region.len as usize, - ) + std::slice::from_raw_parts_mut(region.host_addr as *mut u8, region.len as usize) }, region.vm_addr, ) } else { MemoryRegion::new_readonly( unsafe { - std::slice::from_raw_parts( - region.host_addr.get() as *const u8, - region.len as usize, - ) + std::slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) }, region.vm_addr, ) @@ -1319,8 +1308,8 @@ fn update_caller_account( // because of BorrowedAccount::make_data_mut or by a program that uses the // AccountSharedData API directly (deprecated). let callee_ptr = callee_account.get_data().as_ptr() as u64; - if region.host_addr.get() != callee_ptr { - region.host_addr.set(callee_ptr); + if region.host_addr != callee_ptr { + region.host_addr = callee_ptr; zero_all_mapped_spare_capacity = true; } } @@ -1382,9 +1371,10 @@ fn update_caller_account( let (_realloc_region_index, realloc_region) = caller_account .realloc_region(memory_mapping, is_loader_deprecated)? .unwrap(); // unwrapping here is fine, we already asserted !is_loader_deprecated - let original_state = realloc_region.writable.replace(true); + let original_state = realloc_region.writable; + realloc_region.writable = true; defer! { - realloc_region.writable.set(original_state); + realloc_region.writable = original_state; }; // We need to zero the unused space in the realloc region, starting after the @@ -1491,9 +1481,10 @@ fn update_caller_account( let (_realloc_region_index, realloc_region) = caller_account .realloc_region(memory_mapping, is_loader_deprecated)? .unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated - let original_state = realloc_region.writable.replace(true); + let original_state = realloc_region.writable; + realloc_region.writable = true; defer! { - realloc_region.writable.set(original_state); + realloc_region.writable = original_state; }; translate_slice_mut::( @@ -1530,7 +1521,7 @@ fn update_caller_account( } fn account_data_region<'a>( - memory_mapping: &'a MemoryMapping<'_>, + memory_mapping: &'a mut MemoryMapping<'_>, vm_data_addr: u64, original_data_len: usize, ) -> Result, Error> { @@ -1540,14 +1531,16 @@ fn account_data_region<'a>( // We can trust vm_data_addr to point to the correct region because we // enforce that in CallerAccount::from_(sol_)account_info. - let (data_region_index, data_region) = memory_mapping.region(AccessType::Load, vm_data_addr)?; + let (data_region_index, data_region) = memory_mapping + .find_region(vm_data_addr) + .ok_or_else(|| Box::new(InstructionError::MissingAccount))?; // vm_data_addr must always point to the beginning of the region debug_assert_eq!(data_region.vm_addr, vm_data_addr); Ok(Some((data_region_index, data_region))) } fn account_realloc_region<'a>( - memory_mapping: &'a MemoryMapping<'_>, + memory_mapping: &'a mut MemoryMapping<'_>, vm_data_addr: u64, original_data_len: usize, is_loader_deprecated: bool, @@ -1557,13 +1550,14 @@ fn account_realloc_region<'a>( } let realloc_vm_addr = vm_data_addr.saturating_add(original_data_len as u64); - let (realloc_region_index, realloc_region) = - memory_mapping.region(AccessType::Load, realloc_vm_addr)?; + let (realloc_region_index, realloc_region) = memory_mapping + .find_region(realloc_vm_addr) + .ok_or_else(|| Box::new(InstructionError::MissingAccount))?; debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); debug_assert!((MAX_PERMITTED_DATA_INCREASE ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) .contains(&(realloc_region.len as usize))); - debug_assert_eq!(realloc_region.cow_callback_payload, u32::MAX); + debug_assert_eq!(realloc_region.access_violation_handler_payload, None); Ok(Some((realloc_region_index, realloc_region))) } @@ -2027,9 +2021,14 @@ mod tests { // check that the caller account data pointer always matches the callee account data pointer assert_eq!( - translate_slice::(&memory_mapping, caller_account.vm_data_addr, 1, true,) - .unwrap() - .as_ptr(), + translate_slice::( + &mut memory_mapping, + caller_account.vm_data_addr, + 1, + true, + ) + .unwrap() + .as_ptr(), callee_account.get_data().as_ptr() ); @@ -2040,7 +2039,7 @@ mod tests { assert_eq!(data_len, serialized_len()); let realloc_area = translate_slice::( - &memory_mapping, + &mut memory_mapping, caller_account .vm_data_addr .saturating_add(caller_account.original_data_len as u64), @@ -2192,7 +2191,7 @@ mod tests { assert_eq!(callee_account.get_data().len(), 3); assert!(callee_account.capacity() >= caller_account.original_data_len); let data = translate_slice::( - &memory_mapping, + &mut memory_mapping, caller_account.vm_data_addr, callee_account.get_data().len() as u64, true, @@ -2227,7 +2226,7 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( mock_caller_account.regions.split_off(0), &config, SBPFVersion::V3, @@ -2243,7 +2242,7 @@ mod tests { update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2283,7 +2282,7 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( mock_caller_account.regions.split_off(0), &config, SBPFVersion::V3, @@ -2299,7 +2298,7 @@ mod tests { update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2318,7 +2317,7 @@ mod tests { caller_account.owner = &mut owner; update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2356,7 +2355,7 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( mock_caller_account.regions.split_off(0), &config, SBPFVersion::V3, @@ -2371,7 +2370,7 @@ mod tests { assert_matches!( update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2389,7 +2388,7 @@ mod tests { assert_matches!( update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2407,7 +2406,7 @@ mod tests { assert_matches!( update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2479,7 +2478,7 @@ mod tests { *caller_account.ref_to_len_in_vm = len as u64; update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, @@ -2498,7 +2497,7 @@ mod tests { caller_account.owner = &mut owner; update_callee_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, false, &caller_account, callee_account, diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index dfc46ed0daf582..b321b872b9d88b 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -209,7 +209,7 @@ fn memmove_non_contiguous( src_addr: u64, n: u64, accounts: &[SerializedAccountMetadata], - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, resize_area: bool, ) -> Result { let reverse = dst_addr.wrapping_sub(src_addr) < n; @@ -248,7 +248,7 @@ fn memcmp_non_contiguous( dst_addr: u64, n: u64, accounts: &[SerializedAccountMetadata], - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, resize_area: bool, ) -> Result { let memcmp_chunk = |s1_addr, s2_addr, chunk_len| { @@ -312,7 +312,7 @@ fn memset_non_contiguous( c: u8, n: u64, accounts: &[SerializedAccountMetadata], - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, check_aligned: bool, ) -> Result { let dst_chunk_iter = MemoryChunkIterator::new( @@ -326,7 +326,7 @@ fn memset_non_contiguous( for item in dst_chunk_iter { let (dst_region, dst_vm_addr, dst_len) = item?; let dst_host_addr = dst_region - .vm_to_host(dst_vm_addr, dst_len as u64) + .vm_to_host(AccessType::Store, dst_vm_addr, dst_len as u64) .ok_or_else(|| { EbpfError::AccessViolation(AccessType::Store, dst_vm_addr, dst_len as u64, "") })?; @@ -344,7 +344,7 @@ fn iter_memory_pair_chunks( dst_addr: u64, n_bytes: u64, accounts: &[SerializedAccountMetadata], - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, reverse: bool, resize_area: bool, mut fun: F, @@ -419,19 +419,14 @@ where ( src_region - .vm_to_host(src_addr, chunk_len as u64) + .vm_to_host(src_access, src_addr, chunk_len as u64) .ok_or_else(|| { - EbpfError::AccessViolation(AccessType::Load, src_addr, chunk_len as u64, "") + EbpfError::AccessViolation(src_access, src_addr, chunk_len as u64, "") })?, dst_region - .vm_to_host(dst_addr, chunk_len as u64) + .vm_to_host(dst_access, dst_addr, chunk_len as u64) .ok_or_else(|| { - EbpfError::AccessViolation( - AccessType::Store, - dst_addr, - chunk_len as u64, - "", - ) + EbpfError::AccessViolation(dst_access, dst_addr, chunk_len as u64, "") })?, ) }; @@ -467,7 +462,7 @@ where } struct MemoryChunkIterator<'a> { - memory_mapping: &'a MemoryMapping<'a>, + memory_mapping: &'a mut MemoryMapping<'a>, accounts: &'a [SerializedAccountMetadata], access_type: AccessType, initial_vm_addr: u64, @@ -482,7 +477,7 @@ struct MemoryChunkIterator<'a> { impl<'a> MemoryChunkIterator<'a> { fn new( - memory_mapping: &'a MemoryMapping, + memory_mapping: &'a mut MemoryMapping, accounts: &'a [SerializedAccountMetadata], access_type: AccessType, vm_addr: u64, @@ -511,9 +506,11 @@ impl<'a> MemoryChunkIterator<'a> { } fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, Error> { - match self.memory_mapping.region(self.access_type, vm_addr) { - Ok((_region_index, region)) => Ok(region), - Err(error) => match error { + match self.memory_mapping.map(self.access_type, vm_addr, self.len) { + solana_sbpf::error::ProgramResult::Ok(_) => { + Ok(self.memory_mapping.find_region(vm_addr).unwrap().1) + } + solana_sbpf::error::ProgramResult::Err(error) => match error { EbpfError::AccessViolation(access_type, _vm_addr, _len, name) => Err(Box::new( EbpfError::AccessViolation(access_type, self.initial_vm_addr, self.len, name), )), @@ -583,11 +580,12 @@ impl<'a> Iterator for MemoryChunkIterator<'a> { } let vm_addr = self.vm_addr_start; + let region_vm_addr_end = region.vm_addr_range().end; - let chunk_len = if region.vm_addr_end <= self.vm_addr_end { + let chunk_len = if region_vm_addr_end <= self.vm_addr_end { // consume the whole region - let len = region.vm_addr_end.saturating_sub(self.vm_addr_start); - self.vm_addr_start = region.vm_addr_end; + let len = region_vm_addr_end.saturating_sub(self.vm_addr_start); + self.vm_addr_start = region_vm_addr_end; len } else { // consume part of the region @@ -692,10 +690,11 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); + let mut memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); let mut src_chunk_iter = - MemoryChunkIterator::new(&memory_mapping, &[], AccessType::Load, 0, 1, true).unwrap(); + MemoryChunkIterator::new(&mut memory_mapping, &[], AccessType::Load, 0, 1, true) + .unwrap(); src_chunk_iter.next().unwrap().unwrap(); } @@ -706,11 +705,17 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); + let mut memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); - let mut src_chunk_iter = - MemoryChunkIterator::new(&memory_mapping, &[], AccessType::Load, u64::MAX, 1, true) - .unwrap(); + let mut src_chunk_iter = MemoryChunkIterator::new( + &mut memory_mapping, + &[], + AccessType::Load, + u64::MAX, + 1, + true, + ) + .unwrap(); src_chunk_iter.next().unwrap().unwrap(); } @@ -721,7 +726,7 @@ mod tests { ..Config::default() }; let mem1 = vec![0xFF; 42]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![MemoryRegion::new_readonly(&mem1, MM_RODATA_START)], &config, SBPFVersion::V3, @@ -730,7 +735,7 @@ mod tests { // check oob at the lower bound on the first next() let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START - 1, @@ -746,7 +751,7 @@ mod tests { // check oob at the upper bound. Since the memory mapping isn't empty, // this always happens on the second next(). let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START, @@ -762,7 +767,7 @@ mod tests { // check oob at the upper bound on the first next_back() let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START, @@ -778,7 +783,7 @@ mod tests { // check oob at the upper bound on the 2nd next_back() let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START - 1, @@ -801,7 +806,7 @@ mod tests { ..Config::default() }; let mem1 = vec![0xFF; 42]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![MemoryRegion::new_readonly(&mem1, MM_RODATA_START)], &config, SBPFVersion::V3, @@ -810,7 +815,7 @@ mod tests { // check lower bound let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START - 1, @@ -822,7 +827,7 @@ mod tests { // check upper bound let mut src_chunk_iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, MM_RODATA_START + 42, @@ -841,7 +846,7 @@ mod tests { ] { for rev in [true, false] { let iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, vm_addr, @@ -871,7 +876,7 @@ mod tests { }; let mem1 = vec![0x11; 8]; let mem2 = vec![0x22; 4]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_readonly(&mem1, MM_RODATA_START), MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), @@ -892,7 +897,7 @@ mod tests { ] { for rev in [false, true] { let iter = MemoryChunkIterator::new( - &memory_mapping, + &mut memory_mapping, &[], AccessType::Load, vm_addr, @@ -920,7 +925,7 @@ mod tests { }; let mem1 = vec![0x11; 8]; let mem2 = vec![0x22; 4]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_readonly(&mem1, MM_RODATA_START), MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), @@ -939,7 +944,7 @@ mod tests { MM_RODATA_START + 8, 8, &[], - &memory_mapping, + &mut memory_mapping, false, true, |_src, _dst, _len| Ok::<_, Error>(0), @@ -956,7 +961,7 @@ mod tests { MM_RODATA_START + 2, 3, &[], - &memory_mapping, + &mut memory_mapping, false, true, |_src, _dst, _len| Ok::<_, Error>(0), @@ -974,7 +979,7 @@ mod tests { }; let mem1 = vec![0x11; 8]; let mem2 = vec![0x22; 4]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_readonly(&mem1, MM_RODATA_START), MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), @@ -989,7 +994,7 @@ mod tests { MM_RODATA_START + 8, 4, &[], - &memory_mapping, + &mut memory_mapping, true, ) .unwrap(); @@ -1040,7 +1045,7 @@ mod tests { MM_RODATA_START + src_offset as u64, len as u64, &[], - &memory_mapping, + &mut memory_mapping, true, ) .unwrap(); @@ -1061,7 +1066,7 @@ mod tests { }; let mut mem1 = vec![0x11; 8]; let mem2 = vec![0x22; 4]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_writable(&mut mem1, MM_RODATA_START), MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), @@ -1072,7 +1077,8 @@ mod tests { .unwrap(); assert_eq!( - memset_non_contiguous(MM_RODATA_START, 0x33, 9, &[], &memory_mapping, true).unwrap(), + memset_non_contiguous(MM_RODATA_START, 0x33, 9, &[], &mut memory_mapping, true) + .unwrap(), 0 ); } @@ -1087,7 +1093,7 @@ mod tests { let mut mem2 = vec![0x22; 2]; let mut mem3 = vec![0x33; 3]; let mut mem4 = vec![0x44; 4]; - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_readonly(&mem1, MM_RODATA_START), MemoryRegion::new_writable(&mut mem2, MM_RODATA_START + 1), @@ -1100,7 +1106,7 @@ mod tests { .unwrap(); assert_eq!( - memset_non_contiguous(MM_RODATA_START + 1, 0x55, 7, &[], &memory_mapping, true) + memset_non_contiguous(MM_RODATA_START + 1, 0x55, 7, &[], &mut memory_mapping, true) .unwrap(), 0 ); @@ -1119,7 +1125,7 @@ mod tests { let mem1 = b"foo".to_vec(); let mem2 = b"barbad".to_vec(); let mem3 = b"foobarbad".to_vec(); - let memory_mapping = MemoryMapping::new( + let mut memory_mapping = MemoryMapping::new( vec![ MemoryRegion::new_readonly(&mem1, MM_RODATA_START), MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 3), @@ -1137,7 +1143,7 @@ mod tests { MM_RODATA_START + 9, 9, &[], - &memory_mapping, + &mut memory_mapping, true ) .unwrap(), @@ -1151,7 +1157,7 @@ mod tests { MM_RODATA_START + 1, 8, &[], - &memory_mapping, + &mut memory_mapping, true ) .unwrap(), @@ -1165,7 +1171,7 @@ mod tests { MM_RODATA_START + 11, 5, &[], - &memory_mapping, + &mut memory_mapping, true ) .unwrap(), @@ -1193,7 +1199,7 @@ mod tests { offset += *region_len; } - let memory_mapping = MemoryMapping::new(regs, config, SBPFVersion::V3).unwrap(); + let mut memory_mapping = MemoryMapping::new(regs, config, SBPFVersion::V3).unwrap(); (mem, memory_mapping) } diff --git a/programs/sbf/Cargo.lock b/programs/sbf/Cargo.lock index 25eee996c4d888..0a7963cb30dddb 100644 --- a/programs/sbf/Cargo.lock +++ b/programs/sbf/Cargo.lock @@ -8690,9 +8690,9 @@ dependencies = [ [[package]] name = "solana-sbpf" -version = "0.11.2" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "642335ab08889cd963790faeaa7824e320a5ada4b6eb93ebfc842fa03ddef425" +checksum = "3c7a3d3cff34df928b804917bf111d3ede779af406703580cd7ed8fb239f5acf" dependencies = [ "byteorder 1.5.0", "combine 3.8.1", @@ -9481,6 +9481,7 @@ dependencies = [ "solana-instructions-sysvar", "solana-pubkey", "solana-rent", + "solana-sbpf", "solana-sdk-ids", ] diff --git a/programs/sbf/Cargo.toml b/programs/sbf/Cargo.toml index ba7109342610a6..5c9b6d27cdfe87 100644 --- a/programs/sbf/Cargo.toml +++ b/programs/sbf/Cargo.toml @@ -151,7 +151,7 @@ solana-sbf-rust-mem-dep = { path = "rust/mem_dep", version = "=3.0.0" } solana-sbf-rust-param-passing-dep = { path = "rust/param_passing_dep", version = "=3.0.0" } solana-sbf-rust-realloc-dep = { path = "rust/realloc_dep", version = "=3.0.0" } solana-sbf-rust-realloc-invoke-dep = { path = "rust/realloc_invoke_dep", version = "=3.0.0" } -solana-sbpf = "=0.11.2" +solana-sbpf = "=0.12.0" solana-sdk-ids = "=2.2.1" solana-secp256k1-recover = "=2.2.1" solana-sha256-hasher = { version = "=2.2.1", features = ["sha2"] } diff --git a/programs/sbf/benches/bpf_loader.rs b/programs/sbf/benches/bpf_loader.rs index 9089c16d434945..8b0a7a48237da9 100644 --- a/programs/sbf/benches/bpf_loader.rs +++ b/programs/sbf/benches/bpf_loader.rs @@ -332,24 +332,19 @@ fn clone_regions(regions: &[MemoryRegion]) -> Vec { regions .iter() .map(|region| { - let mut new_region = if region.writable.get() { + let mut new_region = if region.writable { MemoryRegion::new_writable( - slice::from_raw_parts_mut( - region.host_addr.get() as *mut _, - region.len as usize, - ), + slice::from_raw_parts_mut(region.host_addr as *mut _, region.len as usize), region.vm_addr, ) } else { MemoryRegion::new_readonly( - slice::from_raw_parts( - region.host_addr.get() as *const _, - region.len as usize, - ), + slice::from_raw_parts(region.host_addr as *const _, region.len as usize), region.vm_addr, ) }; - new_region.cow_callback_payload = region.cow_callback_payload; + new_region.access_violation_handler_payload = + region.access_violation_handler_payload; new_region }) .collect() diff --git a/svm/examples/Cargo.lock b/svm/examples/Cargo.lock index 5e15ae32107fdf..e6662636318ba4 100644 --- a/svm/examples/Cargo.lock +++ b/svm/examples/Cargo.lock @@ -7741,9 +7741,9 @@ checksum = "61f1bc1357b8188d9c4a3af3fc55276e56987265eb7ad073ae6f8180ee54cecf" [[package]] name = "solana-sbpf" -version = "0.11.2" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "642335ab08889cd963790faeaa7824e320a5ada4b6eb93ebfc842fa03ddef425" +checksum = "3c7a3d3cff34df928b804917bf111d3ede779af406703580cd7ed8fb239f5acf" dependencies = [ "byteorder", "combine 3.8.1", @@ -8567,6 +8567,7 @@ dependencies = [ "solana-instructions-sysvar", "solana-pubkey", "solana-rent", + "solana-sbpf", "solana-sdk-ids", ] diff --git a/transaction-context/Cargo.toml b/transaction-context/Cargo.toml index f88b7d447d9b4d..6b3abb6ba72cb8 100644 --- a/transaction-context/Cargo.toml +++ b/transaction-context/Cargo.toml @@ -26,6 +26,7 @@ solana-account = { workspace = true } solana-instruction = { workspace = true, features = ["std"] } solana-instructions-sysvar = { workspace = true } solana-pubkey = { workspace = true } +solana-sbpf = { workspace = true } [target.'cfg(not(target_os = "solana"))'.dependencies] bincode = { workspace = true, optional = true } diff --git a/transaction-context/src/lib.rs b/transaction-context/src/lib.rs index 5864d77fae8482..600ac67c15ea48 100644 --- a/transaction-context/src/lib.rs +++ b/transaction-context/src/lib.rs @@ -9,6 +9,7 @@ use { solana_instruction::error::InstructionError, solana_instructions_sysvar as instructions, solana_pubkey::Pubkey, + solana_sbpf::memory_region::{AccessType, AccessViolationHandler, MemoryRegion}, std::{ cell::{Ref, RefCell, RefMut}, collections::HashSet, @@ -484,29 +485,40 @@ impl TransactionContext { } /// Returns a new account data write access handler - pub fn account_data_write_access_handler(&self) -> Box Result> { + pub fn access_violation_handler(&self) -> AccessViolationHandler { let accounts = Rc::clone(&self.accounts); - Box::new(move |index_in_transaction| { - // The two calls below can't really fail. If they fail because of a bug, - // whatever is writing will trigger an EbpfError::AccessViolation like - // if the region was readonly, and the transaction will fail gracefully. - let mut account = accounts - .accounts - .get(index_in_transaction as usize) - .ok_or(())? - .try_borrow_mut() - .map_err(|_| ())?; - accounts - .touch(index_in_transaction as IndexOfAccount) - .map_err(|_| ())?; - - if account.is_shared() { - // See BorrowedAccount::make_data_mut() as to why we reserve extra - // MAX_PERMITTED_DATA_INCREASE bytes here. - account.reserve(MAX_PERMITTED_DATA_INCREASE); - } - Ok(account.data_as_mut_slice().as_mut_ptr() as u64) - }) + Box::new( + move |region: &mut MemoryRegion, + _address_space_reserved_for_account: u64, + _access_type: AccessType, + _vm_addr: u64, + _len: u64| { + let Some(index_in_transaction) = region.access_violation_handler_payload else { + return; + }; + + // The two calls below can't really fail. If they fail because of a bug, + // whatever is writing will trigger an EbpfError::AccessViolation like + // if the region was readonly, and the transaction will fail gracefully. + let Some(account) = accounts.accounts.get(index_in_transaction as usize) else { + return; + }; + let Ok(mut account) = account.try_borrow_mut() else { + return; + }; + if accounts.touch(index_in_transaction).is_err() { + return; + } + + if account.is_shared() { + // See BorrowedAccount::make_data_mut() as to why we reserve extra + // MAX_PERMITTED_DATA_INCREASE bytes here. + account.reserve(MAX_PERMITTED_DATA_INCREASE); + } + region.host_addr = account.data_as_mut_slice().as_mut_ptr() as u64; + region.writable = true; + }, + ) } } From aa2fed0e79bbc1c137a561dc4568bb3ba56720af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Sat, 12 Apr 2025 15:17:56 +0000 Subject: [PATCH 02/27] Splits off aligment padding from realloc region. --- program-runtime/src/serialization.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index b2f358c225a9bd..b07975fd59af8a 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -124,16 +124,18 @@ impl Serializer { self.fill_write(MAX_PERMITTED_DATA_INCREASE + align_offset, 0) .map_err(|_| InstructionError::InvalidArgument)?; } else { + // put the realloc padding in its own region + self.fill_write(MAX_PERMITTED_DATA_INCREASE, 0) + .map_err(|_| InstructionError::InvalidArgument)?; + self.push_region(account.can_data_be_changed().is_ok()); // The deserialization code is going to align the vm_addr to // BPF_ALIGN_OF_U128. Always add one BPF_ALIGN_OF_U128 worth of // padding and shift the start of the next region, so that once // vm_addr is aligned, the corresponding host_addr is aligned // too. - self.fill_write(MAX_PERMITTED_DATA_INCREASE + BPF_ALIGN_OF_U128, 0) + self.fill_write(BPF_ALIGN_OF_U128, 0) .map_err(|_| InstructionError::InvalidArgument)?; self.region_start += BPF_ALIGN_OF_U128.saturating_sub(align_offset); - // put the realloc padding in its own region - self.push_region(account.can_data_be_changed().is_ok()); } } From 8e27b71fc3b17a98237287b6c38741f4d2406d5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Sat, 12 Apr 2025 16:51:32 +0000 Subject: [PATCH 03/27] Only serialize MAX_PERMITTED_DATA_INCREASE when copy_account_data. --- program-runtime/src/serialization.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index b07975fd59af8a..4894b70b8fe4f2 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -125,8 +125,6 @@ impl Serializer { .map_err(|_| InstructionError::InvalidArgument)?; } else { // put the realloc padding in its own region - self.fill_write(MAX_PERMITTED_DATA_INCREASE, 0) - .map_err(|_| InstructionError::InvalidArgument)?; self.push_region(account.can_data_be_changed().is_ok()); // The deserialization code is going to align the vm_addr to // BPF_ALIGN_OF_U128. Always add one BPF_ALIGN_OF_U128 worth of @@ -444,10 +442,11 @@ fn serialize_parameters_aligned( + size_of::() // owner + size_of::() // lamports + size_of::() // data len - + MAX_PERMITTED_DATA_INCREASE + size_of::(); // rent epoch if copy_account_data { - size += data_len + (data_len as *const u8).align_offset(BPF_ALIGN_OF_U128); + size += data_len + + MAX_PERMITTED_DATA_INCREASE + + (data_len as *const u8).align_offset(BPF_ALIGN_OF_U128); } else { size += BPF_ALIGN_OF_U128; } @@ -568,6 +567,7 @@ fn deserialize_parameters_aligned>( _ => {} } start += pre_len; // data + start += MAX_PERMITTED_DATA_INCREASE; // realloc padding } else { // See Serializer::write_account() as to why we have this // padding before the realloc region here. @@ -594,7 +594,6 @@ fn deserialize_parameters_aligned>( _ => {} } } - start += MAX_PERMITTED_DATA_INCREASE; start += alignment_offset; start += size_of::(); // rent_epoch if borrowed_account.get_owner().to_bytes() != owner { From 6d92677d8446b1bd5f1f233057bd358cf01ea145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 8 May 2025 10:58:02 +0000 Subject: [PATCH 04/27] Separate data writeback, resize and address bump. --- program-runtime/src/serialization.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index 4894b70b8fe4f2..019ccf70916b32 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -555,23 +555,17 @@ fn deserialize_parameters_aligned>( { return Err(InstructionError::InvalidRealloc); } - // The redundant check helps to avoid the expensive data comparison if we can - let alignment_offset = (pre_len as *const u8).align_offset(BPF_ALIGN_OF_U128); if copy_account_data { let data = buffer .get(start..start + post_len) .ok_or(InstructionError::InvalidArgument)?; + // The redundant check helps to avoid the expensive data comparison if we can match borrowed_account.can_data_be_resized(post_len) { Ok(()) => borrowed_account.set_data_from_slice(data)?, Err(err) if borrowed_account.get_data() != data => return Err(err), _ => {} } - start += pre_len; // data - start += MAX_PERMITTED_DATA_INCREASE; // realloc padding } else { - // See Serializer::write_account() as to why we have this - // padding before the realloc region here. - start += BPF_ALIGN_OF_U128.saturating_sub(alignment_offset); let data = buffer .get(start..start + MAX_PERMITTED_DATA_INCREASE) .ok_or(InstructionError::InvalidArgument)?; @@ -594,7 +588,15 @@ fn deserialize_parameters_aligned>( _ => {} } } - start += alignment_offset; + start += if copy_account_data { + let alignment_offset = (pre_len as *const u8).align_offset(BPF_ALIGN_OF_U128); + pre_len // data + .saturating_add(MAX_PERMITTED_DATA_INCREASE) // realloc padding + .saturating_add(alignment_offset) + } else { + // See Serializer::write_account() as to why we have this + BPF_ALIGN_OF_U128 + }; start += size_of::(); // rent_epoch if borrowed_account.get_owner().to_bytes() != owner { // Change the owner at the end so that we are allowed to change the lamports and data before From 1ad2e5969bf812c64f23cf4fdb4b4e7a2402510e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 25 Apr 2025 10:05:56 +0000 Subject: [PATCH 05/27] Throws InstructionError::AccountDataTooSmall for load accesses and InstructionError::InvalidRealloc for write accesses beyond the current account length. --- programs/bpf_loader/src/lib.rs | 75 ++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 477c2a83295972..57159b8e3104bd 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1692,44 +1692,67 @@ fn execute<'a, 'b: 'a>( } if direct_mapping { - if let EbpfError::AccessViolation( - AccessType::Store, - address, - _size, - _section_name, - ) = error + if let EbpfError::SyscallError(err) = error { + error = err + .downcast::() + .map(|err| *err) + .unwrap_or_else(EbpfError::SyscallError); + } + if let EbpfError::AccessViolation(access_type, vm_addr, len, _section_name) = + error { // If direct_mapping is enabled and a program tries to write to a readonly // region we'll get a memory access violation. Map it to a more specific // error so it's easier for developers to see what happened. - if let Some((instruction_account_index, _)) = account_region_addrs - .iter() - .enumerate() - .find(|(_, vm_region)| vm_region.contains(&address)) + if let Some((instruction_account_index, vm_addr_range)) = + account_region_addrs + .iter() + .enumerate() + .find(|(_, vm_addr_range)| vm_addr_range.contains(&vm_addr)) { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; - let account = instruction_context.try_borrow_instruction_account( transaction_context, instruction_account_index as IndexOfAccount, )?; - - error = EbpfError::SyscallError(Box::new( - #[allow(deprecated)] - if !invoke_context - .get_feature_set() - .remove_accounts_executable_flag_checks - && account.is_executable() - { - InstructionError::ExecutableDataModified - } else if account.is_writable() { - InstructionError::ExternalAccountDataModified - } else { - InstructionError::ReadonlyDataModified - }, - )); + if vm_addr.saturating_add(len) <= vm_addr_range.end { + // The access was within the range of the accounts address space, + // but it might not be within the range of the actual data. + let is_access_outside_of_data = vm_addr + .saturating_add(len) + .saturating_sub(vm_addr_range.start) + as usize + > account.get_data().len(); + error = EbpfError::SyscallError(Box::new( + #[allow(deprecated)] + match access_type { + AccessType::Store => { + if let Err(err) = account.can_data_be_changed() { + err + } else { + // The store was allowed but failed, + // thus it must have been an attempt to grow the account. + debug_assert!(is_access_outside_of_data); + InstructionError::InvalidRealloc + } + } + AccessType::Load => { + // Loads should only fail when they are outside of the account data. + debug_assert!(is_access_outside_of_data); + if account.can_data_be_changed().is_err() { + // Load beyond readonly account data happened because the program + // expected more data than there actually is. + InstructionError::AccountDataTooSmall + } else { + // Load beyond writable account data also attempted to grow. + InstructionError::InvalidRealloc + } + } + }, + )); + } } } } From ffb73b200c6c6eb370536bcada7c7eb09ae950fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 16 Apr 2025 15:42:47 +0000 Subject: [PATCH 06/27] Removes the realloc padding region, leaving its address space unmapped. --- program-runtime/src/serialization.rs | 43 +++++++++------ programs/bpf_loader/src/syscalls/cpi.rs | 70 ++++++------------------- 2 files changed, 44 insertions(+), 69 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index 019ccf70916b32..1b59e8b7cc359a 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -22,6 +22,23 @@ use { /// SBF VM. const MAX_INSTRUCTION_ACCOUNTS: u8 = NON_DUP_MARKER; +/// Creates the account data direct mapping in serialization and CPI return +pub fn create_memory_region_of_account( + account: &mut BorrowedAccount<'_>, + vaddr: u64, +) -> Result { + let can_data_be_changed = account.can_data_be_changed().is_ok(); + let mut memory_region = if can_data_be_changed && !account.is_shared() { + MemoryRegion::new_writable(account.get_data_mut()?, vaddr) + } else { + MemoryRegion::new_readonly(account.get_data(), vaddr) + }; + if can_data_be_changed { + memory_region.access_violation_handler_payload = Some(account.get_index_in_transaction()); + } + Ok(memory_region) +} + #[allow(dead_code)] enum SerializeAccount<'a> { Account(IndexOfAccount, BorrowedAccount<'a>), @@ -99,19 +116,17 @@ impl Serializer { } else { self.push_region(true); let vaddr = self.vaddr; - if !account.get_data().is_empty() { - let writable = account.can_data_be_changed().is_ok(); - let shared = account.is_shared(); - let mut new_region = if writable && !shared { - MemoryRegion::new_writable(account.get_data_mut()?, self.vaddr) - } else { - MemoryRegion::new_readonly(account.get_data(), self.vaddr) - }; - if writable && shared { - new_region.access_violation_handler_payload = - Some(account.get_index_in_transaction()); - } - self.vaddr += new_region.len; + let address_space_reserved_for_account = if self.aligned { + account + .get_data() + .len() + .saturating_add(MAX_PERMITTED_DATA_INCREASE) + } else { + account.get_data().len() + }; + if address_space_reserved_for_account > 0 { + let new_region = create_memory_region_of_account(account, self.vaddr)?; + self.vaddr += address_space_reserved_for_account as u64; self.regions.push(new_region); } vaddr @@ -124,8 +139,6 @@ impl Serializer { self.fill_write(MAX_PERMITTED_DATA_INCREASE + align_offset, 0) .map_err(|_| InstructionError::InvalidArgument)?; } else { - // put the realloc padding in its own region - self.push_region(account.can_data_be_changed().is_ok()); // The deserialization code is going to align the vm_addr to // BPF_ALIGN_OF_U128. Always add one BPF_ALIGN_OF_U128 worth of // padding and shift the start of the next region, so that once diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 3be10bd9941299..6a6fac6979e83b 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -4,7 +4,9 @@ use { scopeguard::defer, solana_loader_v3_interface::instruction as bpf_loader_upgradeable, solana_measure::measure::Measure, - solana_program_runtime::invoke_context::SerializedAccountMetadata, + solana_program_runtime::{ + invoke_context::SerializedAccountMetadata, serialization::create_memory_region_of_account, + }, solana_sbpf::{ebpf, memory_region::MemoryRegion}, solana_stable_layout::stable_instruction::StableInstruction, solana_transaction_context::BorrowedAccount, @@ -1198,60 +1200,20 @@ fn update_caller_account_perms( callee_account: &mut BorrowedAccount<'_>, is_loader_deprecated: bool, ) -> Result<(), Error> { - let CallerAccount { - original_data_len, - vm_data_addr, - .. - } = caller_account; - - if let Some((region_index, region)) = - account_data_region(memory_mapping, *vm_data_addr, *original_data_len)? - { - let writable = callee_account.can_data_be_changed().is_ok(); - let shared = callee_account.is_shared(); - let mut new_region = if writable && !shared { - MemoryRegion::new_writable( - unsafe { - std::slice::from_raw_parts_mut(region.host_addr as *mut u8, region.len as usize) - }, - region.vm_addr, - ) - } else { - MemoryRegion::new_readonly( - unsafe { - std::slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) - }, - region.vm_addr, - ) - }; - if writable && shared { - new_region.access_violation_handler_payload = - Some(callee_account.get_index_in_transaction()); - } - memory_mapping.replace_region(region_index, new_region)?; - } + let address_space_reserved_for_account = if is_loader_deprecated { + caller_account.original_data_len + } else { + caller_account + .original_data_len + .saturating_add(MAX_PERMITTED_DATA_INCREASE) + }; - if let Some((region_index, region)) = account_realloc_region( + if let Some((region_index, region)) = account_data_region( memory_mapping, - *vm_data_addr, - *original_data_len, - is_loader_deprecated, + caller_account.vm_data_addr, + address_space_reserved_for_account, )? { - let new_region = if callee_account.can_data_be_changed().is_ok() { - MemoryRegion::new_writable( - unsafe { - std::slice::from_raw_parts_mut(region.host_addr as *mut u8, region.len as usize) - }, - region.vm_addr, - ) - } else { - MemoryRegion::new_readonly( - unsafe { - std::slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) - }, - region.vm_addr, - ) - }; + let new_region = create_memory_region_of_account(callee_account, region.vm_addr)?; memory_mapping.replace_region(region_index, new_region)?; } @@ -1523,9 +1485,9 @@ fn update_caller_account( fn account_data_region<'a>( memory_mapping: &'a mut MemoryMapping<'_>, vm_data_addr: u64, - original_data_len: usize, + address_space_reserved_for_account: usize, ) -> Result, Error> { - if original_data_len == 0 { + if address_space_reserved_for_account == 0 { return Ok(None); } From ef76677d3fdb47786949ee385d69912c32862d02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 11 Apr 2025 13:53:07 +0000 Subject: [PATCH 07/27] Resizes / reallocs account on demand in TransactionContext::access_violation_handler(). --- transaction-context/src/lib.rs | 60 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/transaction-context/src/lib.rs b/transaction-context/src/lib.rs index 600ac67c15ea48..59eb032c60a9d3 100644 --- a/transaction-context/src/lib.rs +++ b/transaction-context/src/lib.rs @@ -31,6 +31,9 @@ static_assertions::const_assert_eq!( #[cfg(not(target_os = "solana"))] const MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION: i64 = MAX_PERMITTED_DATA_LENGTH as i64 * 2; +// Note: With direct mapping programs can grow accounts faster than they intend to, +// because the AccessViolationHandler might grow an account up to +// MAX_PERMITTED_DATA_LENGTH at once. #[cfg(test)] static_assertions::const_assert_eq!( MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, @@ -489,32 +492,71 @@ impl TransactionContext { let accounts = Rc::clone(&self.accounts); Box::new( move |region: &mut MemoryRegion, - _address_space_reserved_for_account: u64, - _access_type: AccessType, - _vm_addr: u64, - _len: u64| { + address_space_reserved_for_account: u64, + access_type: AccessType, + vm_addr: u64, + len: u64| { + if access_type == AccessType::Load { + return; + } let Some(index_in_transaction) = region.access_violation_handler_payload else { + // This region is not a writable account. return; }; + let requested_length = + vm_addr.saturating_add(len).saturating_sub(region.vm_addr) as usize; + if requested_length > address_space_reserved_for_account as usize { + // Requested access goes further than the account region. + return; + } - // The two calls below can't really fail. If they fail because of a bug, + // The four calls below can't really fail. If they fail because of a bug, // whatever is writing will trigger an EbpfError::AccessViolation like // if the region was readonly, and the transaction will fail gracefully. let Some(account) = accounts.accounts.get(index_in_transaction as usize) else { + debug_assert!(false); return; }; let Ok(mut account) = account.try_borrow_mut() else { + debug_assert!(false); return; }; if accounts.touch(index_in_transaction).is_err() { + debug_assert!(false); return; } + let Ok(remaining_allowed_growth) = + accounts.resize_delta.try_borrow().map(|resize_delta| { + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION + .saturating_sub(*resize_delta) + .max(0) as usize + }) + else { + debug_assert!(false); + return; + }; - if account.is_shared() { - // See BorrowedAccount::make_data_mut() as to why we reserve extra - // MAX_PERMITTED_DATA_INCREASE bytes here. - account.reserve(MAX_PERMITTED_DATA_INCREASE); + if requested_length > region.len as usize { + // Realloc immediately here to fit the requested access, + // then later in CPI or deserialization realloc again to the + // account length the program stored in AccountInfo. + let old_len = account.data().len(); + let new_len = (address_space_reserved_for_account as usize) + .min(MAX_PERMITTED_DATA_LENGTH as usize) + .min(old_len.saturating_add(remaining_allowed_growth)); + // The last two min operations ensure the following: + debug_assert!(accounts.can_data_be_resized(old_len, new_len).is_ok()); + if accounts + .update_accounts_resize_delta(old_len, new_len) + .is_err() + { + return; + } + account.resize(new_len, 0); + region.len = new_len as u64; } + + // Potentially unshare / make the account shared data unique (CoW logic). region.host_addr = account.data_as_mut_slice().as_mut_ptr() as u64; region.writable = true; }, From 05e1a84598e7ff0a97ba20dd9bae2778e8758fc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 16 Apr 2025 15:50:37 +0000 Subject: [PATCH 08/27] Stops zeroing of spare capacity in update_caller_account(). --- programs/bpf_loader/src/syscalls/cpi.rs | 144 ++---------------------- 1 file changed, 7 insertions(+), 137 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 6a6fac6979e83b..925a10bdec29ab 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1239,44 +1239,6 @@ fn update_caller_account( *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - let mut zero_all_mapped_spare_capacity = false; - if direct_mapping { - if let Some((_region_index, region)) = account_data_region( - memory_mapping, - caller_account.vm_data_addr, - caller_account.original_data_len, - )? { - // Since each instruction account is directly mapped in a memory region with a *fixed* - // length, upon returning from CPI we must ensure that the current capacity is at least - // the original length (what is mapped in memory), so that the account's memory region - // never points to an invalid address. - // - // Note that the capacity can be smaller than the original length only if the account is - // reallocated using the AccountSharedData API directly (deprecated) or using - // BorrowedAccount::set_data_from_slice(), which implements an optimization to avoid an - // extra allocation. - let min_capacity = caller_account.original_data_len; - if callee_account.capacity() < min_capacity { - callee_account - .reserve(min_capacity.saturating_sub(callee_account.get_data().len()))?; - zero_all_mapped_spare_capacity = true; - } - - // If an account's data pointer has changed we must update the corresponding - // MemoryRegion in the caller's address space. Address spaces are fixed so we don't need - // to update the MemoryRegion's length. - // - // An account's data pointer can change if the account is reallocated because of CoW, - // because of BorrowedAccount::make_data_mut or by a program that uses the - // AccountSharedData API directly (deprecated). - let callee_ptr = callee_account.get_data().as_ptr() as u64; - if region.host_addr != callee_ptr { - region.host_addr = callee_ptr; - zero_all_mapped_spare_capacity = true; - } - } - } - let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); if prev_len != post_len { @@ -1297,75 +1259,19 @@ fn update_caller_account( return Err(Box::new(InstructionError::InvalidRealloc)); } - // If the account has been shrunk, we're going to zero the unused memory - // *that was previously used*. - if post_len < prev_len { - if direct_mapping { - // We have two separate regions to zero out: the account data - // and the realloc region. Here we zero the realloc region, the - // data region is zeroed further down below. - // - // This is done for compatibility but really only necessary for - // the fringe case of a program calling itself, see - // TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS. - // - // Zeroing the realloc region isn't necessary in the normal - // invoke case because consider the following scenario: - // - // 1. Caller grows an account (prev_len > original_data_len) - // 2. Caller assigns the account to the callee (needed for 3 to - // work) - // 3. Callee shrinks the account (post_len < prev_len) - // - // In order for the caller to assign the account to the callee, - // the caller _must_ either set the account length to zero, - // therefore making prev_len > original_data_len impossible, - // or it must zero the account data, therefore making the - // zeroing we do here redundant. - if prev_len > caller_account.original_data_len { - // If we get here and prev_len > original_data_len, then - // we've already returned InvalidRealloc for the - // bpf_loader_deprecated case. - debug_assert!(!is_loader_deprecated); - - // Temporarily configure the realloc region as writable then set it back to - // whatever state it had. - let (_realloc_region_index, realloc_region) = caller_account - .realloc_region(memory_mapping, is_loader_deprecated)? - .unwrap(); // unwrapping here is fine, we already asserted !is_loader_deprecated - let original_state = realloc_region.writable; - realloc_region.writable = true; - defer! { - realloc_region.writable = original_state; - }; - - // We need to zero the unused space in the realloc region, starting after the - // last byte of the new data which might be > original_data_len. - let dirty_realloc_start = caller_account.original_data_len.max(post_len); - // and we want to zero up to the old length - let dirty_realloc_len = prev_len.saturating_sub(dirty_realloc_start); - let serialized_data = translate_slice_mut::( - memory_mapping, - caller_account - .vm_data_addr - .saturating_add(dirty_realloc_start as u64), - dirty_realloc_len as u64, - invoke_context.get_check_aligned(), - )?; - serialized_data.fill(0); - } - } else { + // when direct mapping is enabled we don't cache the serialized data in + // caller_account.serialized_data. See CallerAccount::from_account_info. + if !direct_mapping { + // If the account has been shrunk, we're going to zero the unused memory + // *that was previously used*. + if post_len < prev_len { caller_account .serialized_data .get_mut(post_len..) .ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))? .fill(0); } - } - - // when direct mapping is enabled we don't cache the serialized data in - // caller_account.serialized_data. See CallerAccount::from_account_info. - if !direct_mapping { + // Set the length of caller_account.serialized_data to post_len. caller_account.serialized_data = translate_slice_mut::( memory_mapping, caller_account.vm_data_addr, @@ -1388,42 +1294,6 @@ fn update_caller_account( } if direct_mapping { - // Here we zero the account data region. - // - // If zero_all_mapped_spare_capacity=true, we need to zero regardless of whether the account - // size changed, because the underlying vector holding the account might have been - // reallocated and contain uninitialized memory in the spare capacity. - // - // See TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION for an example of - // this case. - let spare_len = if zero_all_mapped_spare_capacity { - // In the unlikely case where the account data vector has - // changed - which can happen during CoW - we zero the whole - // extra capacity up to the original data length. - // - // The extra capacity up to original data length is - // accessible from the vm and since it's uninitialized - // memory, it could be a source of non determinism. - caller_account.original_data_len - } else { - // If the allocation has not changed, we only zero the - // difference between the previous and current lengths. The - // rest of the memory contains whatever it contained before, - // which is deterministic. - prev_len - } - .saturating_sub(post_len); - - if spare_len > 0 { - let dst = callee_account - .spare_data_capacity_mut()? - .get_mut(..spare_len) - .ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))? - .as_mut_ptr(); - // Safety: we check bounds above - unsafe { ptr::write_bytes(dst, 0, spare_len) }; - } - // Propagate changes to the realloc region in the callee up to the caller. let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if realloc_bytes_used > 0 { From 842c64f1747661fee800fde02e3526d6a900a298 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 11 Apr 2025 16:07:49 +0000 Subject: [PATCH 09/27] Stops moving the contents of the realloc region around. --- program-runtime/src/serialization.rs | 24 +-------- programs/bpf_loader/src/syscalls/cpi.rs | 68 ++----------------------- 2 files changed, 7 insertions(+), 85 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index 1b59e8b7cc359a..1944a8aab7c59e 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -578,28 +578,8 @@ fn deserialize_parameters_aligned>( Err(err) if borrowed_account.get_data() != data => return Err(err), _ => {} } - } else { - let data = buffer - .get(start..start + MAX_PERMITTED_DATA_INCREASE) - .ok_or(InstructionError::InvalidArgument)?; - match borrowed_account.can_data_be_resized(post_len) { - Ok(()) => { - borrowed_account.set_data_length(post_len)?; - let allocated_bytes = post_len.saturating_sub(pre_len); - if allocated_bytes > 0 { - borrowed_account - .get_data_mut()? - .get_mut(pre_len..pre_len.saturating_add(allocated_bytes)) - .ok_or(InstructionError::InvalidArgument)? - .copy_from_slice( - data.get(0..allocated_bytes) - .ok_or(InstructionError::InvalidArgument)?, - ); - } - } - Err(err) if borrowed_account.get_data().len() != post_len => return Err(err), - _ => {} - } + } else if borrowed_account.get_data().len() != post_len { + borrowed_account.set_data_length(post_len)?; } start += if copy_account_data { let alignment_offset = (pre_len as *const u8).align_offset(BPF_ALIGN_OF_U128); diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 925a10bdec29ab..bcace940dbae06 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1124,8 +1124,8 @@ fn cpi_common( // When true is returned, the caller account must be updated after CPI. This // is only set for direct mapping when the pointer may have changed. fn update_callee_account( - invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, + _invoke_context: &InvokeContext, + _memory_mapping: &MemoryMapping, is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, @@ -1152,21 +1152,6 @@ fn update_callee_account( // pointer to data may have changed, so caller must be updated must_update_caller = true; } - if realloc_bytes_used > 0 { - let serialized_data = translate_slice::( - memory_mapping, - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64), - realloc_bytes_used as u64, - invoke_context.get_check_aligned(), - )?; - callee_account - .get_data_mut()? - .get_mut(caller_account.original_data_len..post_len) - .ok_or(SyscallError::InvalidLength)? - .copy_from_slice(serialized_data); - } } Err(err) if prev_len != post_len => { return Err(Box::new(err)); @@ -1231,7 +1216,7 @@ fn update_caller_account_perms( fn update_caller_account( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping<'_>, - is_loader_deprecated: bool, + _is_loader_deprecated: bool, caller_account: &mut CallerAccount<'_>, callee_account: &mut BorrowedAccount<'_>, direct_mapping: bool, @@ -1293,51 +1278,8 @@ fn update_caller_account( *serialized_len_ptr = post_len as u64; } - if direct_mapping { - // Propagate changes to the realloc region in the callee up to the caller. - let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); - if realloc_bytes_used > 0 { - // In the is_loader_deprecated case, we must have failed with - // InvalidRealloc by now. - debug_assert!(!is_loader_deprecated); - - let to_slice = { - // If a callee reallocs an account, we write into the caller's - // realloc region regardless of whether the caller has write - // permissions to the account or not. If the callee has been able to - // make changes, it means they had permissions to do so, and here - // we're just going to reflect those changes to the caller's frame. - // - // Therefore we temporarily configure the realloc region as writable - // then set it back to whatever state it had. - let (_realloc_region_index, realloc_region) = caller_account - .realloc_region(memory_mapping, is_loader_deprecated)? - .unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated - let original_state = realloc_region.writable; - realloc_region.writable = true; - defer! { - realloc_region.writable = original_state; - }; - - translate_slice_mut::( - memory_mapping, - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64), - realloc_bytes_used as u64, - invoke_context.get_check_aligned(), - )? - }; - let from_slice = callee_account - .get_data() - .get(caller_account.original_data_len..post_len) - .ok_or(SyscallError::InvalidLength)?; - if to_slice.len() != from_slice.len() { - return Err(Box::new(InstructionError::AccountDataTooSmall)); - } - to_slice.copy_from_slice(from_slice); - } - } else { + if !direct_mapping { + // Propagate changes in the callee up to the caller. let to_slice = &mut caller_account.serialized_data; let from_slice = callee_account .get_data() From 5b9ddc98fe2efe8caa95d75df42c39a3c66b3cb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 13:45:08 +0000 Subject: [PATCH 10/27] Renames update_caller_account_perms() => update_caller_account_region(). --- programs/bpf_loader/src/syscalls/cpi.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index bcace940dbae06..ba0d64abfbfbdf 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1083,7 +1083,7 @@ fn cpi_common( if let Some(caller_account) = caller_account { let mut callee_account = instruction_context .try_borrow_instruction_account(transaction_context, *index_in_caller)?; - update_caller_account_perms( + update_caller_account_region( memory_mapping, caller_account, &mut callee_account, @@ -1179,7 +1179,7 @@ fn update_callee_account( Ok(must_update_caller) } -fn update_caller_account_perms( +fn update_caller_account_region( memory_mapping: &mut MemoryMapping, caller_account: &CallerAccount, callee_account: &mut BorrowedAccount<'_>, From 84863a2b4314c3296b970daa0f52f90009a57399 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 13:49:04 +0000 Subject: [PATCH 11/27] Splits TranslatedAccount update_caller_account_region and update_caller_account_info. --- programs/bpf_loader/src/syscalls/cpi.rs | 54 ++++++++++++++----------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index ba0d64abfbfbdf..13e9f7e39b0bc7 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -327,7 +327,12 @@ impl<'a> CallerAccount<'a> { } } -type TranslatedAccounts<'a> = Vec<(IndexOfAccount, Option>)>; +struct TranslatedAccount<'a> { + index_in_caller: IndexOfAccount, + caller_account: CallerAccount<'a>, + update_caller_account_region: bool, + update_caller_account_info: bool, +} /// Implemented by language specific data structure translators trait SyscallInvokeSigned { @@ -343,7 +348,7 @@ trait SyscallInvokeSigned { is_loader_deprecated: bool, memory_mapping: &MemoryMapping<'_>, invoke_context: &mut InvokeContext, - ) -> Result, Error>; + ) -> Result>, Error>; fn translate_signers( program_id: &Pubkey, signers_seeds_addr: u64, @@ -442,7 +447,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { is_loader_deprecated: bool, memory_mapping: &MemoryMapping<'_>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result>, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -664,7 +669,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { is_loader_deprecated: bool, memory_mapping: &MemoryMapping<'_>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result>, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -792,7 +797,7 @@ fn translate_and_update_accounts<'a, T, F>( invoke_context: &mut InvokeContext, memory_mapping: &MemoryMapping<'_>, do_translate: F, -) -> Result, Error> +) -> Result>, Error> where F: Fn( &InvokeContext, @@ -883,13 +888,12 @@ where direct_mapping, )?; - let caller_account = - if instruction_account.is_writable || (direct_mapping && update_caller) { - Some(caller_account) - } else { - None - }; - accounts.push((instruction_account.index_in_caller, caller_account)); + accounts.push(TranslatedAccount { + index_in_caller: instruction_account.index_in_caller, + caller_account, + update_caller_account_region: instruction_account.is_writable || update_caller, + update_caller_account_info: instruction_account.is_writable, + }); } else { ic_msg!( invoke_context, @@ -1079,13 +1083,15 @@ fn cpi_common( // isn't strictly required as we forbid updates to an account to touch // other accounts, but since we did have bugs around this in the past, // it's better to be safe than sorry. - for (index_in_caller, caller_account) in accounts.iter() { - if let Some(caller_account) = caller_account { - let mut callee_account = instruction_context - .try_borrow_instruction_account(transaction_context, *index_in_caller)?; + for translate_account in accounts.iter() { + let mut callee_account = instruction_context.try_borrow_instruction_account( + transaction_context, + translate_account.index_in_caller, + )?; + if translate_account.update_caller_account_region { update_caller_account_region( memory_mapping, - caller_account, + &translate_account.caller_account, &mut callee_account, is_loader_deprecated, )?; @@ -1093,15 +1099,17 @@ fn cpi_common( } } - for (index_in_caller, caller_account) in accounts.iter_mut() { - if let Some(caller_account) = caller_account { - let mut callee_account = instruction_context - .try_borrow_instruction_account(transaction_context, *index_in_caller)?; + for translate_account in accounts.iter_mut() { + let mut callee_account = instruction_context.try_borrow_instruction_account( + transaction_context, + translate_account.index_in_caller, + )?; + if translate_account.update_caller_account_info { update_caller_account( invoke_context, memory_mapping, is_loader_deprecated, - caller_account, + &mut translate_account.caller_account, &mut callee_account, direct_mapping, )?; @@ -2336,7 +2344,7 @@ mod tests { ) .unwrap(); assert_eq!(accounts.len(), 1); - let caller_account = accounts[0].1.as_ref().unwrap(); + let caller_account = &accounts[0].caller_account; assert_eq!(caller_account.serialized_data, account.data()); assert_eq!(caller_account.original_data_len, original_data_len); } From 1c518a48f4cae49e59c89ce34191cfd0874fa3d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 8 May 2025 18:23:05 +0000 Subject: [PATCH 12/27] Reformulates realloc constraint in terms of address_space_reserved_for_account. --- programs/bpf_loader/src/syscalls/cpi.rs | 46 ++++++++++++------------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 13e9f7e39b0bc7..08d382d4d72be3 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1148,23 +1148,20 @@ fn update_callee_account( if direct_mapping { let prev_len = callee_account.get_data().len(); let post_len = *caller_account.ref_to_len_in_vm as usize; - match callee_account.can_data_be_resized(post_len) { - Ok(()) => { - let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); - // bpf_loader_deprecated programs don't have a realloc region - if is_loader_deprecated && realloc_bytes_used > 0 { - return Err(InstructionError::InvalidRealloc.into()); - } - if prev_len != post_len { - callee_account.set_data_length(post_len)?; - // pointer to data may have changed, so caller must be updated - must_update_caller = true; - } - } - Err(err) if prev_len != post_len => { - return Err(Box::new(err)); + if prev_len != post_len { + let address_space_reserved_for_account = if is_loader_deprecated { + caller_account.original_data_len + } else { + caller_account + .original_data_len + .saturating_add(MAX_PERMITTED_DATA_INCREASE) + }; + if post_len > address_space_reserved_for_account { + return Err(InstructionError::InvalidRealloc.into()); } - _ => {} + callee_account.set_data_length(post_len)?; + // pointer to data may have changed, so caller must be updated + must_update_caller = true; } } else { // The redundant check helps to avoid the expensive data comparison if we can @@ -1235,16 +1232,17 @@ fn update_caller_account( let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); if prev_len != post_len { - let max_increase = if direct_mapping && !invoke_context.get_check_aligned() { - 0 + let is_loader_deprecated = !invoke_context.get_check_aligned(); + let address_space_reserved_for_account = if direct_mapping && is_loader_deprecated { + caller_account.original_data_len } else { - MAX_PERMITTED_DATA_INCREASE - }; - let data_overflow = post_len - > caller_account + caller_account .original_data_len - .saturating_add(max_increase); - if data_overflow { + .saturating_add(MAX_PERMITTED_DATA_INCREASE) + }; + if post_len > address_space_reserved_for_account { + let max_increase = + address_space_reserved_for_account.saturating_sub(caller_account.original_data_len); ic_msg!( invoke_context, "Account data size realloc limited to {max_increase} in inner instructions", From a25cc85ecfe217f3292adba32bbb595560efd8ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 25 Apr 2025 14:01:58 +0000 Subject: [PATCH 13/27] Removes noncontiguous versions of memops. --- programs/bpf_loader/src/syscalls/mem_ops.rs | 725 ++------------------ 1 file changed, 46 insertions(+), 679 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index b321b872b9d88b..579278ad511fbd 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -84,47 +84,33 @@ declare_builtin_function!( ) -> Result { mem_op_consume(invoke_context, n)?; - if invoke_context - .get_feature_set() - .bpf_account_data_direct_mapping - { - translate_mut!( - memory_mapping, - invoke_context.get_check_aligned(), - let cmp_result_ref_mut: &mut i32 = map(cmp_result_addr)?; - ); - let syscall_context = invoke_context.get_syscall_context()?; + let s1 = translate_slice::( + memory_mapping, + s1_addr, + n, + invoke_context.get_check_aligned(), + )?; + let s2 = translate_slice::( + memory_mapping, + s2_addr, + n, + invoke_context.get_check_aligned(), + )?; - *cmp_result_ref_mut = memcmp_non_contiguous(s1_addr, s2_addr, n, &syscall_context.accounts_metadata, memory_mapping, invoke_context.get_check_aligned())?; - } else { - let s1 = translate_slice::( - memory_mapping, - s1_addr, - n, - invoke_context.get_check_aligned(), - )?; - let s2 = translate_slice::( - memory_mapping, - s2_addr, - n, - invoke_context.get_check_aligned(), - )?; - - debug_assert_eq!(s1.len(), n as usize); - debug_assert_eq!(s2.len(), n as usize); - // Safety: - // memcmp is marked unsafe since it assumes that the inputs are at least - // `n` bytes long. `s1` and `s2` are guaranteed to be exactly `n` bytes - // long because `translate_slice` would have failed otherwise. - let result = unsafe { memcmp(s1, s2, n as usize) }; - - translate_mut!( - memory_mapping, - invoke_context.get_check_aligned(), - let cmp_result_ref_mut: &mut i32 = map(cmp_result_addr)?; - ); - *cmp_result_ref_mut = result; - } + debug_assert_eq!(s1.len(), n as usize); + debug_assert_eq!(s2.len(), n as usize); + // Safety: + // memcmp is marked unsafe since it assumes that the inputs are at least + // `n` bytes long. `s1` and `s2` are guaranteed to be exactly `n` bytes + // long because `translate_slice` would have failed otherwise. + let result = unsafe { memcmp(s1, s2, n as usize) }; + + translate_mut!( + memory_mapping, + invoke_context.get_check_aligned(), + let cmp_result_ref_mut: &mut i32 = map(cmp_result_addr)?; + ); + *cmp_result_ref_mut = result; Ok(0) } @@ -144,90 +130,39 @@ declare_builtin_function!( ) -> Result { mem_op_consume(invoke_context, n)?; - if invoke_context - .get_feature_set() - .bpf_account_data_direct_mapping - { - let syscall_context = invoke_context.get_syscall_context()?; - - memset_non_contiguous(dst_addr, c as u8, n, &syscall_context.accounts_metadata, memory_mapping, invoke_context.get_check_aligned()) - } else { - translate_mut!( - memory_mapping, - invoke_context.get_check_aligned(), - let s: &mut [u8] = map(dst_addr, n)?; - ); - s.fill(c as u8); - Ok(0) - } - } -); - -fn memmove( - invoke_context: &mut InvokeContext, - dst_addr: u64, - src_addr: u64, - n: u64, - memory_mapping: &MemoryMapping, -) -> Result { - if invoke_context - .get_feature_set() - .bpf_account_data_direct_mapping - { - let syscall_context = invoke_context.get_syscall_context()?; - - memmove_non_contiguous( - dst_addr, - src_addr, - n, - &syscall_context.accounts_metadata, - memory_mapping, - invoke_context.get_check_aligned(), - ) - } else { translate_mut!( memory_mapping, invoke_context.get_check_aligned(), - let dst_ref_mut: &mut [u8] = map(dst_addr, n)?; + let s: &mut [u8] = map(dst_addr, n)?; ); - let dst_ptr = dst_ref_mut.as_mut_ptr(); - let src_ptr = translate_slice::( - memory_mapping, - src_addr, - n, - invoke_context.get_check_aligned(), - )? - .as_ptr(); - - unsafe { std::ptr::copy(src_ptr, dst_ptr, n as usize) }; + s.fill(c as u8); Ok(0) } -} +); -fn memmove_non_contiguous( +fn memmove( + invoke_context: &mut InvokeContext, dst_addr: u64, src_addr: u64, n: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &mut MemoryMapping, - resize_area: bool, + memory_mapping: &MemoryMapping, ) -> Result { - let reverse = dst_addr.wrapping_sub(src_addr) < n; - iter_memory_pair_chunks( - AccessType::Load, + translate_mut!( + memory_mapping, + invoke_context.get_check_aligned(), + let dst_ref_mut: &mut [u8] = map(dst_addr, n)?; + ); + let dst_ptr = dst_ref_mut.as_mut_ptr(); + let src_ptr = translate_slice::( + memory_mapping, src_addr, - AccessType::Store, - dst_addr, n, - accounts, - memory_mapping, - reverse, - resize_area, - |src_host_addr, dst_host_addr, chunk_len| { - unsafe { std::ptr::copy(src_host_addr, dst_host_addr as *mut u8, chunk_len) }; - Ok(0) - }, - ) + invoke_context.get_check_aligned(), + )? + .as_ptr(); + + unsafe { std::ptr::copy(src_ptr, dst_ptr, n as usize) }; + Ok(0) } // Marked unsafe since it assumes that the slices are at least `n` bytes long. @@ -243,49 +178,6 @@ unsafe fn memcmp(s1: &[u8], s2: &[u8], n: usize) -> i32 { 0 } -fn memcmp_non_contiguous( - src_addr: u64, - dst_addr: u64, - n: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &mut MemoryMapping, - resize_area: bool, -) -> Result { - let memcmp_chunk = |s1_addr, s2_addr, chunk_len| { - let res = unsafe { - let s1 = slice::from_raw_parts(s1_addr, chunk_len); - let s2 = slice::from_raw_parts(s2_addr, chunk_len); - // Safety: - // memcmp is marked unsafe since it assumes that s1 and s2 are exactly chunk_len - // long. The whole point of iter_memory_pair_chunks is to find same length chunks - // across two memory regions. - memcmp(s1, s2, chunk_len) - }; - if res != 0 { - return Err(MemcmpError::Diff(res).into()); - } - Ok(0) - }; - match iter_memory_pair_chunks( - AccessType::Load, - src_addr, - AccessType::Load, - dst_addr, - n, - accounts, - memory_mapping, - false, - resize_area, - memcmp_chunk, - ) { - Ok(res) => Ok(res), - Err(error) => match error.downcast_ref() { - Some(MemcmpError::Diff(diff)) => Ok(*diff), - _ => Err(error), - }, - } -} - #[derive(Debug)] enum MemcmpError { Diff(i32), @@ -307,35 +199,6 @@ impl std::error::Error for MemcmpError { } } -fn memset_non_contiguous( - dst_addr: u64, - c: u8, - n: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &mut MemoryMapping, - check_aligned: bool, -) -> Result { - let dst_chunk_iter = MemoryChunkIterator::new( - memory_mapping, - accounts, - AccessType::Store, - dst_addr, - n, - check_aligned, - )?; - for item in dst_chunk_iter { - let (dst_region, dst_vm_addr, dst_len) = item?; - let dst_host_addr = dst_region - .vm_to_host(AccessType::Store, dst_vm_addr, dst_len as u64) - .ok_or_else(|| { - EbpfError::AccessViolation(AccessType::Store, dst_vm_addr, dst_len as u64, "") - })?; - unsafe { slice::from_raw_parts_mut(dst_host_addr as *mut u8, dst_len).fill(c) } - } - - Ok(0) -} - #[allow(clippy::too_many_arguments)] fn iter_memory_pair_chunks( src_access: AccessType, @@ -683,502 +546,6 @@ mod tests { .collect::>() } - #[test] - #[should_panic(expected = "AccessViolation")] - fn test_memory_chunk_iterator_no_regions() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); - - let mut src_chunk_iter = - MemoryChunkIterator::new(&mut memory_mapping, &[], AccessType::Load, 0, 1, true) - .unwrap(); - src_chunk_iter.next().unwrap().unwrap(); - } - - #[test] - #[should_panic(expected = "AccessViolation")] - fn test_memory_chunk_iterator_new_out_of_bounds_upper() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); - - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - u64::MAX, - 1, - true, - ) - .unwrap(); - src_chunk_iter.next().unwrap().unwrap(); - } - - #[test] - fn test_memory_chunk_iterator_out_of_bounds() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0xFF; 42]; - let mut memory_mapping = MemoryMapping::new( - vec![MemoryRegion::new_readonly(&mem1, MM_RODATA_START)], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - // check oob at the lower bound on the first next() - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START - 1, - 42, - true, - ) - .unwrap(); - assert_matches!( - src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 42, "unknown") if *addr == MM_RODATA_START - 1 - ); - - // check oob at the upper bound. Since the memory mapping isn't empty, - // this always happens on the second next(). - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START, - 43, - true, - ) - .unwrap(); - assert!(src_chunk_iter.next().unwrap().is_ok()); - assert_matches!( - src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 43, "program") if *addr == MM_RODATA_START - ); - - // check oob at the upper bound on the first next_back() - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START, - 43, - true, - ) - .unwrap() - .rev(); - assert_matches!( - src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 43, "program") if *addr == MM_RODATA_START - ); - - // check oob at the upper bound on the 2nd next_back() - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START - 1, - 43, - true, - ) - .unwrap() - .rev(); - assert!(src_chunk_iter.next().unwrap().is_ok()); - assert_matches!( - src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 43, "unknown") if *addr == MM_RODATA_START - 1 - ); - } - - #[test] - fn test_memory_chunk_iterator_one() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0xFF; 42]; - let mut memory_mapping = MemoryMapping::new( - vec![MemoryRegion::new_readonly(&mem1, MM_RODATA_START)], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - // check lower bound - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START - 1, - 1, - true, - ) - .unwrap(); - assert!(src_chunk_iter.next().unwrap().is_err()); - - // check upper bound - let mut src_chunk_iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - MM_RODATA_START + 42, - 1, - true, - ) - .unwrap(); - assert!(src_chunk_iter.next().unwrap().is_err()); - - for (vm_addr, len) in [ - (MM_RODATA_START, 0), - (MM_RODATA_START + 42, 0), - (MM_RODATA_START, 1), - (MM_RODATA_START, 42), - (MM_RODATA_START + 41, 1), - ] { - for rev in [true, false] { - let iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - vm_addr, - len, - true, - ) - .unwrap(); - let res = if rev { - to_chunk_vec(iter.rev()) - } else { - to_chunk_vec(iter) - }; - if len == 0 { - assert_eq!(res, &[]); - } else { - assert_eq!(res, &[(vm_addr, len as usize)]); - } - } - } - } - - #[test] - fn test_memory_chunk_iterator_two() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0x11; 8]; - let mem2 = vec![0x22; 4]; - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_readonly(&mem1, MM_RODATA_START), - MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - for (vm_addr, len, mut expected) in [ - (MM_RODATA_START, 8, vec![(MM_RODATA_START, 8)]), - ( - MM_RODATA_START + 7, - 2, - vec![(MM_RODATA_START + 7, 1), (MM_RODATA_START + 8, 1)], - ), - (MM_RODATA_START + 8, 4, vec![(MM_RODATA_START + 8, 4)]), - ] { - for rev in [false, true] { - let iter = MemoryChunkIterator::new( - &mut memory_mapping, - &[], - AccessType::Load, - vm_addr, - len, - true, - ) - .unwrap(); - let res = if rev { - expected.reverse(); - to_chunk_vec(iter.rev()) - } else { - to_chunk_vec(iter) - }; - - assert_eq!(res, expected); - } - } - } - - #[test] - fn test_iter_memory_pair_chunks_short() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0x11; 8]; - let mem2 = vec![0x22; 4]; - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_readonly(&mem1, MM_RODATA_START), - MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - // dst is shorter than src - assert_matches!( - iter_memory_pair_chunks( - AccessType::Load, - MM_RODATA_START, - AccessType::Load, - MM_RODATA_START + 8, - 8, - &[], - &mut memory_mapping, - false, - true, - |_src, _dst, _len| Ok::<_, Error>(0), - ).unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 8, "program") if *addr == MM_RODATA_START + 8 - ); - - // src is shorter than dst - assert_matches!( - iter_memory_pair_chunks( - AccessType::Load, - MM_RODATA_START + 10, - AccessType::Load, - MM_RODATA_START + 2, - 3, - &[], - &mut memory_mapping, - false, - true, - |_src, _dst, _len| Ok::<_, Error>(0), - ).unwrap_err().downcast_ref().unwrap(), - EbpfError::AccessViolation(AccessType::Load, addr, 3, "program") if *addr == MM_RODATA_START + 10 - ); - } - - #[test] - #[should_panic(expected = "AccessViolation(Store, 4294967296, 4")] - fn test_memmove_non_contiguous_readonly() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0x11; 8]; - let mem2 = vec![0x22; 4]; - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_readonly(&mem1, MM_RODATA_START), - MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - memmove_non_contiguous( - MM_RODATA_START, - MM_RODATA_START + 8, - 4, - &[], - &mut memory_mapping, - true, - ) - .unwrap(); - } - - #[test_case(&[], (0, 0, 0); "no regions")] - #[test_case(&[10], (1, 10, 0); "single region 0 len")] - #[test_case(&[10], (0, 5, 5); "single region no overlap")] - #[test_case(&[10], (0, 0, 10) ; "single region complete overlap")] - #[test_case(&[10], (2, 0, 5); "single region partial overlap start")] - #[test_case(&[10], (0, 1, 6); "single region partial overlap middle")] - #[test_case(&[10], (2, 5, 5); "single region partial overlap end")] - #[test_case(&[3, 5], (0, 5, 2) ; "two regions no overlap, single source region")] - #[test_case(&[4, 7], (0, 5, 5) ; "two regions no overlap, multiple source regions")] - #[test_case(&[3, 8], (0, 0, 11) ; "two regions complete overlap")] - #[test_case(&[2, 9], (3, 0, 5) ; "two regions partial overlap start")] - #[test_case(&[3, 9], (1, 2, 5) ; "two regions partial overlap middle")] - #[test_case(&[7, 3], (2, 6, 4) ; "two regions partial overlap end")] - #[test_case(&[2, 6, 3, 4], (0, 10, 2) ; "many regions no overlap, single source region")] - #[test_case(&[2, 1, 2, 5, 6], (2, 10, 4) ; "many regions no overlap, multiple source regions")] - #[test_case(&[8, 1, 3, 6], (0, 0, 18) ; "many regions complete overlap")] - #[test_case(&[7, 3, 1, 4, 5], (5, 0, 8) ; "many regions overlap start")] - #[test_case(&[1, 5, 2, 9, 3], (5, 4, 8) ; "many regions overlap middle")] - #[test_case(&[3, 9, 1, 1, 2, 1], (2, 9, 8) ; "many regions overlap end")] - fn test_memmove_non_contiguous( - regions: &[usize], - (src_offset, dst_offset, len): (usize, usize, usize), - ) { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let (mem, memory_mapping) = build_memory_mapping(regions, &config); - - // flatten the memory so we can memmove it with ptr::copy - let mut expected_memory = flatten_memory(&mem); - unsafe { - std::ptr::copy( - expected_memory.as_ptr().add(src_offset), - expected_memory.as_mut_ptr().add(dst_offset), - len, - ) - }; - - // do our memmove - memmove_non_contiguous( - MM_RODATA_START + dst_offset as u64, - MM_RODATA_START + src_offset as u64, - len as u64, - &[], - &mut memory_mapping, - true, - ) - .unwrap(); - - // flatten memory post our memmove - let memory = flatten_memory(&mem); - - // compare libc's memmove with ours - assert_eq!(expected_memory, memory); - } - - #[test] - #[should_panic(expected = "AccessViolation(Store, 4294967296, 9")] - fn test_memset_non_contiguous_readonly() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut mem1 = vec![0x11; 8]; - let mem2 = vec![0x22; 4]; - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_writable(&mut mem1, MM_RODATA_START), - MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 8), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - assert_eq!( - memset_non_contiguous(MM_RODATA_START, 0x33, 9, &[], &mut memory_mapping, true) - .unwrap(), - 0 - ); - } - - #[test] - fn test_memset_non_contiguous() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = vec![0x11; 1]; - let mut mem2 = vec![0x22; 2]; - let mut mem3 = vec![0x33; 3]; - let mut mem4 = vec![0x44; 4]; - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_readonly(&mem1, MM_RODATA_START), - MemoryRegion::new_writable(&mut mem2, MM_RODATA_START + 1), - MemoryRegion::new_writable(&mut mem3, MM_RODATA_START + 3), - MemoryRegion::new_writable(&mut mem4, MM_RODATA_START + 6), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - assert_eq!( - memset_non_contiguous(MM_RODATA_START + 1, 0x55, 7, &[], &mut memory_mapping, true) - .unwrap(), - 0 - ); - assert_eq!(&mem1, &[0x11]); - assert_eq!(&mem2, &[0x55, 0x55]); - assert_eq!(&mem3, &[0x55, 0x55, 0x55]); - assert_eq!(&mem4, &[0x55, 0x55, 0x44, 0x44]); - } - - #[test] - fn test_memcmp_non_contiguous() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mem1 = b"foo".to_vec(); - let mem2 = b"barbad".to_vec(); - let mem3 = b"foobarbad".to_vec(); - let mut memory_mapping = MemoryMapping::new( - vec![ - MemoryRegion::new_readonly(&mem1, MM_RODATA_START), - MemoryRegion::new_readonly(&mem2, MM_RODATA_START + 3), - MemoryRegion::new_readonly(&mem3, MM_RODATA_START + 9), - ], - &config, - SBPFVersion::V3, - ) - .unwrap(); - - // non contiguous src - assert_eq!( - memcmp_non_contiguous( - MM_RODATA_START, - MM_RODATA_START + 9, - 9, - &[], - &mut memory_mapping, - true - ) - .unwrap(), - 0 - ); - - // non contiguous dst - assert_eq!( - memcmp_non_contiguous( - MM_RODATA_START + 10, - MM_RODATA_START + 1, - 8, - &[], - &mut memory_mapping, - true - ) - .unwrap(), - 0 - ); - - // diff - assert_eq!( - memcmp_non_contiguous( - MM_RODATA_START + 1, - MM_RODATA_START + 11, - 5, - &[], - &mut memory_mapping, - true - ) - .unwrap(), - unsafe { memcmp(b"oobar", b"obarb", 5) } - ); - } - fn build_memory_mapping<'a>( regions: &[usize], config: &'a Config, From 5fe3e7efd289c2a4f361282973c55b30d97b3e3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 6 Jun 2025 21:01:52 +0000 Subject: [PATCH 14/27] Adds touch_type_mut() and touch_slice_mut() to translate_mut!(). --- programs/bpf_loader/src/syscalls/mem_ops.rs | 2 +- programs/bpf_loader/src/syscalls/mod.rs | 60 ++++++++++++++++++--- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 579278ad511fbd..2e0e20093604a9 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -145,7 +145,7 @@ fn memmove( dst_addr: u64, src_addr: u64, n: u64, - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, ) -> Result { translate_mut!( memory_mapping, diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 5b49933016c548..5f5490d90b83fd 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -581,10 +581,10 @@ fn address_is_aligned(address: u64) -> bool { // Do not use this directly #[macro_export] macro_rules! translate_inner { - ($memory_mapping:expr, $access_type:expr, $vm_addr:expr, $len:expr $(,)?) => { + ($memory_mapping:expr, $map:ident, $access_type:expr, $vm_addr:expr, $len:expr $(,)?) => { Result::::from( $memory_mapping - .map($access_type, $vm_addr, $len) + .$map($access_type, $vm_addr, $len) .map_err(|err| err.into()), ) }; @@ -595,6 +595,7 @@ macro_rules! translate_type_inner { ($memory_mapping:expr, $access_type:expr, $vm_addr:expr, $T:ty, $check_aligned:expr $(,)?) => {{ let host_addr = translate_inner!( $memory_mapping, + map, $access_type, $vm_addr, size_of::<$T>() as u64 @@ -619,7 +620,7 @@ macro_rules! translate_slice_inner { if isize::try_from(total_size).is_err() { return Err(SyscallError::InvalidLength.into()); } - let host_addr = translate_inner!($memory_mapping, $access_type, $vm_addr, total_size)?; + let host_addr = translate_inner!($memory_mapping, map, $access_type, $vm_addr, total_size)?; if $check_aligned && !address_is_aligned::<$T>(host_addr) { return Err(SyscallError::UnalignedPointer.into()); } @@ -693,12 +694,52 @@ fn translate_slice_mut<'a, T>( ) } -// Safety: This will invalidate previously translated references. -// No other translated references shall be live when calling this. +fn touch_type_mut(memory_mapping: &mut MemoryMapping, vm_addr: u64) -> Result<(), Error> { + translate_inner!( + memory_mapping, + map_with_access_violation_handler, + AccessType::Store, + vm_addr, + size_of::() as u64, + ) + .map(|_| ()) +} +fn touch_slice_mut( + memory_mapping: &mut MemoryMapping, + vm_addr: u64, + element_count: u64, +) -> Result<(), Error> { + if element_count == 0 { + return Ok(()); + } + translate_inner!( + memory_mapping, + map_with_access_violation_handler, + AccessType::Store, + vm_addr, + element_count.saturating_mul(size_of::() as u64), + ) + .map(|_| ()) +} + +// No other translated references can be live when calling this. // Meaning it should generally be at the beginning or end of a syscall and // it should only be called once with all translations passed in one call. #[macro_export] macro_rules! translate_mut { + (internal, $memory_mapping:expr, &mut [$T:ty], $vm_addr_and_element_count:expr) => { + touch_slice_mut::<$T>( + $memory_mapping, + $vm_addr_and_element_count.0, + $vm_addr_and_element_count.1, + )? + }; + (internal, $memory_mapping:expr, &mut $T:ty, $vm_addr:expr) => { + touch_type_mut::<$T>( + $memory_mapping, + $vm_addr, + )? + }; (internal, $memory_mapping:expr, $check_aligned:expr, &mut [$T:ty], $vm_addr_and_element_count:expr) => {{ let slice = translate_slice_mut::<$T>( $memory_mapping, @@ -722,6 +763,7 @@ macro_rules! translate_mut { // This ensures that all the parameters are collected first so that if they depend on previous translations $(let $binding = ($vm_addr $(, $element_count)?);)+ // they are not invalidated by the following translations here: + $(translate_mut!(internal, $memory_mapping, &mut $T, $binding);)+ $(let $binding = translate_mut!(internal, $memory_mapping, $check_aligned, &mut $T, $binding);)+ let host_ranges = [ $(($binding.1, $binding.2),)+ @@ -2236,11 +2278,15 @@ mod tests { for (ok, start, length, value) in cases { if ok { assert_eq!( - translate_inner!(&memory_mapping, AccessType::Load, start, length).unwrap(), + translate_inner!(&memory_mapping, map, AccessType::Load, start, length) + .unwrap(), value ) } else { - assert!(translate_inner!(&memory_mapping, AccessType::Load, start, length).is_err()) + assert!( + translate_inner!(&memory_mapping, map, AccessType::Load, start, length) + .is_err() + ) } } } From 0bb928845f2c4da89f74775674e897978e9935f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Sat, 12 Apr 2025 16:19:55 +0000 Subject: [PATCH 15/27] Removes parameter writable from Serializer::push_region(). --- program-runtime/src/serialization.rs | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index 1944a8aab7c59e..3675397fb020fa 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -114,7 +114,7 @@ impl Serializer { self.write_all(account.get_data()); vm_data_addr } else { - self.push_region(true); + self.push_region(); let vaddr = self.vaddr; let address_space_reserved_for_account = if self.aligned { account @@ -153,26 +153,18 @@ impl Serializer { Ok(vm_data_addr) } - fn push_region(&mut self, writable: bool) { + fn push_region(&mut self) { let range = self.region_start..self.buffer.len(); - let region = if writable { - MemoryRegion::new_writable( - self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), - self.vaddr, - ) - } else { - MemoryRegion::new_readonly( - self.buffer.as_slice().get(range.clone()).unwrap(), - self.vaddr, - ) - }; - self.regions.push(region); + self.regions.push(MemoryRegion::new_writable( + self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), + self.vaddr, + )); self.region_start = range.end; self.vaddr += range.len() as u64; } fn finish(mut self) -> (AlignedMemory, Vec) { - self.push_region(true); + self.push_region(); debug_assert_eq!(self.region_start, self.buffer.len()); (self.buffer, self.regions) } From 22e2768aea89ed826b30802c6a6c7ce95debf142 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 8 May 2025 12:04:58 +0000 Subject: [PATCH 16/27] Inlines account_data_region(). --- programs/bpf_loader/src/syscalls/cpi.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 08d382d4d72be3..a7df5137a1f0c3 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1198,11 +1198,14 @@ fn update_caller_account_region( .saturating_add(MAX_PERMITTED_DATA_INCREASE) }; - if let Some((region_index, region)) = account_data_region( - memory_mapping, - caller_account.vm_data_addr, - address_space_reserved_for_account, - )? { + if address_space_reserved_for_account > 0 { + // We can trust vm_data_addr to point to the correct region because we + // enforce that in CallerAccount::from_(sol_)account_info. + let (region_index, region) = memory_mapping + .find_region(caller_account.vm_data_addr) + .ok_or_else(|| Box::new(InstructionError::MissingAccount))?; + // vm_data_addr must always point to the beginning of the region + debug_assert_eq!(region.vm_addr, caller_account.vm_data_addr); let new_region = create_memory_region_of_account(callee_account, region.vm_addr)?; memory_mapping.replace_region(region_index, new_region)?; } From c7b026d745777b3aa5dd90cc5560568aff68c891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 11 Apr 2025 16:49:51 +0000 Subject: [PATCH 17/27] Cleanup unused code and dependencies. --- Cargo.lock | 1 - programs/bpf_loader/Cargo.toml | 1 - programs/bpf_loader/src/syscalls/cpi.rs | 166 +------- programs/bpf_loader/src/syscalls/mem_ops.rs | 401 +------------------- programs/sbf/Cargo.lock | 1 - svm/examples/Cargo.lock | 1 - transaction-context/src/lib.rs | 32 +- 7 files changed, 10 insertions(+), 593 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3adf119a46f4c4..c7d831f0c34840 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7184,7 +7184,6 @@ dependencies = [ "num-traits", "qualifier_attr", "rand 0.8.5", - "scopeguard", "solana-account", "solana-account-info", "solana-big-mod-exp", diff --git a/programs/bpf_loader/Cargo.toml b/programs/bpf_loader/Cargo.toml index 074374745db3ba..ac081b2ca5d95a 100644 --- a/programs/bpf_loader/Cargo.toml +++ b/programs/bpf_loader/Cargo.toml @@ -31,7 +31,6 @@ bincode = { workspace = true } libsecp256k1 = { workspace = true } num-traits = { workspace = true } qualifier_attr = { workspace = true } -scopeguard = { workspace = true } solana-account = { workspace = true } solana-account-info = { workspace = true } solana-big-mod-exp = { workspace = true } diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index a7df5137a1f0c3..680b5c66e62689 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,16 +1,15 @@ use { super::*, crate::{translate_inner, translate_slice_inner, translate_type_inner}, - scopeguard::defer, solana_loader_v3_interface::instruction as bpf_loader_upgradeable, solana_measure::measure::Measure, solana_program_runtime::{ invoke_context::SerializedAccountMetadata, serialization::create_memory_region_of_account, }, - solana_sbpf::{ebpf, memory_region::MemoryRegion}, + solana_sbpf::ebpf, solana_stable_layout::stable_instruction::StableInstruction, solana_transaction_context::BorrowedAccount, - std::{mem, ptr}, + std::mem, }; const MAX_CPI_INSTRUCTION_DATA_LEN: u64 = 10 * 1024; @@ -312,19 +311,6 @@ impl<'a> CallerAccount<'a> { ref_to_len_in_vm, }) } - - fn realloc_region( - &self, - memory_mapping: &'a mut MemoryMapping<'_>, - is_loader_deprecated: bool, - ) -> Result, Error> { - account_realloc_region( - memory_mapping, - self.vm_data_addr, - self.original_data_len, - is_loader_deprecated, - ) - } } struct TranslatedAccount<'a> { @@ -880,8 +866,6 @@ where // BorrowedAccount (callee_account) so the callee can see the // changes. let update_caller = update_callee_account( - invoke_context, - memory_mapping, is_loader_deprecated, &caller_account, callee_account, @@ -1108,7 +1092,6 @@ fn cpi_common( update_caller_account( invoke_context, memory_mapping, - is_loader_deprecated, &mut translate_account.caller_account, &mut callee_account, direct_mapping, @@ -1132,8 +1115,6 @@ fn cpi_common( // When true is returned, the caller account must be updated after CPI. This // is only set for direct mapping when the pointer may have changed. fn update_callee_account( - _invoke_context: &InvokeContext, - _memory_mapping: &MemoryMapping, is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, @@ -1224,7 +1205,6 @@ fn update_caller_account_region( fn update_caller_account( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping<'_>, - _is_loader_deprecated: bool, caller_account: &mut CallerAccount<'_>, callee_account: &mut BorrowedAccount<'_>, direct_mapping: bool, @@ -1303,47 +1283,6 @@ fn update_caller_account( Ok(()) } -fn account_data_region<'a>( - memory_mapping: &'a mut MemoryMapping<'_>, - vm_data_addr: u64, - address_space_reserved_for_account: usize, -) -> Result, Error> { - if address_space_reserved_for_account == 0 { - return Ok(None); - } - - // We can trust vm_data_addr to point to the correct region because we - // enforce that in CallerAccount::from_(sol_)account_info. - let (data_region_index, data_region) = memory_mapping - .find_region(vm_data_addr) - .ok_or_else(|| Box::new(InstructionError::MissingAccount))?; - // vm_data_addr must always point to the beginning of the region - debug_assert_eq!(data_region.vm_addr, vm_data_addr); - Ok(Some((data_region_index, data_region))) -} - -fn account_realloc_region<'a>( - memory_mapping: &'a mut MemoryMapping<'_>, - vm_data_addr: u64, - original_data_len: usize, - is_loader_deprecated: bool, -) -> Result, Error> { - if is_loader_deprecated { - return Ok(None); - } - - let realloc_vm_addr = vm_data_addr.saturating_add(original_data_len as u64); - let (realloc_region_index, realloc_region) = memory_mapping - .find_region(realloc_vm_addr) - .ok_or_else(|| Box::new(InstructionError::MissingAccount))?; - debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); - debug_assert!((MAX_PERMITTED_DATA_INCREASE - ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) - .contains(&(realloc_region.len as usize))); - debug_assert_eq!(realloc_region.access_violation_handler_payload, None); - Ok(Some((realloc_region_index, realloc_region))) -} - #[allow(clippy::indexing_slicing)] #[allow(clippy::arithmetic_side_effects)] #[cfg(test)] @@ -1597,7 +1536,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1665,7 +1603,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1690,7 +1627,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1707,7 +1643,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1723,7 +1658,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1795,7 +1729,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, true, @@ -1866,7 +1799,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, true, @@ -1885,7 +1817,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1901,7 +1832,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, true, @@ -1959,12 +1889,10 @@ mod tests { let mut callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data().len(), 3); - assert_eq!(callee_account.capacity(), 3); update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, true, @@ -1972,7 +1900,6 @@ mod tests { .unwrap(); assert_eq!(callee_account.get_data().len(), 3); - assert!(callee_account.capacity() >= caller_account.original_data_len); let data = translate_slice::( &mut memory_mapping, caller_account.vm_data_addr, @@ -2005,17 +1932,6 @@ mod tests { false, ); - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - let caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2023,15 +1939,7 @@ mod tests { *caller_account.lamports = 42; *caller_account.owner = Pubkey::new_unique(); - update_callee_account( - &invoke_context, - &mut memory_mapping, - false, - &caller_account, - callee_account, - false, - ) - .unwrap(); + update_callee_account(false, &caller_account, callee_account, false).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_lamports(), 42); @@ -2061,17 +1969,6 @@ mod tests { false, ); - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2079,15 +1976,7 @@ mod tests { let mut data = b"foo".to_vec(); caller_account.serialized_data = &mut data; - update_callee_account( - &invoke_context, - &mut memory_mapping, - false, - &caller_account, - callee_account, - false, - ) - .unwrap(); + update_callee_account(false, &caller_account, callee_account, false).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), caller_account.serialized_data); @@ -2098,15 +1987,7 @@ mod tests { *caller_account.ref_to_len_in_vm = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; - update_callee_account( - &invoke_context, - &mut memory_mapping, - false, - &caller_account, - callee_account, - false, - ) - .unwrap(); + update_callee_account(false, &caller_account, callee_account, false).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), b""); } @@ -2134,17 +2015,6 @@ mod tests { false, ); - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let mut memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2152,8 +2022,6 @@ mod tests { caller_account.serialized_data[0] = b'b'; assert_matches!( update_callee_account( - &invoke_context, - &mut memory_mapping, false, &caller_account, callee_account, @@ -2170,8 +2038,6 @@ mod tests { let callee_account = borrow_instruction_account!(invoke_context, 0); assert_matches!( update_callee_account( - &invoke_context, - &mut memory_mapping, false, &caller_account, callee_account, @@ -2188,8 +2054,6 @@ mod tests { let callee_account = borrow_instruction_account!(invoke_context, 0); assert_matches!( update_callee_account( - &invoke_context, - &mut memory_mapping, false, &caller_account, callee_account, @@ -2259,15 +2123,7 @@ mod tests { (3, b"foo".to_vec()), // < original_data_len, truncates ] { *caller_account.ref_to_len_in_vm = len as u64; - update_callee_account( - &invoke_context, - &mut memory_mapping, - false, - &caller_account, - callee_account, - true, - ) - .unwrap(); + update_callee_account(false, &caller_account, callee_account, true).unwrap(); callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), expected); } @@ -2278,15 +2134,7 @@ mod tests { *caller_account.ref_to_len_in_vm = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; - update_callee_account( - &invoke_context, - &mut memory_mapping, - false, - &caller_account, - callee_account, - true, - ) - .unwrap(); + update_callee_account(false, &caller_account, callee_account, true).unwrap(); callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), b""); } diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 2e0e20093604a9..3c2b3d1e1990ba 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -1,10 +1,4 @@ -use { - super::*, - crate::translate_mut, - solana_program_runtime::invoke_context::SerializedAccountMetadata, - solana_sbpf::{error::EbpfError, memory_region::MemoryRegion}, - std::slice, -}; +use {super::*, crate::translate_mut}; fn mem_op_consume(invoke_context: &mut InvokeContext, n: u64) -> Result<(), Error> { let compute_cost = invoke_context.get_execution_cost(); @@ -178,402 +172,11 @@ unsafe fn memcmp(s1: &[u8], s2: &[u8], n: usize) -> i32 { 0 } -#[derive(Debug)] -enum MemcmpError { - Diff(i32), -} - -impl std::fmt::Display for MemcmpError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - MemcmpError::Diff(diff) => write!(f, "memcmp diff: {diff}"), - } - } -} - -impl std::error::Error for MemcmpError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - MemcmpError::Diff(_) => None, - } - } -} - -#[allow(clippy::too_many_arguments)] -fn iter_memory_pair_chunks( - src_access: AccessType, - src_addr: u64, - dst_access: AccessType, - dst_addr: u64, - n_bytes: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &mut MemoryMapping, - reverse: bool, - resize_area: bool, - mut fun: F, -) -> Result -where - T: Default, - F: FnMut(*const u8, *const u8, usize) -> Result, -{ - let mut src_chunk_iter = MemoryChunkIterator::new( - memory_mapping, - accounts, - src_access, - src_addr, - n_bytes, - resize_area, - )?; - let mut dst_chunk_iter = MemoryChunkIterator::new( - memory_mapping, - accounts, - dst_access, - dst_addr, - n_bytes, - resize_area, - )?; - - let mut src_chunk = None; - let mut dst_chunk = None; - - macro_rules! memory_chunk { - ($chunk_iter:ident, $chunk:ident) => { - if let Some($chunk) = &mut $chunk { - // Keep processing the current chunk - $chunk - } else { - // This is either the first call or we've processed all the bytes in the current - // chunk. Move to the next one. - let chunk = match if reverse { - $chunk_iter.next_back() - } else { - $chunk_iter.next() - } { - Some(item) => item?, - None => break, - }; - $chunk.insert(chunk) - } - }; - } - - loop { - let (src_region, src_chunk_addr, src_remaining) = memory_chunk!(src_chunk_iter, src_chunk); - let (dst_region, dst_chunk_addr, dst_remaining) = memory_chunk!(dst_chunk_iter, dst_chunk); - - // We always process same-length pairs - let chunk_len = *src_remaining.min(dst_remaining); - - let (src_host_addr, dst_host_addr) = { - let (src_addr, dst_addr) = if reverse { - // When scanning backwards not only we want to scan regions from the end, - // we want to process the memory within regions backwards as well. - ( - src_chunk_addr - .saturating_add(*src_remaining as u64) - .saturating_sub(chunk_len as u64), - dst_chunk_addr - .saturating_add(*dst_remaining as u64) - .saturating_sub(chunk_len as u64), - ) - } else { - (*src_chunk_addr, *dst_chunk_addr) - }; - - ( - src_region - .vm_to_host(src_access, src_addr, chunk_len as u64) - .ok_or_else(|| { - EbpfError::AccessViolation(src_access, src_addr, chunk_len as u64, "") - })?, - dst_region - .vm_to_host(dst_access, dst_addr, chunk_len as u64) - .ok_or_else(|| { - EbpfError::AccessViolation(dst_access, dst_addr, chunk_len as u64, "") - })?, - ) - }; - - fun( - src_host_addr as *const u8, - dst_host_addr as *const u8, - chunk_len, - )?; - - // Update how many bytes we have left to scan in each chunk - *src_remaining = src_remaining.saturating_sub(chunk_len); - *dst_remaining = dst_remaining.saturating_sub(chunk_len); - - if !reverse { - // We've scanned `chunk_len` bytes so we move the vm address forward. In reverse - // mode we don't do this since we make progress by decreasing src_len and - // dst_len. - *src_chunk_addr = src_chunk_addr.saturating_add(chunk_len as u64); - *dst_chunk_addr = dst_chunk_addr.saturating_add(chunk_len as u64); - } - - if *src_remaining == 0 { - src_chunk = None; - } - - if *dst_remaining == 0 { - dst_chunk = None; - } - } - - Ok(T::default()) -} - -struct MemoryChunkIterator<'a> { - memory_mapping: &'a mut MemoryMapping<'a>, - accounts: &'a [SerializedAccountMetadata], - access_type: AccessType, - initial_vm_addr: u64, - vm_addr_start: u64, - // exclusive end index (start + len, so one past the last valid address) - vm_addr_end: u64, - len: u64, - account_index: Option, - is_account: Option, - resize_area: bool, -} - -impl<'a> MemoryChunkIterator<'a> { - fn new( - memory_mapping: &'a mut MemoryMapping, - accounts: &'a [SerializedAccountMetadata], - access_type: AccessType, - vm_addr: u64, - len: u64, - resize_area: bool, - ) -> Result, EbpfError> { - let vm_addr_end = vm_addr.checked_add(len).ok_or(EbpfError::AccessViolation( - access_type, - vm_addr, - len, - "unknown", - ))?; - - Ok(MemoryChunkIterator { - memory_mapping, - accounts, - access_type, - initial_vm_addr: vm_addr, - len, - vm_addr_start: vm_addr, - vm_addr_end, - account_index: None, - is_account: None, - resize_area, - }) - } - - fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, Error> { - match self.memory_mapping.map(self.access_type, vm_addr, self.len) { - solana_sbpf::error::ProgramResult::Ok(_) => { - Ok(self.memory_mapping.find_region(vm_addr).unwrap().1) - } - solana_sbpf::error::ProgramResult::Err(error) => match error { - EbpfError::AccessViolation(access_type, _vm_addr, _len, name) => Err(Box::new( - EbpfError::AccessViolation(access_type, self.initial_vm_addr, self.len, name), - )), - EbpfError::StackAccessViolation(access_type, _vm_addr, _len, frame) => { - Err(Box::new(EbpfError::StackAccessViolation( - access_type, - self.initial_vm_addr, - self.len, - frame, - ))) - } - _ => Err(error.into()), - }, - } - } -} - -impl<'a> Iterator for MemoryChunkIterator<'a> { - type Item = Result<(&'a MemoryRegion, u64, usize), Error>; - - fn next(&mut self) -> Option { - if self.vm_addr_start == self.vm_addr_end { - return None; - } - - let region = match self.region(self.vm_addr_start) { - Ok(region) => region, - Err(e) => { - self.vm_addr_start = self.vm_addr_end; - return Some(Err(e)); - } - }; - - let region_is_account; - - let mut account_index = self.account_index.unwrap_or_default(); - self.account_index = Some(account_index); - - loop { - if let Some(account) = self.accounts.get(account_index) { - let account_addr = account.vm_data_addr; - let resize_addr = account_addr.saturating_add(account.original_data_len as u64); - - if resize_addr < region.vm_addr { - // region is after this account, move on next one - account_index = account_index.saturating_add(1); - self.account_index = Some(account_index); - } else { - region_is_account = (account.original_data_len != 0 && region.vm_addr == account_addr) - // unaligned programs do not have a resize area - || (self.resize_area && region.vm_addr == resize_addr); - break; - } - } else { - // address is after all the accounts - region_is_account = false; - break; - } - } - - if let Some(is_account) = self.is_account { - if is_account != region_is_account { - return Some(Err(SyscallError::InvalidLength.into())); - } - } else { - self.is_account = Some(region_is_account); - } - - let vm_addr = self.vm_addr_start; - let region_vm_addr_end = region.vm_addr_range().end; - - let chunk_len = if region_vm_addr_end <= self.vm_addr_end { - // consume the whole region - let len = region_vm_addr_end.saturating_sub(self.vm_addr_start); - self.vm_addr_start = region_vm_addr_end; - len - } else { - // consume part of the region - let len = self.vm_addr_end.saturating_sub(self.vm_addr_start); - self.vm_addr_start = self.vm_addr_end; - len - }; - - Some(Ok((region, vm_addr, chunk_len as usize))) - } -} - -impl DoubleEndedIterator for MemoryChunkIterator<'_> { - fn next_back(&mut self) -> Option { - if self.vm_addr_start == self.vm_addr_end { - return None; - } - - let region = match self.region(self.vm_addr_end.saturating_sub(1)) { - Ok(region) => region, - Err(e) => { - self.vm_addr_start = self.vm_addr_end; - return Some(Err(e)); - } - }; - - let region_is_account; - - let mut account_index = self - .account_index - .unwrap_or_else(|| self.accounts.len().saturating_sub(1)); - self.account_index = Some(account_index); - - loop { - let Some(account) = self.accounts.get(account_index) else { - // address is after all the accounts - region_is_account = false; - break; - }; - - let account_addr = account.vm_data_addr; - let resize_addr = account_addr.saturating_add(account.original_data_len as u64); - - if account_index > 0 && account_addr > region.vm_addr { - account_index = account_index.saturating_sub(1); - - self.account_index = Some(account_index); - } else { - region_is_account = (account.original_data_len != 0 && region.vm_addr == account_addr) - // unaligned programs do not have a resize area - || (self.resize_area && region.vm_addr == resize_addr); - break; - } - } - - if let Some(is_account) = self.is_account { - if is_account != region_is_account { - return Some(Err(SyscallError::InvalidLength.into())); - } - } else { - self.is_account = Some(region_is_account); - } - - let chunk_len = if region.vm_addr >= self.vm_addr_start { - // consume the whole region - let len = self.vm_addr_end.saturating_sub(region.vm_addr); - self.vm_addr_end = region.vm_addr; - len - } else { - // consume part of the region - let len = self.vm_addr_end.saturating_sub(self.vm_addr_start); - self.vm_addr_end = self.vm_addr_start; - len - }; - - Some(Ok((region, self.vm_addr_end, chunk_len as usize))) - } -} - #[cfg(test)] #[allow(clippy::indexing_slicing)] #[allow(clippy::arithmetic_side_effects)] mod tests { - use { - super::*, - assert_matches::assert_matches, - solana_sbpf::{ebpf::MM_RODATA_START, program::SBPFVersion}, - test_case::test_case, - }; - - fn to_chunk_vec<'a>( - iter: impl Iterator>, - ) -> Vec<(u64, usize)> { - iter.flat_map(|res| res.map(|(_, vm_addr, len)| (vm_addr, len))) - .collect::>() - } - - fn build_memory_mapping<'a>( - regions: &[usize], - config: &'a Config, - ) -> (Vec>, MemoryMapping<'a>) { - let mut regs = vec![]; - let mut mem = Vec::new(); - let mut offset = 0; - for (i, region_len) in regions.iter().enumerate() { - mem.push( - (0..*region_len) - .map(|x| (i * 10 + x) as u8) - .collect::>(), - ); - regs.push(MemoryRegion::new_writable( - &mut mem[i], - MM_RODATA_START + offset as u64, - )); - offset += *region_len; - } - - let mut memory_mapping = MemoryMapping::new(regs, config, SBPFVersion::V3).unwrap(); - - (mem, memory_mapping) - } - - fn flatten_memory(mem: &[Vec]) -> Vec { - mem.iter().flatten().copied().collect() - } + use super::*; #[test] fn test_is_nonoverlapping() { diff --git a/programs/sbf/Cargo.lock b/programs/sbf/Cargo.lock index 0a7963cb30dddb..4f17e8ada0f98d 100644 --- a/programs/sbf/Cargo.lock +++ b/programs/sbf/Cargo.lock @@ -5691,7 +5691,6 @@ dependencies = [ "libsecp256k1 0.6.0", "num-traits", "qualifier_attr", - "scopeguard", "solana-account", "solana-account-info", "solana-big-mod-exp", diff --git a/svm/examples/Cargo.lock b/svm/examples/Cargo.lock index e6662636318ba4..b711cfaf11ba39 100644 --- a/svm/examples/Cargo.lock +++ b/svm/examples/Cargo.lock @@ -5524,7 +5524,6 @@ dependencies = [ "libsecp256k1", "num-traits", "qualifier_attr", - "scopeguard", "solana-account", "solana-account-info", "solana-big-mod-exp", diff --git a/transaction-context/src/lib.rs b/transaction-context/src/lib.rs index 59eb032c60a9d3..1b5368258b4f85 100644 --- a/transaction-context/src/lib.rs +++ b/transaction-context/src/lib.rs @@ -3,7 +3,7 @@ #![cfg_attr(docsrs, feature(doc_auto_cfg))] #[cfg(not(target_os = "solana"))] -use {solana_account::WritableAccount, solana_rent::Rent, std::mem::MaybeUninit}; +use {solana_account::WritableAccount, solana_rent::Rent}; use { solana_account::{AccountSharedData, ReadableAccount}, solana_instruction::error::InstructionError, @@ -959,16 +959,6 @@ impl BorrowedAccount<'_> { Ok(self.account.data_as_mut_slice()) } - /// Returns the spare capacity of the vector backing the account data. - /// - /// This method should only ever be used during CPI, where after a shrinking - /// realloc we want to zero the spare capacity. - #[cfg(not(target_os = "solana"))] - pub fn spare_data_capacity_mut(&mut self) -> Result<&mut [MaybeUninit], InstructionError> { - debug_assert!(!self.account.is_shared()); - Ok(self.account.spare_data_capacity_mut()) - } - /// Overwrites the account data and size (transaction wide). /// /// You should always prefer set_data_from_slice(). Calling this method is @@ -1041,26 +1031,6 @@ impl BorrowedAccount<'_> { Ok(()) } - /// Reserves capacity for at least additional more elements to be inserted - /// in the given account. Does nothing if capacity is already sufficient. - #[cfg(not(target_os = "solana"))] - pub fn reserve(&mut self, additional: usize) -> Result<(), InstructionError> { - // Note that we don't need to call can_data_be_changed() here nor - // touch() the account. reserve() only changes the capacity of the - // memory that holds the account but it doesn't actually change content - // nor length of the account. - self.make_data_mut(); - self.account.reserve(additional); - - Ok(()) - } - - /// Returns the number of bytes the account can hold without reallocating. - #[cfg(not(target_os = "solana"))] - pub fn capacity(&self) -> usize { - self.account.capacity() - } - /// Returns whether the underlying AccountSharedData is shared. /// /// The data is shared if the account has been loaded from the accounts database and has never From a18306a7dcfac4c669090abd11b8fa5143c39d96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:54:19 +0000 Subject: [PATCH 18/27] Runs update_callee_account() and update_caller_account() tests with direct_mapping as well. --- programs/bpf_loader/src/syscalls/cpi.rs | 75 +++++++++++++++++-------- 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 680b5c66e62689..992cddd1089322 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1308,6 +1308,7 @@ mod tests { rc::Rc, slice, }, + test_case::test_matrix, }; macro_rules! mock_invoke_context { @@ -1492,8 +1493,8 @@ mod tests { assert_eq!(caller_account.serialized_data, account.data()); } - #[test] - fn test_update_caller_account_lamports_owner() { + #[test_matrix([false, true])] + fn test_update_caller_account_lamports_owner(direct_mapping: bool) { let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( @@ -1538,7 +1539,7 @@ mod tests { &memory_mapping, &mut caller_account, &mut callee_account, - false, + direct_mapping, ) .unwrap(); @@ -1910,8 +1911,8 @@ mod tests { assert_eq!(data, callee_account.get_data()); } - #[test] - fn test_update_callee_account_lamports_owner() { + #[test_matrix([false, true])] + fn test_update_callee_account_lamports_owner(direct_mapping: bool) { let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); let account = transaction_accounts[1].1.clone(); @@ -1939,15 +1940,15 @@ mod tests { *caller_account.lamports = 42; *caller_account.owner = Pubkey::new_unique(); - update_callee_account(false, &caller_account, callee_account, false).unwrap(); + update_callee_account(false, &caller_account, callee_account, direct_mapping).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_lamports(), 42); assert_eq!(caller_account.owner, callee_account.get_owner()); } - #[test] - fn test_update_callee_account_data() { + #[test_matrix([false, true])] + fn test_update_callee_account_data_writable(direct_mapping: bool) { let transaction_accounts = transaction_with_one_writable_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); @@ -1970,16 +1971,32 @@ mod tests { ); let mut caller_account = mock_caller_account.caller_account(); + let callee_account = borrow_instruction_account!(invoke_context, 0); + // direct mapping does not copy data in update_callee_account() + caller_account.serialized_data[0] = b'b'; + update_callee_account(false, &caller_account, callee_account, false).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); + assert_eq!(callee_account.get_data(), b"boobar"); - let mut data = b"foo".to_vec(); + // growing resize + let mut data = b"foobarbaz".to_vec(); + *caller_account.ref_to_len_in_vm = data.len() as u64; caller_account.serialized_data = &mut data; + assert_eq!( + update_callee_account(false, &caller_account, callee_account, direct_mapping).unwrap(), + direct_mapping, + ); - update_callee_account(false, &caller_account, callee_account, false).unwrap(); - + // truncating resize + let mut data = b"baz".to_vec(); + *caller_account.ref_to_len_in_vm = data.len() as u64; + caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!(callee_account.get_data(), caller_account.serialized_data); + assert_eq!( + update_callee_account(false, &caller_account, callee_account, direct_mapping).unwrap(), + direct_mapping, + ); // close the account let mut data = Vec::new(); @@ -1987,13 +2004,26 @@ mod tests { *caller_account.ref_to_len_in_vm = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; - update_callee_account(false, &caller_account, callee_account, false).unwrap(); + let callee_account = borrow_instruction_account!(invoke_context, 0); + update_callee_account(false, &caller_account, callee_account, direct_mapping).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), b""); + + // growing beyond address_space_reserved_for_account + *caller_account.ref_to_len_in_vm = (7 + MAX_PERMITTED_DATA_INCREASE) as u64; + let result = update_callee_account(false, &caller_account, callee_account, direct_mapping); + if direct_mapping { + assert_matches!( + result, + Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc + ); + } else { + result.unwrap(); + } } - #[test] - fn test_update_callee_account_data_readonly() { + #[test_matrix([false, true])] + fn test_update_callee_account_data_readonly(direct_mapping: bool) { let transaction_accounts = transaction_with_one_readonly_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); @@ -2014,11 +2044,10 @@ mod tests { account.data(), false, ); - let mut caller_account = mock_caller_account.caller_account(); - let callee_account = borrow_instruction_account!(invoke_context, 0); + // direct mapping does not copy data in update_callee_account() caller_account.serialized_data[0] = b'b'; assert_matches!( update_callee_account( @@ -2030,34 +2059,32 @@ mod tests { Err(error) if error.downcast_ref::().unwrap() == &InstructionError::ExternalAccountDataModified ); - // without direct mapping + // growing resize let mut data = b"foobarbaz".to_vec(); *caller_account.ref_to_len_in_vm = data.len() as u64; caller_account.serialized_data = &mut data; - let callee_account = borrow_instruction_account!(invoke_context, 0); assert_matches!( update_callee_account( false, &caller_account, callee_account, - false, + direct_mapping, ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::AccountDataSizeChanged ); - // with direct mapping + // truncating resize let mut data = b"baz".to_vec(); - *caller_account.ref_to_len_in_vm = 9; + *caller_account.ref_to_len_in_vm = data.len() as u64; caller_account.serialized_data = &mut data; - let callee_account = borrow_instruction_account!(invoke_context, 0); assert_matches!( update_callee_account( false, &caller_account, callee_account, - true, + direct_mapping, ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::AccountDataSizeChanged ); From 911a806f13154f12bb3a0271db34c6d26a4f78ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:06:45 +0000 Subject: [PATCH 19/27] Removes test_update_caller_account_data_capacity_direct_mapping(). --- programs/bpf_loader/src/syscalls/cpi.rs | 69 ------------------------- 1 file changed, 69 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 992cddd1089322..0749830ae3582f 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1842,75 +1842,6 @@ mod tests { assert_eq!(data_len, 0); } - #[test] - fn test_update_caller_account_data_capacity_direct_mapping() { - let transaction_accounts = - transaction_with_one_writable_instruction_account(b"foobar".to_vec()); - let account = transaction_accounts[1].1.clone(); - - mock_invoke_context!( - invoke_context, - transaction_context, - b"instruction data", - transaction_accounts, - &[0], - &[1] - ); - - let mut mock_caller_account = MockCallerAccount::new( - account.lamports(), - *account.owner(), - 0xFFFFFFFF00000000, - account.data(), - true, - ); - - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - - let mut caller_account = mock_caller_account.caller_account(); - - { - let mut account = invoke_context - .transaction_context - .get_account_at_index(1) - .unwrap() - .try_borrow_mut() - .unwrap(); - account.set_data(b"baz".to_vec()); - } - - let mut callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!(callee_account.get_data().len(), 3); - - update_caller_account( - &invoke_context, - &memory_mapping, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - - assert_eq!(callee_account.get_data().len(), 3); - let data = translate_slice::( - &mut memory_mapping, - caller_account.vm_data_addr, - callee_account.get_data().len() as u64, - true, - ) - .unwrap(); - assert_eq!(data, callee_account.get_data()); - } - #[test_matrix([false, true])] fn test_update_callee_account_lamports_owner(direct_mapping: bool) { let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); From 098261b9f51b95e644bd5c70e487f44afeb67094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:58:56 +0000 Subject: [PATCH 20/27] Removes test_update_caller_account_data_direct_mapping(). --- programs/bpf_loader/src/syscalls/cpi.rs | 174 ------------------------ 1 file changed, 174 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 0749830ae3582f..80909c0b5de526 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1668,180 +1668,6 @@ mod tests { assert_eq!(data_len, 0); } - #[test] - fn test_update_caller_account_data_direct_mapping() { - let transaction_accounts = - transaction_with_one_writable_instruction_account(b"foobar".to_vec()); - let account = transaction_accounts[1].1.clone(); - let original_data_len = account.data().len(); - - mock_invoke_context!( - invoke_context, - transaction_context, - b"instruction data", - transaction_accounts, - &[0], - &[1] - ); - - let mut mock_caller_account = MockCallerAccount::new( - account.lamports(), - *account.owner(), - 0xFFFFFFFF00000000, - account.data(), - true, - ); - - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - - let data_slice = mock_caller_account.data_slice(); - let len_ptr = unsafe { - data_slice - .as_ptr() - .offset(-(mem::size_of::() as isize)) - }; - let serialized_len = || unsafe { *len_ptr.cast::() as usize }; - let mut caller_account = mock_caller_account.caller_account(); - - let mut callee_account = borrow_instruction_account!(invoke_context, 0); - - for change_ptr in [false, true] { - for (new_value, expected_realloc_used) in [ - (b"foobazbad".to_vec(), 3), // > original_data_len, writes into realloc - (b"foo".to_vec(), 0), // < original_data_len, zeroes account capacity + realloc capacity - (b"foobaz".to_vec(), 0), // = original_data_len - (vec![], 0), // check lower bound - ] { - if change_ptr { - callee_account.set_data(new_value).unwrap(); - } else { - callee_account.set_data_from_slice(&new_value).unwrap(); - } - - update_caller_account( - &invoke_context, - &memory_mapping, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - - // check that the caller account data pointer always matches the callee account data pointer - assert_eq!( - translate_slice::( - &mut memory_mapping, - caller_account.vm_data_addr, - 1, - true, - ) - .unwrap() - .as_ptr(), - callee_account.get_data().as_ptr() - ); - - let data_len = callee_account.get_data().len(); - // the account info length must get updated - assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); - // the length slot in the serialization parameters must be updated - assert_eq!(data_len, serialized_len()); - - let realloc_area = translate_slice::( - &mut memory_mapping, - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64), - MAX_PERMITTED_DATA_INCREASE as u64, - invoke_context.get_check_aligned(), - ) - .unwrap(); - - if data_len < original_data_len { - // if an account gets resized below its original data length, - // the spare capacity is zeroed - let original_data_slice = unsafe { - slice::from_raw_parts(callee_account.get_data().as_ptr(), original_data_len) - }; - - let spare_capacity = &original_data_slice[original_data_len - data_len..]; - assert!( - is_zeroed(spare_capacity), - "dirty account spare capacity {spare_capacity:?}", - ); - } - - // if an account gets extended past its original length, the end - // gets written in the realloc padding - assert_eq!( - &realloc_area[..expected_realloc_used], - &callee_account.get_data()[data_len - expected_realloc_used..] - ); - - // the unused realloc padding is always zeroed - assert!( - is_zeroed(&realloc_area[expected_realloc_used..]), - "dirty realloc padding {realloc_area:?}", - ); - } - } - - callee_account - .set_data_length(original_data_len + MAX_PERMITTED_DATA_INCREASE) - .unwrap(); - update_caller_account( - &invoke_context, - &memory_mapping, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - assert!( - is_zeroed(caller_account.serialized_data), - "dirty realloc padding {:?}", - caller_account.serialized_data - ); - - callee_account - .set_data_length(original_data_len + MAX_PERMITTED_DATA_INCREASE + 1) - .unwrap(); - assert_matches!( - update_caller_account( - &invoke_context, - &memory_mapping, - &mut caller_account, - &mut callee_account, - false, - ), - Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc - ); - - // close the account - callee_account.set_data_length(0).unwrap(); - callee_account - .set_owner(system_program::id().as_ref()) - .unwrap(); - update_caller_account( - &invoke_context, - &memory_mapping, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - let data_len = callee_account.get_data().len(); - assert_eq!(data_len, 0); - } - #[test_matrix([false, true])] fn test_update_callee_account_lamports_owner(direct_mapping: bool) { let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); From e6cb1f5447a96f0b594d17c2062e81f8f3dd6765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:07:23 +0000 Subject: [PATCH 21/27] Removes test_update_callee_account_data_direct_mapping(). --- programs/bpf_loader/src/syscalls/cpi.rs | 76 ------------------------- 1 file changed, 76 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 80909c0b5de526..7db952fccd8c99 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1847,82 +1847,6 @@ mod tests { ); } - #[test] - fn test_update_callee_account_data_direct_mapping() { - let transaction_accounts = - transaction_with_one_writable_instruction_account(b"foobar".to_vec()); - let account = transaction_accounts[1].1.clone(); - - mock_invoke_context!( - invoke_context, - transaction_context, - b"instruction data", - transaction_accounts, - &[0], - &[1] - ); - - let mut mock_caller_account = MockCallerAccount::new( - 1234, - *account.owner(), - 0xFFFFFFFF00000000, - account.data(), - true, - ); - - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new( - mock_caller_account.regions.split_off(0), - &config, - SBPFVersion::V3, - ) - .unwrap(); - - let mut caller_account = mock_caller_account.caller_account(); - - let mut callee_account = borrow_instruction_account!(invoke_context, 0); - - // this is done when a writable account is mapped, and it ensures - // through make_data_mut() that the account is made writable and resized - // with enough padding to hold the realloc padding - callee_account.get_data_mut().unwrap(); - - let serialized_data = translate_slice_mut::( - &memory_mapping, - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64), - 3, - invoke_context.get_check_aligned(), - ) - .unwrap(); - serialized_data.copy_from_slice(b"baz"); - - for (len, expected) in [ - (9, b"foobarbaz".to_vec()), // > original_data_len, copies from realloc region - (6, b"foobar".to_vec()), // == original_data_len, truncates - (3, b"foo".to_vec()), // < original_data_len, truncates - ] { - *caller_account.ref_to_len_in_vm = len as u64; - update_callee_account(false, &caller_account, callee_account, true).unwrap(); - callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!(callee_account.get_data(), expected); - } - - // close the account - let mut data = Vec::new(); - caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; - let mut owner = system_program::id(); - caller_account.owner = &mut owner; - update_callee_account(false, &caller_account, callee_account, true).unwrap(); - callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!(callee_account.get_data(), b""); - } - #[test] fn test_translate_accounts_rust() { let transaction_accounts = From bd4ad4425b3ad6287c622cab2f240c3dbf5c8e5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:07:49 +0000 Subject: [PATCH 22/27] Removes test_cpi_change_account_data_memory_allocation(). --- programs/sbf/rust/invoke/src/lib.rs | 119 ------------------------ programs/sbf/rust/invoke_dep/src/lib.rs | 1 - programs/sbf/tests/programs.rs | 83 ----------------- 3 files changed, 203 deletions(-) diff --git a/programs/sbf/rust/invoke/src/lib.rs b/programs/sbf/rust/invoke/src/lib.rs index b75cce77aabe66..0c571f544502ee 100644 --- a/programs/sbf/rust/invoke/src/lib.rs +++ b/programs/sbf/rust/invoke/src/lib.rs @@ -1227,125 +1227,6 @@ fn process_instruction<'a>( ) .unwrap(); } - TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION => { - msg!("TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION"); - const CALLEE_PROGRAM_INDEX: usize = 2; - let account = &accounts[ARGUMENT_INDEX]; - let callee_program_id = accounts[CALLEE_PROGRAM_INDEX].key; - let original_data_len = account.data_len(); - - // Initial data is all [0xFF; 20] - assert_eq!(&*account.data.borrow(), &[0xFF; 20]); - - // Realloc to [0xFE; 10] - invoke( - &create_instruction( - *callee_program_id, - &[ - (account.key, true, false), - (callee_program_id, false, false), - ], - vec![0xFE; 10], - ), - accounts, - ) - .unwrap(); - - // Check that [10..20] is zeroed - let new_len = account.data_len(); - assert_eq!(&*account.data.borrow(), &[0xFE; 10]); - assert_eq!( - unsafe { - slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - original_data_len - new_len, - ) - }, - &vec![0; original_data_len - new_len] - ); - - // Realloc to [0xFD; 5] - invoke( - &create_instruction( - *callee_program_id, - &[ - (accounts[ARGUMENT_INDEX].key, true, false), - (callee_program_id, false, false), - ], - vec![0xFD; 5], - ), - accounts, - ) - .unwrap(); - - // Check that [5..20] is zeroed - let new_len = account.data_len(); - assert_eq!(&*account.data.borrow(), &[0xFD; 5]); - assert_eq!( - unsafe { - slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - original_data_len - new_len, - ) - }, - &vec![0; original_data_len - new_len] - ); - - // Realloc to [0xFC; 2] - invoke( - &create_instruction( - *callee_program_id, - &[ - (accounts[ARGUMENT_INDEX].key, true, false), - (callee_program_id, false, false), - ], - vec![0xFC; 2], - ), - accounts, - ) - .unwrap(); - - // Check that [2..20] is zeroed - let new_len = account.data_len(); - assert_eq!(&*account.data.borrow(), &[0xFC; 2]); - assert_eq!( - unsafe { - slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - original_data_len - new_len, - ) - }, - &vec![0; original_data_len - new_len] - ); - - // Realloc to [0xFC; 2]. Here we keep the same length, but realloc the underlying - // vector. CPI must zero even if the length is unchanged. - invoke( - &create_instruction( - *callee_program_id, - &[ - (accounts[ARGUMENT_INDEX].key, true, false), - (callee_program_id, false, false), - ], - vec![0xFC; 2], - ), - accounts, - ) - .unwrap(); - - // Check that [2..20] is zeroed - let new_len = account.data_len(); - assert_eq!(&*account.data.borrow(), &[0xFC; 2]); - assert_eq!( - unsafe { - slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - original_data_len - new_len, - ) - }, - &vec![0; original_data_len - new_len] - ); - } TEST_WRITE_ACCOUNT => { msg!("TEST_WRITE_ACCOUNT"); let target_account_index = instruction_data[1] as usize; diff --git a/programs/sbf/rust/invoke_dep/src/lib.rs b/programs/sbf/rust/invoke_dep/src/lib.rs index 654e724878716f..a5ae2c9922e3df 100644 --- a/programs/sbf/rust/invoke_dep/src/lib.rs +++ b/programs/sbf/rust/invoke_dep/src/lib.rs @@ -38,7 +38,6 @@ pub const TEST_CPI_INVALID_KEY_POINTER: u8 = 35; pub const TEST_CPI_INVALID_OWNER_POINTER: u8 = 36; pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 37; pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 38; -pub const TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION: u8 = 39; pub const TEST_WRITE_ACCOUNT: u8 = 40; pub const TEST_CALLEE_ACCOUNT_UPDATES: u8 = 41; pub const TEST_STACK_HEAP_ZEROED: u8 = 42; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 73ee58e1967eb4..617c8bac16a68f 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -4043,89 +4043,6 @@ fn test_cpi_account_data_updates() { } } -#[test] -#[cfg(feature = "sbf_rust")] -fn test_cpi_change_account_data_memory_allocation() { - use solana_program_runtime::{declare_process_instruction, loaded_programs::ProgramCacheEntry}; - - solana_logger::setup(); - - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config(100_123_456_789); - - let bank = Bank::new_for_tests(&genesis_config); - - declare_process_instruction!(MockBuiltin, 42, |invoke_context| { - let transaction_context = &invoke_context.transaction_context; - let instruction_context = transaction_context.get_current_instruction_context()?; - let instruction_data = instruction_context.get_instruction_data(); - let mut borrowed_account = - instruction_context.try_borrow_instruction_account(transaction_context, 0)?; - - // Test changing the account data both in place and by changing the - // underlying vector. CPI will have to detect the vector change and - // update the corresponding memory region. In all cases CPI will have - // to zero the spare bytes correctly. - match instruction_data[0] { - 0xFE => borrowed_account.set_data(instruction_data.to_vec()), - 0xFD => borrowed_account.set_data_from_slice(instruction_data), - 0xFC => { - // Exercise the update_caller_account capacity check where account len != capacity. - let mut data = instruction_data.to_vec(); - data.reserve_exact(1); - borrowed_account.set_data(data) - } - _ => panic!(), - } - .unwrap(); - - Ok(()) - }); - - let builtin_program_id = Pubkey::new_unique(); - bank.get_transaction_processor().add_builtin( - &bank, - builtin_program_id, - "test_cpi_change_account_data_memory_allocation_builtin", - ProgramCacheEntry::new_builtin(0, 42, MockBuiltin::vm), - ); - - let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); - let mut bank_client = BankClient::new_shared(bank); - let authority_keypair = Keypair::new(); - - let (bank, invoke_program_id) = load_program_of_loader_v4( - &mut bank_client, - &bank_forks, - &mint_keypair, - &authority_keypair, - "solana_sbf_rust_invoke", - ); - - let account_keypair = Keypair::new(); - let mint_pubkey = mint_keypair.pubkey(); - let account_metas = vec![ - AccountMeta::new(mint_pubkey, true), - AccountMeta::new(account_keypair.pubkey(), false), - AccountMeta::new_readonly(builtin_program_id, false), - AccountMeta::new_readonly(invoke_program_id, false), - ]; - - let mut account = AccountSharedData::new(42, 20, &builtin_program_id); - account.set_data(vec![0xFF; 20]); - bank.store_account(&account_keypair.pubkey(), &account); - let mut instruction_data = vec![TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION]; - instruction_data.extend_from_slice(builtin_program_id.as_ref()); - let instruction = - Instruction::new_with_bytes(invoke_program_id, &instruction_data, account_metas.clone()); - - let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); - assert!(result.is_ok(), "{result:?}"); -} - #[test] #[cfg(any(feature = "sbf_c", feature = "sbf_rust"))] fn test_cpi_invalid_account_info_pointers() { From f83e6bc6d5ebf71f1a9e2a3ef5fda4e83cd65a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Sat, 26 Apr 2025 13:57:16 +0000 Subject: [PATCH 23/27] Adjusts tests. --- program-runtime/src/serialization.rs | 6 ++- programs/sbf/c/src/float/float.c | 2 +- .../sbf/rust/deprecated_loader/src/lib.rs | 42 +++++++++++-------- programs/sbf/rust/invoke/src/lib.rs | 40 ++++++++++-------- programs/sbf/tests/programs.rs | 39 +++++++++++------ svm/tests/mock_bank.rs | 4 +- 6 files changed, 79 insertions(+), 54 deletions(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index 3675397fb020fa..ecad5cb7bf6fd1 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -1313,8 +1313,10 @@ mod tests { } fn concat_regions(regions: &[MemoryRegion]) -> AlignedMemory { - let len = regions.iter().fold(0, |len, region| len + region.len) as usize; - let mut mem = AlignedMemory::zero_filled(len); + let last_region = regions.last().unwrap(); + let mut mem = AlignedMemory::zero_filled( + (last_region.vm_addr - MM_INPUT_START + last_region.len) as usize, + ); for region in regions { let host_slice = unsafe { slice::from_raw_parts(region.host_addr as *const u8, region.len as usize) diff --git a/programs/sbf/c/src/float/float.c b/programs/sbf/c/src/float/float.c index d194a8ef65dfca..82d4e0e01e3540 100644 --- a/programs/sbf/c/src/float/float.c +++ b/programs/sbf/c/src/float/float.c @@ -15,7 +15,7 @@ extern uint64_t entrypoint(const uint8_t *input) { return ERROR_INVALID_ARGUMENT; } /* test float conversion to int compiles and works */ - uint32_t data = *(uint32_t *)(params.ka[0].data); + uint32_t data = 0; uint32_t new_data = data + 1; data += 1.5; sol_assert(data == new_data); diff --git a/programs/sbf/rust/deprecated_loader/src/lib.rs b/programs/sbf/rust/deprecated_loader/src/lib.rs index 38a5950614ab30..516bf7f23cb492 100644 --- a/programs/sbf/rust/deprecated_loader/src/lib.rs +++ b/programs/sbf/rust/deprecated_loader/src/lib.rs @@ -144,7 +144,8 @@ fn process_instruction( let realloc_program_id = accounts[REALLOC_PROGRAM_INDEX].key; let realloc_program_owner = accounts[REALLOC_PROGRAM_INDEX].owner; let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; - let new_len = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap()); + let direct_mapping = instruction_data[1]; + let new_len = usize::from_le_bytes(instruction_data[2..10].try_into().unwrap()); let prev_len = account.data_len(); let expected = account.data.borrow()[..new_len].to_vec(); let mut instruction_data = vec![REALLOC, 0]; @@ -165,7 +166,9 @@ fn process_instruction( // deserialize_parameters_unaligned predates realloc support, and // hardcodes the account data length to the original length. - if !solana_sdk_ids::bpf_loader_deprecated::check_id(realloc_program_owner) { + if !solana_program::bpf_loader_deprecated::check_id(realloc_program_owner) + && direct_mapping == 0 + { assert_eq!(&*account.data.borrow(), &expected); assert_eq!( unsafe { @@ -187,7 +190,7 @@ fn process_instruction( let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; let prev_data = { - let data = &instruction_data[9..]; + let data = &instruction_data[10..]; let prev_len = account.data_len(); account.resize(prev_len + data.len())?; account.data.borrow_mut()[prev_len..].copy_from_slice(data); @@ -205,8 +208,9 @@ fn process_instruction( }; let mut expected = account.data.borrow().to_vec(); - let new_len = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap()); - expected.extend_from_slice(&instruction_data[9..]); + let direct_mapping = instruction_data[1]; + let new_len = usize::from_le_bytes(instruction_data[2..10].try_into().unwrap()); + expected.extend_from_slice(&instruction_data[10..]); let mut instruction_data = vec![TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED]; instruction_data.extend_from_slice(&new_len.to_le_bytes()); @@ -224,19 +228,21 @@ fn process_instruction( .unwrap(); assert_eq!(*account.data.borrow(), &prev_data[..new_len]); - assert_eq!( - unsafe { - std::slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - prev_data.len() - new_len, - ) - }, - &vec![0; prev_data.len() - new_len] - ); - assert_eq!( - unsafe { *account.data.borrow().as_ptr().add(prev_data.len()) }, - SENTINEL - ); + if direct_mapping == 0 { + assert_eq!( + unsafe { + std::slice::from_raw_parts( + account.data.borrow().as_ptr().add(new_len), + prev_data.len() - new_len, + ) + }, + &vec![0; prev_data.len() - new_len] + ); + assert_eq!( + unsafe { *account.data.borrow().as_ptr().add(prev_data.len()) }, + SENTINEL + ); + } } Some(&TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED) => { msg!("DEPRECATED LOADER TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED"); diff --git a/programs/sbf/rust/invoke/src/lib.rs b/programs/sbf/rust/invoke/src/lib.rs index 0c571f544502ee..e3164b91d407bc 100644 --- a/programs/sbf/rust/invoke/src/lib.rs +++ b/programs/sbf/rust/invoke/src/lib.rs @@ -1024,7 +1024,8 @@ fn process_instruction<'a>( let realloc_program_id = accounts[REALLOC_PROGRAM_INDEX].key; let realloc_program_owner = accounts[REALLOC_PROGRAM_INDEX].owner; let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; - let new_len = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap()); + let direct_mapping = instruction_data[1]; + let new_len = usize::from_le_bytes(instruction_data[2..10].try_into().unwrap()); let prev_len = account.data_len(); let expected = account.data.borrow()[..new_len].to_vec(); let mut instruction_data = vec![REALLOC, 0]; @@ -1045,7 +1046,7 @@ fn process_instruction<'a>( // deserialize_parameters_unaligned predates realloc support, and // hardcodes the account data length to the original length. - if !bpf_loader_deprecated::check_id(realloc_program_owner) { + if !bpf_loader_deprecated::check_id(realloc_program_owner) && direct_mapping == 0 { assert_eq!(&*account.data.borrow(), &expected); assert_eq!( unsafe { @@ -1066,7 +1067,7 @@ fn process_instruction<'a>( let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; let prev_data = { - let data = &instruction_data[9..]; + let data = &instruction_data[10..]; let prev_len = account.data_len(); account.resize(prev_len + data.len())?; account.data.borrow_mut()[prev_len..].copy_from_slice(data); @@ -1084,8 +1085,9 @@ fn process_instruction<'a>( }; let mut expected = account.data.borrow().to_vec(); - let new_len = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap()); - expected.extend_from_slice(&instruction_data[9..]); + let direct_mapping = instruction_data[1]; + let new_len = usize::from_le_bytes(instruction_data[2..10].try_into().unwrap()); + expected.extend_from_slice(&instruction_data[10..]); let mut instruction_data = vec![TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED]; instruction_data.extend_from_slice(&new_len.to_le_bytes()); @@ -1103,19 +1105,21 @@ fn process_instruction<'a>( .unwrap(); assert_eq!(*account.data.borrow(), &prev_data[..new_len]); - assert_eq!( - unsafe { - slice::from_raw_parts( - account.data.borrow().as_ptr().add(new_len), - prev_data.len() - new_len, - ) - }, - &vec![0; prev_data.len() - new_len] - ); - assert_eq!( - unsafe { *account.data.borrow().as_ptr().add(prev_data.len()) }, - SENTINEL - ); + if direct_mapping == 0 { + assert_eq!( + unsafe { + slice::from_raw_parts( + account.data.borrow().as_ptr().add(new_len), + prev_data.len() - new_len, + ) + }, + &vec![0; prev_data.len() - new_len] + ); + assert_eq!( + unsafe { *account.data.borrow().as_ptr().add(prev_data.len()) }, + SENTINEL + ); + } } TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED => { msg!("TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED"); diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 617c8bac16a68f..5e7f71e5c0aa0c 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3679,7 +3679,7 @@ fn test_cpi_account_ownership_writability() { result.unwrap_err().unwrap(), TransactionError::InstructionError( 0, - InstructionError::ExternalAccountDataModified + InstructionError::ExternalAccountDataModified, ) ); } else { @@ -3939,8 +3939,10 @@ fn test_cpi_account_data_updates() { let mut account = AccountSharedData::new(42, 0, &account_metas[2].pubkey); account.set_data(b"foobar".to_vec()); bank.store_account(&account_keypair.pubkey(), &account); - let mut instruction_data = - vec![TEST_CPI_ACCOUNT_UPDATE_CALLEE_SHRINKS_SMALLER_THAN_ORIGINAL_LEN]; + let mut instruction_data = vec![ + TEST_CPI_ACCOUNT_UPDATE_CALLEE_SHRINKS_SMALLER_THAN_ORIGINAL_LEN, + direct_mapping as u8, + ]; instruction_data.extend_from_slice(4usize.to_le_bytes().as_ref()); let instruction = Instruction::new_with_bytes( account_metas[3].pubkey, @@ -3979,7 +3981,10 @@ fn test_cpi_account_data_updates() { let mut account = AccountSharedData::new(42, 0, &account_metas[3].pubkey); account.set_data(b"foo".to_vec()); bank.store_account(&account_keypair.pubkey(), &account); - let mut instruction_data = vec![TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS]; + let mut instruction_data = vec![ + TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS, + direct_mapping as u8, + ]; // realloc to "foobazbad" then shrink to "foobazb" instruction_data.extend_from_slice(7usize.to_le_bytes().as_ref()); instruction_data.extend_from_slice(b"bazbad"); @@ -4013,7 +4018,10 @@ fn test_cpi_account_data_updates() { let mut account = AccountSharedData::new(42, 0, &account_metas[3].pubkey); account.set_data(b"foo".to_vec()); bank.store_account(&account_keypair.pubkey(), &account); - let mut instruction_data = vec![TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS]; + let mut instruction_data = vec![ + TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS, + direct_mapping as u8, + ]; // realloc to "foobazbad" then shrink to "f" instruction_data.extend_from_slice(1usize.to_le_bytes().as_ref()); instruction_data.extend_from_slice(b"bazbad"); @@ -5134,12 +5142,12 @@ fn test_mem_syscalls_overlap_account_begin_or_end() { let message = Message::new(&[instruction], Some(&mint_pubkey)); let tx = Transaction::new(&[&mint_keypair], message.clone(), bank.last_blockhash()); let (result, _, logs, _) = process_transaction_and_record_inner(&bank, tx); + let last_line = logs.last().unwrap(); if direct_mapping { - assert!(logs.last().unwrap().ends_with(" failed: InvalidLength")); - } else if result.is_err() { - // without direct mapping, we should never get the InvalidLength error - assert!(!logs.last().unwrap().ends_with(" failed: InvalidLength")); + assert!(last_line.contains(" failed: Access violation"), "{logs:?}"); + } else { + assert!(result.is_ok(), "{logs:?}"); } } @@ -5154,14 +5162,19 @@ fn test_mem_syscalls_overlap_account_begin_or_end() { let message = Message::new(&[instruction], Some(&mint_pubkey)); let tx = Transaction::new(&[&mint_keypair], message.clone(), bank.last_blockhash()); let (result, _, logs, _) = process_transaction_and_record_inner(&bank, tx); + let last_line = logs.last().unwrap(); - if direct_mapping && !deprecated { - // we have a resize area + if direct_mapping && (!deprecated || instr < 8) { assert!( - logs.last().unwrap().ends_with(" failed: InvalidLength"), - "{logs:?}" + last_line.contains(" failed: account data too small") + || last_line.contains(" failed: Failed to reallocate account data") + || last_line.contains(" failed: Access violation"), + "{logs:?}", ); } else { + // direct_mapping && deprecated && instr >= 8 succeeds with zero-length accounts + // because there is no MemoryRegion for the account, + // so there can be no error when leaving that non-existent region. assert!(result.is_ok(), "{logs:?}"); } } diff --git a/svm/tests/mock_bank.rs b/svm/tests/mock_bank.rs index ada6d1855340ed..0e15b7003315ff 100644 --- a/svm/tests/mock_bank.rs +++ b/svm/tests/mock_bank.rs @@ -353,7 +353,7 @@ pub fn create_custom_loader<'a>() -> BuiltinProgram> { max_call_depth: compute_budget.max_call_depth, stack_frame_size: compute_budget.stack_frame_size, enable_address_translation: true, - enable_stack_frame_gaps: true, + enable_stack_frame_gaps: false, instruction_meter_checkpoint_distance: 10000, enable_instruction_meter: true, enable_instruction_tracing: true, @@ -363,7 +363,7 @@ pub fn create_custom_loader<'a>() -> BuiltinProgram> { sanitize_user_provided_values: true, enabled_sbpf_versions: SBPFVersion::V0..=SBPFVersion::V3, optimize_rodata: false, - aligned_memory_mapping: true, + aligned_memory_mapping: false, }; // These functions are system calls the compile contract calls during execution, so they From ee6390a31ea0993280326d78cdbd5a456514391c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 May 2025 11:03:08 +0000 Subject: [PATCH 24/27] Adds test_deny_access_beyond_current_length(). --- programs/sbf/rust/invoke/src/lib.rs | 10 ++++ programs/sbf/rust/invoke_dep/src/lib.rs | 1 + programs/sbf/tests/programs.rs | 68 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/programs/sbf/rust/invoke/src/lib.rs b/programs/sbf/rust/invoke/src/lib.rs index e3164b91d407bc..b56f965f36fd4c 100644 --- a/programs/sbf/rust/invoke/src/lib.rs +++ b/programs/sbf/rust/invoke/src/lib.rs @@ -1231,6 +1231,16 @@ fn process_instruction<'a>( ) .unwrap(); } + TEST_READ_ACCOUNT => { + msg!("TEST_READ_ACCOUNT"); + let account_index = instruction_data[1] as usize; + let account = &accounts[account_index]; + let byte_index = usize::from_le_bytes(instruction_data[2..10].try_into().unwrap()); + let data = unsafe { + *(account.data.borrow().get_unchecked(byte_index) as *const u8).cast::() + }; + assert_eq!(data, 0); + } TEST_WRITE_ACCOUNT => { msg!("TEST_WRITE_ACCOUNT"); let target_account_index = instruction_data[1] as usize; diff --git a/programs/sbf/rust/invoke_dep/src/lib.rs b/programs/sbf/rust/invoke_dep/src/lib.rs index a5ae2c9922e3df..089773fcc9a126 100644 --- a/programs/sbf/rust/invoke_dep/src/lib.rs +++ b/programs/sbf/rust/invoke_dep/src/lib.rs @@ -38,6 +38,7 @@ pub const TEST_CPI_INVALID_KEY_POINTER: u8 = 35; pub const TEST_CPI_INVALID_OWNER_POINTER: u8 = 36; pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 37; pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 38; +pub const TEST_READ_ACCOUNT: u8 = 39; pub const TEST_WRITE_ACCOUNT: u8 = 40; pub const TEST_CALLEE_ACCOUNT_UPDATES: u8 = 41; pub const TEST_STACK_HEAP_ZEROED: u8 = 42; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 5e7f71e5c0aa0c..402f2f91184a6a 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -4242,6 +4242,74 @@ fn test_program_sbf_deplete_cost_meter_with_divide_by_zero() { assert_eq!(result.executed_units, u64::from(compute_unit_limit)); } +#[test] +#[cfg(feature = "sbf_rust")] +fn test_deny_access_beyond_current_length() { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(100_123_456_789); + + for direct_mapping in [false, true] { + let mut bank = Bank::new_for_tests(&genesis_config); + let feature_set = Arc::make_mut(&mut bank.feature_set); + // by default test banks have all features enabled, so we only need to + // disable when needed + if !direct_mapping { + feature_set.deactivate(&feature_set::bpf_account_data_direct_mapping::id()); + } + let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); + let mut bank_client = BankClient::new_shared(bank); + let authority_keypair = Keypair::new(); + + let (bank, invoke_program_id) = load_program_of_loader_v4( + &mut bank_client, + &bank_forks, + &mint_keypair, + &authority_keypair, + "solana_sbf_rust_invoke", + ); + let account = AccountSharedData::new(42, 0, &invoke_program_id); + let readonly_account_keypair = Keypair::new(); + let writable_account_keypair = Keypair::new(); + bank.store_account(&readonly_account_keypair.pubkey(), &account); + bank.store_account(&writable_account_keypair.pubkey(), &account); + + let mint_pubkey = mint_keypair.pubkey(); + let account_metas = vec![ + AccountMeta::new(mint_pubkey, true), + AccountMeta::new_readonly(readonly_account_keypair.pubkey(), false), + AccountMeta::new(writable_account_keypair.pubkey(), false), + AccountMeta::new_readonly(invoke_program_id, false), + ]; + + for (instruction_account_index, expected_error) in [ + (1, InstructionError::AccountDataTooSmall), + (2, InstructionError::InvalidRealloc), + ] { + let mut instruction_data = vec![TEST_READ_ACCOUNT, instruction_account_index]; + instruction_data.extend_from_slice(3usize.to_le_bytes().as_ref()); + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &instruction_data, + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + if direct_mapping { + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, expected_error) + ); + } else { + result.unwrap(); + } + } + } +} + #[test] #[cfg(feature = "sbf_rust")] fn test_deny_executable_write() { From cf73421565a96633bd2d8a5debaf4dd63dd5abd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 16 May 2025 16:14:32 +0000 Subject: [PATCH 25/27] Adds test_access_violation_handler(). --- program-runtime/src/serialization.rs | 218 ++++++++++++++++++++++++++- 1 file changed, 217 insertions(+), 1 deletion(-) diff --git a/program-runtime/src/serialization.rs b/program-runtime/src/serialization.rs index ecad5cb7bf6fd1..563b37ad17ff2e 100644 --- a/program-runtime/src/serialization.rs +++ b/program-runtime/src/serialization.rs @@ -598,10 +598,13 @@ mod tests { use { super::*, crate::with_mock_invoke_context, - solana_account::{Account, AccountSharedData, WritableAccount}, + solana_account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, solana_account_info::AccountInfo, solana_program_entrypoint::deserialize, + solana_rent::Rent, + solana_sbpf::{memory_region::MemoryMapping, program::SBPFVersion, vm::Config}, solana_sdk_ids::bpf_loader, + solana_system_interface::MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, solana_transaction_context::InstructionAccount, std::{ cell::RefCell, @@ -1326,4 +1329,217 @@ mod tests { } mem } + + #[test] + fn test_access_violation_handler() { + let program_id = Pubkey::new_unique(); + let shared_account = AccountSharedData::new(0, 4, &program_id); + let mut transaction_context = TransactionContext::new( + vec![ + ( + Pubkey::new_unique(), + AccountSharedData::new(0, 4, &program_id), + ), // readonly + (Pubkey::new_unique(), shared_account.clone()), // writable shared + ( + Pubkey::new_unique(), + AccountSharedData::new(0, 0, &program_id), + ), // another writable account + ( + Pubkey::new_unique(), + AccountSharedData::new( + 0, + MAX_PERMITTED_DATA_LENGTH as usize - 0x100, + &program_id, + ), + ), // almost max sized writable account + ( + Pubkey::new_unique(), + AccountSharedData::new(0, 0, &program_id), + ), // writable dummy to burn accounts_resize_delta + ( + Pubkey::new_unique(), + AccountSharedData::new(0, 0x3000, &program_id), + ), // writable dummy to burn accounts_resize_delta + (program_id, AccountSharedData::default()), // program + ], + Rent::default(), + /* max_instruction_stack_depth */ 1, + /* max_instruction_trace_length */ 1, + ); + let program_indices = [6]; + let transaction_accounts_indexes = [0, 1, 2, 3, 4, 5]; + let instruction_accounts = + deduplicated_instruction_accounts(&transaction_accounts_indexes, |index| index > 0); + let instruction_data = []; + transaction_context + .get_next_instruction_context() + .unwrap() + .configure(&program_indices, &instruction_accounts, &instruction_data); + transaction_context.push().unwrap(); + let instruction_context = transaction_context + .get_current_instruction_context() + .unwrap(); + let account_start_offsets = [ + MM_INPUT_START, + MM_INPUT_START + 4 + MAX_PERMITTED_DATA_INCREASE as u64, + MM_INPUT_START + (4 + MAX_PERMITTED_DATA_INCREASE as u64) * 2, + MM_INPUT_START + (4 + MAX_PERMITTED_DATA_INCREASE as u64) * 3, + ]; + let regions = account_start_offsets + .iter() + .enumerate() + .map(|(index_in_instruction, account_start_offset)| { + create_memory_region_of_account( + &mut instruction_context + .try_borrow_instruction_account( + &transaction_context, + index_in_instruction as IndexOfAccount, + ) + .unwrap(), + *account_start_offset, + ) + .unwrap() + }) + .collect::>(); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let mut memory_mapping = MemoryMapping::new_with_access_violation_handler( + regions, + &config, + SBPFVersion::V3, + transaction_context.access_violation_handler(), + ) + .unwrap(); + + // Reading readonly account is allowed + memory_mapping + .load::(account_start_offsets[0]) + .unwrap(); + + // Reading writable account is allowed + memory_mapping + .load::(account_start_offsets[1]) + .unwrap(); + + // Reading beyond readonly accounts current size is denied + memory_mapping + .load::(account_start_offsets[0] + 4) + .unwrap_err(); + + // Writing to readonly account is denied + memory_mapping + .store::(0, account_start_offsets[0]) + .unwrap_err(); + + // Writing to shared writable account makes it unique (CoW logic) + assert!(transaction_context + .accounts() + .try_borrow(1) + .unwrap() + .is_shared()); + memory_mapping + .store::(0, account_start_offsets[1]) + .unwrap(); + assert!(!transaction_context + .accounts() + .try_borrow(1) + .unwrap() + .is_shared()); + assert_eq!( + transaction_context + .accounts() + .try_borrow(1) + .unwrap() + .data() + .len(), + 4, + ); + + // Reading beyond writable accounts current size grows is denied + memory_mapping + .load::(account_start_offsets[1] + 4) + .unwrap_err(); + + // Writing beyond writable accounts current size grows it + // to original length plus MAX_PERMITTED_DATA_INCREASE + memory_mapping + .store::(0, account_start_offsets[1] + 4) + .unwrap(); + assert_eq!( + transaction_context + .accounts() + .try_borrow(1) + .unwrap() + .data() + .len(), + 4 + MAX_PERMITTED_DATA_INCREASE, + ); + assert!( + transaction_context + .accounts() + .try_borrow(1) + .unwrap() + .data() + .len() + < 0x3000 + ); + + // Writing beyond almost max sized writable accounts current size only grows it + // to MAX_PERMITTED_DATA_LENGTH + memory_mapping + .store::(0, account_start_offsets[3] + MAX_PERMITTED_DATA_LENGTH - 4) + .unwrap(); + assert_eq!( + transaction_context + .accounts() + .try_borrow(3) + .unwrap() + .data() + .len(), + MAX_PERMITTED_DATA_LENGTH as usize, + ); + + // Accessing the rest of the address space reserved for + // the almost max sized writable account is denied + memory_mapping + .load::(account_start_offsets[3] + MAX_PERMITTED_DATA_LENGTH) + .unwrap_err(); + memory_mapping + .store::(0, account_start_offsets[3] + MAX_PERMITTED_DATA_LENGTH) + .unwrap_err(); + + // Burn through most of the accounts_resize_delta budget + let remaining_allowed_growth: usize = 0x700; + for index_in_instruction in 4..6 { + let mut borrowed_account = instruction_context + .try_borrow_instruction_account(&transaction_context, index_in_instruction) + .unwrap(); + borrowed_account + .set_data(vec![0u8; MAX_PERMITTED_DATA_LENGTH as usize]) + .unwrap(); + } + assert_eq!( + transaction_context.accounts_resize_delta().unwrap(), + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION + - remaining_allowed_growth as i64, + ); + + // Writing beyond empty writable accounts current size + // only grows it to fill up MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION + memory_mapping + .store::(0, account_start_offsets[2] + 0x500) + .unwrap(); + assert_eq!( + transaction_context + .accounts() + .try_borrow(2) + .unwrap() + .data() + .len(), + remaining_allowed_growth, + ); + } } From b84fba47d039923da65dda702d14e1a4f97c95f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Mon, 16 Jun 2025 14:36:57 +0000 Subject: [PATCH 26/27] Moves the `post_len > address_space_reserved_for_account` check outside of `if prev_len != post_len`. --- programs/bpf_loader/src/syscalls/cpi.rs | 37 +++++++++++++------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 7db952fccd8c99..d1d5b6619bc8b6 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1214,25 +1214,26 @@ fn update_caller_account( let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); - if prev_len != post_len { - let is_loader_deprecated = !invoke_context.get_check_aligned(); - let address_space_reserved_for_account = if direct_mapping && is_loader_deprecated { - caller_account.original_data_len - } else { - caller_account - .original_data_len - .saturating_add(MAX_PERMITTED_DATA_INCREASE) - }; - if post_len > address_space_reserved_for_account { - let max_increase = - address_space_reserved_for_account.saturating_sub(caller_account.original_data_len); - ic_msg!( - invoke_context, - "Account data size realloc limited to {max_increase} in inner instructions", - ); - return Err(Box::new(InstructionError::InvalidRealloc)); - } + let is_loader_deprecated = !invoke_context.get_check_aligned(); + let address_space_reserved_for_account = if direct_mapping && is_loader_deprecated { + caller_account.original_data_len + } else { + caller_account + .original_data_len + .saturating_add(MAX_PERMITTED_DATA_INCREASE) + }; + + if post_len > address_space_reserved_for_account && (direct_mapping || prev_len != post_len) { + let max_increase = + address_space_reserved_for_account.saturating_sub(caller_account.original_data_len); + ic_msg!( + invoke_context, + "Account data size realloc limited to {max_increase} in inner instructions", + ); + return Err(Box::new(InstructionError::InvalidRealloc)); + } + if prev_len != post_len { // when direct mapping is enabled we don't cache the serialized data in // caller_account.serialized_data. See CallerAccount::from_account_info. if !direct_mapping { From 6f5321ad649b2ecc87f551ce347f489d7a8256a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Mon, 16 Jun 2025 14:42:59 +0000 Subject: [PATCH 27/27] Swaps the execution order of `update_caller_account()` and `update_caller_account_region()`. --- programs/bpf_loader/src/syscalls/cpi.rs | 40 ++++++++++++------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index d1d5b6619bc8b6..f5ed17c877380e 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1062,11 +1062,23 @@ fn cpi_common( .get_feature_set() .bpf_account_data_direct_mapping; + for translate_account in accounts.iter_mut() { + let mut callee_account = instruction_context.try_borrow_instruction_account( + transaction_context, + translate_account.index_in_caller, + )?; + if translate_account.update_caller_account_info { + update_caller_account( + invoke_context, + memory_mapping, + &mut translate_account.caller_account, + &mut callee_account, + direct_mapping, + )?; + } + } + if direct_mapping { - // Update all perms at once before doing account data updates. This - // isn't strictly required as we forbid updates to an account to touch - // other accounts, but since we did have bugs around this in the past, - // it's better to be safe than sorry. for translate_account in accounts.iter() { let mut callee_account = instruction_context.try_borrow_instruction_account( transaction_context, @@ -1083,22 +1095,6 @@ fn cpi_common( } } - for translate_account in accounts.iter_mut() { - let mut callee_account = instruction_context.try_borrow_instruction_account( - transaction_context, - translate_account.index_in_caller, - )?; - if translate_account.update_caller_account_info { - update_caller_account( - invoke_context, - memory_mapping, - &mut translate_account.caller_account, - &mut callee_account, - direct_mapping, - )?; - } - } - invoke_context.execute_time = Some(Measure::start("execute")); Ok(SUCCESS) } @@ -1202,6 +1198,10 @@ fn update_caller_account_region( // // This method updates caller_account so the CPI caller can see the callee's // changes. +// +// Safety: Once `direct_mapping` is enabled all fields of [CallerAccount] used +// in this function should never point inside the address space reserved for +// accounts (regardless of the current size of an account). fn update_caller_account( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping<'_>,