From 953c30746989daa5e96c71cd650f1f02a241751a Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Jul 2023 13:18:51 +0700 Subject: [PATCH 01/12] transaction_context: update make_data_mut comment --- sdk/src/transaction_context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 39132524495d3e..cdfb162fc475a1 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -1000,8 +1000,8 @@ impl<'a> BorrowedAccount<'a> { // 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. + // NOTE: The account memory region CoW code in bpf_loader::create_vm() implements the same + // logic and must be kept in sync. if self.account.is_shared() { self.account.reserve(MAX_PERMITTED_DATA_INCREASE); } From 3404d23256c7105969ed343ab253701b903166fb Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 13 Jun 2023 07:50:19 +0000 Subject: [PATCH 02/12] bpf_loader: cpi: pass SerializeAccountMetadata to CallerAccount::from* We now have a way to provide CallerAccount with trusted values coming from our internal serialization code and not from untrusted vm space --- programs/bpf_loader/src/syscalls/cpi.rs | 28 +++++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 3a690ec3609a99..17ae639fd2ecf0 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_program_runtime::invoke_context::SerializedAccountMetadata, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -51,11 +52,11 @@ impl<'a> CallerAccount<'a> { is_loader_deprecated: bool, _vm_addr: u64, account_info: &AccountInfo, - original_data_len: usize, + account_metadata: &SerializedAccountMetadata, ) -> Result, Error> { + let original_data_len = account_metadata.original_data_len; // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. - let lamports = { // Double translate lamports out of RefCell let ptr = translate_type::( @@ -161,8 +162,9 @@ impl<'a> CallerAccount<'a> { is_loader_deprecated: bool, vm_addr: u64, account_info: &SolAccountInfo, - original_data_len: usize, + account_metadata: &SerializedAccountMetadata, ) -> Result, Error> { + let original_data_len = account_metadata.original_data_len; // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. @@ -719,7 +721,14 @@ fn translate_and_update_accounts<'a, T, F>( do_translate: F, ) -> Result, Error> where - F: Fn(&InvokeContext, &MemoryMapping, bool, u64, &T, usize) -> Result, Error>, + F: Fn( + &InvokeContext, + &MemoryMapping, + bool, + u64, + &T, + &SerializedAccountMetadata, + ) -> Result, Error>, { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -768,7 +777,7 @@ where } else if let Some(caller_account_index) = account_info_keys.iter().position(|key| *key == account_key) { - let original_data_len = accounts_metadata + let serialized_metadata = accounts_metadata .get(instruction_account.index_in_caller as usize) .ok_or_else(|| { ic_msg!( @@ -777,8 +786,7 @@ where account_key ); Box::new(InstructionError::MissingAccount) - })? - .original_data_len; + })?; // build the CallerAccount corresponding to this account. let caller_account = @@ -792,7 +800,7 @@ where account_infos .get(caller_account_index) .ok_or(SyscallError::InvalidLength)?, - original_data_len, + serialized_metadata, )?; // before initiating CPI, the caller may have modified the @@ -1457,7 +1465,9 @@ mod tests { false, vm_addr, account_info, - account.data().len(), + &SerializedAccountMetadata { + original_data_len: account.data().len(), + }, ) .unwrap(); assert_eq!(*caller_account.lamports, account.lamports()); From 6e7bd643c146f00747b7d8dc8c6046b275acb59d Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 13 Jun 2023 11:29:16 +0000 Subject: [PATCH 03/12] bpf_loader: direct_mapping: enforce account info pointers to be immutable When direct mapping is enabled, we might need to update account data memory regions across CPI calls. Since the only way we have to retrieve the regions is based on their vm addresses, we enforce vm addresses to be stable. Accounts can still be mutated and resized of course, but it must be done in place. This also locks all other AccountInfo pointers, since there's no legitimate reason to make them point to anything else. --- program-runtime/src/invoke_context.rs | 4 + programs/bpf_loader/src/serialization.rs | 78 ++++++--- programs/bpf_loader/src/syscalls/cpi.rs | 194 ++++++++++++++++------- programs/bpf_loader/src/syscalls/mod.rs | 2 + 4 files changed, 196 insertions(+), 82 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 3506671dadc4d4..a105048ac3388c 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -153,6 +153,10 @@ pub struct SyscallContext { #[derive(Debug, Clone)] pub struct SerializedAccountMetadata { pub original_data_len: usize, + pub vm_data_addr: u64, + pub vm_key_addr: u64, + pub vm_lamports_addr: u64, + pub vm_owner_addr: u64, } pub struct InvokeContext<'a> { diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 768ba792116b26..c4f5dbe5fcd3b3 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -55,8 +55,12 @@ impl Serializer { self.buffer.fill_write(num, value) } - pub fn write(&mut self, value: T) { + pub fn write(&mut self, value: T) -> u64 { self.debug_assert_alignment::(); + let vaddr = self + .vaddr + .saturating_add(self.buffer.len() as u64) + .saturating_sub(self.region_start as u64); // Safety: // in serialize_parameters_(aligned|unaligned) first we compute the // required size then we write into the newly allocated buffer. There's @@ -68,22 +72,35 @@ impl Serializer { unsafe { self.buffer.write_unchecked(value); } + + vaddr } - fn write_all(&mut self, value: &[u8]) { + fn write_all(&mut self, value: &[u8]) -> u64 { + let vaddr = self + .vaddr + .saturating_add(self.buffer.len() as u64) + .saturating_sub(self.region_start as u64); // Safety: // see write() - the buffer is guaranteed to be large enough unsafe { self.buffer.write_all_unchecked(value); } + + vaddr } - fn write_account(&mut self, account: &mut BorrowedAccount<'_>) -> Result<(), InstructionError> { - if self.copy_account_data { + fn write_account( + &mut self, + account: &mut BorrowedAccount<'_>, + ) -> Result { + let vm_data_addr = if self.copy_account_data { + let vm_data_addr = self.vaddr.saturating_add(self.buffer.len() as u64); self.write_all(account.get_data()); + vm_data_addr } else { - self.push_account_region(account)?; - } + self.push_account_region(account)? + }; if self.aligned { let align_offset = @@ -103,14 +120,15 @@ impl Serializer { } } - Ok(()) + Ok(vm_data_addr) } fn push_account_region( &mut self, account: &mut BorrowedAccount<'_>, - ) -> Result<(), InstructionError> { + ) -> Result { self.push_region(); + let vaddr = self.vaddr; let account_len = account.get_data().len(); if account_len > 0 { let region = if account.can_data_be_changed().is_ok() { @@ -137,7 +155,7 @@ impl Serializer { self.vaddr += region.len; self.regions.push(region); } - Ok(()) + Ok(vaddr) } fn push_region(&mut self) { @@ -315,19 +333,23 @@ fn serialize_parameters_unaligned( s.write(position as u8); } SerializeAccount::Account(_, mut account) => { - accounts_metadata.push(SerializedAccountMetadata { - original_data_len: account.get_data().len(), - }); 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()); + let vm_key_addr = s.write_all(account.get_key().as_ref()); + let vm_lamports_addr = s.write::(account.get_lamports().to_le()); s.write::((account.get_data().len() as u64).to_le()); - s.write_account(&mut account)?; - s.write_all(account.get_owner().as_ref()); + let vm_data_addr = s.write_account(&mut account)?; + let vm_owner_addr = s.write_all(account.get_owner().as_ref()); s.write::(account.is_executable() as u8); s.write::((account.get_rent_epoch()).to_le()); + accounts_metadata.push(SerializedAccountMetadata { + original_data_len: account.get_data().len(), + vm_key_addr, + vm_lamports_addr, + vm_owner_addr, + vm_data_addr, + }); } }; } @@ -406,7 +428,7 @@ fn serialize_parameters_aligned( ), InstructionError, > { - let mut account_lengths = Vec::with_capacity(accounts.len()); + let mut accounts_metadata = Vec::with_capacity(accounts.len()); // Calculate size in order to alloc once let mut size = size_of::(); for account in &accounts { @@ -444,23 +466,27 @@ fn serialize_parameters_aligned( for account in accounts { match account { SerializeAccount::Account(_, mut borrowed_account) => { - account_lengths.push(SerializedAccountMetadata { - original_data_len: borrowed_account.get_data().len(), - }); s.write::(NON_DUP_MARKER); s.write::(borrowed_account.is_signer() as u8); s.write::(borrowed_account.is_writable() as u8); s.write::(borrowed_account.is_executable() as u8); s.write_all(&[0u8, 0, 0, 0]); - s.write_all(borrowed_account.get_key().as_ref()); - s.write_all(borrowed_account.get_owner().as_ref()); - s.write::(borrowed_account.get_lamports().to_le()); + let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref()); + let vm_owner_addr = s.write_all(borrowed_account.get_owner().as_ref()); + let vm_lamports_addr = 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)?; + let vm_data_addr = s.write_account(&mut borrowed_account)?; s.write::((borrowed_account.get_rent_epoch()).to_le()); + accounts_metadata.push(SerializedAccountMetadata { + original_data_len: borrowed_account.get_data().len(), + vm_key_addr, + vm_owner_addr, + vm_lamports_addr, + vm_data_addr, + }); } SerializeAccount::Duplicate(position) => { - account_lengths.push(account_lengths.get(position as usize).unwrap().clone()); + accounts_metadata.push(accounts_metadata.get(position as usize).unwrap().clone()); s.write::(position as u8); s.write_all(&[0u8, 0, 0, 0, 0, 0, 0]); } @@ -471,7 +497,7 @@ fn serialize_parameters_aligned( s.write_all(program_id.as_ref()); let (mem, regions) = s.finish(); - Ok((mem, regions, account_lengths)) + Ok((mem, regions, accounts_metadata)) } pub fn deserialize_parameters_aligned>( diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 17ae639fd2ecf0..26eefd345b40ca 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -13,6 +13,25 @@ use { std::{mem, ptr, slice}, }; +fn check_account_info_pointer( + invoke_context: &InvokeContext, + vm_addr: u64, + expected_vm_addr: u64, + field: &str, +) -> Result<(), Error> { + if vm_addr != expected_vm_addr { + ic_msg!( + invoke_context, + "Invalid account info pointer `{}': {:#x} != {:#x}", + field, + vm_addr, + expected_vm_addr + ); + return Err(SyscallError::InvalidPointer.into()); + } + Ok(()) +} + /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. /// /// At the start of a CPI, this can be different from the data stored in the @@ -54,7 +73,25 @@ impl<'a> CallerAccount<'a> { account_info: &AccountInfo, account_metadata: &SerializedAccountMetadata, ) -> Result, Error> { - let original_data_len = account_metadata.original_data_len; + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + + if direct_mapping { + check_account_info_pointer( + invoke_context, + account_info.key as *const _ as u64, + account_metadata.vm_key_addr, + "key", + )?; + check_account_info_pointer( + invoke_context, + account_info.owner as *const _ as u64, + account_metadata.vm_owner_addr, + "owner", + )?; + } + // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. let lamports = { @@ -64,8 +101,17 @@ impl<'a> CallerAccount<'a> { account_info.lamports.as_ptr() as u64, invoke_context.get_check_aligned(), )?; + if direct_mapping { + check_account_info_pointer( + invoke_context, + *ptr, + account_metadata.vm_lamports_addr, + "lamports", + )?; + } translate_type_mut::(memory_mapping, *ptr, invoke_context.get_check_aligned())? }; + let owner = translate_type_mut::( memory_mapping, account_info.owner as *const _ as u64, @@ -79,6 +125,14 @@ impl<'a> CallerAccount<'a> { account_info.data.as_ptr() as *const _ as u64, invoke_context.get_check_aligned(), )?; + if direct_mapping { + check_account_info_pointer( + invoke_context, + data.as_ptr() as u64, + account_metadata.vm_data_addr, + "data", + )?; + } consume_compute_meter( invoke_context, @@ -111,17 +165,14 @@ 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 serialized_data = translate_slice_mut::( memory_mapping, - if bpf_account_data_direct_mapping { + if direct_mapping { vm_data_addr.saturating_add(original_data_len as u64) } else { vm_data_addr }, - if bpf_account_data_direct_mapping { + if direct_mapping { if is_loader_deprecated { 0 } else { @@ -145,7 +196,7 @@ impl<'a> CallerAccount<'a> { Ok(CallerAccount { lamports, owner, - original_data_len, + original_data_len: account_metadata.original_data_len, serialized_data, vm_data_addr, ref_to_len_in_vm, @@ -159,15 +210,47 @@ impl<'a> CallerAccount<'a> { fn from_sol_account_info( invoke_context: &InvokeContext, memory_mapping: &MemoryMapping, - is_loader_deprecated: bool, + _is_loader_deprecated: bool, vm_addr: u64, account_info: &SolAccountInfo, account_metadata: &SerializedAccountMetadata, ) -> Result, Error> { - let original_data_len = account_metadata.original_data_len; + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + + if direct_mapping { + check_account_info_pointer( + invoke_context, + account_info.key_addr, + account_metadata.vm_key_addr, + "key", + )?; + + check_account_info_pointer( + invoke_context, + account_info.owner_addr, + account_metadata.vm_owner_addr, + "owner", + )?; + + check_account_info_pointer( + invoke_context, + account_info.lamports_addr, + account_metadata.vm_lamports_addr, + "lamports", + )?; + + check_account_info_pointer( + invoke_context, + account_info.data_addr, + account_metadata.vm_data_addr, + "data", + )?; + } + // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. - let lamports = translate_type_mut::( memory_mapping, account_info.lamports_addr, @@ -178,7 +261,6 @@ impl<'a> CallerAccount<'a> { account_info.owner_addr, invoke_context.get_check_aligned(), )?; - let vm_data_addr = account_info.data_addr; consume_compute_meter( invoke_context, @@ -188,28 +270,18 @@ impl<'a> CallerAccount<'a> { .unwrap_or(u64::MAX), )?; - let bpf_account_data_direct_mapping = invoke_context - .feature_set - .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - let serialized_data = translate_slice_mut::( - memory_mapping, - if bpf_account_data_direct_mapping { - vm_data_addr.saturating_add(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(), - )?; + let serialized_data = if direct_mapping { + // See comment in CallerAccount::from_account_info() + &mut [] + } else { + translate_slice_mut::( + memory_mapping, + account_info.data_addr, + account_info.data_len, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; // we already have the host addr we want: &mut account_info.data_len. // The account info might be read only in the vm though, so we translate @@ -244,9 +316,9 @@ impl<'a> CallerAccount<'a> { Ok(CallerAccount { lamports, owner, - original_data_len, + original_data_len: account_metadata.original_data_len, serialized_data, - vm_data_addr, + vm_data_addr: account_info.data_addr, ref_to_len_in_vm, serialized_len_ptr, executable: account_info.executable, @@ -1155,7 +1227,11 @@ fn update_caller_account( // 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. + // spaces are fixed so we don't need to update the MemoryRegion's + // length. + // + // We can trust vm_data_addr to point to the correct region because we + // enforce that in CallerAccount::from_(sol_)account_info. 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 { @@ -1449,7 +1525,8 @@ mod tests { let key = Pubkey::new_unique(); let vm_addr = MM_INPUT_START; - let (_mem, region) = MockAccountInfo::new(key, &account).into_region(vm_addr); + let (_mem, region, account_metadata) = + MockAccountInfo::new(key, &account).into_region(vm_addr); let config = Config { aligned_memory_mapping: false, @@ -1465,9 +1542,7 @@ mod tests { false, vm_addr, account_info, - &SerializedAccountMetadata { - original_data_len: account.data().len(), - }, + &account_metadata, ) .unwrap(); assert_eq!(*caller_account.lamports, account.lamports()); @@ -2162,6 +2237,16 @@ mod tests { let key = transaction_accounts[1].0; let original_data_len = account.data().len(); + let vm_addr = MM_INPUT_START; + let (_mem, region, account_metadata) = + MockAccountInfo::new(key, &account).into_region(vm_addr); + + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new(vec![region], &config, &SBPFVersion::V2).unwrap(); + mock_invoke_context!( invoke_context, transaction_context, @@ -2170,21 +2255,8 @@ mod tests { &[0], &[1, 1] ); - mock_create_vm!( - _vm, - Vec::new(), - vec![SerializedAccountMetadata { original_data_len }], - &mut invoke_context - ); - let vm_addr = MM_INPUT_START; - let (_mem, region) = MockAccountInfo::new(key, &account).into_region(vm_addr); - - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new(vec![region], &config, &SBPFVersion::V2).unwrap(); + mock_create_vm!(_vm, Vec::new(), vec![account_metadata], &mut invoke_context); let accounts = SyscallInvokeSignedRust::translate_accounts( &[ @@ -2481,7 +2553,7 @@ mod tests { } } - fn into_region(self, vm_addr: u64) -> (Vec, MemoryRegion) { + fn into_region(self, vm_addr: u64) -> (Vec, MemoryRegion, SerializedAccountMetadata) { let size = mem::size_of::() + mem::size_of::() * 2 + mem::size_of::>>() @@ -2542,7 +2614,17 @@ mod tests { } let region = MemoryRegion::new_writable(data.as_mut_slice(), vm_addr as u64); - (data, region) + ( + data, + region, + SerializedAccountMetadata { + original_data_len: self.data.len(), + vm_key_addr: key_addr as u64, + vm_lamports_addr: lamports_addr as u64, + vm_owner_addr: owner_addr as u64, + vm_data_addr: data_addr as u64, + }, + ) } } diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 9190fd70e2aae4..d8e39961291501 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -123,6 +123,8 @@ pub enum SyscallError { }, #[error("InvalidAttribute")] InvalidAttribute, + #[error("Invalid pointer")] + InvalidPointer, } type Error = Box; From dd2a9b3f610b1a0e65aa80e17c3673312bdff279 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Jul 2023 21:06:25 +0700 Subject: [PATCH 04/12] bpf_loader: cpi: access ref_to_len_in_vm through VmValue Direct mapping needs to translate vm values at each access since permissions of the underlying memory might have changed. --- programs/bpf_loader/src/syscalls/cpi.rs | 160 ++++++++++++++++-------- 1 file changed, 110 insertions(+), 50 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 26eefd345b40ca..78fb5803a4df66 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -32,11 +32,46 @@ fn check_account_info_pointer( Ok(()) } +enum VmValue<'a, 'b, T> { + VmAddress { + vm_addr: u64, + memory_mapping: &'b MemoryMapping<'a>, + check_aligned: bool, + }, + // Once direct mapping is activated, this variant can be removed and the + // enum can be made a struct. + Translated(&'a mut T), +} + +impl<'a, 'b, T> VmValue<'a, 'b, T> { + fn get(&self) -> Result<&T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } + + fn get_mut(&mut self) -> Result<&mut T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type_mut(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } +} + /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. /// /// At the start of a CPI, this can be different from the data stored in the /// corresponding BorrowedAccount, and needs to be synched. -struct CallerAccount<'a> { +struct CallerAccount<'a, 'b> { lamports: &'a mut u64, owner: &'a mut Pubkey, // The original data length of the account at the start of the current @@ -56,23 +91,23 @@ struct CallerAccount<'a> { // 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, - ref_to_len_in_vm: &'a mut u64, + ref_to_len_in_vm: VmValue<'b, 'a, u64>, // To be removed once `feature_set::move_serialized_len_ptr_in_cpi` is active everywhere serialized_len_ptr: *mut u64, executable: bool, rent_epoch: u64, } -impl<'a> CallerAccount<'a> { +impl<'a, 'b> CallerAccount<'a, 'b> { // Create a CallerAccount given an AccountInfo. fn from_account_info( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, is_loader_deprecated: bool, _vm_addr: u64, account_info: &AccountInfo, account_metadata: &SerializedAccountMetadata, - ) -> Result, Error> { + ) -> Result, Error> { let direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); @@ -141,14 +176,23 @@ impl<'a> CallerAccount<'a> { .unwrap_or(u64::MAX), )?; - let translated = translate( - memory_mapping, - AccessType::Store, - (account_info.data.as_ptr() as *const u64 as u64) - .saturating_add(size_of::() as u64), - 8, - )? as *mut u64; - let ref_to_len_in_vm = unsafe { &mut *translated }; + let ref_to_len_in_vm = if direct_mapping { + VmValue::VmAddress { + vm_addr: (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64), + memory_mapping, + check_aligned: invoke_context.get_check_aligned(), + } + } else { + let translated = translate( + memory_mapping, + AccessType::Store, + (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64), + 8, + )? as *mut u64; + VmValue::Translated(unsafe { &mut *translated }) + }; let serialized_len_ptr = if invoke_context .feature_set .is_active(&feature_set::move_serialized_len_ptr_in_cpi::id()) @@ -209,12 +253,12 @@ impl<'a> CallerAccount<'a> { // Create a CallerAccount given a SolAccountInfo. fn from_sol_account_info( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, _is_loader_deprecated: bool, vm_addr: u64, account_info: &SolAccountInfo, account_metadata: &SerializedAccountMetadata, - ) -> Result, Error> { + ) -> Result, Error> { let direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); @@ -290,13 +334,22 @@ impl<'a> CallerAccount<'a> { let data_len_vm_addr = vm_addr .saturating_add(&account_info.data_len as *const u64 as u64) .saturating_sub(account_info as *const _ as *const u64 as u64); - let data_len_addr = translate( - memory_mapping, - AccessType::Store, - data_len_vm_addr, - size_of::() as u64, - )?; - let ref_to_len_in_vm = unsafe { &mut *(data_len_addr as *mut u64) }; + + let ref_to_len_in_vm = if direct_mapping { + VmValue::VmAddress { + vm_addr: data_len_vm_addr, + memory_mapping, + check_aligned: invoke_context.get_check_aligned(), + } + } else { + let data_len_addr = translate( + memory_mapping, + AccessType::Store, + data_len_vm_addr, + size_of::() as u64, + )?; + VmValue::Translated(unsafe { &mut *(data_len_addr as *mut u64) }) + }; let ref_of_len_in_input_buffer = (account_info.data_addr as *mut u8 as u64).saturating_sub(8); @@ -327,7 +380,7 @@ impl<'a> CallerAccount<'a> { } } -type TranslatedAccounts<'a> = Vec<(IndexOfAccount, Option>)>; +type TranslatedAccounts<'a, 'b> = Vec<(IndexOfAccount, Option>)>; /// Implemented by language specific data structure translators trait SyscallInvokeSigned { @@ -336,15 +389,15 @@ trait SyscallInvokeSigned { memory_mapping: &MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result; - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error>; + ) -> Result, Error>; fn translate_signers( program_id: &Pubkey, signers_seeds_addr: u64, @@ -430,15 +483,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -664,15 +717,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -781,7 +834,7 @@ where // Finish translating accounts, build CallerAccount values and update callee // accounts in preparation of executing the callee. -fn translate_and_update_accounts<'a, T, F>( +fn translate_and_update_accounts<'a, 'b, T, F>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_info_keys: &[&Pubkey], @@ -789,18 +842,18 @@ fn translate_and_update_accounts<'a, T, F>( account_infos_addr: u64, is_loader_deprecated: bool, invoke_context: &mut InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, do_translate: F, -) -> Result, Error> +) -> Result, Error> where F: Fn( &InvokeContext, - &MemoryMapping, + &'b MemoryMapping<'a>, bool, u64, &T, &SerializedAccountMetadata, - ) -> Result, Error>, + ) -> Result, Error>, { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -1118,7 +1171,7 @@ fn update_callee_account( if direct_mapping { let prev_len = callee_account.get_data().len(); - let post_len = *caller_account.ref_to_len_in_vm as usize; + let post_len = *caller_account.ref_to_len_in_vm.get()? as usize; match callee_account .can_data_be_resized(post_len) .and_then(|_| callee_account.can_data_be_changed()) @@ -1238,7 +1291,8 @@ fn update_caller_account( region.host_addr.set(callee_ptr); } } - let prev_len = *caller_account.ref_to_len_in_vm as usize; + + let prev_len = *caller_account.ref_to_len_in_vm.get()? 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() { @@ -1302,7 +1356,7 @@ fn update_caller_account( )?; } // this is the len field in the AccountInfo::data slice - *caller_account.ref_to_len_in_vm = post_len as u64; + *caller_account.ref_to_len_in_vm.get_mut()? = post_len as u64; // this is the len field in the serialized parameters if invoke_context @@ -1549,7 +1603,7 @@ mod tests { assert_eq!(caller_account.owner, account.owner()); assert_eq!(caller_account.original_data_len, account.data().len()); assert_eq!( - *caller_account.ref_to_len_in_vm as usize, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize, account.data().len() ); assert_eq!(caller_account.serialized_data, account.data()); @@ -1677,7 +1731,10 @@ mod tests { .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, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); assert_eq!(data_len, serialized_len()); assert_eq!(data_len, caller_account.serialized_data.len()); assert_eq!( @@ -1822,7 +1879,10 @@ mod tests { 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); + assert_eq!( + data_len, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); // the length slot in the serialization parameters must be updated assert_eq!(data_len, serialized_len()); @@ -2069,7 +2129,7 @@ mod tests { // close the account let mut data = Vec::new(); caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( @@ -2125,7 +2185,7 @@ mod tests { // without direct mapping let mut data = b"foobarbaz".to_vec(); - *caller_account.ref_to_len_in_vm = data.len() as u64; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = data.len() as u64; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2142,7 +2202,7 @@ mod tests { // with direct mapping let mut data = b"baz".to_vec(); - *caller_account.ref_to_len_in_vm = 9; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 9; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2198,7 +2258,7 @@ mod tests { (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; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = len as u64; update_callee_account( &invoke_context, false, @@ -2214,7 +2274,7 @@ mod tests { // close the account let mut data = Vec::new(); caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( @@ -2370,7 +2430,7 @@ mod tests { } } - fn caller_account(&mut self) -> CallerAccount<'_> { + fn caller_account(&mut self) -> CallerAccount<'_, '_> { let data = &mut self.data[mem::size_of::()..]; CallerAccount { lamports: &mut self.lamports, @@ -2378,7 +2438,7 @@ mod tests { 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, + ref_to_len_in_vm: VmValue::Translated(&mut self.len), serialized_len_ptr: std::ptr::null_mut(), executable: false, rent_epoch: 0, From 9788a4efffdb1436bdcb5f0891111398d5a4da0b Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 6 Jun 2023 13:22:29 +0000 Subject: [PATCH 05/12] direct mapping: improve memory permission tracking across CPI calls Ensure that the data and realloc regions of an account always track the account's permissions. In order to do this, we also need to split realloc regions in their own self contained regions, where before we had: [account fields][account data][account realloc + more account fields + next account fields][next account data][...] we now have: [account fields][account data][account realloc][more account fields + next account fields][next account data][...] Tested in TEST_[FORBID|ALLOW]_WRITE_AFTER_OWNERSHIP_CHANGE* Additionally when direct mapping is on, we must update all perms at once before doing account data updates. Otherwise, updating an account might write into another account whose perms we haven't updated yet. Tested in TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE. --- Cargo.lock | 1 + programs/bpf_loader/Cargo.toml | 1 + programs/bpf_loader/src/serialization.rs | 23 +- programs/bpf_loader/src/syscalls/cpi.rs | 447 +++++++++++++++++++---- programs/sbf/Cargo.lock | 1 + 5 files changed, 389 insertions(+), 84 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b171214e65866a..d0bf159c257ecb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5372,6 +5372,7 @@ dependencies = [ "log", "memoffset 0.9.0", "rand 0.8.5", + "scopeguard", "solana-measure", "solana-program-runtime", "solana-sdk", diff --git a/programs/bpf_loader/Cargo.toml b/programs/bpf_loader/Cargo.toml index e7123ae0cb4d00..411d9f3654b974 100644 --- a/programs/bpf_loader/Cargo.toml +++ b/programs/bpf_loader/Cargo.toml @@ -15,6 +15,7 @@ byteorder = { workspace = true } libsecp256k1 = { workspace = true } log = { workspace = true } rand = { workspace = true } +scopeguard = { workspace = true } solana-measure = { workspace = true } solana-program-runtime = { workspace = true } solana-sdk = { workspace = true } diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index c4f5dbe5fcd3b3..72ee3ffea95c5f 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -117,6 +117,8 @@ impl Serializer { 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); + // put the realloc padding in its own region + self.push_region(account.can_data_be_changed().is_ok()); } } @@ -127,7 +129,7 @@ impl Serializer { &mut self, account: &mut BorrowedAccount<'_>, ) -> Result { - self.push_region(); + self.push_region(true); let vaddr = self.vaddr; let account_len = account.get_data().len(); if account_len > 0 { @@ -158,19 +160,26 @@ impl Serializer { Ok(vaddr) } - fn push_region(&mut self) { + fn push_region(&mut self, writable: bool) { let range = self.region_start..self.buffer.len(); - let region = MemoryRegion::new_writable( - self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), - self.vaddr, - ); + let region = if writable { + MemoryRegion::new_writable( + self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), + self.vaddr, + ) + } else { + MemoryRegion::new_readonly( + self.buffer.as_slice().get(range.clone()).unwrap(), + self.vaddr, + ) + }; self.regions.push(region); self.region_start = range.end; self.vaddr += range.len() as u64; } fn finish(mut self) -> (AlignedMemory, Vec) { - self.push_region(); + self.push_region(true); debug_assert_eq!(self.region_start, self.buffer.len()); (self.buffer, self.regions) } diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 78fb5803a4df66..e0838a106c8dce 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,7 +1,9 @@ use { super::*, crate::declare_syscall, + scopeguard::defer, solana_program_runtime::invoke_context::SerializedAccountMetadata, + solana_rbpf::memory_region::{MemoryRegion, MemoryState}, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -10,7 +12,7 @@ use { }, transaction_context::BorrowedAccount, }, - std::{mem, ptr, slice}, + std::{mem, ptr}, }; fn check_account_info_pointer( @@ -83,10 +85,8 @@ struct CallerAccount<'a, 'b> { // 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. + // This is only set when direct mapping is off (see the relevant comment in + // CallerAccount::from_account_info). 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. @@ -103,7 +103,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> { fn from_account_info( invoke_context: &InvokeContext, memory_mapping: &'b MemoryMapping<'a>, - is_loader_deprecated: bool, _vm_addr: u64, account_info: &AccountInfo, account_metadata: &SerializedAccountMetadata, @@ -209,26 +208,29 @@ impl<'a, 'b> CallerAccount<'a, 'b> { }; let vm_data_addr = data.as_ptr() as u64; - let serialized_data = translate_slice_mut::( - memory_mapping, - if direct_mapping { - vm_data_addr.saturating_add(original_data_len as u64) - } else { - vm_data_addr - }, - if 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(), - )?; - + let serialized_data = if direct_mapping { + // when direct mapping is enabled, the permissions on the + // realloc region can change during CPI so we must delay + // translating until when we know whether we're going to mutate + // the realloc region or not. Consider this case: + // + // [caller can't write to an account] <- we are here + // [callee grows and assigns account to the caller] + // [caller can now write to the account] + // + // If we always translated the realloc area here, we'd get a + // memory access violation since we can't write to the account + // _yet_, but we will be able to once the caller returns. + &mut [] + } else { + translate_slice_mut::( + memory_mapping, + vm_data_addr, + data.len() as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; ( serialized_data, vm_data_addr, @@ -254,7 +256,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> { fn from_sol_account_info( invoke_context: &InvokeContext, memory_mapping: &'b MemoryMapping<'a>, - _is_loader_deprecated: bool, vm_addr: u64, account_info: &SolAccountInfo, account_metadata: &SerializedAccountMetadata, @@ -378,6 +379,19 @@ impl<'a, 'b> CallerAccount<'a, 'b> { rent_epoch: account_info.rent_epoch, }) } + + fn realloc_region( + &self, + memory_mapping: &'b MemoryMapping<'_>, + is_loader_deprecated: bool, + ) -> Result, Error> { + account_realloc_region( + memory_mapping, + self.vm_data_addr, + self.original_data_len, + is_loader_deprecated, + ) + } } type TranslatedAccounts<'a, 'b> = Vec<(IndexOfAccount, Option>)>; @@ -849,7 +863,6 @@ where F: Fn( &InvokeContext, &'b MemoryMapping<'a>, - bool, u64, &T, &SerializedAccountMetadata, @@ -918,7 +931,6 @@ 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, ), @@ -934,6 +946,7 @@ where // changes. update_callee_account( invoke_context, + memory_mapping, is_loader_deprecated, &caller_account, callee_account, @@ -1129,6 +1142,25 @@ fn cpi_common( .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + if direct_mapping { + // Update all perms at once before doing account data updates. This + // isn't strictly required as we forbid updates to an account to touch + // other accounts, but since we did have bugs around this in the past, + // it's better to be safe than sorry. + for (index_in_caller, caller_account) in accounts.iter() { + if let Some(caller_account) = caller_account { + let callee_account = instruction_context + .try_borrow_instruction_account(transaction_context, *index_in_caller)?; + update_caller_account_perms( + memory_mapping, + caller_account, + &callee_account, + is_loader_deprecated, + )?; + } + } + } + for (index_in_caller, caller_account) in accounts.iter_mut() { if let Some(caller_account) = caller_account { let mut callee_account = instruction_context @@ -1157,6 +1189,7 @@ fn cpi_common( // changes. fn update_callee_account( invoke_context: &InvokeContext, + memory_mapping: &MemoryMapping, is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, @@ -1177,16 +1210,28 @@ fn update_callee_account( .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 { + // bpf_loader_deprecated programs don't have a realloc region + if is_loader_deprecated && realloc_bytes_used > 0 { + return Err(InstructionError::InvalidRealloc.into()); + } + callee_account.set_data_length(post_len)?; + if realloc_bytes_used > 0 { + let serialized_data = translate_slice::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + realloc_bytes_used as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; callee_account .get_data_mut()? .get_mut(caller_account.original_data_len..post_len) .ok_or(SyscallError::InvalidLength)? .copy_from_slice( - caller_account - .serialized_data + serialized_data .get(0..realloc_bytes_used) .ok_or(SyscallError::InvalidLength)?, ); @@ -1247,6 +1292,63 @@ fn update_callee_account( Ok(()) } +fn update_caller_account_perms( + memory_mapping: &MemoryMapping, + caller_account: &CallerAccount, + callee_account: &BorrowedAccount<'_>, + is_loader_deprecated: bool, +) -> Result<(), Error> { + let CallerAccount { + original_data_len, + vm_data_addr, + .. + } = caller_account; + + let data_region = account_data_region(memory_mapping, *vm_data_addr, *original_data_len)?; + if let Some(region) = data_region { + match ( + region.state.get(), + callee_account.can_data_be_changed().is_ok(), + ) { + (MemoryState::Readable, true) => { + // 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. + if callee_account.is_shared() { + let index_in_transaction = callee_account.get_index_in_transaction(); + region + .state + .set(MemoryState::Cow(index_in_transaction as u64)); + } else { + region.state.set(MemoryState::Writable); + } + } + + (MemoryState::Writable | MemoryState::Cow(_), false) => { + region.state.set(MemoryState::Readable); + } + _ => {} + } + } + let realloc_region = account_realloc_region( + memory_mapping, + *vm_data_addr, + *original_data_len, + is_loader_deprecated, + )?; + if let Some(region) = realloc_region { + region + .state + .set(if callee_account.can_data_be_changed().is_ok() { + MemoryState::Writable + } else { + MemoryState::Readable + }); + } + + Ok(()) +} + // Update the given account after executing CPI. // // caller_account and callee_account describe to the same account. At CPI exit @@ -1266,34 +1368,36 @@ fn update_caller_account( *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - if direct_mapping && caller_account.original_data_len > 0 { - // 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()))? - } + if direct_mapping { + if let Some(region) = account_data_region( + memory_mapping, + caller_account.vm_data_addr, + caller_account.original_data_len, + )? { + // Since each instruction account is directly mapped in a memory region + // with a *fixed* length, upon returning from CPI we must ensure that the + // current capacity is at least the original length (what is mapped in + // memory), so that the account's memory region never points to an + // invalid address. + let min_capacity = caller_account.original_data_len; + if callee_account.capacity() < min_capacity { + callee_account.reserve(min_capacity.saturating_sub(callee_account.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. Address - // spaces are fixed so we don't need to update the MemoryRegion's - // length. - // - // We can trust vm_data_addr to point to the correct region because we - // enforce that in CallerAccount::from_(sol_)account_info. - 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); + // If an account's data pointer has changed - because of CoW, reserve() as called above + // 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 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.get()? as usize; let post_len = callee_account.get_data().len(); + let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if prev_len != post_len { let max_increase = if direct_mapping && !invoke_context.get_check_aligned() { 0 @@ -1311,8 +1415,15 @@ fn update_caller_account( ); return Err(Box::new(InstructionError::InvalidRealloc)); } + + // If the account has been shrunk, we're going to zero the unused memory + // *that was previously used*. if post_len < prev_len { if direct_mapping { + // We have two separate regions to zero out: the account data + // and the realloc region. + // + // Here we zero the account data region. if post_len < caller_account.original_data_len { // zero the spare capacity in the account data. We only need // to zero up to the original data length, everything else @@ -1326,15 +1437,57 @@ fn update_caller_account( // Safety: we check bounds above unsafe { ptr::write_bytes(dst, 0, spare_len) }; } + // Here we zero the realloc region. + // + // This is done for compatibility but really only necessary for + // the fringe case of a program calling itself, see + // TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS. + // + // Zeroing the realloc region isn't necessary in the normal + // invoke case because consider the following scenario: + // + // 1. Caller grows an account (prev_len > original_data_len) + // 2. Caller assigns the account to the callee (needed for 3 to + // work) + // 3. Callee shrinks the account (post_len < prev_len) + // + // In order for the caller to assign the account to the callee, + // the caller _must_ either set the account length to zero, + // therefore making prev_len > original_data_len impossible, + // or it must zero the account data, therefore making the + // zeroing we do here redundant. + if prev_len > caller_account.original_data_len { + // If we get here and prev_len > original_data_len, then + // we've already returned InvalidRealloc for the + // bpf_loader_deprecated case. + debug_assert!(!is_loader_deprecated); + + // Temporarily configure the realloc region as writable then set it back to + // whatever state it had. + let realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we already asserted !is_loader_deprecated + let original_state = realloc_region.state.replace(MemoryState::Writable); + defer! { + realloc_region.state.set(original_state); + }; - // 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); + // We need to zero the unused space in the realloc region, starting after the + // last byte of the new data which might be > original_data_len. + let dirty_realloc_start = caller_account.original_data_len.max(post_len); + // and we want to zero up to the old length + let dirty_realloc_len = prev_len.saturating_sub(dirty_realloc_start); + let serialized_data = translate_slice_mut::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(dirty_realloc_start as u64), + dirty_realloc_len as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + serialized_data.fill(0); + } } else { caller_account .serialized_data @@ -1344,8 +1497,8 @@ fn update_caller_account( } } - // with direct_mapping on, serialized_data is fixed and holds the - // realloc padding + // when direct mapping is enabled we don't cache the serialized data in + // caller_account.serialized_data. See CallerAccount::from_account_info. if !direct_mapping { caller_account.serialized_data = translate_slice_mut::( memory_mapping, @@ -1377,7 +1530,6 @@ 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.serialized_data; let from_slice = callee_account @@ -1388,21 +1540,88 @@ fn update_caller_account( 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 = caller_account - .serialized_data - .get_mut(0..realloc_bytes_used) - .ok_or(SyscallError::InvalidLength)?; + } else if realloc_bytes_used > 0 { + // In the is_loader_deprecated case, we must have failed with + // InvalidRealloc by now. + debug_assert!(!is_loader_deprecated); + + let to_slice = { + // If a callee reallocs an account, we write into the caller's + // realloc region regardless of whether the caller has write + // permissions to the account or not. If the callee has been able to + // make changes, it means they had permissions to do so, and here + // we're just going to reflect those changes to the caller's frame. + // + // Therefore we temporarily configure the realloc region as writable + // then set it back to whatever state it had. + let realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated + let original_state = realloc_region.state.replace(MemoryState::Writable); + defer! { + realloc_region.state.set(original_state); + }; + + translate_slice_mut::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + realloc_bytes_used as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; let from_slice = callee_account .get_data() .get(caller_account.original_data_len..post_len) .ok_or(SyscallError::InvalidLength)?; + if to_slice.len() != from_slice.len() { + return Err(Box::new(InstructionError::AccountDataTooSmall)); + } to_slice.copy_from_slice(from_slice); } Ok(()) } +fn account_data_region<'a>( + memory_mapping: &'a MemoryMapping<'_>, + vm_data_addr: u64, + original_data_len: usize, +) -> Result, Error> { + if original_data_len == 0 { + return Ok(None); + } + + // We can trust vm_data_addr to point to the correct region because we + // enforce that in CallerAccount::from_(sol_)account_info. + let data_region = memory_mapping.region(AccessType::Load, vm_data_addr)?; + // vm_data_addr must always point to the beginning of the region + debug_assert_eq!(data_region.vm_addr, vm_data_addr); + Ok(Some(data_region)) +} + +fn account_realloc_region<'a>( + memory_mapping: &'a MemoryMapping<'_>, + vm_data_addr: u64, + original_data_len: usize, + is_loader_deprecated: bool, +) -> Result, Error> { + if is_loader_deprecated { + return Ok(None); + } + + let realloc_vm_addr = vm_data_addr.saturating_add(original_data_len as u64); + let realloc_region = memory_mapping.region(AccessType::Load, realloc_vm_addr)?; + debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); + debug_assert!((MAX_PERMITTED_DATA_INCREASE + ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) + .contains(&(realloc_region.len as usize))); + debug_assert!(!matches!(realloc_region.state.get(), MemoryState::Cow(_))); + Ok(Some(realloc_region)) +} + #[allow(clippy::indexing_slicing)] #[allow(clippy::integer_arithmetic)] #[cfg(test)] @@ -1593,7 +1812,6 @@ mod tests { let caller_account = CallerAccount::from_account_info( &invoke_context, &memory_mapping, - false, vm_addr, account_info, &account_metadata, @@ -1886,8 +2104,16 @@ mod tests { // 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; + let realloc_area = translate_slice::( + &memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + MAX_PERMITTED_DATA_INCREASE as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + ) + .unwrap(); if data_len < original_data_len { // if an account gets resized below its original data length, @@ -2063,6 +2289,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2072,6 +2309,7 @@ mod tests { update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2107,6 +2345,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2116,6 +2365,7 @@ mod tests { update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2134,6 +2384,7 @@ mod tests { caller_account.owner = &mut owner; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2167,6 +2418,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2175,6 +2437,7 @@ mod tests { assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2192,6 +2455,7 @@ mod tests { assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2209,6 +2473,7 @@ mod tests { assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2241,6 +2506,17 @@ mod tests { true, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let mut callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2250,8 +2526,17 @@ mod tests { // 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; + let serialized_data = translate_slice_mut::( + &memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + 3, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + ) + .unwrap(); + serialized_data.copy_from_slice(b"baz"); for (len, expected) in [ (9, b"foobarbaz".to_vec()), // > original_data_len, copies from realloc region @@ -2261,6 +2546,7 @@ mod tests { *caller_account.ref_to_len_in_vm.get_mut().unwrap() = len as u64; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2279,6 +2565,7 @@ mod tests { caller_account.owner = &mut owner; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2358,6 +2645,7 @@ mod tests { data: Vec, len: u64, regions: Vec, + direct_mapping: bool, } impl MockCallerAccount { @@ -2417,6 +2705,7 @@ mod tests { data: d, len: data.len() as u64, regions, + direct_mapping, } } @@ -2431,7 +2720,11 @@ mod tests { } fn caller_account(&mut self) -> CallerAccount<'_, '_> { - let data = &mut self.data[mem::size_of::()..]; + let data = if self.direct_mapping { + &mut [] + } else { + &mut self.data[mem::size_of::()..] + }; CallerAccount { lamports: &mut self.lamports, owner: &mut self.owner, diff --git a/programs/sbf/Cargo.lock b/programs/sbf/Cargo.lock index 1b32410a4fc776..4aa3d33d156643 100644 --- a/programs/sbf/Cargo.lock +++ b/programs/sbf/Cargo.lock @@ -4613,6 +4613,7 @@ dependencies = [ "libsecp256k1 0.6.0", "log", "rand 0.8.5", + "scopeguard", "solana-measure", "solana-program-runtime", "solana-sdk", From 69f70161bbee239d5b2c880b627a75009f371125 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 19 Jul 2023 14:32:29 +0700 Subject: [PATCH 06/12] bpf_loader: serialization: address review comment don't return vm_addr from push_account_region --- programs/bpf_loader/src/serialization.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 72ee3ffea95c5f..3099d94de31634 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -99,7 +99,10 @@ impl Serializer { self.write_all(account.get_data()); vm_data_addr } else { - self.push_account_region(account)? + self.push_region(true); + let vaddr = self.vaddr; + self.push_account_region(account)?; + vaddr }; if self.aligned { @@ -128,11 +131,8 @@ impl Serializer { fn push_account_region( &mut self, account: &mut BorrowedAccount<'_>, - ) -> Result { - self.push_region(true); - let vaddr = self.vaddr; - let account_len = account.get_data().len(); - if account_len > 0 { + ) -> Result<(), InstructionError> { + if !account.get_data().is_empty() { 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 @@ -157,7 +157,8 @@ impl Serializer { self.vaddr += region.len; self.regions.push(region); } - Ok(vaddr) + + Ok(()) } fn push_region(&mut self, writable: bool) { From 11087cfaa6999bf2060d0848f5d0bd291b849239 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 23 Jul 2023 13:32:17 +0700 Subject: [PATCH 07/12] bpf_loader: rename push_account_region to push_account_data_region --- programs/bpf_loader/src/serialization.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 3099d94de31634..90e990dc6578fd 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -101,7 +101,7 @@ impl Serializer { } else { self.push_region(true); let vaddr = self.vaddr; - self.push_account_region(account)?; + self.push_account_data_region(account)?; vaddr }; @@ -128,7 +128,7 @@ impl Serializer { Ok(vm_data_addr) } - fn push_account_region( + fn push_account_data_region( &mut self, account: &mut BorrowedAccount<'_>, ) -> Result<(), InstructionError> { From 511514884c1295fa138d00aae4c32676a60bf855 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 16 Jul 2023 15:43:49 +0700 Subject: [PATCH 08/12] cpi: fix slow edge case zeroing extra account capacity after shrinking an account When returning from CPI we need to zero all the account memory up to the original length only if we know we're potentially dealing with uninitialized memory. When we know that the spare capacity has deterministic content, we only need to zero new_len..prev_len. This fixes a slow edge case that was triggerable by the following scenario: - load a large account (say 10MB) into the vm - shrink to 10 bytes - would memset 10..10MB - shrink to 9 bytes - would memset 9..10MB - shrink to 8 bytes - would memset 8..10MB - ... Now instead in the scenario above the following will happen: - load a large account (say 10MB) into the vm - shrink to 10 bytes - memsets 10..10MB - shrink to 9 bytes - memsets 9..10 - shrink to 8 bytes - memset 8..9 - ... --- programs/bpf_loader/src/syscalls/cpi.rs | 29 ++++++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index e0838a106c8dce..661159b9c146be 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1368,6 +1368,7 @@ fn update_caller_account( *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); + let mut zero_all_mapped_spare_capacity = false; if direct_mapping { if let Some(region) = account_data_region( memory_mapping, @@ -1381,7 +1382,8 @@ 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()))? + callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))?; + zero_all_mapped_spare_capacity = true; } // If an account's data pointer has changed - because of CoW, reserve() as called above @@ -1391,6 +1393,7 @@ fn update_caller_account( let callee_ptr = callee_account.get_data().as_ptr() as u64; if region.host_addr.get() != callee_ptr { region.host_addr.set(callee_ptr); + zero_all_mapped_spare_capacity = true; } } } @@ -1424,11 +1427,24 @@ fn update_caller_account( // and the realloc region. // // Here we zero the account data region. - if post_len < caller_account.original_data_len { - // 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 spare_len = if zero_all_mapped_spare_capacity { + // In the unlikely case where the account data vector has + // changed - which can happen during CoW - we zero the whole + // extra capacity up to the original data length. + // + // The extra capacity up to original data length is + // accessible from the vm and since it's uninitialized + // memory, it could be a source of non determinism. + caller_account.original_data_len + } else { + // If the allocation has not changed, we only zero the + // difference between the previous and current lengths. The + // rest of the memory contains whatever it contained before, + // which is deterministic. + prev_len + } + .saturating_sub(post_len); + if spare_len > 0 { let dst = callee_account .spare_data_capacity_mut()? .get_mut(..spare_len) @@ -1437,6 +1453,7 @@ fn update_caller_account( // Safety: we check bounds above unsafe { ptr::write_bytes(dst, 0, spare_len) }; } + // Here we zero the realloc region. // // This is done for compatibility but really only necessary for From 03b90e949e6c3e31b6b11648ec4c14edf3a76ea3 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 23 Jul 2023 13:49:58 +0700 Subject: [PATCH 09/12] bpf_loader: add account_data_region_memory_state helper Shared between serialization and CPI to figure out the MemoryState of an account. --- programs/bpf_loader/src/serialization.rs | 38 ++++++++++++------------ programs/bpf_loader/src/syscalls/cpi.rs | 28 +++-------------- 2 files changed, 23 insertions(+), 43 deletions(-) diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 90e990dc6578fd..1650b845a9a24a 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -6,7 +6,7 @@ use { solana_rbpf::{ aligned_memory::{AlignedMemory, Pod}, ebpf::{HOST_ALIGN, MM_INPUT_START}, - memory_region::MemoryRegion, + memory_region::{MemoryRegion, MemoryState}, }, solana_sdk::{ bpf_loader_deprecated, @@ -133,26 +133,14 @@ impl Serializer { account: &mut BorrowedAccount<'_>, ) -> Result<(), InstructionError> { if !account.get_data().is_empty() { - 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 index_in_transaction = account.get_index_in_transaction(); - - MemoryRegion::new_cow( - account.get_data(), - self.vaddr, - index_in_transaction 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. + let region = match account_data_region_memory_state(account) { + MemoryState::Readable => MemoryRegion::new_readonly(account.get_data(), self.vaddr), + MemoryState::Writable => { MemoryRegion::new_writable(account.get_data_mut()?, self.vaddr) } - } else { - MemoryRegion::new_readonly(account.get_data(), self.vaddr) + MemoryState::Cow(index_in_transaction) => { + MemoryRegion::new_cow(account.get_data(), self.vaddr, index_in_transaction) + } }; self.vaddr += region.len; self.regions.push(region); @@ -615,6 +603,18 @@ pub fn deserialize_parameters_aligned>( Ok(()) } +pub(crate) fn account_data_region_memory_state(account: &BorrowedAccount<'_>) -> MemoryState { + if account.can_data_be_changed().is_ok() { + if account.is_shared() { + MemoryState::Cow(account.get_index_in_transaction() as u64) + } else { + MemoryState::Writable + } + } else { + MemoryState::Readable + } +} + #[cfg(test)] #[allow(clippy::indexing_slicing)] mod tests { diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 661159b9c146be..e153f75b1839ae 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,6 +1,6 @@ use { super::*, - crate::declare_syscall, + crate::{declare_syscall, serialization::account_data_region_memory_state}, scopeguard::defer, solana_program_runtime::invoke_context::SerializedAccountMetadata, solana_rbpf::memory_region::{MemoryRegion, MemoryState}, @@ -1306,29 +1306,9 @@ fn update_caller_account_perms( let data_region = account_data_region(memory_mapping, *vm_data_addr, *original_data_len)?; if let Some(region) = data_region { - match ( - region.state.get(), - callee_account.can_data_be_changed().is_ok(), - ) { - (MemoryState::Readable, true) => { - // 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. - if callee_account.is_shared() { - let index_in_transaction = callee_account.get_index_in_transaction(); - region - .state - .set(MemoryState::Cow(index_in_transaction as u64)); - } else { - region.state.set(MemoryState::Writable); - } - } - - (MemoryState::Writable | MemoryState::Cow(_), false) => { - region.state.set(MemoryState::Readable); - } - _ => {} - } + region + .state + .set(account_data_region_memory_state(callee_account)); } let realloc_region = account_realloc_region( memory_mapping, From 863f0210408a40e44d4827a771faf3158a76bb98 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 24 Aug 2023 00:00:07 +0700 Subject: [PATCH 10/12] cpi: direct_mapping: error out if ref_to_len_in_vm points to account memory If ref_to_len_in_vm is allowed to be in account memory, calles could mutate it, essentially letting callees directly mutate callers memory. --- programs/bpf_loader/src/syscalls/cpi.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index e153f75b1839ae..c17600aaa10a89 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -3,7 +3,10 @@ use { crate::{declare_syscall, serialization::account_data_region_memory_state}, scopeguard::defer, solana_program_runtime::invoke_context::SerializedAccountMetadata, - solana_rbpf::memory_region::{MemoryRegion, MemoryState}, + solana_rbpf::{ + ebpf, + memory_region::{MemoryRegion, MemoryState}, + }, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -176,9 +179,16 @@ impl<'a, 'b> CallerAccount<'a, 'b> { )?; let ref_to_len_in_vm = if direct_mapping { + let vm_addr = (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64); + // In the same vein as the other check_account_info_pointer() checks, we don't lock + // this pointer to a specific address but we don't want it to be inside accounts, or + // callees might be able to write to the pointed memory. + if vm_addr >= ebpf::MM_INPUT_START { + return Err(SyscallError::InvalidPointer.into()); + } VmValue::VmAddress { - vm_addr: (account_info.data.as_ptr() as *const u64 as u64) - .saturating_add(size_of::() as u64), + vm_addr, memory_mapping, check_aligned: invoke_context.get_check_aligned(), } @@ -337,6 +347,12 @@ impl<'a, 'b> CallerAccount<'a, 'b> { .saturating_sub(account_info as *const _ as *const u64 as u64); let ref_to_len_in_vm = if direct_mapping { + // In the same vein as the other check_account_info_pointer() checks, we don't lock this + // pointer to a specific address but we don't want it to be inside accounts, or callees + // might be able to write to the pointed memory. + if data_len_vm_addr >= ebpf::MM_INPUT_START { + return Err(SyscallError::InvalidPointer.into()); + } VmValue::VmAddress { vm_addr: data_len_vm_addr, memory_mapping, From b6a5ea1f56e9a799d1107df567c123686132756e Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Thu, 27 Jul 2023 20:14:45 +0700 Subject: [PATCH 11/12] bpf_loader: direct_mapping: map AccessViolation -> InstructionError Return the proper ReadonlyDataModified / ExecutableDataModified / ExternalAccountDataModified depending on where the violation occurs --- programs/bpf_loader/src/lib.rs | 82 +++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 25 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 99d3f583c95881..57b77559f2373d 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1495,12 +1495,19 @@ fn execute<'a, 'b: 'a>( let log_collector = invoke_context.get_log_collector(); let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; - let program_id = *instruction_context.get_last_program_key(transaction_context)?; + let (program_id, is_loader_deprecated) = { + let program_account = + instruction_context.try_borrow_last_program_account(transaction_context)?; + ( + *program_account.get_key(), + *program_account.get_owner() == bpf_loader_deprecated::id(), + ) + }; #[cfg(any(target_os = "windows", not(target_arch = "x86_64")))] let use_jit = false; #[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))] let use_jit = executable.get_compiled_program().is_some(); - let bpf_account_data_direct_mapping = invoke_context + let direct_mapping = invoke_context .feature_set .is_active(&bpf_account_data_direct_mapping::id()); @@ -1511,18 +1518,26 @@ fn execute<'a, 'b: 'a>( invoke_context .feature_set .is_active(&cap_bpf_program_instruction_accounts::ID), - !bpf_account_data_direct_mapping, + !direct_mapping, )?; 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 + // save the account addresses so in case we hit an AccessViolation error we + // can map to a more specific error + let account_region_addrs = accounts_metadata .iter() - .map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len)) + .map(|m| { + let vm_end = m + .vm_data_addr + .saturating_add(m.original_data_len as u64) + .saturating_add(if !is_loader_deprecated { + MAX_PERMITTED_DATA_INCREASE as u64 + } else { + 0 + }); + m.vm_data_addr..vm_end + }) .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; @@ -1574,22 +1589,43 @@ fn execute<'a, 'b: 'a>( }; Err(Box::new(error) as Box) } - ProgramResult::Err(error) => { - let error = match error.downcast_ref() { - Some(EbpfError::AccessViolation( + ProgramResult::Err(mut error) => { + if direct_mapping { + if let Some(EbpfError::AccessViolation( _pc, AccessType::Store, address, _size, _section_name, - )) 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.downcast_ref() + { + // If direct_mapping is enabled and a program tries to write to a readonly + // region we'll get a memory access violation. Map it to a more specific + // error so it's easier for developers to see what happened. + if let Some((instruction_account_index, _)) = account_region_addrs + .iter() + .enumerate() + .find(|(_, vm_region)| vm_region.contains(address)) + { + let transaction_context = &invoke_context.transaction_context; + let instruction_context = + transaction_context.get_current_instruction_context()?; + + let account = instruction_context.try_borrow_instruction_account( + transaction_context, + instruction_account_index as IndexOfAccount, + )?; + + error = Box::new(if account.is_executable() { + InstructionError::ExecutableDataModified + } else if account.is_writable() { + InstructionError::ExternalAccountDataModified + } else { + InstructionError::ReadonlyDataModified + }) + } } - _ => error, - }; + } Err(error) } _ => Ok(()), @@ -1615,12 +1651,8 @@ 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(), - !bpf_account_data_direct_mapping, - ) - .map_err(|error| Box::new(error) as Box) + deserialize_parameters(invoke_context, parameter_bytes.as_slice(), !direct_mapping) + .map_err(|error| Box::new(error) as Box) }); deserialize_time.stop(); From 7095299c52edd64459faf94e420999c0e4071277 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 30 Aug 2023 07:22:25 +0700 Subject: [PATCH 12/12] bpf_loader: cpi: remove unnecessary infallible slice::get call --- programs/bpf_loader/src/syscalls/cpi.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index c17600aaa10a89..e18052acc0c691 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1246,11 +1246,7 @@ fn update_callee_account( .get_data_mut()? .get_mut(caller_account.original_data_len..post_len) .ok_or(SyscallError::InvalidLength)? - .copy_from_slice( - serialized_data - .get(0..realloc_bytes_used) - .ok_or(SyscallError::InvalidLength)?, - ); + .copy_from_slice(serialized_data); } } Err(err) if prev_len != post_len => {