diff --git a/Cargo.lock b/Cargo.lock index 9805614930efd8..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", @@ -10269,9 +10268,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 +11303,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..563b37ad17ff2e 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>), @@ -97,20 +114,19 @@ impl Serializer { self.write_all(account.get_data()); vm_data_addr } else { - self.push_region(true); + self.push_region(); 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.cow_callback_payload = account.get_index_in_transaction() as u32; - } - 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 @@ -128,37 +144,27 @@ impl Serializer { // 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()); } } 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) } @@ -441,10 +447,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; } @@ -553,46 +560,28 @@ 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 + } 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); + 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 - // 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)?; - 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), - _ => {} - } - } - start += MAX_PERMITTED_DATA_INCREASE; - start += alignment_offset; + 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 @@ -609,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, @@ -1324,15 +1316,230 @@ 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.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) } 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, + ); + } } 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/lib.rs b/programs/bpf_loader/src/lib.rs index df4c9297ba1c1a..57159b8e3104bd 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(), )?) } @@ -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 + } + } + }, + )); + } } } } diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index e54a8b6a8849dd..f5ed17c877380e 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,14 +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, - solana_sbpf::{ebpf, memory_region::MemoryRegion}, + solana_program_runtime::{ + invoke_context::SerializedAccountMetadata, serialization::create_memory_region_of_account, + }, + 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; @@ -310,22 +311,14 @@ impl<'a> CallerAccount<'a> { ref_to_len_in_vm, }) } - - fn realloc_region( - &self, - memory_mapping: &'a MemoryMapping<'_>, - is_loader_deprecated: bool, - ) -> Result, Error> { - account_realloc_region( - memory_mapping, - self.vm_data_addr, - self.original_data_len, - is_loader_deprecated, - ) - } } -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 { @@ -341,7 +334,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, @@ -440,7 +433,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, @@ -662,7 +655,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, @@ -790,7 +783,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, @@ -873,21 +866,18 @@ 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, 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, @@ -1072,40 +1062,39 @@ fn cpi_common( .get_feature_set() .bpf_account_data_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 (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)?; - update_caller_account_perms( - memory_mapping, - caller_account, - &mut callee_account, - is_loader_deprecated, - )?; - } - } - } - - 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, )?; } } + if direct_mapping { + 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, + &translate_account.caller_account, + &mut callee_account, + is_loader_deprecated, + )?; + } + } + } + invoke_context.execute_time = Some(Measure::start("execute")); Ok(SUCCESS) } @@ -1122,8 +1111,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<'_>, @@ -1138,38 +1125,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; - } - 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)); + 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 @@ -1192,77 +1161,29 @@ 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<'_>, 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.get() 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, - ) - }, - region.vm_addr, - ) - }; - if writable && shared { - new_region.cow_callback_payload = callee_account.get_index_in_transaction() as u32; - } - 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( - memory_mapping, - *vm_data_addr, - *original_data_len, - is_loader_deprecated, - )? { - 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, - ) - }, - region.vm_addr, - ) - } else { - MemoryRegion::new_readonly( - unsafe { - std::slice::from_raw_parts( - region.host_addr.get() as *const u8, - region.len as usize, - ) - }, - region.vm_addr, - ) - }; + 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)?; } @@ -1277,10 +1198,13 @@ fn update_caller_account_perms( // // 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<'_>, - is_loader_deprecated: bool, caller_account: &mut CallerAccount<'_>, callee_account: &mut BorrowedAccount<'_>, direct_mapping: bool, @@ -1288,132 +1212,41 @@ 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; - } + let prev_len = *caller_account.ref_to_len_in_vm as usize; + let post_len = callee_account.get_data().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 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.get() != callee_ptr { - region.host_addr.set(callee_ptr); - zero_all_mapped_spare_capacity = true; - } - } + 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)); } - 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 - } else { - MAX_PERMITTED_DATA_INCREASE - }; - let data_overflow = post_len - > caller_account - .original_data_len - .saturating_add(max_increase); - if data_overflow { - ic_msg!( - invoke_context, - "Account data size realloc limited to {max_increase} in inner instructions", - ); - 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.replace(true); - defer! { - realloc_region.writable.set(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, @@ -1435,86 +1268,8 @@ fn update_caller_account( *serialized_len_ptr = post_len as u64; } - 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 { - // 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.replace(true); - defer! { - realloc_region.writable.set(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() @@ -1529,44 +1284,6 @@ fn update_caller_account( Ok(()) } -fn account_data_region<'a>( - memory_mapping: &'a MemoryMapping<'_>, - vm_data_addr: u64, - original_data_len: usize, -) -> Result, Error> { - if original_data_len == 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.region(AccessType::Load, vm_data_addr)?; - // 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<'_>, - 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.region(AccessType::Load, realloc_vm_addr)?; - 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); - Ok(Some((realloc_region_index, realloc_region))) -} - #[allow(clippy::indexing_slicing)] #[allow(clippy::arithmetic_side_effects)] #[cfg(test)] @@ -1592,6 +1309,7 @@ mod tests { rc::Rc, slice, }, + test_case::test_matrix, }; macro_rules! mock_invoke_context { @@ -1776,8 +1494,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!( @@ -1820,10 +1538,9 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, - false, + direct_mapping, ) .unwrap(); @@ -1888,7 +1605,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1913,7 +1629,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1930,7 +1645,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1946,7 +1660,6 @@ mod tests { update_caller_account( &invoke_context, &memory_mapping, - false, &mut caller_account, &mut callee_account, false, @@ -1956,253 +1669,8 @@ 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, - false, - &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::(&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::( - &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, - false, - &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, - false, - &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, - false, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - let data_len = callee_account.get_data().len(); - 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); - assert_eq!(callee_account.capacity(), 3); - - update_caller_account( - &invoke_context, - &memory_mapping, - false, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); - - assert_eq!(callee_account.get_data().len(), 3); - assert!(callee_account.capacity() >= caller_account.original_data_len); - let data = translate_slice::( - &memory_mapping, - caller_account.vm_data_addr, - callee_account.get_data().len() as u64, - true, - ) - .unwrap(); - 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(); @@ -2223,17 +1691,6 @@ mod tests { false, ); - 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 caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2241,23 +1698,15 @@ mod tests { *caller_account.lamports = 42; *caller_account.owner = Pubkey::new_unique(); - update_callee_account( - &invoke_context, - &memory_mapping, - 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(); @@ -2279,36 +1728,33 @@ mod tests { false, ); - 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 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( - &invoke_context, - &memory_mapping, - 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(); @@ -2316,21 +1762,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( - &invoke_context, - &memory_mapping, - 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(); @@ -2351,27 +1802,13 @@ mod tests { account.data(), false, ); - - 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 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( - &invoke_context, - &memory_mapping, false, &caller_account, callee_account, @@ -2380,135 +1817,37 @@ 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( - &invoke_context, - &memory_mapping, 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( - &invoke_context, - &memory_mapping, false, &caller_account, callee_account, - true, + direct_mapping, ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::AccountDataSizeChanged ); } - #[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( - &invoke_context, - &memory_mapping, - 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( - &invoke_context, - &memory_mapping, - 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 = @@ -2563,7 +1902,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); } diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index dfc46ed0daf582..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(); @@ -84,47 +78,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()?; - - *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(), - )?; + 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) }; + 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; - } + 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 +124,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: &MemoryMapping, - resize_area: bool, + memory_mapping: &mut 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,964 +172,11 @@ 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: &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), -} - -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, - } - } -} - -fn memset_non_contiguous( - dst_addr: u64, - c: u8, - n: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &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(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, - src_addr: u64, - dst_access: AccessType, - dst_addr: u64, - n_bytes: u64, - accounts: &[SerializedAccountMetadata], - memory_mapping: &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_addr, chunk_len as u64) - .ok_or_else(|| { - EbpfError::AccessViolation(AccessType::Load, src_addr, chunk_len as u64, "") - })?, - dst_region - .vm_to_host(dst_addr, chunk_len as u64) - .ok_or_else(|| { - EbpfError::AccessViolation( - AccessType::Store, - 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 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 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.region(self.access_type, vm_addr) { - Ok((_region_index, region)) => Ok(region), - 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 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::>() - } - - #[test] - #[should_panic(expected = "AccessViolation")] - fn test_memory_chunk_iterator_no_regions() { - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new(vec![], &config, SBPFVersion::V3).unwrap(); - - let mut src_chunk_iter = - MemoryChunkIterator::new(&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 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(); - 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 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( - &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( - &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( - &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( - &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 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( - &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( - &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( - &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 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( - &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 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, - &[], - &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, - &[], - &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 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, - &[], - &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, - &[], - &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 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, &[], &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 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, &[], &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 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, - &[], - &memory_mapping, - true - ) - .unwrap(), - 0 - ); - - // non contiguous dst - assert_eq!( - memcmp_non_contiguous( - MM_RODATA_START + 10, - MM_RODATA_START + 1, - 8, - &[], - &memory_mapping, - true - ) - .unwrap(), - 0 - ); - - // diff - assert_eq!( - memcmp_non_contiguous( - MM_RODATA_START + 1, - MM_RODATA_START + 11, - 5, - &[], - &memory_mapping, - true - ) - .unwrap(), - unsafe { memcmp(b"oobar", b"obarb", 5) } - ); - } - - 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 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/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() + ) } } } diff --git a/programs/sbf/Cargo.lock b/programs/sbf/Cargo.lock index 25eee996c4d888..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", @@ -8690,9 +8689,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 +9480,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/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 b75cce77aabe66..b56f965f36fd4c 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"); @@ -1227,124 +1231,15 @@ 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_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"); diff --git a/programs/sbf/rust/invoke_dep/src/lib.rs b/programs/sbf/rust/invoke_dep/src/lib.rs index 654e724878716f..089773fcc9a126 100644 --- a/programs/sbf/rust/invoke_dep/src/lib.rs +++ b/programs/sbf/rust/invoke_dep/src/lib.rs @@ -38,7 +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_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION: u8 = 39; +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 73ee58e1967eb4..402f2f91184a6a 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"); @@ -4043,89 +4051,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() { @@ -4317,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() { @@ -5217,12 +5210,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:?}"); } } @@ -5237,14 +5230,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/examples/Cargo.lock b/svm/examples/Cargo.lock index 5e15ae32107fdf..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", @@ -7741,9 +7740,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 +8566,7 @@ dependencies = [ "solana-instructions-sysvar", "solana-pubkey", "solana-rent", + "solana-sbpf", "solana-sdk-ids", ] 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 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..1b5368258b4f85 100644 --- a/transaction-context/src/lib.rs +++ b/transaction-context/src/lib.rs @@ -3,12 +3,13 @@ #![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, solana_instructions_sysvar as instructions, solana_pubkey::Pubkey, + solana_sbpf::memory_region::{AccessType, AccessViolationHandler, MemoryRegion}, std::{ cell::{Ref, RefCell, RefMut}, collections::HashSet, @@ -30,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, @@ -484,29 +488,79 @@ 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| { + 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 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 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; + }, + ) } } @@ -905,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 @@ -987,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