From eccc3e9eda067f3d7d21563095acdf927b0fdc94 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 23 Sep 2022 21:42:54 +0000 Subject: [PATCH 01/44] AccountSharedData: make data_mut() private This ensures that the inner Vec is never handed out. This is in preparation of enforcing that the capacity of the inner vec never shrinks, which is required for direct mapping. --- sdk/src/account.rs | 19 ++++++++++++------- sdk/src/transaction_context.rs | 4 ++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/sdk/src/account.rs b/sdk/src/account.rs index fca8f8b71f5165..4a2710739e854b 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -174,7 +174,6 @@ pub trait WritableAccount: ReadableAccount { fn saturating_sub_lamports(&mut self, lamports: u64) { self.set_lamports(self.lamports().saturating_sub(lamports)) } - fn data_mut(&mut self) -> &mut Vec; fn data_as_mut_slice(&mut self) -> &mut [u8]; fn set_owner(&mut self, owner: Pubkey); fn copy_into_owner_from_slice(&mut self, source: &[u8]); @@ -228,9 +227,6 @@ impl WritableAccount for Account { fn set_lamports(&mut self, lamports: u64) { self.lamports = lamports; } - fn data_mut(&mut self) -> &mut Vec { - &mut self.data - } fn data_as_mut_slice(&mut self) -> &mut [u8] { &mut self.data } @@ -267,9 +263,6 @@ impl WritableAccount for AccountSharedData { fn set_lamports(&mut self, lamports: u64) { self.lamports = lamports; } - fn data_mut(&mut self) -> &mut Vec { - Arc::make_mut(&mut self.data) - } fn data_as_mut_slice(&mut self) -> &mut [u8] { &mut self.data_mut()[..] } @@ -540,6 +533,18 @@ impl Account { } impl AccountSharedData { + fn data_mut(&mut self) -> &mut Vec { + Arc::make_mut(&mut self.data) + } + + pub fn resize(&mut self, new_len: usize, value: u8) { + self.data_mut().resize(new_len, value) + } + + pub fn extend_from_slice(&mut self, data: &[u8]) { + self.data_mut().extend_from_slice(data) + } + pub fn set_data_from_slice(&mut self, new_data: &[u8]) { let data = match Arc::get_mut(&mut self.data) { // The buffer isn't shared, so we're going to memcpy in place. diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 023607374ed9bc..8f9b165549ddc9 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -832,7 +832,7 @@ impl<'a> BorrowedAccount<'a> { } self.touch()?; self.update_accounts_resize_delta(new_length)?; - self.account.data_mut().resize(new_length, 0); + self.account.resize(new_length, 0); Ok(()) } @@ -849,7 +849,7 @@ impl<'a> BorrowedAccount<'a> { self.touch()?; self.update_accounts_resize_delta(new_len)?; - self.account.data_mut().extend_from_slice(data); + self.account.extend_from_slice(data); Ok(()) } From bd79f9abf924022740817f1111c3a77f62f08cd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 8 Sep 2022 16:37:40 +0200 Subject: [PATCH 02/44] Adds the feature bpf_account_data_direct_mapping. --- sdk/src/feature_set.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 25dfacec6e8e5f..37b1e4dfb704b0 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -617,6 +617,9 @@ pub mod delay_visibility_of_program_deployment { pub mod apply_cost_tracker_during_replay { solana_sdk::declare_id!("2ry7ygxiYURULZCrypHhveanvP5tzZ4toRwVp89oCNSj"); } +pub mod bpf_account_data_direct_mapping { + solana_sdk::declare_id!("9gwzizfABsKUereT6phZZxbTzuAnovkgwpVVpdcSxv9h"); +} pub mod add_set_tx_loaded_accounts_data_size_instruction { solana_sdk::declare_id!("G6vbf1UBok8MWb8m25ex86aoQHeKTzDKzuZADHkShqm6"); @@ -822,6 +825,7 @@ lazy_static! { (clean_up_delegation_errors::id(), "Return InsufficientDelegation instead of InsufficientFunds or InsufficientStake where applicable #31206"), (vote_state_add_vote_latency::id(), "replace Lockout with LandedVote (including vote latency) in vote state #31264"), (checked_arithmetic_in_fee_validation::id(), "checked arithmetic in fee validation #31273"), + (bpf_account_data_direct_mapping::id(), "use memory regions to map account data into the rbpf vm instead of copying the data"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From 4f8ecd5acc741a70a3f9371dc55ac1d25d989d99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 26 Jul 2022 23:04:57 +0200 Subject: [PATCH 03/44] Remaps EbpfError::AccessViolation into InstructionError::ReadonlyDataModified. --- programs/bpf_loader/src/lib.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index cf4f841cadf89c..549e136871c33f 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -18,9 +18,10 @@ use { }, solana_rbpf::{ aligned_memory::AlignedMemory, - ebpf::{self, HOST_ALIGN, MM_HEAP_START}, + ebpf::{self, HOST_ALIGN, MM_HEAP_START, MM_INPUT_START}, elf::Executable, - memory_region::{MemoryCowCallback, MemoryMapping, MemoryRegion}, + error::EbpfError, + memory_region::{AccessType, MemoryCowCallback, MemoryMapping, MemoryRegion}, verifier::RequisiteVerifier, vm::{ContextObject, EbpfVm, ProgramResult, VerifiedExecutable}, }, @@ -1597,7 +1598,21 @@ fn execute<'a, 'b: 'a>( }; Err(Box::new(error) as Box) } - ProgramResult::Err(error) => Err(error), + ProgramResult::Err(error) => { + let error = match error.downcast_ref() { + Some(EbpfError::AccessViolation( + _pc, + AccessType::Store, + address, + _size, + _section_name, + )) if *address >= MM_INPUT_START => { + InstructionError::ReadonlyDataModified.into() + } + _ => error, + }; + Err(error) + } _ => Ok(()), } }; From c494dc37b79d64b2dd9a6f53592fa08ed33774d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 7 Sep 2022 09:54:38 +0200 Subject: [PATCH 04/44] WIP: Memory regions for each instruction account in create_vm(). --- program-test/src/lib.rs | 3 +- programs/bpf_loader/benches/serialization.rs | 12 +- programs/bpf_loader/src/lib.rs | 25 +- programs/bpf_loader/src/serialization.rs | 116 +++++-- programs/bpf_loader/src/syscalls/cpi.rs | 339 +++++++++++++++---- programs/sbf/benches/bpf_loader.rs | 2 + programs/sbf/tests/programs.rs | 2 +- 7 files changed, 380 insertions(+), 119 deletions(-) diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 87369ab9a663c7..f93088f0d80f65 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -123,7 +123,8 @@ pub fn builtin_process_instruction( invoke_context .transaction_context .get_current_instruction_context()?, - true, + true, // should_cap_ix_accounts + true, // copy_account_data // There is no VM so direct mapping can not be implemented here )?; // Deserialize data back into instruction params diff --git a/programs/bpf_loader/benches/serialization.rs b/programs/bpf_loader/benches/serialization.rs index e14f55476ca542..5f14524a1bbb7c 100644 --- a/programs/bpf_loader/benches/serialization.rs +++ b/programs/bpf_loader/benches/serialization.rs @@ -126,7 +126,8 @@ fn bench_serialize_unaligned(bencher: &mut Bencher) { .get_current_instruction_context() .unwrap(); bencher.iter(|| { - let _ = serialize_parameters(&transaction_context, instruction_context, true).unwrap(); + let _ = + serialize_parameters(&transaction_context, instruction_context, true, false).unwrap(); }); } @@ -138,7 +139,8 @@ fn bench_serialize_aligned(bencher: &mut Bencher) { .unwrap(); bencher.iter(|| { - let _ = serialize_parameters(&transaction_context, instruction_context, true).unwrap(); + let _ = + serialize_parameters(&transaction_context, instruction_context, true, false).unwrap(); }); } @@ -149,7 +151,8 @@ fn bench_serialize_unaligned_max_accounts(bencher: &mut Bencher) { .get_current_instruction_context() .unwrap(); bencher.iter(|| { - let _ = serialize_parameters(&transaction_context, instruction_context, true).unwrap(); + let _ = + serialize_parameters(&transaction_context, instruction_context, true, false).unwrap(); }); } @@ -161,6 +164,7 @@ fn bench_serialize_aligned_max_accounts(bencher: &mut Bencher) { .unwrap(); bencher.iter(|| { - let _ = serialize_parameters(&transaction_context, instruction_context, true).unwrap(); + let _ = + serialize_parameters(&transaction_context, instruction_context, true, false).unwrap(); }); } diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 549e136871c33f..030ede79da7027 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -31,11 +31,12 @@ use { clock::Slot, entrypoint::SUCCESS, feature_set::{ - cap_accounts_data_allocations_per_transaction, cap_bpf_program_instruction_accounts, - delay_visibility_of_program_deployment, disable_deploy_of_alloc_free_syscall, - enable_bpf_loader_extend_program_ix, enable_bpf_loader_set_authority_checked_ix, - enable_program_redeployment_cooldown, limit_max_instruction_trace_length, - native_programs_consume_cu, remove_bpf_loader_incorrect_program_id, FeatureSet, + bpf_account_data_direct_mapping, cap_accounts_data_allocations_per_transaction, + cap_bpf_program_instruction_accounts, delay_visibility_of_program_deployment, + disable_deploy_of_alloc_free_syscall, enable_bpf_loader_extend_program_ix, + enable_bpf_loader_set_authority_checked_ix, enable_program_redeployment_cooldown, + limit_max_instruction_trace_length, native_programs_consume_cu, + remove_bpf_loader_incorrect_program_id, FeatureSet, }, instruction::{AccountMeta, InstructionError}, loader_instruction::LoaderInstruction, @@ -1525,6 +1526,9 @@ fn execute<'a, 'b: 'a>( let use_jit = false; #[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))] let use_jit = executable.get_executable().get_compiled_program().is_some(); + let bpf_account_data_direct_mapping = invoke_context + .feature_set + .is_active(&bpf_account_data_direct_mapping::id()); let mut serialize_time = Measure::start("serialize"); let (parameter_bytes, regions, account_lengths) = serialization::serialize_parameters( @@ -1533,6 +1537,7 @@ fn execute<'a, 'b: 'a>( invoke_context .feature_set .is_active(&cap_bpf_program_instruction_accounts::ID), + !bpf_account_data_direct_mapping, )?; serialize_time.stop(); @@ -1621,12 +1626,14 @@ fn execute<'a, 'b: 'a>( fn deserialize_parameters( invoke_context: &mut InvokeContext, parameter_bytes: &[u8], + copy_account_data: bool, ) -> Result<(), InstructionError> { serialization::deserialize_parameters( invoke_context.transaction_context, invoke_context .transaction_context .get_current_instruction_context()?, + copy_account_data, parameter_bytes, &invoke_context.get_syscall_context()?.orig_account_lengths, ) @@ -1634,8 +1641,12 @@ fn execute<'a, 'b: 'a>( let mut deserialize_time = Measure::start("deserialize"); let execute_or_deserialize_result = execution_result.and_then(|_| { - deserialize_parameters(invoke_context, parameter_bytes.as_slice()) - .map_err(|error| Box::new(error) as Box) + deserialize_parameters( + invoke_context, + parameter_bytes.as_slice(), + !bpf_account_data_direct_mapping, + ) + .map_err(|error| Box::new(error) as Box) }); deserialize_time.stop(); diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 6b595e3c0708c2..08d575d71e82b7 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -123,6 +123,7 @@ pub fn serialize_parameters( transaction_context: &TransactionContext, instruction_context: &InstructionContext, should_cap_ix_accounts: bool, + copy_account_data: bool, ) -> Result<(AlignedMemory, Vec, Vec), InstructionError> { let num_ix_accounts = instruction_context.get_number_of_instruction_accounts(); if should_cap_ix_accounts && num_ix_accounts > MAX_INSTRUCTION_ACCOUNTS as IndexOfAccount { @@ -156,9 +157,19 @@ pub fn serialize_parameters( } if is_loader_deprecated { - serialize_parameters_unaligned(transaction_context, instruction_context, accounts) + serialize_parameters_unaligned( + transaction_context, + instruction_context, + accounts, + copy_account_data, + ) } else { - serialize_parameters_aligned(transaction_context, instruction_context, accounts) + serialize_parameters_aligned( + transaction_context, + instruction_context, + accounts, + copy_account_data, + ) } .map(|(buffer, regions)| (buffer, regions, account_lengths)) } @@ -166,6 +177,7 @@ pub fn serialize_parameters( pub fn deserialize_parameters( transaction_context: &TransactionContext, instruction_context: &InstructionContext, + copy_account_data: bool, buffer: &[u8], account_lengths: &[usize], ) -> Result<(), InstructionError> { @@ -177,6 +189,7 @@ pub fn deserialize_parameters( deserialize_parameters_unaligned( transaction_context, instruction_context, + copy_account_data, buffer, account_lengths, ) @@ -184,6 +197,7 @@ pub fn deserialize_parameters( deserialize_parameters_aligned( transaction_context, instruction_context, + copy_account_data, buffer, account_lengths, ) @@ -194,6 +208,7 @@ fn serialize_parameters_unaligned( transaction_context: &TransactionContext, instruction_context: &InstructionContext, accounts: Vec, + copy_account_data: bool, ) -> Result<(AlignedMemory, Vec), InstructionError> { // Calculate size in order to alloc once let mut size = size_of::(); @@ -207,10 +222,12 @@ fn serialize_parameters_unaligned( + size_of::() // key + size_of::() // lamports + size_of::() // data len - + account.get_data().len() // data + size_of::() // owner + size_of::() // executable + size_of::(); // rent_epoch + if copy_account_data { + size += account.get_data().len(); + } } } } @@ -254,6 +271,7 @@ fn serialize_parameters_unaligned( pub fn deserialize_parameters_unaligned( transaction_context: &TransactionContext, instruction_context: &InstructionContext, + copy_account_data: bool, buffer: &[u8], account_lengths: &[usize], ) -> Result<(), InstructionError> { @@ -280,20 +298,22 @@ pub fn deserialize_parameters_unaligned( } start += size_of::() // lamports + size_of::(); // data length - let data = buffer - .get(start..start + pre_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(data.len()) - .and_then(|_| borrowed_account.can_data_be_changed()) - { - Ok(()) => borrowed_account.set_data_from_slice(data)?, - Err(err) if borrowed_account.get_data() != data => return Err(err), - _ => {} + if copy_account_data { + let data = buffer + .get(start..start + pre_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(data.len()) + .and_then(|_| borrowed_account.can_data_be_changed()) + { + Ok(()) => borrowed_account.set_data_from_slice(data)?, + Err(err) if borrowed_account.get_data() != data => return Err(err), + _ => {} + } + start += pre_len; // data } - start += pre_len // data - + size_of::() // owner + start += size_of::() // owner + size_of::() // executable + size_of::(); // rent_epoch } @@ -305,6 +325,7 @@ fn serialize_parameters_aligned( transaction_context: &TransactionContext, instruction_context: &InstructionContext, accounts: Vec, + copy_account_data: bool, ) -> Result<(AlignedMemory, Vec), InstructionError> { // Calculate size in order to alloc once let mut size = size_of::(); @@ -322,10 +343,13 @@ fn serialize_parameters_aligned( + size_of::() // owner + size_of::() // lamports + size_of::() // data len - + data_len + MAX_PERMITTED_DATA_INCREASE - + (data_len as *const u8).align_offset(BPF_ALIGN_OF_U128) + size_of::(); // rent epoch + if copy_account_data { + size += data_len + (data_len as *const u8).align_offset(BPF_ALIGN_OF_U128); + } else { + size += BPF_ALIGN_OF_U128; + } } } } @@ -374,6 +398,7 @@ fn serialize_parameters_aligned( pub fn deserialize_parameters_aligned( transaction_context: &TransactionContext, instruction_context: &InstructionContext, + copy_account_data: bool, buffer: &[u8], account_lengths: &[usize], ) -> Result<(), InstructionError> { @@ -418,21 +443,45 @@ pub fn deserialize_parameters_aligned( { return Err(InstructionError::InvalidRealloc); } - let data_end = start + post_len; - let data = buffer - .get(start..data_end) - .ok_or(InstructionError::InvalidArgument)?; // The redundant check helps to avoid the expensive data comparison if we can - match borrowed_account - .can_data_be_resized(data.len()) - .and_then(|_| borrowed_account.can_data_be_changed()) - { - Ok(()) => borrowed_account.set_data_from_slice(data)?, - Err(err) if borrowed_account.get_data() != data => return Err(err), - _ => {} + 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)?; + match borrowed_account + .can_data_be_resized(post_len) + .and_then(|_| borrowed_account.can_data_be_changed()) + { + Ok(()) => borrowed_account.set_data_from_slice(data)?, + Err(err) if borrowed_account.get_data() != data => return Err(err), + _ => {} + } + start += *pre_len; // data + } else { + 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) + .and_then(|_| borrowed_account.can_data_be_changed()) + { + 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()? + [*pre_len..pre_len.saturating_add(allocated_bytes)] + .copy_from_slice(&data[0..allocated_bytes]); + } + } + Err(err) if borrowed_account.get_data().len() != post_len => return Err(err), + _ => {} + } } - start += *pre_len + MAX_PERMITTED_DATA_INCREASE; // data - start += (start as *const u8).align_offset(BPF_ALIGN_OF_U128); + start += MAX_PERMITTED_DATA_INCREASE; + start += alignment_offset; 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 @@ -579,6 +628,7 @@ mod tests { &transaction_context, instruction_context, should_cap_ix_accounts, + true, ); assert_eq!( serialization_result.as_ref().err(), @@ -720,6 +770,7 @@ mod tests { invoke_context.transaction_context, instruction_context, true, + true, ) .unwrap(); @@ -765,6 +816,7 @@ mod tests { deserialize_parameters( invoke_context.transaction_context, instruction_context, + true, serialized.as_slice(), &account_lengths, ) @@ -796,6 +848,7 @@ mod tests { invoke_context.transaction_context, instruction_context, true, + true, ) .unwrap(); @@ -824,6 +877,7 @@ mod tests { deserialize_parameters( invoke_context.transaction_context, instruction_context, + true, serialized.as_slice(), &account_lengths, ) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index d9b5d29bfc4793..1f6af25e62f5fd 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,6 +1,7 @@ use { super::*, crate::declare_syscall, + solana_rbpf::memory_region::MemoryRegion, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -20,7 +21,12 @@ struct CallerAccount<'a> { lamports: &'a mut u64, owner: &'a mut Pubkey, original_data_len: usize, - data: &'a mut [u8], + // After the activation of bpf_account_data_direct_mapping this + // no longer holds the account data followed by the reallocation padding, + // but only the reallocation padding. That is because after the activation + // we already share the account data between runtime and program and mapping it + // forward and backward would create aliasing mutable references. + account_data_or_only_realloc_padding: &'a mut [u8], // Given the corresponding input AccountInfo::data, vm_data_addr points to // the pointer field and ref_to_len_in_vm points to the length field. vm_data_addr: u64, @@ -36,6 +42,7 @@ impl<'a> CallerAccount<'a> { fn from_account_info( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping, + is_loader_deprecated: bool, _vm_addr: u64, account_info: &AccountInfo, original_data_len: usize, @@ -58,7 +65,12 @@ impl<'a> CallerAccount<'a> { invoke_context.get_check_aligned(), )?; - let (data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr) = { + let ( + account_data_or_only_realloc_padding, + vm_data_addr, + ref_to_len_in_vm, + serialized_len_ptr, + ) = { // Double translate data out of RefCell let data = *translate_type::<&[u8]>( memory_mapping, @@ -95,14 +107,32 @@ impl<'a> CallerAccount<'a> { )? }; let vm_data_addr = data.as_ptr() as u64; + + let bpf_account_data_direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + let account_data_or_only_realloc_padding = translate_slice_mut::( + memory_mapping, + if bpf_account_data_direct_mapping { + vm_data_addr + original_data_len as u64 + } else { + vm_data_addr + }, + if bpf_account_data_direct_mapping { + if is_loader_deprecated { + 0 + } else { + MAX_PERMITTED_DATA_INCREASE + } + } else { + data.len() + } as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + ( - translate_slice_mut::( - memory_mapping, - vm_data_addr, - data.len() as u64, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?, + account_data_or_only_realloc_padding, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -113,7 +143,7 @@ impl<'a> CallerAccount<'a> { lamports, owner, original_data_len, - data, + account_data_or_only_realloc_padding, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -126,6 +156,7 @@ impl<'a> CallerAccount<'a> { fn from_sol_account_info( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping, + is_loader_deprecated: bool, vm_addr: u64, account_info: &SolAccountInfo, original_data_len: usize, @@ -152,10 +183,25 @@ impl<'a> CallerAccount<'a> { .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit), )?; - let data = translate_slice_mut::( + let bpf_account_data_direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + let account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, - vm_data_addr, - account_info.data_len, + if bpf_account_data_direct_mapping { + vm_data_addr + original_data_len as u64 + } else { + vm_data_addr + }, + if bpf_account_data_direct_mapping { + if is_loader_deprecated { + 0 + } else { + MAX_PERMITTED_DATA_INCREASE as u64 + } + } else { + account_info.data_len + }, invoke_context.get_check_aligned(), invoke_context.get_check_size(), )?; @@ -194,7 +240,7 @@ impl<'a> CallerAccount<'a> { lamports, owner, original_data_len, - data, + account_data_or_only_realloc_padding, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -218,6 +264,7 @@ trait SyscallInvokeSigned { program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, + is_loader_deprecated: bool, memory_mapping: &mut MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result, Error>; @@ -310,6 +357,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, + is_loader_deprecated: bool, memory_mapping: &mut MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result, Error> { @@ -327,6 +375,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { &account_info_keys, account_infos, account_infos_addr, + is_loader_deprecated, invoke_context, memory_mapping, CallerAccount::from_account_info, @@ -541,6 +590,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, + is_loader_deprecated: bool, memory_mapping: &mut MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result, Error> { @@ -558,6 +608,7 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { &account_info_keys, account_infos, account_infos_addr, + is_loader_deprecated, invoke_context, memory_mapping, CallerAccount::from_sol_account_info, @@ -657,12 +708,13 @@ fn translate_and_update_accounts<'a, T, F>( account_info_keys: &[&Pubkey], account_infos: &[T], account_infos_addr: u64, + is_loader_deprecated: bool, invoke_context: &mut InvokeContext, memory_mapping: &MemoryMapping, do_translate: F, ) -> Result, Error> where - F: Fn(&InvokeContext, &MemoryMapping, u64, &T, usize) -> Result, Error>, + F: Fn(&InvokeContext, &MemoryMapping, bool, u64, &T, usize) -> Result, Error>, { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -722,6 +774,7 @@ where do_translate( invoke_context, memory_mapping, + is_loader_deprecated, account_infos_addr.saturating_add( caller_account_index.saturating_mul(mem::size_of::()) as u64, ), @@ -735,7 +788,12 @@ where // account (caller_account). We need to update the corresponding // BorrowedAccount (callee_account) so the callee can see the // changes. - update_callee_account(invoke_context, &caller_account, callee_account)?; + update_callee_account( + invoke_context, + is_loader_deprecated, + &caller_account, + callee_account, + )?; let caller_account = if instruction_account.is_writable { Some(caller_account) @@ -887,14 +945,20 @@ fn cpi_common( memory_mapping, invoke_context, )?; + let is_loader_deprecated = *instruction_context + .try_borrow_last_program_account(transaction_context)? + .get_owner() + == bpf_loader_deprecated::id(); let (instruction_accounts, program_indices) = invoke_context.prepare_instruction(&instruction, &signers)?; check_authorized_program(&instruction.program_id, &instruction.data, invoke_context)?; + let mut accounts = S::translate_accounts( &instruction_accounts, &program_indices, account_infos_addr, account_infos_len, + is_loader_deprecated, memory_mapping, invoke_context, )?; @@ -918,13 +982,14 @@ fn cpi_common( // Synchronize the callee's account changes so the caller can see them. for (index_in_caller, caller_account) in accounts.iter_mut() { if let Some(caller_account) = caller_account { - let callee_account = instruction_context + let mut callee_account = instruction_context .try_borrow_instruction_account(transaction_context, *index_in_caller)?; update_caller_account( invoke_context, memory_mapping, + is_loader_deprecated, caller_account, - &callee_account, + &mut callee_account, )?; } } @@ -942,27 +1007,60 @@ fn cpi_common( // changes. fn update_callee_account( invoke_context: &InvokeContext, + is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, ) -> Result<(), Error> { let is_disable_cpi_setting_executable_and_rent_epoch_active = invoke_context .feature_set .is_active(&disable_cpi_setting_executable_and_rent_epoch::id()); + let bpf_account_data_direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); if callee_account.get_lamports() != *caller_account.lamports { callee_account.set_lamports(*caller_account.lamports)?; } - // The redundant check helps to avoid the expensive data comparison if we can - match callee_account - .can_data_be_resized(caller_account.data.len()) - .and_then(|_| callee_account.can_data_be_changed()) - { - Ok(()) => callee_account.set_data_from_slice(caller_account.data)?, - Err(err) if callee_account.get_data() != caller_account.data => { - return Err(Box::new(err)); + if bpf_account_data_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) + .and_then(|_| callee_account.can_data_be_changed()) + { + Ok(()) => { + callee_account.set_data_length(post_len)?; + let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); + if !is_loader_deprecated && realloc_bytes_used > 0 { + callee_account.get_data_mut()?[caller_account.original_data_len..post_len] + .copy_from_slice( + &caller_account.account_data_or_only_realloc_padding + [0..realloc_bytes_used], + ); + } + } + Err(err) if prev_len != post_len => { + return Err(Box::new(err)); + } + _ => {} + } + } else { + // The redundant check helps to avoid the expensive data comparison if we can + match callee_account + .can_data_be_resized(caller_account.account_data_or_only_realloc_padding.len()) + .and_then(|_| callee_account.can_data_be_changed()) + { + Ok(()) => callee_account + .set_data_from_slice(&caller_account.account_data_or_only_realloc_padding)?, + Err(err) + if callee_account.get_data() + != caller_account.account_data_or_only_realloc_padding => + { + return Err(Box::new(err)); + } + _ => {} } - _ => {} } if !is_disable_cpi_setting_executable_and_rent_epoch_active @@ -1011,15 +1109,47 @@ fn update_callee_account( // changes. fn update_caller_account( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &mut MemoryMapping, + is_loader_deprecated: bool, caller_account: &mut CallerAccount, - callee_account: &BorrowedAccount<'_>, + callee_account: &mut BorrowedAccount<'_>, ) -> Result<(), Error> { + let bpf_account_data_direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - let new_len = callee_account.get_data().len(); - if caller_account.data.len() != new_len { - let data_overflow = new_len + + if bpf_account_data_direct_mapping && caller_account.original_data_len > 0 { + let regions = memory_mapping.get_regions(); + let memory_region_index = regions + .iter() + .position(|memory_region| memory_region.vm_addr == caller_account.vm_data_addr) + .ok_or(Box::new(InstructionError::ProgramEnvironmentSetupFailure))?; + let account_region = if let Ok(data) = callee_account.get_data_mut() { + let data = unsafe { + std::slice::from_raw_parts_mut( + data.as_mut_ptr(), + regions[memory_region_index].len as usize, + ) + }; + MemoryRegion::new_writable(data, caller_account.vm_data_addr) + } else { + let data = unsafe { + std::slice::from_raw_parts( + callee_account.get_data().as_ptr(), + regions[memory_region_index].len as usize, + ) + }; + MemoryRegion::new_readonly(data, caller_account.vm_data_addr) + }; + memory_mapping.replace_region(memory_region_index, account_region)?; + } + 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 data_overflow = post_len > caller_account .original_data_len .saturating_add(MAX_PERMITTED_DATA_INCREASE); @@ -1031,22 +1161,44 @@ fn update_caller_account( ); return Err(Box::new(InstructionError::InvalidRealloc)); } - if new_len < caller_account.data.len() { - caller_account - .data - .get_mut(new_len..) - .ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))? - .fill(0); + if post_len < prev_len { + if bpf_account_data_direct_mapping { + unsafe { + std::slice::from_raw_parts_mut( + callee_account.get_data_mut()?.as_mut_ptr().add(post_len), + prev_len.saturating_sub(post_len), + ) + .fill(0); + } + } else { + caller_account + .account_data_or_only_realloc_padding + .get_mut(post_len..) + .ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))? + .fill(0); + } } - caller_account.data = translate_slice_mut::( + caller_account.account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, - caller_account.vm_data_addr, - new_len as u64, + if bpf_account_data_direct_mapping { + caller_account.vm_data_addr + caller_account.original_data_len as u64 + } else { + caller_account.vm_data_addr + }, + if bpf_account_data_direct_mapping { + if is_loader_deprecated { + 0 + } else { + MAX_PERMITTED_DATA_INCREASE + } + } else { + post_len + } as u64, false, // Don't care since it is byte aligned invoke_context.get_check_size(), )?; // this is the len field in the AccountInfo::data slice - *caller_account.ref_to_len_in_vm = new_len as u64; + *caller_account.ref_to_len_in_vm = post_len as u64; // this is the len field in the serialized parameters if invoke_context @@ -1060,22 +1212,33 @@ fn update_caller_account( .saturating_sub(std::mem::size_of::() as u64), invoke_context.get_check_aligned(), )?; - *serialized_len_ptr = new_len as u64; + *serialized_len_ptr = post_len as u64; } else { unsafe { - *caller_account.serialized_len_ptr = new_len as u64; + *caller_account.serialized_len_ptr = post_len as u64; } } } - let to_slice = &mut caller_account.data; - let from_slice = callee_account - .get_data() - .get(0..new_len) - .ok_or(SyscallError::InvalidLength)?; - if to_slice.len() != from_slice.len() { - return Err(Box::new(InstructionError::AccountDataTooSmall)); + let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); + if !bpf_account_data_direct_mapping { + let to_slice = &mut caller_account.account_data_or_only_realloc_padding; + let from_slice = callee_account + .get_data() + .get(0..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 !is_loader_deprecated && realloc_bytes_used > 0 { + let to_slice = + &mut caller_account.account_data_or_only_realloc_padding[0..realloc_bytes_used]; + let from_slice = callee_account + .get_data() + .get(caller_account.original_data_len..post_len) + .ok_or(SyscallError::InvalidLength)?; + to_slice.copy_from_slice(from_slice); } - to_slice.copy_from_slice(from_slice); Ok(()) } @@ -1092,6 +1255,7 @@ mod tests { solana_sdk::{ account::{Account, AccountSharedData}, clock::Epoch, + feature_set::bpf_account_data_direct_mapping, instruction::Instruction, transaction_context::TransactionAccount, }, @@ -1129,7 +1293,9 @@ mod tests { .into_iter() .map(|a| (a.0, a.1)) .collect::>(); - with_mock_invoke_context!($invoke_context, transaction_context, transaction_accounts); + with_mock_invoke_context!($invoke_context, $transaction_context, transaction_accounts); + let feature_set = Arc::make_mut(&mut $invoke_context.feature_set); + feature_set.deactivate(&bpf_account_data_direct_mapping::id()); $invoke_context .transaction_context .get_next_instruction_context() @@ -1246,6 +1412,7 @@ mod tests { let caller_account = CallerAccount::from_account_info( &invoke_context, &memory_mapping, + false, vm_addr, account_info, account.data().len(), @@ -1258,7 +1425,10 @@ mod tests { *caller_account.ref_to_len_in_vm as usize, account.data().len() ); - assert_eq!(caller_account.data, account.data()); + assert_eq!( + caller_account.account_data_or_only_realloc_padding, + account.data() + ); assert_eq!(caller_account.executable, account.executable()); assert_eq!(caller_account.rent_epoch, account.rent_epoch()); } @@ -1288,7 +1458,7 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = + let mut memory_mapping = MemoryMapping::new(mock_caller_account.regions.split_off(0), &config).unwrap(); let mut caller_account = mock_caller_account.caller_account(); @@ -1304,9 +1474,10 @@ mod tests { update_caller_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, + false, &mut caller_account, - &callee_account, + &mut callee_account, ) .unwrap(); @@ -1345,7 +1516,7 @@ mod tests { aligned_memory_mapping: false, ..Config::default() }; - let memory_mapping = + let mut memory_mapping = MemoryMapping::new(mock_caller_account.regions.split_off(0), &config).unwrap(); let data_slice = mock_caller_account.data_slice(); @@ -1366,22 +1537,32 @@ mod tests { (b"foobaz".to_vec(), MAX_PERMITTED_DATA_INCREASE), (b"foobazbad".to_vec(), MAX_PERMITTED_DATA_INCREASE - 3), ] { - assert_eq!(caller_account.data, callee_account.get_data()); + assert_eq!( + caller_account.account_data_or_only_realloc_padding, + callee_account.get_data() + ); callee_account.set_data_from_slice(&new_value).unwrap(); update_caller_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, + false, &mut caller_account, - &callee_account, + &mut callee_account, ) .unwrap(); let data_len = callee_account.get_data().len(); assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); assert_eq!(data_len, serialized_len()); - assert_eq!(data_len, caller_account.data.len()); - assert_eq!(callee_account.get_data(), &caller_account.data[..data_len]); + assert_eq!( + data_len, + caller_account.account_data_or_only_realloc_padding.len() + ); + assert_eq!( + callee_account.get_data(), + &caller_account.account_data_or_only_realloc_padding[..data_len] + ); assert_eq!(data_slice[data_len..].len(), expected_realloc_size); assert!(is_zeroed(&data_slice[data_len..])); } @@ -1391,9 +1572,10 @@ mod tests { .unwrap(); update_caller_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, + false, &mut caller_account, - &callee_account, + &mut callee_account, ) .unwrap(); let data_len = callee_account.get_data().len(); @@ -1406,9 +1588,10 @@ mod tests { assert!(matches!( update_caller_account( &invoke_context, - &memory_mapping, + &mut memory_mapping, + false, &mut caller_account, - &callee_account, + &mut callee_account, ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc, )); @@ -1450,7 +1633,7 @@ mod tests { *caller_account.lamports = 42; *caller_account.owner = Pubkey::new_unique(); - update_callee_account(&invoke_context, &caller_account, callee_account).unwrap(); + update_callee_account(&invoke_context, false, &caller_account, callee_account).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_lamports(), 42); @@ -1479,12 +1662,15 @@ mod tests { let callee_account = borrow_instruction_account!(invoke_context, 0); let mut data = b"foo".to_vec(); - caller_account.data = &mut data; + caller_account.account_data_or_only_realloc_padding = &mut data; - update_callee_account(&invoke_context, &caller_account, callee_account).unwrap(); + update_callee_account(&invoke_context, false, &caller_account, callee_account).unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!(callee_account.get_data(), caller_account.data); + assert_eq!( + callee_account.get_data(), + caller_account.account_data_or_only_realloc_padding + ); } #[test] @@ -1538,6 +1724,7 @@ mod tests { &[0], vm_addr, 1, + false, &mut memory_mapping, &mut invoke_context, ) @@ -1545,12 +1732,14 @@ mod tests { assert_eq!(accounts.len(), 2); assert!(accounts[0].1.is_none()); let caller_account = accounts[1].1.as_ref().unwrap(); - assert_eq!(caller_account.data, account.data()); + assert_eq!( + caller_account.account_data_or_only_realloc_padding, + account.data() + ); assert_eq!(caller_account.original_data_len, original_data_len); } pub type TestTransactionAccount = (Pubkey, AccountSharedData, bool); - struct MockCallerAccount { lamports: u64, owner: Pubkey, @@ -1571,7 +1760,7 @@ mod tests { // the memory region must include the realloc data let regions = vec![MemoryRegion::new_writable(d.as_mut_slice(), vm_addr)]; - // caller_account.data must have the actual data length + // caller_account.account_data_or_only_realloc_padding must have the actual data length d.truncate(mem::size_of::() + data.len()); MockCallerAccount { @@ -1600,7 +1789,7 @@ mod tests { lamports: &mut self.lamports, owner: &mut self.owner, original_data_len: data.len(), - data, + account_data_or_only_realloc_padding: data, vm_data_addr: self.vm_addr + mem::size_of::() as u64, ref_to_len_in_vm: &mut self.len, serialized_len_ptr: std::ptr::null_mut(), diff --git a/programs/sbf/benches/bpf_loader.rs b/programs/sbf/benches/bpf_loader.rs index a2ba7df20fa841..d3bd58561018aa 100644 --- a/programs/sbf/benches/bpf_loader.rs +++ b/programs/sbf/benches/bpf_loader.rs @@ -245,6 +245,7 @@ fn bench_create_vm(bencher: &mut Bencher) { .get_current_instruction_context() .unwrap(), true, // should_cap_ix_accounts + true, // copy_account_data ) .unwrap(); @@ -275,6 +276,7 @@ fn bench_instruction_count_tuner(_bencher: &mut Bencher) { .get_current_instruction_context() .unwrap(), true, // should_cap_ix_accounts + true, // copy_account_data ) .unwrap(); diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 7861b3d0a2935e..23fb7d33c0cd4c 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -1110,7 +1110,7 @@ fn test_program_sbf_invoke_sanity() { .map(|ix| &message.account_keys[ix.instruction.program_id_index as usize]) .cloned() .collect(); - assert_eq!(invoked_programs, vec![system_program::id()]); + assert_eq!(invoked_programs, vec![]); assert_eq!( result.unwrap_err(), TransactionError::InstructionError(0, InstructionError::ProgramFailedToComplete) From 0b19f8998d0eeb6ee5034c0b7279eaa71b85cfc2 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 7 Sep 2022 13:00:02 +0100 Subject: [PATCH 05/44] Fix serialization benches, run both copy and !copy variants --- programs/bpf_loader/benches/serialization.rs | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/programs/bpf_loader/benches/serialization.rs b/programs/bpf_loader/benches/serialization.rs index 5f14524a1bbb7c..87d1fc4f8a8c21 100644 --- a/programs/bpf_loader/benches/serialization.rs +++ b/programs/bpf_loader/benches/serialization.rs @@ -131,6 +131,18 @@ fn bench_serialize_unaligned(bencher: &mut Bencher) { }); } +#[bench] +fn bench_serialize_unaligned_copy_account_data(bencher: &mut Bencher) { + let transaction_context = create_inputs(bpf_loader_deprecated::id(), 7); + let instruction_context = transaction_context + .get_current_instruction_context() + .unwrap(); + bencher.iter(|| { + let _ = + serialize_parameters(&transaction_context, instruction_context, true, true).unwrap(); + }); +} + #[bench] fn bench_serialize_aligned(bencher: &mut Bencher) { let transaction_context = create_inputs(bpf_loader::id(), 7); @@ -144,6 +156,19 @@ fn bench_serialize_aligned(bencher: &mut Bencher) { }); } +#[bench] +fn bench_serialize_aligned_copy_account_data(bencher: &mut Bencher) { + let transaction_context = create_inputs(bpf_loader::id(), 7); + let instruction_context = transaction_context + .get_current_instruction_context() + .unwrap(); + + bencher.iter(|| { + let _ = + serialize_parameters(&transaction_context, instruction_context, true, true).unwrap(); + }); +} + #[bench] fn bench_serialize_unaligned_max_accounts(bencher: &mut Bencher) { let transaction_context = create_inputs(bpf_loader_deprecated::id(), 255); From 6ff6d7e20bed52c06e606e8d211f033516070b6b Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 24 Sep 2022 22:56:36 +0000 Subject: [PATCH 06/44] rbpf-cli: fix build --- rbpf-cli/src/main.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rbpf-cli/src/main.rs b/rbpf-cli/src/main.rs index 30ad73435cfc30..4408603bdc92a4 100644 --- a/rbpf-cli/src/main.rs +++ b/rbpf-cli/src/main.rs @@ -235,6 +235,7 @@ before execting it in the virtual machine.", .get_current_instruction_context() .unwrap(), true, // should_cap_ix_accounts + true, ) .unwrap(); From c890819352ab74cdfc3fc282f2ec3affd023d8fb Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 25 Sep 2022 11:31:15 +0000 Subject: [PATCH 07/44] BorrowedAccount: ensure that account capacity is never reduced Accounts can be directly mapped in address space. Their capacity can't be reduced mid transaction as that would create holes in vm address space that point to invalid host memory. --- sdk/src/account.rs | 8 +++++++ sdk/src/transaction_context.rs | 40 +++++++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 4a2710739e854b..5e489b227b283c 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -533,6 +533,14 @@ impl Account { } impl AccountSharedData { + pub fn is_shared(&self) -> bool { + Arc::strong_count(&self.data) > 1 + } + + pub fn reserve(&mut self, additional: usize) { + self.data_mut().reserve(additional) + } + fn data_mut(&mut self) -> &mut Vec { Arc::make_mut(&mut self.data) } diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 8f9b165549ddc9..24207a86f92099 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -17,6 +17,7 @@ use { instruction::InstructionError, pubkey::Pubkey, }, + solana_program::entrypoint::MAX_PERMITTED_DATA_INCREASE, std::{ cell::{RefCell, RefMut}, collections::HashSet, @@ -782,23 +783,26 @@ impl<'a> BorrowedAccount<'a> { pub fn get_data_mut(&mut self) -> Result<&mut [u8], InstructionError> { self.can_data_be_changed()?; self.touch()?; + self.make_data_mut(); Ok(self.account.data_as_mut_slice()) } /// Overwrites the account data and size (transaction wide). /// - /// Call this when you have an owned buffer and want to replace the account - /// data with it. - /// - /// If you have a slice, use [`Self::set_data_from_slice()`]. + /// You should almost always prefer set_data_from_slice() to this method. + /// This is currently only used by tests and the program-test crate. #[cfg(not(target_os = "solana"))] - pub fn set_data(&mut self, data: Vec) -> Result<(), InstructionError> { + pub fn set_data(&mut self, mut data: Vec) -> Result<(), InstructionError> { self.can_data_be_resized(data.len())?; self.can_data_be_changed()?; self.touch()?; + + // self.account might be mapped into a MemoryRegion. Ensure that its + // capacity doesn't shrink potentially leaving a hole in vm address + // space. The extra capacity will be zeroed by the runtime. + data.reserve(self.account.data().len().saturating_sub(data.len())); self.update_accounts_resize_delta(data.len())?; self.account.set_data(data); - Ok(()) } @@ -806,14 +810,19 @@ impl<'a> BorrowedAccount<'a> { /// /// Call this when you have a slice of data you do not own and want to /// replace the account data with it. - /// - /// If you have an owned buffer (eg [`Vec`]), use [`Self::set_data()`]. #[cfg(not(target_os = "solana"))] pub fn set_data_from_slice(&mut self, data: &[u8]) -> Result<(), InstructionError> { self.can_data_be_resized(data.len())?; self.can_data_be_changed()?; self.touch()?; self.update_accounts_resize_delta(data.len())?; + // Calling make_data_mut() here guarantees that set_data_from_slice() + // copies in places, extending the account capacity if necessary but + // never reducing it. This is required as the account migh be directly + // mapped into a MemoryRegion, and therefore reducing capacity would + // leave a hole in the vm address space. After CPI or upon program + // termination, the runtime will zero the extra capacity. + self.make_data_mut(); self.account.set_data_from_slice(data); Ok(()) @@ -849,10 +858,25 @@ impl<'a> BorrowedAccount<'a> { self.touch()?; self.update_accounts_resize_delta(new_len)?; + // Even if extend_from_slice never reduces capacity, still realloc using + // make_data_mut() if necessary so that we grow the account of the full + // max realloc length in one go, avoiding smaller reallocations. + self.make_data_mut(); self.account.extend_from_slice(data); Ok(()) } + fn make_data_mut(&mut self) { + // if the account is still shared, it means this is the first time we're + // about to write into it. Make the account mutable by copying it in a + // buffer with MAX_PERMITTED_DATA_INCREASE capacity so that if the + // transaction reallocs, we don't have to copy the whole account data a + // second time to fullfill the realloc. + if self.account.is_shared() { + self.account.reserve(MAX_PERMITTED_DATA_INCREASE); + } + } + /// Deserializes the account data into a state #[cfg(not(target_os = "solana"))] pub fn get_state(&self) -> Result { From c8b2c2b22233688e1bffc7bba91d83ed3d2a90a7 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 4 Oct 2022 10:27:25 +0100 Subject: [PATCH 08/44] bpf_load: run serialization tests for both copy and !copy account data --- programs/bpf_loader/src/serialization.rs | 737 ++++++++++++----------- 1 file changed, 397 insertions(+), 340 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 08d575d71e82b7..79c411dfb22088 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -502,13 +502,12 @@ mod tests { account_info::AccountInfo, bpf_loader, entrypoint::deserialize, - sysvar::rent::Rent, transaction_context::InstructionAccount, }, std::{ cell::RefCell, rc::Rc, - slice::{from_raw_parts, from_raw_parts_mut}, + slice::{self, from_raw_parts, from_raw_parts_mut}, }, }; @@ -522,374 +521,419 @@ mod tests { name: &'static str, } - for TestCase { - num_ix_accounts, - append_dup_account, - should_cap_ix_accounts, - expected_err, - name, - } in [ - TestCase { - name: "serialize max accounts without cap", - num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), - should_cap_ix_accounts: false, - append_dup_account: false, - expected_err: None, - }, - TestCase { - name: "serialize max accounts and append dup without cap", - num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), - should_cap_ix_accounts: false, - append_dup_account: true, - expected_err: None, - }, - TestCase { - name: "serialize max accounts with cap", - num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), - should_cap_ix_accounts: true, - append_dup_account: false, - expected_err: None, - }, - TestCase { - name: "serialize too many accounts with cap", - num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, - should_cap_ix_accounts: true, - append_dup_account: false, - expected_err: Some(InstructionError::MaxAccountsExceeded), - }, - TestCase { - name: "serialize too many accounts and append dup with cap", - num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), - should_cap_ix_accounts: true, - append_dup_account: true, - expected_err: Some(InstructionError::MaxAccountsExceeded), - }, - // This test case breaks parameter deserialization and can be cleaned up - // when should_cap_ix_accounts is enabled. - // - // TestCase { - // name: "serialize too many accounts and append dup without cap", - // num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, - // should_cap_ix_accounts: false, - // append_dup_account: true, - // expected_err: None, - // }, - ] { - let program_id = solana_sdk::pubkey::new_rand(); - let mut transaction_accounts = vec![( - program_id, - AccountSharedData::from(Account { - lamports: 0, - data: vec![], - owner: bpf_loader::id(), - executable: true, - rent_epoch: 0, - }), - )]; - for _ in 0..num_ix_accounts { - transaction_accounts.push(( - Pubkey::new_unique(), + for copy_account_data in [true] { + for TestCase { + num_ix_accounts, + append_dup_account, + should_cap_ix_accounts, + expected_err, + name, + } in [ + TestCase { + name: "serialize max accounts without cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: false, + append_dup_account: false, + expected_err: None, + }, + TestCase { + name: "serialize max accounts and append dup without cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: false, + append_dup_account: true, + expected_err: None, + }, + TestCase { + name: "serialize max accounts with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: true, + append_dup_account: false, + expected_err: None, + }, + TestCase { + name: "serialize too many accounts with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, + should_cap_ix_accounts: true, + append_dup_account: false, + expected_err: Some(InstructionError::MaxAccountsExceeded), + }, + TestCase { + name: "serialize too many accounts and append dup with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: true, + append_dup_account: true, + expected_err: Some(InstructionError::MaxAccountsExceeded), + }, + // This test case breaks parameter deserialization and can be cleaned up + // when should_cap_ix_accounts is enabled. + // + // TestCase { + // name: "serialize too many accounts and append dup without cap", + // num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, + // should_cap_ix_accounts: false, + // append_dup_account: true, + // expected_err: None, + // }, + ] { + let program_id = solana_sdk::pubkey::new_rand(); + let mut transaction_accounts = vec![( + program_id, AccountSharedData::from(Account { lamports: 0, data: vec![], - owner: program_id, - executable: false, + owner: bpf_loader::id(), + executable: true, rent_epoch: 0, }), - )); + )]; + for _ in 0..num_ix_accounts { + transaction_accounts.push(( + Pubkey::new_unique(), + AccountSharedData::from(Account { + lamports: 0, + data: vec![], + owner: program_id, + executable: false, + rent_epoch: 0, + }), + )); + } + let mut instruction_accounts: Vec<_> = (0..num_ix_accounts as IndexOfAccount) + .map(|index_in_callee| InstructionAccount { + index_in_transaction: index_in_callee + 1, + index_in_caller: index_in_callee + 1, + index_in_callee, + is_signer: false, + is_writable: false, + }) + .collect(); + if append_dup_account { + instruction_accounts.push(instruction_accounts.last().cloned().unwrap()); + } + let program_indices = [0]; + let instruction_data = vec![]; + + with_mock_invoke_context!( + invoke_context, + transaction_context, + transaction_accounts + ); + invoke_context + .transaction_context + .get_next_instruction_context() + .unwrap() + .configure(&program_indices, &instruction_accounts, &instruction_data); + invoke_context.push().unwrap(); + let instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + + let serialization_result = serialize_parameters( + invoke_context.transaction_context, + instruction_context, + should_cap_ix_accounts, + copy_account_data, + ); + assert_eq!( + serialization_result.as_ref().err(), + expected_err.as_ref(), + "{} test case failed", + name + ); + if expected_err.is_some() { + continue; + } + + let (mut serialized, regions, _account_lengths) = serialization_result.unwrap(); + let mut serialized_regions = concat_regions(®ions); + let (de_program_id, de_accounts, de_instruction_data) = unsafe { + deserialize( + if copy_account_data { + serialized.as_slice_mut() + } else { + serialized_regions.as_slice_mut() + } + .first_mut() + .unwrap() as *mut u8, + ) + }; + assert_eq!(de_program_id, &program_id); + assert_eq!(de_instruction_data, &instruction_data); + for account_info in de_accounts { + let index_in_transaction = invoke_context + .transaction_context + .find_index_of_account(account_info.key) + .unwrap(); + let account = invoke_context + .transaction_context + .get_account_at_index(index_in_transaction) + .unwrap() + .borrow(); + assert_eq!(account.lamports(), account_info.lamports()); + assert_eq!(account.data(), &account_info.data.borrow()[..]); + assert_eq!(account.owner(), account_info.owner); + assert_eq!(account.executable(), account_info.executable); + assert_eq!(account.rent_epoch(), account_info.rent_epoch); + } } - let mut instruction_accounts: Vec<_> = (0..num_ix_accounts as IndexOfAccount) - .map(|index_in_callee| InstructionAccount { - index_in_transaction: index_in_callee + 1, - index_in_caller: index_in_callee + 1, - index_in_callee, - is_signer: false, - is_writable: false, - }) + } + } + + #[test] + fn test_serialize_parameters() { + for copy_account_data in [false, true] { + let program_id = solana_sdk::pubkey::new_rand(); + let transaction_accounts = vec![ + ( + program_id, + AccountSharedData::from(Account { + lamports: 0, + data: vec![], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 0, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 1, + data: vec![1u8, 2, 3, 4, 5], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 100, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 2, + data: vec![11u8, 12, 13, 14, 15, 16, 17, 18, 19], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 200, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 3, + data: vec![], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 3100, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 4, + data: vec![1u8, 2, 3, 4, 5], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 100, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 5, + data: vec![11u8, 12, 13, 14, 15, 16, 17, 18, 19], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 200, + }), + ), + ( + solana_sdk::pubkey::new_rand(), + AccountSharedData::from(Account { + lamports: 6, + data: vec![], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 3100, + }), + ), + ]; + let instruction_accounts: Vec = [1, 1, 2, 3, 4, 4, 5, 6] + .into_iter() + .enumerate() + .map( + |(index_in_instruction, index_in_transaction)| InstructionAccount { + index_in_transaction, + index_in_caller: index_in_transaction, + index_in_callee: index_in_transaction - 1, + is_signer: false, + is_writable: index_in_instruction >= 4, + }, + ) .collect(); - if append_dup_account { - instruction_accounts.push(instruction_accounts.last().cloned().unwrap()); - } + let instruction_data = vec![1u8, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]; let program_indices = [0]; - let instruction_data = vec![]; - - let mut transaction_context = - TransactionContext::new(transaction_accounts, Some(Rent::default()), 1, 1); - transaction_context + let mut original_accounts = transaction_accounts.clone(); + with_mock_invoke_context!(invoke_context, transaction_context, transaction_accounts); + invoke_context + .transaction_context .get_next_instruction_context() .unwrap() .configure(&program_indices, &instruction_accounts, &instruction_data); - transaction_context.push().unwrap(); - let instruction_context = transaction_context - .get_instruction_context_at_index_in_trace(0) + invoke_context.push().unwrap(); + let instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() .unwrap(); - let serialization_result = serialize_parameters( - &transaction_context, + // check serialize_parameters_aligned + let (mut serialized, regions, account_lengths) = serialize_parameters( + invoke_context.transaction_context, instruction_context, - should_cap_ix_accounts, true, - ); + copy_account_data, + ) + .unwrap(); + + let mut serialized_regions = concat_regions(®ions); + if copy_account_data { + assert_eq!(serialized.as_slice(), serialized_regions.as_slice()); + } + let (de_program_id, de_accounts, de_instruction_data) = unsafe { + deserialize( + if copy_account_data { + serialized.as_slice_mut() + } else { + serialized_regions.as_slice_mut() + } + .first_mut() + .unwrap() as *mut u8, + ) + }; + + assert_eq!(&program_id, de_program_id); + assert_eq!(instruction_data, de_instruction_data); assert_eq!( - serialization_result.as_ref().err(), - expected_err.as_ref(), - "{name} test case failed" + (de_instruction_data.first().unwrap() as *const u8).align_offset(BPF_ALIGN_OF_U128), + 0 ); - if expected_err.is_some() { - continue; - } + for account_info in de_accounts { + let index_in_transaction = invoke_context + .transaction_context + .find_index_of_account(account_info.key) + .unwrap(); + let account = invoke_context + .transaction_context + .get_account_at_index(index_in_transaction) + .unwrap() + .borrow(); + assert_eq!(account.lamports(), account_info.lamports()); + assert_eq!(account.data(), &account_info.data.borrow()[..]); + assert_eq!(account.owner(), account_info.owner); + assert_eq!(account.executable(), account_info.executable); + assert_eq!(account.rent_epoch(), account_info.rent_epoch); - let (mut serialized, _regions, _account_lengths) = serialization_result.unwrap(); - let (de_program_id, de_accounts, de_instruction_data) = - unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) }; - assert_eq!(de_program_id, &program_id); - assert_eq!(de_instruction_data, &instruction_data); - for (index, account_info) in de_accounts.into_iter().enumerate() { - let ix_account = &instruction_accounts.get(index).unwrap(); assert_eq!( - account_info.key, - transaction_context - .get_key_of_account_at_index(ix_account.index_in_transaction) - .unwrap() + (*account_info.lamports.borrow() as *const u64).align_offset(BPF_ALIGN_OF_U128), + 0 + ); + assert_eq!( + account_info + .data + .borrow() + .as_ptr() + .align_offset(BPF_ALIGN_OF_U128), + 0 ); - assert_eq!(account_info.owner, &program_id); - assert!(!account_info.executable); - assert!(account_info.data_is_empty()); - assert!(!account_info.is_writable); - assert!(!account_info.is_signer); } - } - } - #[test] - fn test_serialize_parameters() { - let program_id = solana_sdk::pubkey::new_rand(); - let transaction_accounts = vec![ - ( - program_id, - AccountSharedData::from(Account { - lamports: 0, - data: vec![], - owner: bpf_loader::id(), - executable: true, - rent_epoch: 0, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 1, - data: vec![1u8, 2, 3, 4, 5], - owner: bpf_loader::id(), - executable: false, - rent_epoch: 100, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 2, - data: vec![11u8, 12, 13, 14, 15, 16, 17, 18, 19], - owner: bpf_loader::id(), - executable: true, - rent_epoch: 200, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 3, - data: vec![], - owner: bpf_loader::id(), - executable: false, - rent_epoch: 3100, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 4, - data: vec![1u8, 2, 3, 4, 5], - owner: bpf_loader::id(), - executable: false, - rent_epoch: 100, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 5, - data: vec![11u8, 12, 13, 14, 15, 16, 17, 18, 19], - owner: bpf_loader::id(), - executable: true, - rent_epoch: 200, - }), - ), - ( - solana_sdk::pubkey::new_rand(), - AccountSharedData::from(Account { - lamports: 6, - data: vec![], - owner: bpf_loader::id(), - executable: false, - rent_epoch: 3100, - }), - ), - ]; - let instruction_accounts: Vec = [1, 1, 2, 3, 4, 4, 5, 6] - .into_iter() - .enumerate() - .map( - |(index_in_instruction, index_in_transaction)| InstructionAccount { - index_in_transaction, - index_in_caller: index_in_transaction, - index_in_callee: index_in_transaction - 1, - is_signer: false, - is_writable: index_in_instruction >= 4, - }, + deserialize_parameters( + invoke_context.transaction_context, + instruction_context, + copy_account_data, + serialized.as_slice(), + &account_lengths, ) - .collect(); - let instruction_data = vec![1u8, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]; - let program_indices = [0]; - let mut original_accounts = transaction_accounts.clone(); - with_mock_invoke_context!(invoke_context, transaction_context, transaction_accounts); - invoke_context - .transaction_context - .get_next_instruction_context() - .unwrap() - .configure(&program_indices, &instruction_accounts, &instruction_data); - invoke_context.push().unwrap(); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() .unwrap(); + for (index_in_transaction, (_key, original_account)) in + original_accounts.iter().enumerate() + { + let account = invoke_context + .transaction_context + .get_account_at_index(index_in_transaction as IndexOfAccount) + .unwrap() + .borrow(); + assert_eq!(&*account, original_account); + } - // check serialize_parameters_aligned - - let (mut serialized, _regions, account_lengths) = serialize_parameters( - invoke_context.transaction_context, - instruction_context, - true, - true, - ) - .unwrap(); - - let (de_program_id, de_accounts, de_instruction_data) = - unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) }; - - assert_eq!(&program_id, de_program_id); - assert_eq!(instruction_data, de_instruction_data); - assert_eq!( - (de_instruction_data.first().unwrap() as *const u8).align_offset(BPF_ALIGN_OF_U128), - 0 - ); - for account_info in de_accounts { - let index_in_transaction = invoke_context - .transaction_context - .find_index_of_account(account_info.key) - .unwrap(); - let account = invoke_context - .transaction_context - .get_account_at_index(index_in_transaction) + // check serialize_parameters_unaligned + original_accounts + .first_mut() .unwrap() - .borrow(); - assert_eq!(account.lamports(), account_info.lamports()); - assert_eq!(account.data(), &account_info.data.borrow()[..]); - assert_eq!(account.owner(), account_info.owner); - assert_eq!(account.executable(), account_info.executable); - assert_eq!(account.rent_epoch(), account_info.rent_epoch); - - assert_eq!( - (*account_info.lamports.borrow() as *const u64).align_offset(BPF_ALIGN_OF_U128), - 0 - ); - assert_eq!( - account_info - .data - .borrow() - .as_ptr() - .align_offset(BPF_ALIGN_OF_U128), - 0 - ); - } - - deserialize_parameters( - invoke_context.transaction_context, - instruction_context, - true, - serialized.as_slice(), - &account_lengths, - ) - .unwrap(); - for (index_in_transaction, (_key, original_account)) in original_accounts.iter().enumerate() - { - let account = invoke_context + .1 + .set_owner(bpf_loader_deprecated::id()); + invoke_context .transaction_context - .get_account_at_index(index_in_transaction as IndexOfAccount) + .get_account_at_index(0) .unwrap() - .borrow(); - assert_eq!(&*account, original_account); - } - - // check serialize_parameters_unaligned - original_accounts - .first_mut() - .unwrap() - .1 - .set_owner(bpf_loader_deprecated::id()); - invoke_context - .transaction_context - .get_account_at_index(0) - .unwrap() - .borrow_mut() - .set_owner(bpf_loader_deprecated::id()); - - let (mut serialized, _regions, account_lengths) = serialize_parameters( - invoke_context.transaction_context, - instruction_context, - true, - true, - ) - .unwrap(); + .borrow_mut() + .set_owner(bpf_loader_deprecated::id()); - let (de_program_id, de_accounts, de_instruction_data) = unsafe { - deserialize_unaligned(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) - }; - assert_eq!(&program_id, de_program_id); - assert_eq!(instruction_data, de_instruction_data); - for account_info in de_accounts { - let index_in_transaction = invoke_context - .transaction_context - .find_index_of_account(account_info.key) - .unwrap(); - let account = invoke_context - .transaction_context - .get_account_at_index(index_in_transaction) - .unwrap() - .borrow(); - assert_eq!(account.lamports(), account_info.lamports()); - assert_eq!(account.data(), &account_info.data.borrow()[..]); - assert_eq!(account.owner(), account_info.owner); - assert_eq!(account.executable(), account_info.executable); - assert_eq!(account.rent_epoch(), account_info.rent_epoch); - } + let (mut serialized, regions, account_lengths) = serialize_parameters( + invoke_context.transaction_context, + instruction_context, + true, + copy_account_data, + ) + .unwrap(); + let mut serialized_regions = concat_regions(®ions); + + let (de_program_id, de_accounts, de_instruction_data) = unsafe { + deserialize_unaligned( + if copy_account_data { + serialized.as_slice_mut() + } else { + serialized_regions.as_slice_mut() + } + .first_mut() + .unwrap() as *mut u8, + ) + }; + assert_eq!(&program_id, de_program_id); + assert_eq!(instruction_data, de_instruction_data); + for account_info in de_accounts { + let index_in_transaction = invoke_context + .transaction_context + .find_index_of_account(account_info.key) + .unwrap(); + let account = invoke_context + .transaction_context + .get_account_at_index(index_in_transaction) + .unwrap() + .borrow(); + assert_eq!(account.lamports(), account_info.lamports()); + assert_eq!(account.data(), &account_info.data.borrow()[..]); + assert_eq!(account.owner(), account_info.owner); + assert_eq!(account.executable(), account_info.executable); + assert_eq!(account.rent_epoch(), account_info.rent_epoch); + } - deserialize_parameters( - invoke_context.transaction_context, - instruction_context, - true, - serialized.as_slice(), - &account_lengths, - ) - .unwrap(); - for (index_in_transaction, (_key, original_account)) in original_accounts.iter().enumerate() - { - let account = invoke_context - .transaction_context - .get_account_at_index(index_in_transaction as IndexOfAccount) - .unwrap() - .borrow(); - assert_eq!(&*account, original_account); + deserialize_parameters( + invoke_context.transaction_context, + instruction_context, + copy_account_data, + serialized.as_slice(), + &account_lengths, + ) + .unwrap(); + for (index_in_transaction, (_key, original_account)) in + original_accounts.iter().enumerate() + { + let account = invoke_context + .transaction_context + .get_account_at_index(index_in_transaction as IndexOfAccount) + .unwrap() + .borrow(); + assert_eq!(&*account, original_account); + } } } @@ -979,4 +1023,17 @@ mod tests { (program_id, accounts, instruction_data) } + + 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); + for region in regions { + let host_slice = unsafe { + slice::from_raw_parts(region.host_addr.get() 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 + } } From 4df1264a9ec09f44ac905c5d9ce3a1867131de30 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 4 Oct 2022 10:30:31 +0100 Subject: [PATCH 09/44] bpf_loader: add Serializer::write_account --- programs/bpf_loader/src/serialization.rs | 61 ++++++++++++++++++------ programs/bpf_loader/src/syscalls/mod.rs | 3 +- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 79c411dfb22088..07b2ce6218a670 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -17,7 +17,7 @@ use { BorrowedAccount, IndexOfAccount, InstructionContext, TransactionContext, }, }, - std::{mem, mem::size_of}, + std::mem::{self, size_of}, }; /// Maximum number of instruction accounts that can be serialized into the @@ -35,16 +35,18 @@ struct Serializer { vaddr: u64, region_start: usize, aligned: bool, + copy_account_data: bool, } impl Serializer { - fn new(size: usize, start_addr: u64, aligned: bool) -> Serializer { + fn new(size: usize, start_addr: u64, aligned: bool, copy_account_data: bool) -> Serializer { Serializer { buffer: AlignedMemory::with_capacity(size), regions: Vec::new(), region_start: 0, vaddr: start_addr, aligned, + copy_account_data, } } @@ -75,19 +77,48 @@ impl Serializer { } } - fn write_account(&mut self, account: &BorrowedAccount<'_>) -> Result<(), InstructionError> { - self.write_all(account.get_data()); + fn write_account(&mut self, account: &mut BorrowedAccount<'_>) -> Result<(), InstructionError> { + if self.copy_account_data { + self.write_all(account.get_data()); + } else { + self.push_account_region(account); + } if self.aligned { let align_offset = (account.get_data().len() as *const u8).align_offset(BPF_ALIGN_OF_U128); - self.fill_write(MAX_PERMITTED_DATA_INCREASE + align_offset, 0) - .map_err(|_| InstructionError::InvalidArgument)?; + if self.copy_account_data { + self.fill_write(MAX_PERMITTED_DATA_INCREASE + align_offset, 0) + .map_err(|_| InstructionError::InvalidArgument)?; + } else { + // The deserialization code is going to align the vm_addr to + // BPF_ALIGN_OF_U128. Always add one BPF_ALIGN_OF_U128 worth of + // padding and shift the start of the next region, so that once + // vm_addr is aligned, the corresponding host_addr is aligned + // too. + self.fill_write(MAX_PERMITTED_DATA_INCREASE + BPF_ALIGN_OF_U128, 0) + .map_err(|_| InstructionError::InvalidArgument)?; + self.region_start += BPF_ALIGN_OF_U128.saturating_sub(align_offset); + } } Ok(()) } + fn push_account_region(&mut self, account: &mut BorrowedAccount<'_>) { + self.push_region(); + let account_len = account.get_data().len(); + if account_len > 0 { + let region = if let Ok(data) = account.get_data_mut() { + MemoryRegion::new_writable(data, self.vaddr) + } else { + MemoryRegion::new_readonly(account.get_data(), self.vaddr) + }; + self.vaddr += region.len; + self.regions.push(region); + } + } + fn push_region(&mut self) { let range = self.region_start..self.buffer.len(); let region = MemoryRegion::new_writable( @@ -235,21 +266,20 @@ fn serialize_parameters_unaligned( + instruction_context.get_instruction_data().len() // instruction data + size_of::(); // program id - let mut s = Serializer::new(size, MM_INPUT_START, false); + let mut s = Serializer::new(size, MM_INPUT_START, false, copy_account_data); s.write::((accounts.len() as u64).to_le()); for account in accounts { match account { SerializeAccount::Duplicate(position) => s.write(position as u8), - SerializeAccount::Account(_, account) => { + SerializeAccount::Account(_, mut account) => { s.write::(NON_DUP_MARKER); s.write::(account.is_signer() as u8); s.write::(account.is_writable() as u8); s.write_all(account.get_key().as_ref()); s.write::(account.get_lamports().to_le()); s.write::((account.get_data().len() as u64).to_le()); - s.write_account(&account) - .map_err(|_| InstructionError::InvalidArgument)?; + s.write_account(&mut account)?; s.write_all(account.get_owner().as_ref()); s.write::(account.is_executable() as u8); s.write::((account.get_rent_epoch()).to_le()); @@ -357,13 +387,13 @@ fn serialize_parameters_aligned( + instruction_context.get_instruction_data().len() + size_of::(); // program id; - let mut s = Serializer::new(size, MM_INPUT_START, true); + let mut s = Serializer::new(size, MM_INPUT_START, true, copy_account_data); // Serialize into the buffer s.write::((accounts.len() as u64).to_le()); for account in accounts { match account { - SerializeAccount::Account(_, borrowed_account) => { + SerializeAccount::Account(_, mut borrowed_account) => { s.write::(NON_DUP_MARKER); s.write::(borrowed_account.is_signer() as u8); s.write::(borrowed_account.is_writable() as u8); @@ -373,9 +403,8 @@ fn serialize_parameters_aligned( s.write_all(borrowed_account.get_owner().as_ref()); s.write::(borrowed_account.get_lamports().to_le()); s.write::((borrowed_account.get_data().len() as u64).to_le()); - s.write_account(&borrowed_account) - .map_err(|_| InstructionError::InvalidArgument)?; - s.write::((borrowed_account.get_rent_epoch()).to_le()); + s.write_account(&mut borrowed_account)?; + s.write::((borrowed_account.get_rent_epoch() as u64).to_le()); } SerializeAccount::Duplicate(position) => { s.write::(position as u8); @@ -459,6 +488,8 @@ pub fn deserialize_parameters_aligned( } start += *pre_len; // data } 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) diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index e782bda37862e3..cbc7ab3c4fe66b 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -30,6 +30,7 @@ use { big_mod_exp::{big_mod_exp, BigModExpParams}, blake3, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, SUCCESS}, + feature_set::bpf_account_data_direct_mapping, feature_set::FeatureSet, feature_set::{ self, blake3_syscall_enabled, check_syscall_outputs_do_not_overlap, @@ -174,7 +175,7 @@ pub fn create_loader<'a>( enable_elf_vaddr: false, reject_rodata_stack_overlap: false, new_elf_parser: feature_set.is_active(&switch_to_new_elf_parser::id()), - aligned_memory_mapping: true, + aligned_memory_mapping: !feature_set.is_active(&bpf_account_data_direct_mapping::id()), // Warning, do not use `Config::default()` so that configuration here is explicit. }; From e6283706413eaa73916babca39c21746660ab22c Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 4 Oct 2022 22:30:45 +0100 Subject: [PATCH 10/44] fix lints --- programs/bpf_loader/src/serialization.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 07b2ce6218a670..0f8d08acec0426 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -524,6 +524,7 @@ pub fn deserialize_parameters_aligned( } #[cfg(test)] +#[allow(clippy::indexing_slicing)] mod tests { use { super::*, From 7f623b3bd70186f45953a6449e71f14f3e46dd03 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 5 Oct 2022 00:17:28 +0100 Subject: [PATCH 11/44] BorrowedAccount: make_data_mut is host only --- sdk/src/transaction_context.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 24207a86f92099..7ea84258510c01 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -866,6 +866,7 @@ impl<'a> BorrowedAccount<'a> { Ok(()) } + #[cfg(not(target_os = "solana"))] fn make_data_mut(&mut self) { // if the account is still shared, it means this is the first time we're // about to write into it. Make the account mutable by copying it in a From 1211f3e69a9c449b8eda766facc4498e6ab1645c Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 5 Oct 2022 07:55:31 +0100 Subject: [PATCH 12/44] Fix unused import warning --- sdk/src/transaction_context.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 7ea84258510c01..e9280a788bf6ef 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -4,12 +4,15 @@ #[cfg(all(not(target_os = "solana"), debug_assertions))] use crate::signature::Signature; #[cfg(not(target_os = "solana"))] -use crate::{ - account::WritableAccount, - rent::Rent, - system_instruction::{ - MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, MAX_PERMITTED_DATA_LENGTH, +use { + crate::{ + account::WritableAccount, + rent::Rent, + system_instruction::{ + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, MAX_PERMITTED_DATA_LENGTH, + }, }, + solana_program::entrypoint::MAX_PERMITTED_DATA_INCREASE, }; use { crate::{ @@ -17,7 +20,6 @@ use { instruction::InstructionError, pubkey::Pubkey, }, - solana_program::entrypoint::MAX_PERMITTED_DATA_INCREASE, std::{ cell::{RefCell, RefMut}, collections::HashSet, From e9daf203b20fa78f434a2241013f1d5c61bf24c1 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 20 Oct 2022 19:45:50 +0100 Subject: [PATCH 13/44] Fix lints --- programs/bpf_loader/src/serialization.rs | 11 +++++-- programs/bpf_loader/src/syscalls/cpi.rs | 41 +++++++++++++----------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 0f8d08acec0426..a18efbf77cbfb0 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -502,9 +502,14 @@ pub fn deserialize_parameters_aligned( 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()? - [*pre_len..pre_len.saturating_add(allocated_bytes)] - .copy_from_slice(&data[0..allocated_bytes]); + 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), diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 1f6af25e62f5fd..5c7dacb861e688 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -114,7 +114,7 @@ impl<'a> CallerAccount<'a> { let account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, if bpf_account_data_direct_mapping { - vm_data_addr + original_data_len as u64 + vm_data_addr.saturating_add(original_data_len as u64) } else { vm_data_addr }, @@ -189,7 +189,7 @@ impl<'a> CallerAccount<'a> { let account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, if bpf_account_data_direct_mapping { - vm_data_addr + original_data_len as u64 + vm_data_addr.saturating_add(original_data_len as u64) } else { vm_data_addr }, @@ -1033,10 +1033,15 @@ fn update_callee_account( callee_account.set_data_length(post_len)?; let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if !is_loader_deprecated && realloc_bytes_used > 0 { - callee_account.get_data_mut()?[caller_account.original_data_len..post_len] + callee_account + .get_data_mut()? + .get_mut(caller_account.original_data_len..post_len) + .ok_or(SyscallError::InvalidLength)? .copy_from_slice( - &caller_account.account_data_or_only_realloc_padding - [0..realloc_bytes_used], + caller_account + .account_data_or_only_realloc_padding + .get(0..realloc_bytes_used) + .ok_or(SyscallError::InvalidLength)?, ); } } @@ -1052,7 +1057,7 @@ fn update_callee_account( .and_then(|_| callee_account.can_data_be_changed()) { Ok(()) => callee_account - .set_data_from_slice(&caller_account.account_data_or_only_realloc_padding)?, + .set_data_from_slice(caller_account.account_data_or_only_realloc_padding)?, Err(err) if callee_account.get_data() != caller_account.account_data_or_only_realloc_padding => @@ -1127,20 +1132,14 @@ fn update_caller_account( .iter() .position(|memory_region| memory_region.vm_addr == caller_account.vm_data_addr) .ok_or(Box::new(InstructionError::ProgramEnvironmentSetupFailure))?; + let region = regions.get(memory_region_index).unwrap(); let account_region = if let Ok(data) = callee_account.get_data_mut() { - let data = unsafe { - std::slice::from_raw_parts_mut( - data.as_mut_ptr(), - regions[memory_region_index].len as usize, - ) - }; + let data = + unsafe { std::slice::from_raw_parts_mut(data.as_mut_ptr(), region.len as usize) }; MemoryRegion::new_writable(data, caller_account.vm_data_addr) } else { let data = unsafe { - std::slice::from_raw_parts( - callee_account.get_data().as_ptr(), - regions[memory_region_index].len as usize, - ) + std::slice::from_raw_parts(callee_account.get_data().as_ptr(), region.len as usize) }; MemoryRegion::new_readonly(data, caller_account.vm_data_addr) }; @@ -1181,7 +1180,9 @@ fn update_caller_account( caller_account.account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, if bpf_account_data_direct_mapping { - caller_account.vm_data_addr + caller_account.original_data_len as u64 + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64) } else { caller_account.vm_data_addr }, @@ -1231,8 +1232,10 @@ fn update_caller_account( } to_slice.copy_from_slice(from_slice); } else if !is_loader_deprecated && realloc_bytes_used > 0 { - let to_slice = - &mut caller_account.account_data_or_only_realloc_padding[0..realloc_bytes_used]; + let to_slice = caller_account + .account_data_or_only_realloc_padding + .get_mut(0..realloc_bytes_used) + .ok_or(SyscallError::InvalidLength)?; let from_slice = callee_account .get_data() .get(caller_account.original_data_len..post_len) From 68bde9b2ed6fecf7997a86d50c8727c9890b7704 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Oct 2022 08:30:30 +0100 Subject: [PATCH 14/44] cpi: add explicit direct_mapping arg to update_(callee|caller)_account --- programs/bpf_loader/src/syscalls/cpi.rs | 54 +++++++++++++++++-------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 5c7dacb861e688..5379eb5f1810dd 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -732,6 +732,10 @@ where .unwrap() .orig_account_lengths; + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + for (instruction_account_index, instruction_account) in instruction_accounts.iter().enumerate() { if instruction_account_index as IndexOfAccount != instruction_account.index_in_callee { @@ -793,6 +797,7 @@ where is_loader_deprecated, &caller_account, callee_account, + direct_mapping, )?; let caller_account = if instruction_account.is_writable { @@ -980,6 +985,10 @@ fn cpi_common( // CPI exit. // // Synchronize the callee's account changes so the caller can see them. + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + for (index_in_caller, caller_account) in accounts.iter_mut() { if let Some(caller_account) = caller_account { let mut callee_account = instruction_context @@ -990,6 +999,7 @@ fn cpi_common( is_loader_deprecated, caller_account, &mut callee_account, + direct_mapping, )?; } } @@ -1010,19 +1020,16 @@ fn update_callee_account( is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, + direct_mapping: bool, ) -> Result<(), Error> { let is_disable_cpi_setting_executable_and_rent_epoch_active = invoke_context .feature_set .is_active(&disable_cpi_setting_executable_and_rent_epoch::id()); - let bpf_account_data_direct_mapping = invoke_context - .feature_set - .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - if callee_account.get_lamports() != *caller_account.lamports { callee_account.set_lamports(*caller_account.lamports)?; } - if bpf_account_data_direct_mapping { + 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 @@ -1118,15 +1125,12 @@ fn update_caller_account( is_loader_deprecated: bool, caller_account: &mut CallerAccount, callee_account: &mut BorrowedAccount<'_>, + direct_mapping: bool, ) -> Result<(), Error> { - let bpf_account_data_direct_mapping = invoke_context - .feature_set - .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - if bpf_account_data_direct_mapping && caller_account.original_data_len > 0 { + if direct_mapping && caller_account.original_data_len > 0 { let regions = memory_mapping.get_regions(); let memory_region_index = regions .iter() @@ -1161,7 +1165,7 @@ fn update_caller_account( return Err(Box::new(InstructionError::InvalidRealloc)); } if post_len < prev_len { - if bpf_account_data_direct_mapping { + if direct_mapping { unsafe { std::slice::from_raw_parts_mut( callee_account.get_data_mut()?.as_mut_ptr().add(post_len), @@ -1179,14 +1183,14 @@ fn update_caller_account( } caller_account.account_data_or_only_realloc_padding = translate_slice_mut::( memory_mapping, - if bpf_account_data_direct_mapping { + if direct_mapping { caller_account .vm_data_addr .saturating_add(caller_account.original_data_len as u64) } else { caller_account.vm_data_addr }, - if bpf_account_data_direct_mapping { + if direct_mapping { if is_loader_deprecated { 0 } else { @@ -1221,7 +1225,7 @@ fn update_caller_account( } } let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); - if !bpf_account_data_direct_mapping { + if !direct_mapping { let to_slice = &mut caller_account.account_data_or_only_realloc_padding; let from_slice = callee_account .get_data() @@ -1481,6 +1485,7 @@ mod tests { false, &mut caller_account, &mut callee_account, + false, ) .unwrap(); @@ -1552,6 +1557,7 @@ mod tests { false, &mut caller_account, &mut callee_account, + false, ) .unwrap(); @@ -1579,6 +1585,7 @@ mod tests { false, &mut caller_account, &mut callee_account, + false, ) .unwrap(); let data_len = callee_account.get_data().len(); @@ -1595,6 +1602,7 @@ mod tests { false, &mut caller_account, &mut callee_account, + false, ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc, )); @@ -1636,7 +1644,14 @@ mod tests { *caller_account.lamports = 42; *caller_account.owner = Pubkey::new_unique(); - update_callee_account(&invoke_context, false, &caller_account, callee_account).unwrap(); + update_callee_account( + &invoke_context, + false, + &caller_account, + callee_account, + false, + ) + .unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_lamports(), 42); @@ -1667,7 +1682,14 @@ mod tests { let mut data = b"foo".to_vec(); caller_account.account_data_or_only_realloc_padding = &mut data; - update_callee_account(&invoke_context, false, &caller_account, callee_account).unwrap(); + update_callee_account( + &invoke_context, + false, + &caller_account, + callee_account, + false, + ) + .unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!( From 17bd0b3c9eaeac9df66e223009c86008cc54df3d Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Oct 2022 18:32:24 +0100 Subject: [PATCH 15/44] cpi: rename account_data_or_only_realloc_padding to serialized_data --- programs/bpf_loader/src/syscalls/cpi.rs | 85 ++++++++++--------------- 1 file changed, 32 insertions(+), 53 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 5379eb5f1810dd..e9db7d8f3c8a5d 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -21,12 +21,15 @@ struct CallerAccount<'a> { lamports: &'a mut u64, owner: &'a mut Pubkey, original_data_len: usize, - // After the activation of bpf_account_data_direct_mapping this - // no longer holds the account data followed by the reallocation padding, - // but only the reallocation padding. That is because after the activation - // we already share the account data between runtime and program and mapping it - // forward and backward would create aliasing mutable references. - account_data_or_only_realloc_padding: &'a mut [u8], + // This points to the data section for this account, as serialized and + // mapped inside the vm (see serialize_parameters() in + // BpfExecutor::execute). + // + // When direct mapping is off, this includes both the account data _and_ the + // realloc padding. When direct mapping is on, account data is mapped in its + // own separate memory region which is directly mutated from inside the vm, + // and the serialized buffer includes only the realloc padding. + serialized_data: &'a mut [u8], // Given the corresponding input AccountInfo::data, vm_data_addr points to // the pointer field and ref_to_len_in_vm points to the length field. vm_data_addr: u64, @@ -65,12 +68,7 @@ impl<'a> CallerAccount<'a> { invoke_context.get_check_aligned(), )?; - let ( - account_data_or_only_realloc_padding, - vm_data_addr, - ref_to_len_in_vm, - serialized_len_ptr, - ) = { + let (serialized_data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr) = { // Double translate data out of RefCell let data = *translate_type::<&[u8]>( memory_mapping, @@ -111,7 +109,7 @@ impl<'a> CallerAccount<'a> { let bpf_account_data_direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - let account_data_or_only_realloc_padding = translate_slice_mut::( + let serialized_data = translate_slice_mut::( memory_mapping, if bpf_account_data_direct_mapping { vm_data_addr.saturating_add(original_data_len as u64) @@ -132,7 +130,7 @@ impl<'a> CallerAccount<'a> { )?; ( - account_data_or_only_realloc_padding, + serialized_data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -143,7 +141,7 @@ impl<'a> CallerAccount<'a> { lamports, owner, original_data_len, - account_data_or_only_realloc_padding, + serialized_data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -186,7 +184,7 @@ impl<'a> CallerAccount<'a> { let bpf_account_data_direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - let account_data_or_only_realloc_padding = translate_slice_mut::( + let serialized_data = translate_slice_mut::( memory_mapping, if bpf_account_data_direct_mapping { vm_data_addr.saturating_add(original_data_len as u64) @@ -240,7 +238,7 @@ impl<'a> CallerAccount<'a> { lamports, owner, original_data_len, - account_data_or_only_realloc_padding, + serialized_data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, @@ -1046,7 +1044,7 @@ fn update_callee_account( .ok_or(SyscallError::InvalidLength)? .copy_from_slice( caller_account - .account_data_or_only_realloc_padding + .serialized_data .get(0..realloc_bytes_used) .ok_or(SyscallError::InvalidLength)?, ); @@ -1060,15 +1058,11 @@ fn update_callee_account( } else { // The redundant check helps to avoid the expensive data comparison if we can match callee_account - .can_data_be_resized(caller_account.account_data_or_only_realloc_padding.len()) + .can_data_be_resized(caller_account.serialized_data.len()) .and_then(|_| callee_account.can_data_be_changed()) { - Ok(()) => callee_account - .set_data_from_slice(caller_account.account_data_or_only_realloc_padding)?, - Err(err) - if callee_account.get_data() - != caller_account.account_data_or_only_realloc_padding => - { + Ok(()) => callee_account.set_data_from_slice(caller_account.serialized_data)?, + Err(err) if callee_account.get_data() != caller_account.serialized_data => { return Err(Box::new(err)); } _ => {} @@ -1175,13 +1169,13 @@ fn update_caller_account( } } else { caller_account - .account_data_or_only_realloc_padding + .serialized_data .get_mut(post_len..) .ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))? .fill(0); } } - caller_account.account_data_or_only_realloc_padding = translate_slice_mut::( + caller_account.serialized_data = translate_slice_mut::( memory_mapping, if direct_mapping { caller_account @@ -1226,7 +1220,7 @@ fn update_caller_account( } let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if !direct_mapping { - let to_slice = &mut caller_account.account_data_or_only_realloc_padding; + let to_slice = &mut caller_account.serialized_data; let from_slice = callee_account .get_data() .get(0..post_len) @@ -1237,7 +1231,7 @@ fn update_caller_account( to_slice.copy_from_slice(from_slice); } else if !is_loader_deprecated && realloc_bytes_used > 0 { let to_slice = caller_account - .account_data_or_only_realloc_padding + .serialized_data .get_mut(0..realloc_bytes_used) .ok_or(SyscallError::InvalidLength)?; let from_slice = callee_account @@ -1432,10 +1426,7 @@ mod tests { *caller_account.ref_to_len_in_vm as usize, account.data().len() ); - assert_eq!( - caller_account.account_data_or_only_realloc_padding, - account.data() - ); + assert_eq!(caller_account.serialized_data, account.data()); assert_eq!(caller_account.executable, account.executable()); assert_eq!(caller_account.rent_epoch, account.rent_epoch()); } @@ -1545,10 +1536,7 @@ mod tests { (b"foobaz".to_vec(), MAX_PERMITTED_DATA_INCREASE), (b"foobazbad".to_vec(), MAX_PERMITTED_DATA_INCREASE - 3), ] { - assert_eq!( - caller_account.account_data_or_only_realloc_padding, - callee_account.get_data() - ); + assert_eq!(caller_account.serialized_data, callee_account.get_data()); callee_account.set_data_from_slice(&new_value).unwrap(); update_caller_account( @@ -1564,13 +1552,10 @@ mod tests { let data_len = callee_account.get_data().len(); assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); assert_eq!(data_len, serialized_len()); - assert_eq!( - data_len, - caller_account.account_data_or_only_realloc_padding.len() - ); + assert_eq!(data_len, caller_account.serialized_data.len()); assert_eq!( callee_account.get_data(), - &caller_account.account_data_or_only_realloc_padding[..data_len] + &caller_account.serialized_data[..data_len] ); assert_eq!(data_slice[data_len..].len(), expected_realloc_size); assert!(is_zeroed(&data_slice[data_len..])); @@ -1680,7 +1665,7 @@ mod tests { let callee_account = borrow_instruction_account!(invoke_context, 0); let mut data = b"foo".to_vec(); - caller_account.account_data_or_only_realloc_padding = &mut data; + caller_account.serialized_data = &mut data; update_callee_account( &invoke_context, @@ -1692,10 +1677,7 @@ mod tests { .unwrap(); let callee_account = borrow_instruction_account!(invoke_context, 0); - assert_eq!( - callee_account.get_data(), - caller_account.account_data_or_only_realloc_padding - ); + assert_eq!(callee_account.get_data(), caller_account.serialized_data); } #[test] @@ -1757,10 +1739,7 @@ mod tests { assert_eq!(accounts.len(), 2); assert!(accounts[0].1.is_none()); let caller_account = accounts[1].1.as_ref().unwrap(); - assert_eq!( - caller_account.account_data_or_only_realloc_padding, - account.data() - ); + assert_eq!(caller_account.serialized_data, account.data()); assert_eq!(caller_account.original_data_len, original_data_len); } @@ -1785,7 +1764,7 @@ mod tests { // the memory region must include the realloc data let regions = vec![MemoryRegion::new_writable(d.as_mut_slice(), vm_addr)]; - // caller_account.account_data_or_only_realloc_padding must have the actual data length + // caller_account.serialized_data must have the actual data length d.truncate(mem::size_of::() + data.len()); MockCallerAccount { @@ -1814,7 +1793,7 @@ mod tests { lamports: &mut self.lamports, owner: &mut self.owner, original_data_len: data.len(), - account_data_or_only_realloc_padding: data, + serialized_data: data, vm_data_addr: self.vm_addr + mem::size_of::() as u64, ref_to_len_in_vm: &mut self.len, serialized_len_ptr: std::ptr::null_mut(), From 794a0acc2a12ba16e580afa51453af0474ec12d4 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Oct 2022 18:48:33 +0100 Subject: [PATCH 16/44] cpi: add CallerAccount::original_data_len comment --- programs/bpf_loader/src/syscalls/cpi.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index e9db7d8f3c8a5d..adf1cb89d159f2 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -20,6 +20,10 @@ use { struct CallerAccount<'a> { lamports: &'a mut u64, owner: &'a mut Pubkey, + // The original data length of the account at the start of the current + // instruction. We use this to determine wether an account was shrunk or + // grown before or after CPI, and to derive the vm address of the realloc + // region. original_data_len: usize, // This points to the data section for this account, as serialized and // mapped inside the vm (see serialize_parameters() in From 5081d992b518808117ebc735fe2d5a7d35f6ef9d Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Oct 2022 20:11:53 +0100 Subject: [PATCH 17/44] cpi: add update_callee_account direct_mapping test --- programs/bpf_loader/src/syscalls/cpi.rs | 53 +++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index adf1cb89d159f2..a283f5cd6f6a49 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1684,6 +1684,59 @@ mod tests { assert_eq!(callee_account.get_data(), caller_account.serialized_data); } + #[test] + fn test_update_callee_account_data_direct_mapping() { + let transaction_accounts = one_instruction_account(b"foobar".to_vec()); + let account = transaction_accounts[1].1.clone(); + + let mut invoke_context_builder = + MockInvokeContext::new(transaction_accounts, *b"instruction data", [0], &[1]); + let invoke_context = invoke_context_builder.invoke_context(); + + let instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + + let mut mock_caller_account = + MockCallerAccount::new(1234, *account.owner(), 0xFFFFFFFF00000000, account.data()); + + let mut caller_account = mock_caller_account.caller_account(); + + let get_callee = || { + instruction_context + .try_borrow_instruction_account(invoke_context.transaction_context, 0) + .unwrap() + }; + let mut callee_account = get_callee(); + + // 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 mut data = b"baz".to_vec(); + caller_account.serialized_data = &mut data; + + 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, + false, + &caller_account, + callee_account, + true, + ) + .unwrap(); + callee_account = get_callee(); + assert_eq!(callee_account.get_data(), expected); + } + } + #[test] fn test_translate_accounts_rust() { let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); From 2d946d484ba3e1d188abaaaf1d4d42b8793d5630 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 28 Oct 2022 21:01:44 +0100 Subject: [PATCH 18/44] cpi: add test_update_caller_account_data_direct_mapping and fix bug We used to have a bug in zeroing data when shrinking account, where we zeroed the spare account capacity but not the realloc padding. --- programs/bpf_loader/src/syscalls/cpi.rs | 281 +++++++++++++++++++++--- 1 file changed, 249 insertions(+), 32 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index a283f5cd6f6a49..3fc12ea3b6f595 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -10,7 +10,7 @@ use { }, transaction_context::BorrowedAccount, }, - std::mem, + std::{mem, slice}, }; /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. @@ -1150,27 +1150,43 @@ fn update_caller_account( let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); if prev_len != post_len { + let max_increase = if direct_mapping && !invoke_context.get_check_aligned() { + 0 + } else { + MAX_PERMITTED_DATA_INCREASE + }; let data_overflow = post_len > caller_account .original_data_len - .saturating_add(MAX_PERMITTED_DATA_INCREASE); + .saturating_add(max_increase); if data_overflow { ic_msg!( invoke_context, - "Account data size realloc limited to {} in inner instructions", - MAX_PERMITTED_DATA_INCREASE + "Account data size realloc limited to {max_increase} in inner instructions", ); return Err(Box::new(InstructionError::InvalidRealloc)); } if post_len < prev_len { if direct_mapping { - unsafe { - std::slice::from_raw_parts_mut( - callee_account.get_data_mut()?.as_mut_ptr().add(post_len), - prev_len.saturating_sub(post_len), - ) - .fill(0); + if post_len < caller_account.original_data_len { + // zero the spare capacity in the account data + let spare_capacity = unsafe { + slice::from_raw_parts_mut( + callee_account.get_data_mut()?.as_mut_ptr().add(post_len), + caller_account.original_data_len.saturating_sub(post_len), + ) + }; + spare_capacity.fill(0); } + + // zero the spare capacity in the realloc padding + let spare_realloc = unsafe { + slice::from_raw_parts_mut( + caller_account.serialized_data.as_mut_ptr(), + prev_len.saturating_sub(caller_account.original_data_len), + ) + }; + spare_realloc.fill(0); } else { caller_account .serialized_data @@ -1453,8 +1469,13 @@ mod tests { .get_current_instruction_context() .unwrap(); - let mut mock_caller_account = - MockCallerAccount::new(1234, *account.owner(), 0xFFFFFFFF00000000, account.data()); + let mut mock_caller_account = MockCallerAccount::new( + 1234, + *account.owner(), + 0xFFFFFFFF00000000, + account.data(), + false, + ); let config = Config { aligned_memory_mapping: false, @@ -1513,6 +1534,7 @@ mod tests { *account.owner(), 0xFFFFFFFF00000000, account.data(), + false, ); let config = Config { @@ -1609,6 +1631,145 @@ mod tests { }}; } + #[test] + fn test_update_caller_account_data_direct_mapping() { + let transaction_accounts = transaction_with_one_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 instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + + 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 mut memory_mapping = + MemoryMapping::new(mock_caller_account.regions.split_off(0), &config).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 = instruction_context + .try_borrow_instruction_account(invoke_context.transaction_context, 0) + .unwrap(); + + 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 + ] { + callee_account.set_data_from_slice(&new_value).unwrap(); + + update_caller_account( + &invoke_context, + &mut memory_mapping, + false, + &mut caller_account, + &mut callee_account, + true, + ) + .unwrap(); + + 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()); + + // with direct mapping on, serialized_data contains the realloc padding + let realloc_area = &caller_account.serialized_data; + + 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, + &mut 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, + &mut memory_mapping, + false, + &mut caller_account, + &mut callee_account, + false, + ), + Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc + )); + } + #[test] fn test_update_callee_account_lamports_owner() { let transaction_accounts = transaction_with_one_instruction_account(vec![]); @@ -1623,8 +1784,13 @@ mod tests { &[1] ); - let mut mock_caller_account = - MockCallerAccount::new(1234, *account.owner(), 0xFFFFFFFF00000000, account.data()); + let mut mock_caller_account = MockCallerAccount::new( + 1234, + *account.owner(), + 0xFFFFFFFF00000000, + account.data(), + false, + ); let caller_account = mock_caller_account.caller_account(); @@ -1661,8 +1827,13 @@ mod tests { &[1] ); - let mut mock_caller_account = - MockCallerAccount::new(1234, *account.owner(), 0xFFFFFFFF00000000, account.data()); + let mut mock_caller_account = MockCallerAccount::new( + 1234, + *account.owner(), + 0xFFFFFFFF00000000, + account.data(), + false, + ); let mut caller_account = mock_caller_account.caller_account(); @@ -1686,20 +1857,30 @@ mod tests { #[test] fn test_update_callee_account_data_direct_mapping() { - let transaction_accounts = one_instruction_account(b"foobar".to_vec()); + let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); - let mut invoke_context_builder = - MockInvokeContext::new(transaction_accounts, *b"instruction data", [0], &[1]); - let invoke_context = invoke_context_builder.invoke_context(); + mock_invoke_context!( + invoke_context, + transaction_context, + b"instruction data", + transaction_accounts, + &[0], + &[1] + ); let instruction_context = invoke_context .transaction_context .get_current_instruction_context() .unwrap(); - let mut mock_caller_account = - MockCallerAccount::new(1234, *account.owner(), 0xFFFFFFFF00000000, account.data()); + let mut mock_caller_account = MockCallerAccount::new( + 1234, + *account.owner(), + 0xFFFFFFFF00000000, + account.data(), + true, + ); let mut caller_account = mock_caller_account.caller_account(); @@ -1811,18 +1992,54 @@ mod tests { } impl MockCallerAccount { - fn new(lamports: u64, owner: Pubkey, vm_addr: u64, data: &[u8]) -> MockCallerAccount { - // write [len][data] into a vec so we can check that they get - // properly updated by update_caller_account() - let mut d = vec![0; mem::size_of::() + data.len() + MAX_PERMITTED_DATA_INCREASE]; + fn new( + lamports: u64, + owner: Pubkey, + vm_addr: u64, + data: &[u8], + direct_mapping: bool, + ) -> MockCallerAccount { + let mut regions = vec![]; + + let mut d = vec![ + 0; + mem::size_of::() + + if direct_mapping { 0 } else { data.len() } + + MAX_PERMITTED_DATA_INCREASE + ]; + // always write the [len] part even when direct mapping unsafe { ptr::write_unaligned::(d.as_mut_ptr().cast(), data.len() as u64) }; - d[mem::size_of::()..][..data.len()].copy_from_slice(data); - // the memory region must include the realloc data - let regions = vec![MemoryRegion::new_writable(d.as_mut_slice(), vm_addr)]; + // write the account data when not direct mapping + if !direct_mapping { + d[mem::size_of::()..][..data.len()].copy_from_slice(data); + } - // caller_account.serialized_data must have the actual data length - d.truncate(mem::size_of::() + data.len()); + // create a region for [len][data+realloc if !direct_mapping] + let mut region_addr = vm_addr; + let region_len = mem::size_of::() + + if direct_mapping { + 0 + } else { + data.len() + MAX_PERMITTED_DATA_INCREASE + }; + regions.push(MemoryRegion::new_writable(&mut d[..region_len], vm_addr)); + region_addr += region_len as u64; + + if direct_mapping { + // create a region for the directly mapped data + regions.push(MemoryRegion::new_readonly(data, region_addr)); + region_addr += data.len() as u64; + + // create a region for the realloc padding + regions.push(MemoryRegion::new_writable( + &mut d[mem::size_of::()..], + region_addr, + )); + } else { + // caller_account.serialized_data must have the actual data length + d.truncate(mem::size_of::() + data.len()); + } MockCallerAccount { lamports, @@ -1849,7 +2066,7 @@ mod tests { CallerAccount { lamports: &mut self.lamports, owner: &mut self.owner, - original_data_len: data.len(), + original_data_len: self.len as usize, serialized_data: data, vm_data_addr: self.vm_addr + mem::size_of::() as u64, ref_to_len_in_vm: &mut self.len, From 2b76f826af73afaa9823434419cf9ff416ce1765 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 29 Oct 2022 11:21:35 +0100 Subject: [PATCH 19/44] cpi: add tests for mutated readonly accounts --- programs/bpf_loader/src/syscalls/cpi.rs | 150 ++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 11 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 3fc12ea3b6f595..f6153c7cfa1430 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1328,7 +1328,8 @@ mod tests { #[test] fn test_translate_instruction() { - let transaction_accounts = transaction_with_one_instruction_account(b"foo".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foo".to_vec()); mock_invoke_context!( invoke_context, transaction_context, @@ -1372,7 +1373,8 @@ mod tests { #[test] fn test_translate_signers() { - let transaction_accounts = transaction_with_one_instruction_account(b"foo".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foo".to_vec()); mock_invoke_context!( invoke_context, transaction_context, @@ -1407,7 +1409,8 @@ mod tests { #[test] fn test_caller_account_from_account_info() { - let transaction_accounts = transaction_with_one_instruction_account(b"foo".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foo".to_vec()); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( invoke_context, @@ -1453,7 +1456,7 @@ mod tests { #[test] fn test_update_caller_account_lamports_owner() { - let transaction_accounts = transaction_with_one_instruction_account(vec![]); + let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( invoke_context, @@ -1511,7 +1514,8 @@ mod tests { #[test] fn test_update_caller_account_data() { - let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); + 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(); @@ -1633,7 +1637,8 @@ mod tests { #[test] fn test_update_caller_account_data_direct_mapping() { - let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); + 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(); @@ -1772,7 +1777,7 @@ mod tests { #[test] fn test_update_callee_account_lamports_owner() { - let transaction_accounts = transaction_with_one_instruction_account(vec![]); + let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( @@ -1815,7 +1820,8 @@ mod tests { #[test] fn test_update_callee_account_data() { - let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( @@ -1855,9 +1861,100 @@ mod tests { assert_eq!(callee_account.get_data(), caller_account.serialized_data); } + #[test] + fn test_update_callee_account_data_readonly() { + let transaction_accounts = + transaction_with_one_readonly_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 instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + + let mut mock_caller_account = MockCallerAccount::new( + 1234, + *account.owner(), + 0xFFFFFFFF00000000, + account.data(), + false, + ); + + let mut caller_account = mock_caller_account.caller_account(); + + let get_callee = || { + instruction_context + .try_borrow_instruction_account(invoke_context.transaction_context, 0) + .unwrap() + }; + let callee_account = get_callee(); + + caller_account.serialized_data[0] = b'b'; + assert!(matches!( + update_callee_account( + &invoke_context, + false, + &caller_account, + callee_account, + false, + ), + Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::InstructionError(InstructionError::ExternalAccountDataModified) + ) + )); + + // without direct mapping + 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 = get_callee(); + assert!(matches!( + update_callee_account( + &invoke_context, + false, + &caller_account, + callee_account, + false, + ), + Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::InstructionError(InstructionError::AccountDataSizeChanged) + ) + )); + + // with direct mapping + let mut data = b"baz".to_vec(); + *caller_account.ref_to_len_in_vm = 9; + caller_account.serialized_data = &mut data; + + let callee_account = get_callee(); + assert!(matches!( + update_callee_account( + &invoke_context, + false, + &caller_account, + callee_account, + true, + ), + Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::InstructionError(InstructionError::AccountDataSizeChanged) + ) + )); + } + #[test] fn test_update_callee_account_data_direct_mapping() { - let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); mock_invoke_context!( @@ -1920,7 +2017,8 @@ mod tests { #[test] fn test_translate_accounts_rust() { - let transaction_accounts = transaction_with_one_instruction_account(b"foobar".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); let key = transaction_accounts[1].0; let original_data_len = account.data().len(); @@ -2077,7 +2175,9 @@ mod tests { } } - fn transaction_with_one_instruction_account(data: Vec) -> Vec { + fn transaction_with_one_writable_instruction_account( + data: Vec, + ) -> Vec { let program_id = Pubkey::new_unique(); let account = AccountSharedData::from(Account { lamports: 1, @@ -2102,6 +2202,34 @@ mod tests { ] } + fn transaction_with_one_readonly_instruction_account( + data: Vec, + ) -> Vec { + let program_id = Pubkey::new_unique(); + let account_owner = Pubkey::new_unique(); + let account = AccountSharedData::from(Account { + lamports: 1, + data, + owner: account_owner, + executable: false, + rent_epoch: 100, + }); + vec![ + ( + program_id, + AccountSharedData::from(Account { + lamports: 0, + data: vec![], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 0, + }), + false, + ), + (Pubkey::new_unique(), account, true), + ] + } + struct MockInstruction { program_id: Pubkey, accounts: Vec, From 79fea1a62af954581d307747bc553d2e6a371a35 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 29 Oct 2022 22:38:04 +0100 Subject: [PATCH 20/44] cpi: update_caller_account doesn't need to change .serialized_data when direct_mapping is on --- programs/bpf_loader/src/syscalls/cpi.rs | 33 +++++++++---------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index f6153c7cfa1430..95a4fe24a6f283 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1195,27 +1195,18 @@ fn update_caller_account( .fill(0); } } - caller_account.serialized_data = translate_slice_mut::( - memory_mapping, - if direct_mapping { - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64) - } else { - caller_account.vm_data_addr - }, - if direct_mapping { - if is_loader_deprecated { - 0 - } else { - MAX_PERMITTED_DATA_INCREASE - } - } else { - post_len - } as u64, - false, // Don't care since it is byte aligned - invoke_context.get_check_size(), - )?; + + // with direct_mapping on, serialized_data is fixed and holds the + // realloc padding + if !direct_mapping { + caller_account.serialized_data = translate_slice_mut::( + memory_mapping, + caller_account.vm_data_addr, + post_len as u64, + false, // Don't care since it is byte aligned + invoke_context.get_check_size(), + )?; + } // this is the len field in the AccountInfo::data slice *caller_account.ref_to_len_in_vm = post_len as u64; From b218e82f336c38b1bda2b2556b8c27cda20e5647 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 29 Oct 2022 22:40:38 +0100 Subject: [PATCH 21/44] cpi: update_caller_account: ensure that account capacity is always enough Introduce a better way to ensure that account capacity never goes below what might be mapped in memory regions. --- programs/bpf_loader/src/syscalls/cpi.rs | 78 +++++++++++++++++++++++++ sdk/src/account.rs | 4 ++ sdk/src/transaction_context.rs | 30 +++++++--- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 95a4fe24a6f283..36de87f60712f6 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1129,12 +1129,32 @@ fn update_caller_account( *caller_account.owner = *callee_account.get_owner(); if direct_mapping && caller_account.original_data_len > 0 { + // This whole branch can be removed once we stop using AccountSharedData + // within transaction accounts. Mutating an account through + // AccountSharedData (which is possible from builtins), can change the + // allocation (pointer) and the capacity. + // + // 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. + let min_capacity = caller_account.original_data_len; + if callee_account.capacity() < min_capacity { + callee_account + .reserve(min_capacity.saturating_sub(callee_account.capacity())) + .map_err(SyscallError::InstructionError)?; + } + + // replace the region in case the allocation changed let regions = memory_mapping.get_regions(); let memory_region_index = regions .iter() .position(|memory_region| memory_region.vm_addr == caller_account.vm_data_addr) .ok_or(Box::new(InstructionError::ProgramEnvironmentSetupFailure))?; let region = regions.get(memory_region_index).unwrap(); + debug_assert!(callee_account.capacity() >= region.len as usize); + let account_region = if let Ok(data) = callee_account.get_data_mut() { let data = unsafe { std::slice::from_raw_parts_mut(data.as_mut_ptr(), region.len as usize) }; @@ -1766,6 +1786,64 @@ mod tests { )); } + #[test] + fn test_update_caller_account_data_capacity_direct_mapping() { + let transaction_accounts = one_writable_instruction_account(b"foobar".to_vec()); + let account = transaction_accounts[1].1.clone(); + + let mut invoke_context_builder = + MockInvokeContext::new(transaction_accounts, *b"instruction data", [0], &[1]); + let invoke_context = invoke_context_builder.invoke_context(); + + let instruction_context = invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + + 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 mut memory_mapping = + MemoryMapping::new(mock_caller_account.regions.clone(), &config).unwrap(); + + let mut caller_account = mock_caller_account.caller_account(); + + { + let mut account = invoke_context + .transaction_context + .get_account_at_index(1) + .unwrap() + .borrow_mut(); + account.set_data(b"foo".to_vec()); + } + + let mut callee_account = instruction_context + .try_borrow_instruction_account(invoke_context.transaction_context, 0) + .unwrap(); + assert_eq!(callee_account.capacity(), 3); + + update_caller_account( + &invoke_context, + &mut memory_mapping, + false, + &mut caller_account, + &mut callee_account, + true, + ) + .unwrap(); + + assert!(callee_account.capacity() >= caller_account.original_data_len); + } + #[test] fn test_update_callee_account_lamports_owner() { let transaction_accounts = transaction_with_one_writable_instruction_account(vec![]); diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 5e489b227b283c..2455528ca37594 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -541,6 +541,10 @@ impl AccountSharedData { self.data_mut().reserve(additional) } + pub fn capacity(&self) -> usize { + self.data.capacity() + } + fn data_mut(&mut self) -> &mut Vec { Arc::make_mut(&mut self.data) } diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index e9280a788bf6ef..1efbe161bc2b27 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -791,18 +791,17 @@ impl<'a> BorrowedAccount<'a> { /// Overwrites the account data and size (transaction wide). /// - /// You should almost always prefer set_data_from_slice() to this method. - /// This is currently only used by tests and the program-test crate. + /// You should always prefer set_data_from_slice(). Calling this method is + /// currently safe but requires some special casing during CPI when direct + /// account mapping is enabled. + /// + /// Currently only used by tests and the program-test crate. #[cfg(not(target_os = "solana"))] - pub fn set_data(&mut self, mut data: Vec) -> Result<(), InstructionError> { + pub fn set_data(&mut self, data: Vec) -> Result<(), InstructionError> { self.can_data_be_resized(data.len())?; self.can_data_be_changed()?; self.touch()?; - // self.account might be mapped into a MemoryRegion. Ensure that its - // capacity doesn't shrink potentially leaving a hole in vm address - // space. The extra capacity will be zeroed by the runtime. - data.reserve(self.account.data().len().saturating_sub(data.len())); self.update_accounts_resize_delta(data.len())?; self.account.set_data(data); Ok(()) @@ -868,6 +867,23 @@ impl<'a> BorrowedAccount<'a> { 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> { + self.can_data_be_changed()?; + 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() + } + #[cfg(not(target_os = "solana"))] fn make_data_mut(&mut self) { // if the account is still shared, it means this is the first time we're From 0f8aee03c9b4b6bb208e7de5c2d6f115008e9452 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 11 Nov 2022 11:39:18 -0800 Subject: [PATCH 22/44] cpi: zero account capacity using the newly introduced BorrowedAccount::spare_data_capacity_mut() Before we were using BorrowedAccount::get_data_mut() to get the base pointer to the account data, then we were slicing the spare capacity from it. Calling get_data_mut() doesn't work if an account has been closed tho, since the current program doesn't own the account anymore and therefore get_data_mut() errors out. --- programs/bpf_loader/src/syscalls/cpi.rs | 210 +++++++++++++----------- sdk/src/account.rs | 8 +- sdk/src/transaction_context.rs | 11 ++ 3 files changed, 134 insertions(+), 95 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 36de87f60712f6..1d14537cfa8236 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -10,7 +10,7 @@ use { }, transaction_context::BorrowedAccount, }, - std::{mem, slice}, + std::{mem, ptr, slice}, }; /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. @@ -1141,9 +1141,7 @@ fn update_caller_account( // invalid address. let min_capacity = caller_account.original_data_len; if callee_account.capacity() < min_capacity { - callee_account - .reserve(min_capacity.saturating_sub(callee_account.capacity())) - .map_err(SyscallError::InstructionError)?; + callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))? } // replace the region in case the allocation changed @@ -1189,14 +1187,17 @@ fn update_caller_account( if post_len < prev_len { if direct_mapping { if post_len < caller_account.original_data_len { - // zero the spare capacity in the account data - let spare_capacity = unsafe { - slice::from_raw_parts_mut( - callee_account.get_data_mut()?.as_mut_ptr().add(post_len), - caller_account.original_data_len.saturating_sub(post_len), - ) - }; - spare_capacity.fill(0); + // zero the spare capacity in the account data. We only need + // to zero up to the original data length, everything else + // is not accessible from the vm anyway. + let spare_len = caller_account.original_data_len.saturating_sub(post_len); + 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) }; } // zero the spare capacity in the realloc padding @@ -1289,6 +1290,7 @@ mod tests { clock::Epoch, feature_set::bpf_account_data_direct_mapping, instruction::Instruction, + system_program, transaction_context::TransactionAccount, }, std::{ @@ -1337,6 +1339,18 @@ mod tests { }; } + macro_rules! borrow_instruction_account { + ($invoke_context:expr, $index:expr) => {{ + let instruction_context = $invoke_context + .transaction_context + .get_current_instruction_context() + .unwrap(); + instruction_context + .try_borrow_instruction_account($invoke_context.transaction_context, $index) + .unwrap() + }}; + } + #[test] fn test_translate_instruction() { let transaction_accounts = @@ -1478,11 +1492,6 @@ mod tests { &[1] ); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - let mut mock_caller_account = MockCallerAccount::new( 1234, *account.owner(), @@ -1500,9 +1509,7 @@ mod tests { let mut caller_account = mock_caller_account.caller_account(); - let mut callee_account = instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap(); + let mut callee_account = borrow_instruction_account!(invoke_context, 0); callee_account.set_lamports(42).unwrap(); callee_account @@ -1539,11 +1546,6 @@ mod tests { &[1] ); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - let mut mock_caller_account = MockCallerAccount::new( account.lamports(), *account.owner(), @@ -1568,9 +1570,7 @@ mod tests { let serialized_len = || unsafe { *len_ptr.cast::() as usize }; let mut caller_account = mock_caller_account.caller_account(); - let mut callee_account = instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap(); + let mut callee_account = borrow_instruction_account!(invoke_context, 0); for (new_value, expected_realloc_size) in [ (b"foo".to_vec(), MAX_PERMITTED_DATA_INCREASE + 3), @@ -1632,18 +1632,23 @@ mod tests { ), Err(error) if error.downcast_ref::().unwrap() == &InstructionError::InvalidRealloc, )); - } - macro_rules! borrow_instruction_account { - ($invoke_context:expr, $index:expr) => {{ - let instruction_context = $invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - instruction_context - .try_borrow_instruction_account($invoke_context.transaction_context, $index) - .unwrap() - }}; + // 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, + &mut memory_mapping, + false, + &mut caller_account, + &mut callee_account, + false, + ) + .unwrap(); + let data_len = callee_account.get_data().len(); + assert_eq!(data_len, 0); } #[test] @@ -1662,11 +1667,6 @@ mod tests { &[1] ); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - let mut mock_caller_account = MockCallerAccount::new( account.lamports(), *account.owner(), @@ -1691,9 +1691,7 @@ mod tests { let serialized_len = || unsafe { *len_ptr.cast::() as usize }; let mut caller_account = mock_caller_account.caller_account(); - let mut callee_account = instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap(); + let mut callee_account = borrow_instruction_account!(invoke_context, 0); for (new_value, expected_realloc_used) in [ (b"foobazbad".to_vec(), 3), // > original_data_len, writes into realloc @@ -1784,21 +1782,39 @@ mod tests { ), 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, + &mut 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 = one_writable_instruction_account(b"foobar".to_vec()); + let transaction_accounts = + transaction_with_one_writable_instruction_account(b"foobar".to_vec()); let account = transaction_accounts[1].1.clone(); - let mut invoke_context_builder = - MockInvokeContext::new(transaction_accounts, *b"instruction data", [0], &[1]); - let invoke_context = invoke_context_builder.invoke_context(); - - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); + mock_invoke_context!( + invoke_context, + transaction_context, + b"instruction data", + transaction_accounts, + &[0], + &[1] + ); let mut mock_caller_account = MockCallerAccount::new( account.lamports(), @@ -1813,7 +1829,7 @@ mod tests { ..Config::default() }; let mut memory_mapping = - MemoryMapping::new(mock_caller_account.regions.clone(), &config).unwrap(); + MemoryMapping::new(mock_caller_account.regions.split_off(0), &config).unwrap(); let mut caller_account = mock_caller_account.caller_account(); @@ -1826,9 +1842,7 @@ mod tests { account.set_data(b"foo".to_vec()); } - let mut callee_account = instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap(); + let mut callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.capacity(), 3); update_caller_account( @@ -1928,6 +1942,23 @@ mod tests { let callee_account = borrow_instruction_account!(invoke_context, 0); assert_eq!(callee_account.get_data(), caller_account.serialized_data); + + // 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, + false, + &caller_account, + callee_account, + false, + ) + .unwrap(); + let callee_account = borrow_instruction_account!(invoke_context, 0); + assert_eq!(callee_account.get_data(), b""); } #[test] @@ -1945,11 +1976,6 @@ mod tests { &[1] ); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - let mut mock_caller_account = MockCallerAccount::new( 1234, *account.owner(), @@ -1960,12 +1986,7 @@ mod tests { let mut caller_account = mock_caller_account.caller_account(); - let get_callee = || { - instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap() - }; - let callee_account = get_callee(); + let callee_account = borrow_instruction_account!(invoke_context, 0); caller_account.serialized_data[0] = b'b'; assert!(matches!( @@ -1976,9 +1997,7 @@ mod tests { callee_account, false, ), - Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( - SyscallError::InstructionError(InstructionError::ExternalAccountDataModified) - ) + Err(error) if error.downcast_ref::().unwrap() == &InstructionError::ExternalAccountDataModified )); // without direct mapping @@ -1986,7 +2005,7 @@ mod tests { *caller_account.ref_to_len_in_vm = data.len() as u64; caller_account.serialized_data = &mut data; - let callee_account = get_callee(); + let callee_account = borrow_instruction_account!(invoke_context, 0); assert!(matches!( update_callee_account( &invoke_context, @@ -1995,9 +2014,7 @@ mod tests { callee_account, false, ), - Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( - SyscallError::InstructionError(InstructionError::AccountDataSizeChanged) - ) + Err(error) if error.downcast_ref::().unwrap() == &InstructionError::AccountDataSizeChanged )); // with direct mapping @@ -2005,7 +2022,7 @@ mod tests { *caller_account.ref_to_len_in_vm = 9; caller_account.serialized_data = &mut data; - let callee_account = get_callee(); + let callee_account = borrow_instruction_account!(invoke_context, 0); assert!(matches!( update_callee_account( &invoke_context, @@ -2014,9 +2031,7 @@ mod tests { callee_account, true, ), - Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( - SyscallError::InstructionError(InstructionError::AccountDataSizeChanged) - ) + Err(error) if error.downcast_ref::().unwrap() == &InstructionError::AccountDataSizeChanged )); } @@ -2035,11 +2050,6 @@ mod tests { &[1] ); - let instruction_context = invoke_context - .transaction_context - .get_current_instruction_context() - .unwrap(); - let mut mock_caller_account = MockCallerAccount::new( 1234, *account.owner(), @@ -2050,12 +2060,7 @@ mod tests { let mut caller_account = mock_caller_account.caller_account(); - let get_callee = || { - instruction_context - .try_borrow_instruction_account(invoke_context.transaction_context, 0) - .unwrap() - }; - let mut callee_account = get_callee(); + 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 @@ -2079,9 +2084,26 @@ mod tests { true, ) .unwrap(); - callee_account = get_callee(); + 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, + false, + &caller_account, + callee_account, + true, + ) + .unwrap(); + callee_account = borrow_instruction_account!(invoke_context, 0); + assert_eq!(callee_account.get_data(), b""); } #[test] diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 2455528ca37594..0657a5b17637b8 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -13,7 +13,9 @@ use { solana_program::{account_info::AccountInfo, debug_account_data::*, sysvar::Sysvar}, std::{ cell::{Ref, RefCell}, - fmt, ptr, + fmt, + mem::MaybeUninit, + ptr, rc::Rc, sync::Arc, }, @@ -601,6 +603,10 @@ impl AccountSharedData { self.data = Arc::new(data); } + pub fn spare_data_capacity_mut(&mut self) -> &mut [MaybeUninit] { + self.data_mut().spare_capacity_mut() + } + pub fn new(lamports: u64, space: usize, owner: &Pubkey) -> Self { shared_new(lamports, space, owner) } diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 1efbe161bc2b27..c3778a33730dfb 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -13,6 +13,7 @@ use { }, }, solana_program::entrypoint::MAX_PERMITTED_DATA_INCREASE, + std::mem::MaybeUninit, }; use { crate::{ @@ -789,6 +790,16 @@ impl<'a> BorrowedAccount<'a> { 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 From 7cb46e93a068e5f17cf53abb566b05c312f90c02 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 11 Nov 2022 17:19:34 -0800 Subject: [PATCH 23/44] bpf_loader: fix same lint for the umpteenth time --- programs/bpf_loader/src/serialization.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index a18efbf77cbfb0..3fb486afd3b5d0 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -404,7 +404,7 @@ fn serialize_parameters_aligned( s.write::(borrowed_account.get_lamports().to_le()); s.write::((borrowed_account.get_data().len() as u64).to_le()); s.write_account(&mut borrowed_account)?; - s.write::((borrowed_account.get_rent_epoch() as u64).to_le()); + s.write::((borrowed_account.get_rent_epoch()).to_le()); } SerializeAccount::Duplicate(position) => { s.write::(position as u8); From ee7b971d338e5164b3b02ced8ca2698c1f967685 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 15 Dec 2022 21:17:56 +0000 Subject: [PATCH 24/44] bpf_loader: map AccessViolation to ReadonlyDataModified only for account region violations --- programs/bpf_loader/src/lib.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 030ede79da7027..4efacb58068c7f 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -18,7 +18,7 @@ use { }, solana_rbpf::{ aligned_memory::AlignedMemory, - ebpf::{self, HOST_ALIGN, MM_HEAP_START, MM_INPUT_START}, + ebpf::{self, HOST_ALIGN, MM_HEAP_START}, elf::Executable, error::EbpfError, memory_region::{AccessType, MemoryCowCallback, MemoryMapping, MemoryRegion}, @@ -1541,6 +1541,15 @@ fn execute<'a, 'b: 'a>( )?; serialize_time.stop(); + // save the account addresses so in case of AccessViolation below we can + // map to InstructionError::ReadonlyDataModified, which is easier to + // diagnose from developers + let account_region_addrs = regions + .iter() + .map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len)) + .collect::>(); + let addr_is_account_data = |addr: u64| account_region_addrs.iter().any(|r| r.contains(&addr)); + let mut create_vm_time = Measure::start("create_vm"); let mut execute_time; let execution_result = { @@ -1611,8 +1620,11 @@ fn execute<'a, 'b: 'a>( address, _size, _section_name, - )) if *address >= MM_INPUT_START => { - InstructionError::ReadonlyDataModified.into() + )) if addr_is_account_data(*address) => { + // We can get here if direct_mapping is enabled and a program tries to + // write to a readonly account. Map the error to ReadonlyDataModified so + // it's easier for devs to diagnose what happened. + Box::new(InstructionError::ReadonlyDataModified) } _ => error, }; From 460e1857a3dc3cc55141396539174beaf3bfdcdb Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 19 Dec 2022 03:56:05 +0000 Subject: [PATCH 25/44] programs/sbf: realloc: add test for large write after realloc Add a test that after a realloc does a large write that spans the original account length and the realloc area. This ensures that memory mapping works correctly across the boundary. --- programs/sbf/rust/realloc/src/instructions.rs | 12 ++++++ programs/sbf/rust/realloc/src/processor.rs | 38 ++++++++++++++++++- programs/sbf/tests/programs.rs | 34 +++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/programs/sbf/rust/realloc/src/instructions.rs b/programs/sbf/rust/realloc/src/instructions.rs index 75e5cfe3db8d83..831a69029ed166 100644 --- a/programs/sbf/rust/realloc/src/instructions.rs +++ b/programs/sbf/rust/realloc/src/instructions.rs @@ -15,6 +15,7 @@ pub const DEALLOC_AND_ASSIGN_TO_CALLER: u8 = 7; pub const CHECK: u8 = 8; pub const ZERO_INIT: u8 = 9; pub const REALLOC_EXTEND_AND_UNDO: u8 = 10; +pub const EXTEND_AND_WRITE_U64: u8 = 11; pub fn realloc(program_id: &Pubkey, address: &Pubkey, size: usize, bump: &mut u8) -> Instruction { let mut instruction_data = vec![REALLOC, *bump]; @@ -88,3 +89,14 @@ pub fn realloc_extend_and_undo( vec![AccountMeta::new(*address, false)], ) } + +pub fn extend_and_write_u64(program_id: &Pubkey, address: &Pubkey, value: u64) -> Instruction { + let mut instruction_data = vec![EXTEND_AND_WRITE_U64]; + instruction_data.extend_from_slice(&value.to_le_bytes()); + + Instruction::new_with_bytes( + *program_id, + &instruction_data, + vec![AccountMeta::new(*address, false)], + ) +} diff --git a/programs/sbf/rust/realloc/src/processor.rs b/programs/sbf/rust/realloc/src/processor.rs index b0b008ad010e9e..782eb1cd505519 100644 --- a/programs/sbf/rust/realloc/src/processor.rs +++ b/programs/sbf/rust/realloc/src/processor.rs @@ -13,7 +13,7 @@ use { pubkey::Pubkey, system_instruction, system_program, }, - std::convert::TryInto, + std::{convert::TryInto, mem}, }; solana_program::entrypoint!(process_instruction); @@ -61,6 +61,42 @@ fn process_instruction( assert_eq!(new_len, account.data_len()); account.try_borrow_mut_data()?[pre_len..].fill(fill); } + // extend the account and do a 8-bytes write across the original account + // length and the realloc region + EXTEND_AND_WRITE_U64 => { + let pre_len = account.data_len(); + let new_len = mem::size_of::(); + assert!(pre_len < new_len); + account.realloc(new_len, false)?; + assert_eq!(new_len, account.data_len()); + + let (bytes, _) = instruction_data[1..].split_at(new_len); + let value = u64::from_le_bytes(bytes.try_into().unwrap()); + msg!( + "write {} to account {:p}", + value, + account.try_borrow_data().unwrap().as_ptr() + ); + // exercise memory write + unsafe { + *account + .try_borrow_mut_data() + .unwrap() + .as_mut_ptr() + .cast::() = value + }; + // exercise memory read + assert_eq!( + unsafe { + *account + .try_borrow_mut_data() + .unwrap() + .as_ptr() + .cast::() + }, + value + ); + } REALLOC_AND_ASSIGN => { msg!("realloc and assign"); account.realloc(MAX_PERMITTED_DATA_INCREASE, false)?; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 23fb7d33c0cd4c..afa4c8a764d184 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -2921,6 +2921,40 @@ fn test_program_sbf_realloc() { TransactionError::InstructionError(0, InstructionError::InvalidRealloc) ); + // Realloc to 6 bytes + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[ + realloc(&program_id, &pubkey, 6, &mut bump), + ComputeBudgetInstruction::set_accounts_data_size_limit(u32::MAX), + ], + Some(&mint_pubkey), + ), + ) + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(6, data.len()); + + // Extend by 2 bytes and write a u64. This ensures that we can do writes that span the original + // account length (6 bytes) and the realloc data (2 bytes). + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[ + extend_and_write_u64(&program_id, &pubkey, 0x1122334455667788), + ComputeBudgetInstruction::set_accounts_data_size_limit(u32::MAX), + ], + Some(&mint_pubkey), + ), + ) + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(8, data.len()); + assert_eq!(0x1122334455667788, unsafe { *data.as_ptr().cast::() }); + // Realloc to 0 bank_client .send_and_confirm_message( From 12e18b65d5cf60fffdad1af6a10bcbc4093f3655 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 21 Dec 2022 08:02:26 +0000 Subject: [PATCH 26/44] programs/sbf: run test_program_sbf_realloc with both direct_mapping on and off By default test banks test with all features on. This ensures we keep testing the existing code until the new feature is enabled. --- programs/sbf/tests/programs.rs | 477 +++++++++++++++++---------------- 1 file changed, 241 insertions(+), 236 deletions(-) diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index afa4c8a764d184..754bdf6d403040 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -39,7 +39,7 @@ use { clock::MAX_PROCESSING_AGE, compute_budget::ComputeBudgetInstruction, entrypoint::MAX_PERMITTED_DATA_INCREASE, - feature_set::FeatureSet, + feature_set::{self, FeatureSet}, fee::FeeStructure, loader_instruction, message::{v0::LoadedAddresses, SanitizedMessage}, @@ -2761,83 +2761,70 @@ fn test_program_sbf_realloc() { } = create_genesis_config(1_000_000_000_000); let mint_pubkey = mint_keypair.pubkey(); let signer = &[&mint_keypair]; + 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 = Arc::new(bank); + let bank_client = BankClient::new_shared(&bank); - let bank = Bank::new_for_tests(&genesis_config); - let bank = Arc::new(bank); - let bank_client = BankClient::new_shared(&bank); + let program_id = load_program( + &bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_realloc", + ); - let program_id = load_program( - &bank_client, - &bpf_loader::id(), - &mint_keypair, - "solana_sbf_rust_realloc", - ); + let mut bump = 0; + let keypair = Keypair::new(); + let pubkey = keypair.pubkey(); + let account = AccountSharedData::new(START_BALANCE, 5, &program_id); + bank.store_account(&pubkey, &account); - let mut bump = 0; - let keypair = Keypair::new(); - let pubkey = keypair.pubkey(); - let account = AccountSharedData::new(START_BALANCE, 5, &program_id); - bank.store_account(&pubkey, &account); + // Realloc RO account + let mut instruction = realloc(&program_id, &pubkey, 0, &mut bump); + instruction.accounts[0].is_writable = false; + assert_eq!( + bank_client + .send_and_confirm_message(signer, Message::new(&[instruction], Some(&mint_pubkey),),) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) + ); - // Realloc RO account - let mut instruction = realloc(&program_id, &pubkey, 0, &mut bump); - instruction.accounts[0].is_writable = false; - assert_eq!( - bank_client - .send_and_confirm_message(signer, Message::new(&[instruction], Some(&mint_pubkey),),) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) - ); + // Realloc account to overflow + assert_eq!( + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc(&program_id, &pubkey, usize::MAX, &mut bump)], + Some(&mint_pubkey), + ), + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::InvalidRealloc) + ); - // Realloc account to overflow - assert_eq!( + // Realloc account to 0 bank_client .send_and_confirm_message( signer, Message::new( - &[realloc(&program_id, &pubkey, usize::MAX, &mut bump)], + &[realloc(&program_id, &pubkey, 0, &mut bump)], Some(&mint_pubkey), ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::InvalidRealloc) - ); - - // Realloc account to 0 - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[realloc(&program_id, &pubkey, 0, &mut bump)], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(0, data.len()); - - // Realloc account to max then undo - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[realloc_extend_and_undo( - &program_id, - &pubkey, - MAX_PERMITTED_DATA_INCREASE, - &mut bump, - )], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(0, data.len()); + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(0, data.len()); - // Realloc account to max + 1 then undo - assert_eq!( + // Realloc account to max then undo bank_client .send_and_confirm_message( signer, @@ -2845,234 +2832,252 @@ fn test_program_sbf_realloc() { &[realloc_extend_and_undo( &program_id, &pubkey, - MAX_PERMITTED_DATA_INCREASE + 1, + MAX_PERMITTED_DATA_INCREASE, &mut bump, )], Some(&mint_pubkey), ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::InvalidRealloc) - ); + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(0, data.len()); - // Realloc to max + 1 - assert_eq!( + // Realloc account to max + 1 then undo + assert_eq!( + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc_extend_and_undo( + &program_id, + &pubkey, + MAX_PERMITTED_DATA_INCREASE + 1, + &mut bump, + )], + Some(&mint_pubkey), + ), + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::InvalidRealloc) + ); + + // Realloc to max + 1 + assert_eq!( + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc( + &program_id, + &pubkey, + MAX_PERMITTED_DATA_INCREASE + 1, + &mut bump + )], + Some(&mint_pubkey), + ), + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::InvalidRealloc) + ); + + // Realloc to max length in max increase increments + for i in 0..MAX_PERMITTED_DATA_LENGTH as usize / MAX_PERMITTED_DATA_INCREASE { + let mut bump = i as u64; + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc_extend_and_fill( + &program_id, + &pubkey, + MAX_PERMITTED_DATA_INCREASE, + 1, + &mut bump, + )], + Some(&mint_pubkey), + ), + ) + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!((i + 1) * MAX_PERMITTED_DATA_INCREASE, data.len()); + } + for i in 0..data.len() { + assert_eq!(data[i], 1); + } + + // and one more time should fail + assert_eq!( + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc_extend( + &program_id, + &pubkey, + MAX_PERMITTED_DATA_INCREASE, + &mut bump + )], + Some(&mint_pubkey), + ) + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::InvalidRealloc) + ); + + // Realloc to 6 bytes bank_client .send_and_confirm_message( signer, Message::new( - &[realloc( - &program_id, - &pubkey, - MAX_PERMITTED_DATA_INCREASE + 1, - &mut bump - )], + &[realloc(&program_id, &pubkey, 6, &mut bump)], Some(&mint_pubkey), ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::InvalidRealloc) - ); + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(6, data.len()); - // Realloc to max length in max increase increments - for i in 0..MAX_PERMITTED_DATA_LENGTH as usize / MAX_PERMITTED_DATA_INCREASE { - let mut bump = i as u64; + // Extend by 2 bytes and write a u64. This ensures that we can do writes that span the original + // account length (6 bytes) and the realloc data (2 bytes). bank_client .send_and_confirm_message( signer, Message::new( - &[realloc_extend_and_fill( + &[extend_and_write_u64( &program_id, &pubkey, - MAX_PERMITTED_DATA_INCREASE, - 1, - &mut bump, + 0x1122334455667788, )], Some(&mint_pubkey), ), ) .unwrap(); let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!((i + 1) * MAX_PERMITTED_DATA_INCREASE, data.len()); - } - for i in 0..data.len() { - assert_eq!(data[i], 1); - } + assert_eq!(8, data.len()); + assert_eq!(0x1122334455667788, unsafe { *data.as_ptr().cast::() }); - // and one more time should fail - assert_eq!( + // Realloc to 0 bank_client .send_and_confirm_message( signer, Message::new( - &[realloc_extend( - &program_id, - &pubkey, - MAX_PERMITTED_DATA_INCREASE, - &mut bump - )], + &[realloc(&program_id, &pubkey, 0, &mut bump)], Some(&mint_pubkey), - ) + ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::InvalidRealloc) - ); - - // Realloc to 6 bytes - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[ - realloc(&program_id, &pubkey, 6, &mut bump), - ComputeBudgetInstruction::set_accounts_data_size_limit(u32::MAX), - ], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(6, data.len()); - - // Extend by 2 bytes and write a u64. This ensures that we can do writes that span the original - // account length (6 bytes) and the realloc data (2 bytes). - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[ - extend_and_write_u64(&program_id, &pubkey, 0x1122334455667788), - ComputeBudgetInstruction::set_accounts_data_size_limit(u32::MAX), - ], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(8, data.len()); - assert_eq!(0x1122334455667788, unsafe { *data.as_ptr().cast::() }); - - // Realloc to 0 - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[realloc(&program_id, &pubkey, 0, &mut bump)], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(0, data.len()); - - // Realloc and assign - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[Instruction::new_with_bytes( - program_id, - &[REALLOC_AND_ASSIGN], - vec![AccountMeta::new(pubkey, false)], - )], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let account = bank.get_account(&pubkey).unwrap(); - assert_eq!(&solana_sdk::system_program::id(), account.owner()); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(MAX_PERMITTED_DATA_INCREASE, data.len()); + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(0, data.len()); - // Realloc to 0 with wrong owner - assert_eq!( + // Realloc and assign bank_client .send_and_confirm_message( signer, Message::new( - &[realloc(&program_id, &pubkey, 0, &mut bump)], + &[Instruction::new_with_bytes( + program_id, + &[REALLOC_AND_ASSIGN], + vec![AccountMeta::new(pubkey, false)], + )], Some(&mint_pubkey), ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::AccountDataSizeChanged) - ); + .unwrap(); + let account = bank.get_account(&pubkey).unwrap(); + assert_eq!(&solana_sdk::system_program::id(), account.owner()); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(MAX_PERMITTED_DATA_INCREASE, data.len()); - // realloc and assign to self via cpi - assert_eq!( + // Realloc to 0 with wrong owner + assert_eq!( + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc(&program_id, &pubkey, 0, &mut bump)], + Some(&mint_pubkey), + ), + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::AccountDataSizeChanged) + ); + + // realloc and assign to self via cpi + assert_eq!( + bank_client + .send_and_confirm_message( + &[&mint_keypair, &keypair], + Message::new( + &[Instruction::new_with_bytes( + program_id, + &[REALLOC_AND_ASSIGN_TO_SELF_VIA_SYSTEM_PROGRAM], + vec![ + AccountMeta::new(pubkey, true), + AccountMeta::new(solana_sdk::system_program::id(), false), + ], + )], + Some(&mint_pubkey), + ) + ) + .unwrap_err() + .unwrap(), + TransactionError::InstructionError(0, InstructionError::AccountDataSizeChanged) + ); + + // Assign to self and realloc via cpi bank_client .send_and_confirm_message( &[&mint_keypair, &keypair], Message::new( &[Instruction::new_with_bytes( program_id, - &[REALLOC_AND_ASSIGN_TO_SELF_VIA_SYSTEM_PROGRAM], + &[ASSIGN_TO_SELF_VIA_SYSTEM_PROGRAM_AND_REALLOC], vec![ AccountMeta::new(pubkey, true), AccountMeta::new(solana_sdk::system_program::id(), false), ], )], Some(&mint_pubkey), - ) + ), ) - .unwrap_err() - .unwrap(), - TransactionError::InstructionError(0, InstructionError::AccountDataSizeChanged) - ); - - // Assign to self and realloc via cpi - bank_client - .send_and_confirm_message( - &[&mint_keypair, &keypair], - Message::new( - &[Instruction::new_with_bytes( - program_id, - &[ASSIGN_TO_SELF_VIA_SYSTEM_PROGRAM_AND_REALLOC], - vec![ - AccountMeta::new(pubkey, true), - AccountMeta::new(solana_sdk::system_program::id(), false), - ], - )], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let account = bank.get_account(&pubkey).unwrap(); - assert_eq!(&program_id, account.owner()); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(2 * MAX_PERMITTED_DATA_INCREASE, data.len()); + .unwrap(); + let account = bank.get_account(&pubkey).unwrap(); + assert_eq!(&program_id, account.owner()); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(2 * MAX_PERMITTED_DATA_INCREASE, data.len()); - // Realloc to 0 - bank_client - .send_and_confirm_message( - signer, - Message::new( - &[realloc(&program_id, &pubkey, 0, &mut bump)], - Some(&mint_pubkey), - ), - ) - .unwrap(); - let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); - assert_eq!(0, data.len()); + // Realloc to 0 + bank_client + .send_and_confirm_message( + signer, + Message::new( + &[realloc(&program_id, &pubkey, 0, &mut bump)], + Some(&mint_pubkey), + ), + ) + .unwrap(); + let data = bank_client.get_account_data(&pubkey).unwrap().unwrap(); + assert_eq!(0, data.len()); - // zero-init - bank_client - .send_and_confirm_message( - &[&mint_keypair, &keypair], - Message::new( - &[Instruction::new_with_bytes( - program_id, - &[ZERO_INIT], - vec![AccountMeta::new(pubkey, true)], - )], - Some(&mint_pubkey), - ), - ) - .unwrap(); + // zero-init + bank_client + .send_and_confirm_message( + &[&mint_keypair, &keypair], + Message::new( + &[Instruction::new_with_bytes( + program_id, + &[ZERO_INIT], + vec![AccountMeta::new(pubkey, true)], + )], + Some(&mint_pubkey), + ), + ) + .unwrap(); + } } #[test] From 91a6db8d27d03f8335cd0586e91f6ef7b7408976 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 22 Dec 2022 07:47:02 +0000 Subject: [PATCH 27/44] bpf_loader: tweak memcmp syscall Split the actual memcmp code in a separate function. Remove check indexing the slices since the slices are guaranteed to have the correct length by construction. --- programs/bpf_loader/src/syscalls/mem_ops.rs | 33 ++++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index d501311461339f..a7ff9e9c93cdde 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -123,17 +123,14 @@ declare_syscall!( cmp_result_addr, invoke_context.get_check_aligned(), )?; - let mut i = 0; - while i < n as usize { - let a = *s1.get(i).ok_or(SyscallError::InvalidLength)?; - let b = *s2.get(i).ok_or(SyscallError::InvalidLength)?; - if a != b { - *cmp_result = (a as i32).saturating_sub(b as i32); - return Ok(0); - }; - i = i.saturating_add(1); - } - *cmp_result = 0; + + 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. + *cmp_result = unsafe { memcmp(s1, s2, n as usize) }; Ok(0) } ); @@ -165,3 +162,17 @@ declare_syscall!( Ok(0) } ); + +// Marked unsafe since it assumes that the slices are at least `n` bytes long. +#[allow(clippy::indexing_slicing)] +unsafe fn memcmp(s1: &[u8], s2: &[u8], n: usize) -> i32 { + for i in 0..n { + let a = *s1.get_unchecked(i); + let b = *s2.get_unchecked(i); + if a != b { + return (a as i32).saturating_sub(b as i32); + }; + } + + 0 +} From 3c756c1e258dd117e45766f1dc89fc638b67700a Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 22 Dec 2022 07:48:46 +0000 Subject: [PATCH 28/44] bpf_loader: tweak the memset syscall Use slice::fill, which is effectively memset. --- programs/bpf_loader/src/syscalls/mem_ops.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index a7ff9e9c93cdde..e2614beb921427 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -156,9 +156,7 @@ declare_syscall!( invoke_context.get_check_aligned(), invoke_context.get_check_size(), )?; - for val in s.iter_mut().take(n as usize) { - *val = c as u8; - } + s.fill(c as u8); Ok(0) } ); From c4ca5adaccb4d0bac74b2cc17a6c3f90d8714f42 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 8 Jan 2023 21:37:33 +0000 Subject: [PATCH 29/44] bpf_loader: syscalls: update mem syscalls to work with non contiguous memory With direct mapping enabled, accounts can now span multiple memory regions. --- programs/bpf_loader/src/syscalls/mem_ops.rs | 755 ++++++++++++++++++-- 1 file changed, 676 insertions(+), 79 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index e2614beb921427..8af3e92e9edd1f 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -1,4 +1,4 @@ -use {super::*, crate::declare_syscall}; +use {super::*, crate::declare_syscall, solana_rbpf::memory_region::MemoryRegion, std::slice}; fn mem_op_consume(invoke_context: &mut InvokeContext, n: u64) -> Result<(), Error> { let compute_budget = invoke_context.get_compute_budget(); @@ -26,32 +26,8 @@ declare_syscall!( return Err(SyscallError::CopyOverlapping.into()); } - let dst_ptr = translate_slice_mut::( - memory_mapping, - dst_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )? - .as_mut_ptr(); - let src_ptr = translate_slice::( - memory_mapping, - src_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )? - .as_ptr(); - if !is_nonoverlapping(src_ptr as usize, n as usize, dst_ptr as usize, n as usize) { - unsafe { - std::ptr::copy(src_ptr, dst_ptr, n as usize); - } - } else { - unsafe { - std::ptr::copy_nonoverlapping(src_ptr, dst_ptr, n as usize); - } - } - Ok(0) + // host addresses can overlap so we always invoke memmove + memmove(invoke_context, dst_addr, src_addr, n, memory_mapping) } ); @@ -69,24 +45,7 @@ declare_syscall!( ) -> Result { mem_op_consume(invoke_context, n)?; - let dst = translate_slice_mut::( - memory_mapping, - dst_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; - let src = translate_slice::( - memory_mapping, - src_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; - unsafe { - std::ptr::copy(src.as_ptr(), dst.as_mut_ptr(), n as usize); - } - Ok(0) + memmove(invoke_context, dst_addr, src_addr, n, memory_mapping) } ); @@ -104,33 +63,46 @@ declare_syscall!( ) -> Result { mem_op_consume(invoke_context, n)?; - let s1 = translate_slice::( - memory_mapping, - s1_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; - let s2 = translate_slice::( - memory_mapping, - s2_addr, - n, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; - let cmp_result = translate_type_mut::( - memory_mapping, - cmp_result_addr, - 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. - *cmp_result = unsafe { memcmp(s1, s2, n as usize) }; + if invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()) + { + let cmp_result = translate_type_mut::( + memory_mapping, + cmp_result_addr, + invoke_context.get_check_aligned(), + )?; + *cmp_result = memcmp_non_contiguous(s1_addr, s2_addr, n, memory_mapping)?; + } else { + let s1 = translate_slice::( + memory_mapping, + s1_addr, + n, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + let s2 = translate_slice::( + memory_mapping, + s2_addr, + n, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + let cmp_result = translate_type_mut::( + memory_mapping, + cmp_result_addr, + 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. + *cmp_result = unsafe { memcmp(s1, s2, n as usize) }; + } + Ok(0) } ); @@ -140,7 +112,7 @@ declare_syscall!( SyscallMemset, fn inner_call( invoke_context: &mut InvokeContext, - s_addr: u64, + dst_addr: u64, c: u64, n: u64, _arg4: u64, @@ -149,20 +121,89 @@ declare_syscall!( ) -> Result { mem_op_consume(invoke_context, n)?; - let s = translate_slice_mut::( + if invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()) + { + memset_non_contiguous(dst_addr, c as u8, n, memory_mapping) + } else { + let s = translate_slice_mut::( + memory_mapping, + dst_addr, + n, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + 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 + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()) + { + memmove_non_contiguous(dst_addr, src_addr, n, memory_mapping) + } else { + let dst_ptr = translate_slice_mut::( memory_mapping, - s_addr, + dst_addr, n, invoke_context.get_check_aligned(), invoke_context.get_check_size(), - )?; - s.fill(c as u8); + )? + .as_mut_ptr(); + let src_ptr = translate_slice::( + memory_mapping, + src_addr, + n, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + .as_ptr(); + + unsafe { std::ptr::copy(src_ptr, dst_ptr, n as usize) }; Ok(0) } -); +} + +fn memmove_non_contiguous( + dst_addr: u64, + src_addr: u64, + n: u64, + memory_mapping: &MemoryMapping, +) -> Result { + let reverse = dst_addr.wrapping_sub(src_addr) < n; + iter_memory_pair_chunks( + AccessType::Load, + src_addr, + AccessType::Store, + dst_addr, + n, + memory_mapping, + reverse, + |src_host_addr, dst_host_addr, chunk_len| { + unsafe { + std::ptr::copy( + src_host_addr as *const u8, + dst_host_addr as *mut u8, + chunk_len, + ) + }; + Ok(0) + }, + ) +} // Marked unsafe since it assumes that the slices are at least `n` bytes long. -#[allow(clippy::indexing_slicing)] unsafe fn memcmp(s1: &[u8], s2: &[u8], n: usize) -> i32 { for i in 0..n { let a = *s1.get_unchecked(i); @@ -174,3 +215,559 @@ unsafe fn memcmp(s1: &[u8], s2: &[u8], n: usize) -> i32 { 0 } + +fn memcmp_non_contiguous( + src_addr: u64, + dst_addr: u64, + n: u64, + memory_mapping: &MemoryMapping, +) -> Result { + match iter_memory_pair_chunks( + AccessType::Load, + src_addr, + AccessType::Load, + dst_addr, + n, + memory_mapping, + false, + |s1_addr, s2_addr, chunk_len| { + let res = unsafe { + let s1 = slice::from_raw_parts(s1_addr as *const u8, chunk_len); + let s2 = slice::from_raw_parts(s2_addr as *const u8, 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)); + } + Ok(0) + }, + ) { + Ok(res) => Ok(res), + Err(MemcmpError::Diff(diff)) => Ok(diff), + Err(MemcmpError::Ebpf(e)) => Err(e), + } +} + +#[derive(Debug)] +enum MemcmpError { + Ebpf(EbpfError), + Diff(i32), +} + +impl From for MemcmpError { + fn from(err: EbpfError) -> Self { + MemcmpError::Ebpf(err) + } +} + +fn memset_non_contiguous( + dst_addr: u64, + c: u8, + n: u64, + memory_mapping: &MemoryMapping, +) -> Result { + let dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, AccessType::Store, dst_addr, n); + for item in dst_chunk_iter { + let (dst_region, dst_vm_addr, dst_len) = item?; + let dst_host_addr = Result::from(dst_region.vm_to_host(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) +} + +fn iter_memory_pair_chunks( + src_access: AccessType, + src_addr: u64, + dst_access: AccessType, + mut dst_addr: u64, + n: u64, + memory_mapping: &MemoryMapping, + reverse: bool, + mut fun: F, +) -> Result +where + T: Default, + E: From, + F: FnMut(*const u8, *const u8, usize) -> Result, +{ + let mut src_chunk_iter = MemoryChunkIterator::new(memory_mapping, src_access, src_addr, n); + loop { + // iterate source chunks + let (src_region, src_vm_addr, mut src_len) = match if reverse { + src_chunk_iter.next_back() + } else { + src_chunk_iter.next() + } { + Some(item) => item?, + None => break, + }; + + let mut src_host_addr = Result::from(src_region.vm_to_host(src_vm_addr, src_len as u64))?; + let mut dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, dst_access, dst_addr, n); + // iterate over destination chunks until this source chunk has been completely copied + while src_len > 0 { + loop { + let (dst_region, dst_vm_addr, dst_len) = match if reverse { + dst_chunk_iter.next_back() + } else { + dst_chunk_iter.next() + } { + Some(item) => item?, + None => break, + }; + let dst_host_addr = + Result::from(dst_region.vm_to_host(dst_vm_addr, dst_len as u64))?; + let chunk_len = src_len.min(dst_len); + fun( + src_host_addr as *const u8, + dst_host_addr as *const u8, + chunk_len as usize, + )?; + src_len = src_len.saturating_sub(chunk_len); + if reverse { + dst_addr = dst_addr.saturating_sub(chunk_len as u64); + } else { + dst_addr = dst_addr.saturating_add(chunk_len as u64); + } + if src_len == 0 { + break; + } + src_host_addr = src_host_addr.saturating_add(chunk_len as u64); + } + } + } + + Ok(T::default()) +} + +struct MemoryChunkIterator<'a> { + memory_mapping: &'a MemoryMapping<'a>, + 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, +} + +impl<'a> MemoryChunkIterator<'a> { + fn new( + memory_mapping: &'a MemoryMapping, + access_type: AccessType, + vm_addr: u64, + len: u64, + ) -> MemoryChunkIterator<'a> { + MemoryChunkIterator { + memory_mapping, + access_type, + initial_vm_addr: vm_addr, + len, + vm_addr_start: vm_addr, + vm_addr_end: vm_addr.saturating_add(len), + } + } + + fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, EbpfError> { + match self.memory_mapping.region(self.access_type, vm_addr) { + Ok(region) => Ok(region), + Err(EbpfError::AccessViolation(pc, access_type, _vm_addr, _len, name)) => Err( + EbpfError::AccessViolation(pc, access_type, self.initial_vm_addr, self.len, name), + ), + Err(EbpfError::StackAccessViolation(pc, access_type, _vm_addr, _len, frame)) => { + Err(EbpfError::StackAccessViolation( + pc, + access_type, + self.initial_vm_addr, + self.len, + frame, + )) + } + Err(e) => Err(e), + } + } +} + +impl<'a> Iterator for MemoryChunkIterator<'a> { + type Item = Result<(&'a MemoryRegion, u64, usize), EbpfError>; + + 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 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<'a> DoubleEndedIterator for MemoryChunkIterator<'a> { + 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 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)] +mod tests { + use {super::*, solana_rbpf::ebpf::MM_PROGRAM_START}; + + #[test] + fn test_memory_chunk_iterator_empty() { + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new(vec![], &config).unwrap(); + let mut src_chunk_iter = MemoryChunkIterator::new(&memory_mapping, AccessType::Load, 0, 1); + assert!(src_chunk_iter.next().unwrap().is_err()); + } + + 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).unwrap(); + + let mut src_chunk_iter = MemoryChunkIterator::new(&memory_mapping, AccessType::Load, 0, 1); + src_chunk_iter.next().unwrap().unwrap(); + } + + #[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_PROGRAM_START)], + &config, + ) + .unwrap(); + + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START - 1, 1); + assert!(src_chunk_iter.next().unwrap().is_err()); + + for (vm_addr, len) in [ + (MM_PROGRAM_START, 0), + (MM_PROGRAM_START + 42, 0), + (MM_PROGRAM_START, 1), + (MM_PROGRAM_START, 42), + (MM_PROGRAM_START + 41, 1), + ] { + for rev in [true, false] { + let iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len); + 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)]); + } + } + } + + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START + 42, 1); + assert!(src_chunk_iter.next().unwrap().is_err()); + } + + #[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_PROGRAM_START), + MemoryRegion::new_readonly(&mem2, MM_PROGRAM_START + 8), + ], + &config, + ) + .unwrap(); + + for (vm_addr, len, mut expected) in [ + (MM_PROGRAM_START, 8, vec![(MM_PROGRAM_START, 8)]), + ( + MM_PROGRAM_START + 7, + 2, + vec![(MM_PROGRAM_START + 7, 1), (MM_PROGRAM_START + 8, 1)], + ), + (MM_PROGRAM_START + 8, 4, vec![(MM_PROGRAM_START + 8, 4)]), + ] { + for rev in [false, true] { + let iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len); + let res = if rev { + expected.reverse(); + to_chunk_vec(iter.rev()) + } else { + to_chunk_vec(iter) + }; + + assert_eq!(res, expected); + } + } + } + + #[test] + #[should_panic(expected = "AccessViolation(0, 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_PROGRAM_START), + MemoryRegion::new_readonly(&mem2, MM_PROGRAM_START + 8), + ], + &config, + ) + .unwrap(); + + memmove_non_contiguous(MM_PROGRAM_START, MM_PROGRAM_START + 8, 4, &memory_mapping).unwrap(); + } + + #[test] + fn test_overlapping_memmove_non_contiguous_right() { + 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_PROGRAM_START), + MemoryRegion::new_writable(&mut mem2, MM_PROGRAM_START + 1), + MemoryRegion::new_writable(&mut mem3, MM_PROGRAM_START + 3), + MemoryRegion::new_writable(&mut mem4, MM_PROGRAM_START + 6), + ], + &config, + ) + .unwrap(); + + // overlapping memmove right - the implementation will copy backwards + assert_eq!( + memmove_non_contiguous(MM_PROGRAM_START + 1, MM_PROGRAM_START, 7, &memory_mapping) + .unwrap(), + 0 + ); + assert_eq!(&mem1, &[0x11]); + assert_eq!(&mem2, &[0x11, 0x22]); + assert_eq!(&mem3, &[0x22, 0x33, 0x33]); + assert_eq!(&mem4, &[0x33, 0x44, 0x44, 0x44]); + } + + #[test] + fn test_overlapping_memmove_non_contiguous_left() { + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let mut 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_writable(&mut mem1, MM_PROGRAM_START), + MemoryRegion::new_writable(&mut mem2, MM_PROGRAM_START + 1), + MemoryRegion::new_writable(&mut mem3, MM_PROGRAM_START + 3), + MemoryRegion::new_writable(&mut mem4, MM_PROGRAM_START + 6), + ], + &config, + ) + .unwrap(); + + // overlapping memmove left - the implementation will copy forward + assert_eq!( + memmove_non_contiguous(MM_PROGRAM_START, MM_PROGRAM_START + 1, 7, &memory_mapping) + .unwrap(), + 0 + ); + assert_eq!(&mem1, &[0x22]); + assert_eq!(&mem2, &[0x22, 0x33]); + assert_eq!(&mem3, &[0x33, 0x33, 0x44]); + assert_eq!(&mem4, &[0x44, 0x44, 0x44, 0x44]); + } + + #[test] + #[should_panic(expected = "AccessViolation(0, 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_PROGRAM_START), + MemoryRegion::new_readonly(&mem2, MM_PROGRAM_START + 8), + ], + &config, + ) + .unwrap(); + + assert_eq!( + memset_non_contiguous(MM_PROGRAM_START, 0x33, 9, &memory_mapping).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_PROGRAM_START), + MemoryRegion::new_writable(&mut mem2, MM_PROGRAM_START + 1), + MemoryRegion::new_writable(&mut mem3, MM_PROGRAM_START + 3), + MemoryRegion::new_writable(&mut mem4, MM_PROGRAM_START + 6), + ], + &config, + ) + .unwrap(); + + assert_eq!( + memset_non_contiguous(MM_PROGRAM_START + 1, 0x55, 7, &memory_mapping).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_PROGRAM_START), + MemoryRegion::new_readonly(&mem2, MM_PROGRAM_START + 3), + MemoryRegion::new_readonly(&mem3, MM_PROGRAM_START + 9), + ], + &config, + ) + .unwrap(); + + // non contiguous src + assert_eq!( + memcmp_non_contiguous(MM_PROGRAM_START, MM_PROGRAM_START + 9, 9, &memory_mapping) + .unwrap(), + 0 + ); + + // non contiguous dst + assert_eq!( + memcmp_non_contiguous( + MM_PROGRAM_START + 10, + MM_PROGRAM_START + 1, + 8, + &memory_mapping + ) + .unwrap(), + 0 + ); + + // diff + assert_eq!( + memcmp_non_contiguous( + MM_PROGRAM_START + 1, + MM_PROGRAM_START + 11, + 5, + &memory_mapping + ) + .unwrap(), + unsafe { memcmp(b"oobar", b"obarb", 5) } + ); + } +} From f8040ecdec8b2b738946e35ec920b8eeeb4172ce Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 9 Jan 2023 01:45:21 +0000 Subject: [PATCH 30/44] fix lint, rebase mem_ops --- programs/bpf_loader/src/syscalls/mem_ops.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 8af3e92e9edd1f..ae110cc7705efc 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -326,7 +326,7 @@ where fun( src_host_addr as *const u8, dst_host_addr as *const u8, - chunk_len as usize, + chunk_len, )?; src_len = src_len.saturating_sub(chunk_len); if reverse { From 5ade07618c9e90d6dd0e313cae406083b23c7a9f Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 13 Feb 2023 21:16:25 +0000 Subject: [PATCH 31/44] Implement CoW for writable accounts --- programs/bpf_loader/src/serialization.rs | 50 ++++++- programs/bpf_loader/src/syscalls/cpi.rs | 40 +++--- sdk/src/transaction_context.rs | 170 +++++++++++++++++------ 3 files changed, 194 insertions(+), 66 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 3fb486afd3b5d0..fb4780f81223a9 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -8,6 +8,7 @@ use { memory_region::MemoryRegion, }, solana_sdk::{ + account::WritableAccount, bpf_loader_deprecated, entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, NON_DUP_MARKER}, instruction::InstructionError, @@ -17,7 +18,10 @@ use { BorrowedAccount, IndexOfAccount, InstructionContext, TransactionContext, }, }, - std::mem::{self, size_of}, + std::{ + mem::{self, size_of}, + sync::Arc, + }, }; /// Maximum number of instruction accounts that can be serialized into the @@ -81,7 +85,7 @@ impl Serializer { if self.copy_account_data { self.write_all(account.get_data()); } else { - self.push_account_region(account); + self.push_account_region(account)?; } if self.aligned { @@ -105,18 +109,54 @@ impl Serializer { Ok(()) } - fn push_account_region(&mut self, account: &mut BorrowedAccount<'_>) { + fn push_account_region( + &mut self, + account: &mut BorrowedAccount<'_>, + ) -> Result<(), InstructionError> { self.push_region(); let account_len = account.get_data().len(); if account_len > 0 { - let region = if let Ok(data) = account.get_data_mut() { - MemoryRegion::new_writable(data, self.vaddr) + let region = if account.can_data_be_changed().is_ok() { + if account.is_shared() { + // If the account is still shared it means it wasn't written to yet during this + // transaction. We map it as CoW and it'll be copied the first time something + // tries to write into it. + let accounts = Arc::clone(account.transaction_context().accounts()); + let index_in_transaction = account.get_index_in_transaction(); + + MemoryRegion::new_cow( + account.get_data(), + self.vaddr, + Box::new(move || { + // The two calls below can't relly 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 + .try_borrow_mut(index_in_transaction) + .map_err(|_| ())?; + accounts.touch(index_in_transaction).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) + }), + ) + } else { + // The account isn't shared anymore, meaning it was written to earlier during + // this transaction. We can just map as writable no need for any fancy CoW + // business. + MemoryRegion::new_writable(account.get_data_mut()?, self.vaddr) + } } else { MemoryRegion::new_readonly(account.get_data(), self.vaddr) }; self.vaddr += region.len; self.regions.push(region); } + Ok(()) } fn push_region(&mut self) { diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 1d14537cfa8236..f572a7fd4eed5c 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1129,13 +1129,12 @@ fn update_caller_account( *caller_account.owner = *callee_account.get_owner(); if direct_mapping && caller_account.original_data_len > 0 { - // This whole branch can be removed once we stop using AccountSharedData - // within transaction accounts. Mutating an account through - // AccountSharedData (which is possible from builtins), can change the - // allocation (pointer) and the capacity. - // + // If an account's data pointer has changed - because of CoW or because + // of using AccountSharedData directly (deprecated) - we must update the + // corresponding MemoryRegion in the caller's address space. + // Since each instruction account is directly mapped in a memory region - // with a fixed length, upon returning from CPI we must ensure that the + // 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. @@ -1144,7 +1143,7 @@ fn update_caller_account( callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))? } - // replace the region in case the allocation changed + // find the account's MemoryRegion let regions = memory_mapping.get_regions(); let memory_region_index = regions .iter() @@ -1153,17 +1152,24 @@ fn update_caller_account( let region = regions.get(memory_region_index).unwrap(); debug_assert!(callee_account.capacity() >= region.len as usize); - let account_region = if let Ok(data) = callee_account.get_data_mut() { - let data = - unsafe { std::slice::from_raw_parts_mut(data.as_mut_ptr(), region.len as usize) }; - MemoryRegion::new_writable(data, caller_account.vm_data_addr) - } else { - let data = unsafe { - std::slice::from_raw_parts(callee_account.get_data().as_ptr(), region.len as usize) + // If the account's data pointer has changed, update the region + if region.host_addr.get() != callee_account.get_data().as_ptr() as u64 { + let new_region = if let Ok(data) = callee_account.get_data_mut() { + let data = unsafe { + std::slice::from_raw_parts_mut(data.as_mut_ptr(), region.len as usize) + }; + MemoryRegion::new_writable(data, caller_account.vm_data_addr) + } else { + let data = unsafe { + std::slice::from_raw_parts( + callee_account.get_data().as_ptr(), + region.len as usize, + ) + }; + MemoryRegion::new_readonly(data, caller_account.vm_data_addr) }; - MemoryRegion::new_readonly(data, caller_account.vm_data_addr) - }; - memory_mapping.replace_region(memory_region_index, account_region)?; + memory_mapping.replace_region(memory_region_index, new_region)?; + } } let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index c3778a33730dfb..6d9af8a68967dc 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -22,9 +22,10 @@ use { pubkey::Pubkey, }, std::{ - cell::{RefCell, RefMut}, + cell::{Ref, RefCell, RefMut}, collections::HashSet, pin::Pin, + sync::Arc, }, }; @@ -55,15 +56,93 @@ pub struct InstructionAccount { /// An account key and the matching account pub type TransactionAccount = (Pubkey, AccountSharedData); +#[derive(Clone, Debug, PartialEq)] +pub struct TransactionAccounts { + accounts: Vec>, + // FIXME: use bitfield + touched_flags: RefCell>, + is_early_verification_of_account_modifications_enabled: bool, +} + +#[cfg(not(target_os = "solana"))] +impl TransactionAccounts { + fn new( + accounts: Vec>, + is_early_verification_of_account_modifications_enabled: bool, + ) -> TransactionAccounts { + TransactionAccounts { + touched_flags: RefCell::new(vec![false; accounts.len()].into_boxed_slice()), + accounts, + is_early_verification_of_account_modifications_enabled, + } + } + + fn len(&self) -> usize { + self.accounts.len() + } + + pub fn get(&self, index: IndexOfAccount) -> Option<&RefCell> { + self.accounts.get(index as usize) + } + + pub fn touch(&self, index: IndexOfAccount) -> Result<(), InstructionError> { + if self.is_early_verification_of_account_modifications_enabled { + *self + .touched_flags + .borrow_mut() + .get_mut(index as usize) + .ok_or(InstructionError::NotEnoughAccountKeys)? = true; + } + Ok(()) + } + + pub fn touched_count(&self) -> usize { + self.touched_flags + .borrow() + .iter() + .fold(0usize, |accumulator, was_touched| { + accumulator.saturating_add(*was_touched as usize) + }) + } + + pub fn try_borrow( + &self, + index: IndexOfAccount, + ) -> Result, InstructionError> { + self.accounts + .get(index as usize) + .ok_or(InstructionError::MissingAccount)? + .try_borrow() + .map_err(|_| InstructionError::AccountBorrowFailed) + } + + pub fn try_borrow_mut( + &self, + index: IndexOfAccount, + ) -> Result, InstructionError> { + self.accounts + .get(index as usize) + .ok_or(InstructionError::MissingAccount)? + .try_borrow_mut() + .map_err(|_| InstructionError::AccountBorrowFailed) + } + + pub fn into_accounts(self) -> Vec { + self.accounts + .into_iter() + .map(|account| account.into_inner()) + .collect() + } +} + /// Loaded transaction shared between runtime and programs. /// /// This context is valid for the entire duration of a transaction being processed. #[derive(Debug, Clone, PartialEq)] pub struct TransactionContext { account_keys: Pin>, - accounts: Pin]>>, + accounts: Arc, #[cfg(not(target_os = "solana"))] - account_touched_flags: RefCell>>, instruction_stack_capacity: usize, instruction_trace_capacity: usize, instruction_stack: Vec, @@ -88,16 +167,13 @@ impl TransactionContext { instruction_stack_capacity: usize, instruction_trace_capacity: usize, ) -> Self { - let (account_keys, accounts): (Vec, Vec>) = - transaction_accounts - .into_iter() - .map(|(key, account)| (key, RefCell::new(account))) - .unzip(); - let account_touched_flags = vec![false; accounts.len()]; + let (account_keys, accounts): (Vec<_>, Vec<_>) = transaction_accounts + .into_iter() + .map(|(key, account)| (key, RefCell::new(account))) + .unzip(); Self { account_keys: Pin::new(account_keys.into_boxed_slice()), - accounts: Pin::new(accounts.into_boxed_slice()), - account_touched_flags: RefCell::new(Pin::new(account_touched_flags.into_boxed_slice())), + accounts: Arc::new(TransactionAccounts::new(accounts, rent.is_some())), instruction_stack_capacity, instruction_trace_capacity, instruction_stack: Vec::with_capacity(instruction_stack_capacity), @@ -117,10 +193,15 @@ impl TransactionContext { if !self.instruction_stack.is_empty() { return Err(InstructionError::CallDepth); } - Ok(Vec::from(Pin::into_inner(self.accounts)) - .into_iter() - .map(|account| account.into_inner()) - .collect()) + + Ok(Arc::try_unwrap(self.accounts) + .expect("transaction_context.accounts has unexpected outstanding refs") + .into_accounts()) + } + + #[cfg(not(target_os = "solana"))] + pub fn accounts(&self) -> &Arc { + &self.accounts } /// Returns true if `enable_early_verification_of_account_modifications` is active @@ -163,7 +244,7 @@ impl TransactionContext { index_in_transaction: IndexOfAccount, ) -> Result<&RefCell, InstructionError> { self.accounts - .get(index_in_transaction as usize) + .get(index_in_transaction) .ok_or(InstructionError::NotEnoughAccountKeys) } @@ -557,7 +638,7 @@ impl InstructionContext { ) -> Result, InstructionError> { let account = transaction_context .accounts - .get(index_in_transaction as usize) + .get(index_in_transaction) .ok_or(InstructionError::MissingAccount)? .try_borrow_mut() .map_err(|_| InstructionError::AccountBorrowFailed)?; @@ -667,6 +748,11 @@ pub struct BorrowedAccount<'a> { } impl<'a> BorrowedAccount<'a> { + /// Returns the transaction context + pub fn transaction_context(&self) -> &TransactionContext { + self.transaction_context + } + /// Returns the index of this account (transaction wide) #[inline] pub fn get_index_in_transaction(&self) -> IndexOfAccount { @@ -895,6 +981,18 @@ impl<'a> BorrowedAccount<'a> { 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 + /// been written to. Writing to an account unshares it. + /// + /// During account serialization, if an account is shared it'll get mapped as CoW, else it'll + /// get mapped directly as writable. + #[cfg(not(target_os = "solana"))] + pub fn is_shared(&self) -> bool { + self.account.is_shared() + } + #[cfg(not(target_os = "solana"))] fn make_data_mut(&mut self) { // if the account is still shared, it means this is the first time we're @@ -902,6 +1000,9 @@ impl<'a> BorrowedAccount<'a> { // buffer with MAX_PERMITTED_DATA_INCREASE capacity so that if the // transaction reallocs, we don't have to copy the whole account data a // second time to fullfill the realloc. + // + // NOTE: The account memory region CoW code in Serializer::push_account_region() implements + // the same logic and must be kept in sync. if self.account.is_shared() { self.account.reserve(MAX_PERMITTED_DATA_INCREASE); } @@ -1077,19 +1178,9 @@ impl<'a> BorrowedAccount<'a> { #[cfg(not(target_os = "solana"))] fn touch(&self) -> Result<(), InstructionError> { - if self - .transaction_context - .is_early_verification_of_account_modifications_enabled() - { - *self - .transaction_context - .account_touched_flags - .try_borrow_mut() - .map_err(|_| InstructionError::GenericError)? - .get_mut(self.index_in_transaction as usize) - .ok_or(InstructionError::NotEnoughAccountKeys)? = true; - } - Ok(()) + self.transaction_context + .accounts() + .touch(self.index_in_transaction) } #[cfg(not(target_os = "solana"))] @@ -1118,23 +1209,14 @@ pub struct ExecutionRecord { #[cfg(not(target_os = "solana"))] impl From for ExecutionRecord { fn from(context: TransactionContext) -> Self { - let account_touched_flags = context - .account_touched_flags - .try_borrow() - .expect("borrowing transaction_context.account_touched_flags failed"); - let touched_account_count = account_touched_flags - .iter() - .fold(0u64, |accumulator, was_touched| { - accumulator.saturating_add(*was_touched as u64) - }); + let accounts = Arc::try_unwrap(context.accounts) + .expect("transaction_context.accounts has unexpectd outstanding refs"); + let touched_account_count = accounts.touched_count() as u64; + let accounts = accounts.into_accounts(); Self { accounts: Vec::from(Pin::into_inner(context.account_keys)) .into_iter() - .zip( - Vec::from(Pin::into_inner(context.accounts)) - .into_iter() - .map(|account| account.into_inner()), - ) + .zip(accounts) .collect(), return_data: context.return_data, touched_account_count, From 327005e57df0af11863040386415d44a2d8c1f28 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 14 Feb 2023 22:33:03 +0000 Subject: [PATCH 32/44] Fix CI --- runtime/src/bank/tests.rs | 4 +++- sdk/src/transaction_context.rs | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index b9bc16612d72ac..e9ae8c7dd1f42b 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7445,7 +7445,9 @@ fn test_bank_executor_cache() { upgrade_authority_address: None, }) .unwrap(); - programdata_account.data_mut()[programdata_data_offset..].copy_from_slice(&elf); + let mut data = programdata_account.data().to_vec(); + data[programdata_data_offset..].copy_from_slice(&elf); + programdata_account.set_data_from_slice(&data); programdata_account.set_rent_epoch(1); bank.store_account_and_update_capitalization(&key1, &program_account); bank.store_account_and_update_capitalization(&programdata_key, &programdata_account); diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 6d9af8a68967dc..9f108fc781dfe2 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -59,13 +59,13 @@ pub type TransactionAccount = (Pubkey, AccountSharedData); #[derive(Clone, Debug, PartialEq)] pub struct TransactionAccounts { accounts: Vec>, - // FIXME: use bitfield + // FIXXME: use bitfield touched_flags: RefCell>, is_early_verification_of_account_modifications_enabled: bool, } -#[cfg(not(target_os = "solana"))] impl TransactionAccounts { + #[cfg(not(target_os = "solana"))] fn new( accounts: Vec>, is_early_verification_of_account_modifications_enabled: bool, @@ -85,6 +85,7 @@ impl TransactionAccounts { self.accounts.get(index as usize) } + #[cfg(not(target_os = "solana"))] pub fn touch(&self, index: IndexOfAccount) -> Result<(), InstructionError> { if self.is_early_verification_of_account_modifications_enabled { *self @@ -96,6 +97,7 @@ impl TransactionAccounts { Ok(()) } + #[cfg(not(target_os = "solana"))] pub fn touched_count(&self) -> usize { self.touched_flags .borrow() @@ -142,7 +144,6 @@ impl TransactionAccounts { pub struct TransactionContext { account_keys: Pin>, accounts: Arc, - #[cfg(not(target_os = "solana"))] instruction_stack_capacity: usize, instruction_trace_capacity: usize, instruction_stack: Vec, From 292f7f08725a18062e3d95466dd5aa5d7361d0aa Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 22 Feb 2023 09:25:09 +0000 Subject: [PATCH 33/44] Move CoW to the MemoryMapping level --- programs/bpf_loader/src/lib.rs | 30 ++++++- programs/bpf_loader/src/serialization.rs | 24 +----- programs/bpf_loader/src/syscalls/mem_ops.rs | 87 +++++++++++++-------- 3 files changed, 84 insertions(+), 57 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 4efacb58068c7f..a9de8ae88f02d7 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -26,10 +26,11 @@ use { vm::{ContextObject, EbpfVm, ProgramResult, VerifiedExecutable}, }, solana_sdk::{ + account::WritableAccount, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, clock::Slot, - entrypoint::SUCCESS, + entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, feature_set::{ bpf_account_data_direct_mapping, cap_accounts_data_allocations_per_transaction, cap_bpf_program_instruction_accounts, delay_visibility_of_program_deployment, @@ -304,8 +305,31 @@ pub fn create_vm<'a, 'b>( ) -> Result>, Box> { let stack_size = stack.len(); let heap_size = heap.len(); - let memory_mapping = - create_memory_mapping(program.get_executable(), stack, heap, regions, None)?; + let accounts = Arc::clone(invoke_context.transaction_context.accounts()); + let memory_mapping = create_memory_mapping( + program.get_executable(), + stack, + heap, + regions, + Some(Box::new(move |index_in_transaction| { + // The two calls below can't relly 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 + .try_borrow_mut(index_in_transaction as IndexOfAccount) + .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) + })), + )?; invoke_context.set_syscall_context(SyscallContext { allocator: BpfAllocator::new(heap_size as u64), orig_account_lengths, diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index fb4780f81223a9..0f1428256ffcef 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -8,7 +8,6 @@ use { memory_region::MemoryRegion, }, solana_sdk::{ - account::WritableAccount, bpf_loader_deprecated, entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, NON_DUP_MARKER}, instruction::InstructionError, @@ -18,10 +17,7 @@ use { BorrowedAccount, IndexOfAccount, InstructionContext, TransactionContext, }, }, - std::{ - mem::{self, size_of}, - sync::Arc, - }, + std::mem::{self, size_of}, }; /// Maximum number of instruction accounts that can be serialized into the @@ -121,28 +117,12 @@ impl Serializer { // If the account is still shared it means it wasn't written to yet during this // transaction. We map it as CoW and it'll be copied the first time something // tries to write into it. - let accounts = Arc::clone(account.transaction_context().accounts()); let index_in_transaction = account.get_index_in_transaction(); MemoryRegion::new_cow( account.get_data(), self.vaddr, - Box::new(move || { - // The two calls below can't relly 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 - .try_borrow_mut(index_in_transaction) - .map_err(|_| ())?; - accounts.touch(index_in_transaction).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) - }), + index_in_transaction as u64, ) } else { // The account isn't shared anymore, meaning it was written to earlier during diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index ae110cc7705efc..19ebbed734ca07 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -1,4 +1,9 @@ -use {super::*, crate::declare_syscall, solana_rbpf::memory_region::MemoryRegion, std::slice}; +use { + super::*, + crate::declare_syscall, + solana_rbpf::{error::EbpfError, memory_region::MemoryRegion}, + std::slice, +}; fn mem_op_consume(invoke_context: &mut InvokeContext, n: u64) -> Result<(), Error> { let compute_budget = invoke_context.get_compute_budget(); @@ -146,7 +151,7 @@ fn memmove( src_addr: u64, n: u64, memory_mapping: &MemoryMapping, -) -> Result { +) -> Result { if invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()) @@ -180,7 +185,7 @@ fn memmove_non_contiguous( src_addr: u64, n: u64, memory_mapping: &MemoryMapping, -) -> Result { +) -> Result { let reverse = dst_addr.wrapping_sub(src_addr) < n; iter_memory_pair_chunks( AccessType::Load, @@ -221,7 +226,7 @@ fn memcmp_non_contiguous( dst_addr: u64, n: u64, memory_mapping: &MemoryMapping, -) -> Result { +) -> Result { match iter_memory_pair_chunks( AccessType::Load, src_addr, @@ -241,26 +246,37 @@ fn memcmp_non_contiguous( memcmp(s1, s2, chunk_len) }; if res != 0 { - return Err(MemcmpError::Diff(res)); + return Err(MemcmpError::Diff(res).into()); } Ok(0) }, ) { Ok(res) => Ok(res), - Err(MemcmpError::Diff(diff)) => Ok(diff), - Err(MemcmpError::Ebpf(e)) => Err(e), + Err(error) => match error.downcast_ref() { + Some(MemcmpError::Diff(diff)) => Ok(*diff), + _ => Err(error), + }, } } #[derive(Debug)] enum MemcmpError { - Ebpf(EbpfError), Diff(i32), } -impl From for MemcmpError { - fn from(err: EbpfError) -> Self { - MemcmpError::Ebpf(err) +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, + } } } @@ -269,7 +285,7 @@ fn memset_non_contiguous( c: u8, n: u64, memory_mapping: &MemoryMapping, -) -> Result { +) -> Result { let dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, AccessType::Store, dst_addr, n); for item in dst_chunk_iter { let (dst_region, dst_vm_addr, dst_len) = item?; @@ -280,7 +296,7 @@ fn memset_non_contiguous( Ok(0) } -fn iter_memory_pair_chunks( +fn iter_memory_pair_chunks( src_access: AccessType, src_addr: u64, dst_access: AccessType, @@ -289,11 +305,10 @@ fn iter_memory_pair_chunks( memory_mapping: &MemoryMapping, reverse: bool, mut fun: F, -) -> Result +) -> Result where T: Default, - E: From, - F: FnMut(*const u8, *const u8, usize) -> Result, + F: FnMut(*const u8, *const u8, usize) -> Result, { let mut src_chunk_iter = MemoryChunkIterator::new(memory_mapping, src_access, src_addr, n); loop { @@ -372,28 +387,36 @@ impl<'a> MemoryChunkIterator<'a> { } } - fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, EbpfError> { + fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, Error> { match self.memory_mapping.region(self.access_type, vm_addr) { Ok(region) => Ok(region), - Err(EbpfError::AccessViolation(pc, access_type, _vm_addr, _len, name)) => Err( - EbpfError::AccessViolation(pc, access_type, self.initial_vm_addr, self.len, name), - ), - Err(EbpfError::StackAccessViolation(pc, access_type, _vm_addr, _len, frame)) => { - Err(EbpfError::StackAccessViolation( - pc, - access_type, - self.initial_vm_addr, - self.len, - frame, - )) - } - Err(e) => Err(e), + Err(error) => match error.downcast_ref() { + Some(EbpfError::AccessViolation(pc, access_type, _vm_addr, _len, name)) => { + Err(Box::new(EbpfError::AccessViolation( + *pc, + *access_type, + self.initial_vm_addr, + self.len, + name, + ))) + } + Some(EbpfError::StackAccessViolation(pc, access_type, _vm_addr, _len, frame)) => { + Err(Box::new(EbpfError::StackAccessViolation( + *pc, + *access_type, + self.initial_vm_addr, + self.len, + *frame, + ))) + } + _ => Err(error), + }, } } } impl<'a> Iterator for MemoryChunkIterator<'a> { - type Item = Result<(&'a MemoryRegion, u64, usize), EbpfError>; + type Item = Result<(&'a MemoryRegion, u64, usize), Error>; fn next(&mut self) -> Option { if self.vm_addr_start == self.vm_addr_end { @@ -473,7 +496,7 @@ mod tests { } fn to_chunk_vec<'a>( - iter: impl Iterator>, + iter: impl Iterator>, ) -> Vec<(u64, usize)> { iter.flat_map(|res| res.map(|(_, vm_addr, len)| (vm_addr, len))) .collect::>() From f5bd31b86885e33ef0a5348ca34371e5a10576b7 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 27 Feb 2023 02:08:43 +0000 Subject: [PATCH 34/44] Update after rbpf API change --- rbpf-cli/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rbpf-cli/src/main.rs b/rbpf-cli/src/main.rs index 4408603bdc92a4..6297ce36221bc2 100644 --- a/rbpf-cli/src/main.rs +++ b/rbpf-cli/src/main.rs @@ -12,8 +12,8 @@ use { with_mock_invoke_context, }, solana_rbpf::{ - assembler::assemble, elf::Executable, static_analysis::Analysis, - verifier::RequisiteVerifier, vm::VerifiedExecutable, + aligned_memory::AlignedMemory, assembler::assemble, ebpf::HOST_ALIGN, elf::Executable, + static_analysis::Analysis, verifier::RequisiteVerifier, vm::VerifiedExecutable, }, solana_sdk::{ account::AccountSharedData, From b00ae657446c75038adeb293fee58230774da7f3 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 27 Feb 2023 04:29:15 +0000 Subject: [PATCH 35/44] Fix merge screwup --- programs/bpf_loader/src/serialization.rs | 3 +-- programs/bpf_loader/src/syscalls/cpi.rs | 6 ++---- runtime/src/bank/tests.rs | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 0f1428256ffcef..7a36a5229d95ea 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -695,8 +695,7 @@ mod tests { assert_eq!( serialization_result.as_ref().err(), expected_err.as_ref(), - "{} test case failed", - name + "{name} test case failed", ); if expected_err.is_some() { continue; diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index f572a7fd4eed5c..436a3ff5b105cd 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1736,8 +1736,7 @@ mod tests { let spare_capacity = &original_data_slice[original_data_len - data_len..]; assert!( is_zeroed(spare_capacity), - "dirty account spare capacity {:?}", - spare_capacity + "dirty account spare capacity {spare_capacity:?}", ); } @@ -1751,8 +1750,7 @@ mod tests { // the unused realloc padding is always zeroed assert!( is_zeroed(&realloc_area[expected_realloc_used..]), - "dirty realloc padding {:?}", - realloc_area + "dirty realloc padding {realloc_area:?}", ); } diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index e9ae8c7dd1f42b..705aba3ae19b30 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7496,7 +7496,7 @@ fn test_bank_load_program() { upgrade_authority_address: None, }) .unwrap(); - programdata_account.data_mut()[programdata_data_offset..].copy_from_slice(&elf); + programdata_account.data_as_mut_slice()[programdata_data_offset..].copy_from_slice(&elf); programdata_account.set_rent_epoch(1); bank.store_account_and_update_capitalization(&key1, &program_account); bank.store_account_and_update_capitalization(&programdata_key, &programdata_account); From fbe4d84f07fd3fd9f7b15633b5b273c12f21fa54 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 27 Feb 2023 21:38:44 +0000 Subject: [PATCH 36/44] Add create_vm macro. Fix benches. --- rbpf-cli/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rbpf-cli/src/main.rs b/rbpf-cli/src/main.rs index 6297ce36221bc2..4408603bdc92a4 100644 --- a/rbpf-cli/src/main.rs +++ b/rbpf-cli/src/main.rs @@ -12,8 +12,8 @@ use { with_mock_invoke_context, }, solana_rbpf::{ - aligned_memory::AlignedMemory, assembler::assemble, ebpf::HOST_ALIGN, elf::Executable, - static_analysis::Analysis, verifier::RequisiteVerifier, vm::VerifiedExecutable, + assembler::assemble, elf::Executable, static_analysis::Analysis, + verifier::RequisiteVerifier, vm::VerifiedExecutable, }, solana_sdk::{ account::AccountSharedData, From 0645120e9263a422c01eb62facd2e36dd43bf829 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 28 Feb 2023 07:33:44 +0000 Subject: [PATCH 37/44] cpi: simplify update_caller_account Simplify the logic to update a caller's memory region when a callee causes an account data pointer to change (eg during CoW) --- programs/bpf_loader/src/syscalls/cpi.rs | 162 +++++++++++++----------- 1 file changed, 85 insertions(+), 77 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 436a3ff5b105cd..a53629e0a7fba2 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,7 +1,6 @@ use { super::*, crate::declare_syscall, - solana_rbpf::memory_region::MemoryRegion, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -1129,10 +1128,6 @@ fn update_caller_account( *caller_account.owner = *callee_account.get_owner(); if direct_mapping && caller_account.original_data_len > 0 { - // If an account's data pointer has changed - because of CoW or because - // of using AccountSharedData directly (deprecated) - we must update the - // corresponding MemoryRegion in the caller's address space. - // 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 @@ -1143,32 +1138,14 @@ fn update_caller_account( callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))? } - // find the account's MemoryRegion - let regions = memory_mapping.get_regions(); - let memory_region_index = regions - .iter() - .position(|memory_region| memory_region.vm_addr == caller_account.vm_data_addr) - .ok_or(Box::new(InstructionError::ProgramEnvironmentSetupFailure))?; - let region = regions.get(memory_region_index).unwrap(); - debug_assert!(callee_account.capacity() >= region.len as usize); - - // If the account's data pointer has changed, update the region - if region.host_addr.get() != callee_account.get_data().as_ptr() as u64 { - let new_region = if let Ok(data) = callee_account.get_data_mut() { - let data = unsafe { - std::slice::from_raw_parts_mut(data.as_mut_ptr(), region.len as usize) - }; - MemoryRegion::new_writable(data, caller_account.vm_data_addr) - } else { - let data = unsafe { - std::slice::from_raw_parts( - callee_account.get_data().as_ptr(), - region.len as usize, - ) - }; - MemoryRegion::new_readonly(data, caller_account.vm_data_addr) - }; - memory_mapping.replace_region(memory_region_index, new_region)?; + // If an account's data pointer has changed - because of CoW or because + // of using AccountSharedData directly (deprecated) - 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. + let region = memory_mapping.region(AccessType::Load, caller_account.vm_data_addr)?; + let callee_ptr = callee_account.get_data().as_ptr() as u64; + if region.host_addr.get() != callee_ptr { + region.host_addr.set(callee_ptr); } } let prev_len = *caller_account.ref_to_len_in_vm as usize; @@ -1699,59 +1676,79 @@ mod tests { let mut callee_account = borrow_instruction_account!(invoke_context, 0); - 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 - ] { - callee_account.set_data_from_slice(&new_value).unwrap(); + 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, - &mut memory_mapping, - false, - &mut caller_account, - &mut callee_account, - true, - ) - .unwrap(); + update_caller_account( + &invoke_context, + &mut memory_mapping, + false, + &mut caller_account, + &mut callee_account, + true, + ) + .unwrap(); - 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()); + // 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, + true + ) + .unwrap() + .as_ptr(), + callee_account.get_data().as_ptr() + ); - // with direct mapping on, serialized_data contains the realloc padding - let realloc_area = &caller_account.serialized_data; + 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()); + + // with direct mapping on, serialized_data contains the realloc padding + let realloc_area = &caller_account.serialized_data; + + 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 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) - }; + // 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..] + ); - let spare_capacity = &original_data_slice[original_data_len - data_len..]; + // the unused realloc padding is always zeroed assert!( - is_zeroed(spare_capacity), - "dirty account spare capacity {spare_capacity:?}", + is_zeroed(&realloc_area[expected_realloc_used..]), + "dirty realloc padding {realloc_area:?}", ); } - - // 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 @@ -1843,10 +1840,11 @@ mod tests { .get_account_at_index(1) .unwrap() .borrow_mut(); - account.set_data(b"foo".to_vec()); + 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( @@ -1859,7 +1857,17 @@ mod tests { ) .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, + true, + ) + .unwrap(); + assert_eq!(data, callee_account.get_data()); } #[test] From 533f96db79e7f8e15a6a3a71eedb1401d0583426 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 1 Mar 2023 21:42:36 +0000 Subject: [PATCH 38/44] benches/bpf_loader: move serialization out of create_vm bench --- programs/sbf/benches/bpf_loader.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/programs/sbf/benches/bpf_loader.rs b/programs/sbf/benches/bpf_loader.rs index d3bd58561018aa..a9300cd78e3972 100644 --- a/programs/sbf/benches/bpf_loader.rs +++ b/programs/sbf/benches/bpf_loader.rs @@ -5,6 +5,8 @@ use {solana_rbpf::memory_region::MemoryState, std::slice}; +use {solana_rbpf::memory_region::MemoryState, std::slice}; + extern crate test; use { From 9b6277025bf24cce5d2fcc3b5325ce325b4aeeaa Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 2 Mar 2023 08:10:02 +0000 Subject: [PATCH 39/44] benches/bpf_loader: don't copy accounts when direct mapping is on --- programs/sbf/benches/bpf_loader.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/programs/sbf/benches/bpf_loader.rs b/programs/sbf/benches/bpf_loader.rs index a9300cd78e3972..cc6fbe915bf700 100644 --- a/programs/sbf/benches/bpf_loader.rs +++ b/programs/sbf/benches/bpf_loader.rs @@ -3,9 +3,10 @@ #![allow(clippy::uninlined_format_args)] #![allow(clippy::integer_arithmetic)] -use {solana_rbpf::memory_region::MemoryState, std::slice}; - -use {solana_rbpf::memory_region::MemoryState, std::slice}; +use { + solana_rbpf::memory_region::MemoryState, + solana_sdk::feature_set::bpf_account_data_direct_mapping, std::slice, +}; extern crate test; @@ -225,6 +226,9 @@ fn bench_create_vm(bencher: &mut Bencher) { const BUDGET: u64 = 200_000; invoke_context.mock_set_remaining(BUDGET); + let direct_mapping = invoke_context + .feature_set + .is_active(&bpf_account_data_direct_mapping::id()); let loader = create_loader( &invoke_context.feature_set, &ComputeBudget::default(), @@ -246,8 +250,8 @@ fn bench_create_vm(bencher: &mut Bencher) { .transaction_context .get_current_instruction_context() .unwrap(), - true, // should_cap_ix_accounts - true, // copy_account_data + true, // should_cap_ix_accounts + !direct_mapping, // copy_account_data ) .unwrap(); @@ -270,6 +274,10 @@ fn bench_instruction_count_tuner(_bencher: &mut Bencher) { const BUDGET: u64 = 200_000; invoke_context.mock_set_remaining(BUDGET); + let direct_mapping = invoke_context + .feature_set + .is_active(&bpf_account_data_direct_mapping::id()); + // Serialize account data let (_serialized, regions, account_lengths) = serialize_parameters( invoke_context.transaction_context, @@ -277,8 +285,8 @@ fn bench_instruction_count_tuner(_bencher: &mut Bencher) { .transaction_context .get_current_instruction_context() .unwrap(), - true, // should_cap_ix_accounts - true, // copy_account_data + true, // should_cap_ix_accounts + !direct_mapping, // copy_account_data ) .unwrap(); From db536177f25f054fd36172e5ba1a89aa20860d5e Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 15 Mar 2023 11:05:45 +0000 Subject: [PATCH 40/44] Fix review nits --- programs/bpf_loader/src/lib.rs | 2 +- sdk/src/transaction_context.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index a9de8ae88f02d7..347ef8328e8ea8 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -312,7 +312,7 @@ pub fn create_vm<'a, 'b>( heap, regions, Some(Box::new(move |index_in_transaction| { - // The two calls below can't relly fail. If they fail because of a bug, + // 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 diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 9f108fc781dfe2..8f8d1bb038bc20 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -59,7 +59,6 @@ pub type TransactionAccount = (Pubkey, AccountSharedData); #[derive(Clone, Debug, PartialEq)] pub struct TransactionAccounts { accounts: Vec>, - // FIXXME: use bitfield touched_flags: RefCell>, is_early_verification_of_account_modifications_enabled: bool, } From 09c7154454d887e18ff7a9106785b0ecb63cfea7 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Mon, 17 Apr 2023 00:28:12 +0000 Subject: [PATCH 41/44] bpf_loader: mem_ops: handle u64 overflow in MemoryChunkIterator::new When starting at u64::MAX, the chunk iterator would always return the empty sequence (None on the first next()) call, instead of returning a memory access violation. Use checked instead of saturating arithmetic to detect the condition and error out. This commit also adds more tests around boundary conditions. --- programs/bpf_loader/src/syscalls/mem_ops.rs | 172 +++++++++++++++++--- 1 file changed, 147 insertions(+), 25 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 19ebbed734ca07..598914bc7ed84d 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -286,7 +286,7 @@ fn memset_non_contiguous( n: u64, memory_mapping: &MemoryMapping, ) -> Result { - let dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, AccessType::Store, dst_addr, n); + let dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, AccessType::Store, dst_addr, n)?; for item in dst_chunk_iter { let (dst_region, dst_vm_addr, dst_len) = item?; let dst_host_addr = Result::from(dst_region.vm_to_host(dst_vm_addr, dst_len as u64))?; @@ -310,7 +310,8 @@ where T: Default, F: FnMut(*const u8, *const u8, usize) -> Result, { - let mut src_chunk_iter = MemoryChunkIterator::new(memory_mapping, src_access, src_addr, n); + let mut src_chunk_iter = MemoryChunkIterator::new(memory_mapping, src_access, src_addr, n) + .map_err(EbpfError::from)?; loop { // iterate source chunks let (src_region, src_vm_addr, mut src_len) = match if reverse { @@ -323,7 +324,8 @@ where }; let mut src_host_addr = Result::from(src_region.vm_to_host(src_vm_addr, src_len as u64))?; - let mut dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, dst_access, dst_addr, n); + let mut dst_chunk_iter = MemoryChunkIterator::new(memory_mapping, dst_access, dst_addr, n) + .map_err(EbpfError::from)?; // iterate over destination chunks until this source chunk has been completely copied while src_len > 0 { loop { @@ -376,15 +378,22 @@ impl<'a> MemoryChunkIterator<'a> { access_type: AccessType, vm_addr: u64, len: u64, - ) -> MemoryChunkIterator<'a> { - MemoryChunkIterator { + ) -> Result, EbpfError> { + let vm_addr_end = vm_addr.checked_add(len).ok_or(EbpfError::AccessViolation( + 0, + access_type, + vm_addr, + len, + "unknown", + ))?; + Ok(MemoryChunkIterator { memory_mapping, access_type, initial_vm_addr: vm_addr, len, vm_addr_start: vm_addr, - vm_addr_end: vm_addr.saturating_add(len), - } + vm_addr_end, + }) } fn region(&mut self, vm_addr: u64) -> Result<&'a MemoryRegion, Error> { @@ -484,37 +493,96 @@ impl<'a> DoubleEndedIterator for MemoryChunkIterator<'a> { mod tests { use {super::*, solana_rbpf::ebpf::MM_PROGRAM_START}; + 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] - fn test_memory_chunk_iterator_empty() { + #[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).unwrap(); - let mut src_chunk_iter = MemoryChunkIterator::new(&memory_mapping, AccessType::Load, 0, 1); - assert!(src_chunk_iter.next().unwrap().is_err()); - } - fn to_chunk_vec<'a>( - iter: impl Iterator>, - ) -> Vec<(u64, usize)> { - iter.flat_map(|res| res.map(|(_, vm_addr, len)| (vm_addr, len))) - .collect::>() + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, 0, 1).unwrap(); + src_chunk_iter.next().unwrap().unwrap(); } #[test] #[should_panic(expected = "AccessViolation")] - fn test_memory_chunk_iterator_no_regions() { + 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).unwrap(); - let mut src_chunk_iter = MemoryChunkIterator::new(&memory_mapping, AccessType::Load, 0, 1); + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, u64::MAX, 1).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_PROGRAM_START)], + &config, + ) + .unwrap(); + + // check oob at the lower bound on the first next() + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START - 1, 42) + .unwrap(); + assert!(matches!( + src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 42, "unknown") if *addr == MM_PROGRAM_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_PROGRAM_START, 43) + .unwrap(); + assert!(src_chunk_iter.next().unwrap().is_ok()); + assert!(matches!( + src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 43, "program") if *addr == MM_PROGRAM_START + )); + + // check oob at the upper bound on the first next_back() + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START, 43) + .unwrap() + .rev(); + assert!(matches!( + src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 43, "program") if *addr == MM_PROGRAM_START + )); + + // check oob at the upper bound on the 2nd next_back() + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START - 1, 43) + .unwrap() + .rev(); + assert!(src_chunk_iter.next().unwrap().is_ok()); + assert!(matches!( + src_chunk_iter.next().unwrap().unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 43, "unknown") if *addr == MM_PROGRAM_START - 1 + )); + } + #[test] fn test_memory_chunk_iterator_one() { let config = Config { @@ -528,8 +596,16 @@ mod tests { ) .unwrap(); + // check lower bound let mut src_chunk_iter = - MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START - 1, 1); + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START - 1, 1) + .unwrap(); + assert!(src_chunk_iter.next().unwrap().is_err()); + + // check upper bound + let mut src_chunk_iter = + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START + 42, 1) + .unwrap(); assert!(src_chunk_iter.next().unwrap().is_err()); for (vm_addr, len) in [ @@ -541,7 +617,8 @@ mod tests { ] { for rev in [true, false] { let iter = - MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len); + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len) + .unwrap(); let res = if rev { to_chunk_vec(iter.rev()) } else { @@ -554,10 +631,6 @@ mod tests { } } } - - let mut src_chunk_iter = - MemoryChunkIterator::new(&memory_mapping, AccessType::Load, MM_PROGRAM_START + 42, 1); - assert!(src_chunk_iter.next().unwrap().is_err()); } #[test] @@ -588,7 +661,8 @@ mod tests { ] { for rev in [false, true] { let iter = - MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len); + MemoryChunkIterator::new(&memory_mapping, AccessType::Load, vm_addr, len) + .unwrap(); let res = if rev { expected.reverse(); to_chunk_vec(iter.rev()) @@ -601,6 +675,54 @@ mod tests { } } + #[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_PROGRAM_START), + MemoryRegion::new_readonly(&mem2, MM_PROGRAM_START + 8), + ], + &config, + ) + .unwrap(); + + // dst is shorter than src + assert!(matches!( + iter_memory_pair_chunks( + AccessType::Load, + MM_PROGRAM_START, + AccessType::Load, + MM_PROGRAM_START + 8, + 8, + &memory_mapping, + false, + |_src, _dst, _len| Ok::<_, Error>(0), + ).unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 8, "program") if *addr == MM_PROGRAM_START + 8 + )); + + // src is shorter than dst + assert!(matches!( + iter_memory_pair_chunks( + AccessType::Load, + MM_PROGRAM_START + 10, + AccessType::Load, + MM_PROGRAM_START + 2, + 3, + &memory_mapping, + false, + |_src, _dst, _len| Ok::<_, Error>(0), + ).unwrap_err().downcast_ref().unwrap(), + EbpfError::AccessViolation(0, AccessType::Load, addr, 3, "program") if *addr == MM_PROGRAM_START + 10 + )); + } + #[test] #[should_panic(expected = "AccessViolation(0, Store, 4294967296, 4")] fn test_memmove_non_contiguous_readonly() { From 1e18a73e469363f0cb99c4e9334d5b79b818df7b Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 18 Apr 2023 11:52:30 +0000 Subject: [PATCH 42/44] Fix loader-v3 tests: data_mut => data_as_mut_slice --- programs/loader-v3/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/loader-v3/src/lib.rs b/programs/loader-v3/src/lib.rs index 4e1042a3e05d0b..07d6d1403c87e3 100644 --- a/programs/loader-v3/src/lib.rs +++ b/programs/loader-v3/src/lib.rs @@ -691,7 +691,7 @@ mod tests { authority_address, }) .unwrap(); - program_account.data_mut()[loader_v3::LoaderV3State::program_data_offset()..] + program_account.data_as_mut_slice()[loader_v3::LoaderV3State::program_data_offset()..] .copy_from_slice(&elf_bytes); program_account } From 490ccd0f307a9fee28b847b8bdec65fb63eb962a Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 18 Apr 2023 11:59:21 +0000 Subject: [PATCH 43/44] Fix CI --- programs/bpf_loader/src/syscalls/mem_ops.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 598914bc7ed84d..2ecd9a9ad4f77e 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -267,7 +267,7 @@ enum MemcmpError { 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), + MemcmpError::Diff(diff) => write!(f, "memcmp diff: {diff}"), } } } From 9b4cbb389060e8c29f554b67345462911b500630 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 18 Apr 2023 23:52:36 +0000 Subject: [PATCH 44/44] bpf_loader: fix tuner bench: account must be writable With direct mapping on, invalid writes are caught early meaning the tuner would fail on the first store and not consume the whole budget like the benchmark expects. --- programs/sbf/benches/bpf_loader.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/sbf/benches/bpf_loader.rs b/programs/sbf/benches/bpf_loader.rs index cc6fbe915bf700..82ac884767baec 100644 --- a/programs/sbf/benches/bpf_loader.rs +++ b/programs/sbf/benches/bpf_loader.rs @@ -69,7 +69,7 @@ macro_rules! with_mock_invoke_context { index_in_caller: 2, index_in_callee: 0, is_signer: false, - is_writable: false, + is_writable: true, }]; solana_program_runtime::with_mock_invoke_context!( $invoke_context,